Pool/relay stability fixes, compact block relay, and signed auto-update + release automation - #31
Closed
Vic-Nas wants to merge 259 commits into
Closed
Pool/relay stability fixes, compact block relay, and signed auto-update + release automation#31Vic-Nas wants to merge 259 commits into
Vic-Nas wants to merge 259 commits into
Conversation
…ort forwarding needed
…icipant Stratum client
…t lock contention RefreshWallet cleared g_txRows unconditionally, then tried a non-blocking lock (TRY_CRITICAL_BLOCK) on cs_mapWallet to repopulate it. GetBalance() right above it uses a blocking lock and always succeeds -- so a moment where cs_mapWallet is busy (rapid block processing during a sync burst holding it repeatedly, for instance) left the balance correct but wiped the transaction list, and it stayed wiped until some future refresh happened to win the race. Reported symptom matched exactly: balance still reflects blocks won earlier, transaction list empty. Now the clear only happens once the lock is actually acquired, so a busy moment just skips that refresh cycle (keeps showing the last good list) instead of emptying it in the meantime.
A push that bumps VERSION triggers the whole pipeline; if preflight, either build, or the release step itself fails, that version number was still consumed -- the next bump has to skip past it, leaving a permanent gap (we've done this several times this session: 1.2.4 failed, 1.2.5 failed, etc., all burned). New job watches preflight/build-linux/build-windows/release and, if any of them actually failed (not skipped -- a job only shows 'skipped' if something upstream of it never ran), reverts the VERSION-bump commit and pushes that. Scoped to push events only, not workflow_dispatch (which can run on any commit for testing, not necessarily one that bumped VERSION -- nothing to revert there). If the revert can't apply cleanly (main moved on since, most likely from a fix being pushed immediately after seeing the failure), it aborts and leaves a warning rather than forcing anything -- safer to require a manual look in that case than to guess.
…ease cleanup git revert only ever touched VERSION here anyway (it's scoped to exactly the diff of the one commit being reverted, which only ever changes that one file) -- but 'revert commit' sounds like it could do more than that, and there's no reason to rely on that nuance being obvious. Now it's literal: read VERSION's content from the parent of the failed commit, write that back, commit just that. Also swapped the safety check from 'does the revert apply cleanly' to 'is the failed commit still the tip of main' -- simpler to reason about and catches the same case (something pushed since this run started) before touching anything, rather than attempting a change and rolling back if it conflicts.
…eaningless protocol number The Peers panel's 'Version' column was showing CNode::nVersion -- serialize.h's wire protocol constant (VERSION = 101, unchanged since this forked from Bitcoin 0.1), not the app release version. Every peer on any recent build showed identically 101, telling a user nothing about who's actually up to date. Appended BITFLASH_APP_VERSION as an optional 5th field on the existing 'version' message rather than adding a whole new message type -- the version handshake already exists precisely for this kind of exchange. An older peer without this field simply won't have sent it: checked via CDataStream::empty() before reading, not by reading unconditionally and catching the exception, since a thrown exception mid-handler would unwind out and skip everything below it (getblocks, sendcmpct, pex, nick) for any peer on an older build -- not just this one field. Needed a 5-arg PushMessage overload; net.h only had up to 4. Column renamed 'App Version', shows 'unknown' for a peer that hasn't sent one (older build, or the field hasn't arrived yet).
…ersion field Bare '1.2.17' is ambiguous across forks -- two peers can show the same number while running genuinely different code. Now sends '1.2.17 (owner/repo)' via compile-time string literal concatenation of BITFLASH_APP_VERSION and UPDATE_REPO (same pattern UPDATE_REPO is already used with elsewhere); a -self build has no UPDATE_REPO at all, so it sends '1.2.17 (self-built)' instead. Bumped the receive- side length cap from 32 to 96 to comfortably fit a repo name too.
…status bar
Two fixes:
1. 4h was overly conservative -- the check itself is one lightweight
GitHub API GET, nowhere near enough to worry about load even
checked fairly often for one node. 30 minutes.
2. Found a real bug while looking at this: the old 'Up to Date'
modal popup (g_showNoUpdateMsg/DrawNoUpdateDialog) survived an
earlier revert-and-reapply and was still wired up. It made sense
once, as feedback for a one-off manual check button -- but that
button doesn't exist anymore, and with periodic re-checking it
would have popped up every 30 minutes whenever nothing was found,
which is most of the time. Removed entirely.
Replaced both gaps with what was actually being asked for: the status
bar's 'Up to date' text now shows how long ago the last check
completed ('checked 12m ago' etc.), so periodic checking is visibly
happening even when there's nothing to report, instead of either a
recurring popup or total silence.
…ddresses got paid Walked back an earlier decision that was wrong. A UTXO transaction's inputs spend specific previous outputs, and each of those has its own address -- that address is the previous owner of the coins now paid to you, which is what 'who sent this' means here. Not a different or harder question than 'who did I pay' (which was already implemented via the payee address on a Sent row); this was skipped before over a privacy concern that wasn't really the point of what was asked. ExtractSenderAddrs traces each input via CTxDB::ReadDiskTx(prevout), extracts that output's address, applies the same address-book formatting Sent rows already use (split out into a shared FormatAddrList so both paths stay consistent). Skips an input whose previous transaction can't be found rather than failing the whole row -- still shows whatever does resolve. Coinbase transactions have no real sender and are handled separately already (shown as 'Generated'), so this returns empty for those.
…sh#37); fix /gen not actually mining (port of Bitflash-sh#42) Two real gaps, both pulled from upstream, both squarely in "the command line has been neglected" territory. ## Headless build (Bitflash-sh#37) The GUI binary needs libGL present at load time even when started with -nogui/-daemon: the dynamic loader resolves every dependency before main() runs, so the flag arrives far too late. A clean server -- where a node most naturally lives -- fails outright with a shared- library error that gives no hint the program would otherwise work fine. The AppImage additionally needs FUSE, which minimal servers lack. `make linux-node-build` (wired into both `linux` and `linux-self`) produces a second binary, `$(APPNAME)-node-<version>-x86_64`, from the same node objects, without ImGui/GLFW/OpenGL at all -- no graphics or FUSE dependency. Two small moves were needed: headless.cpp provides an empty MainFrameRepaint() (declared in headers_core.h, previously only ever defined in gui.cpp, which a headless build doesn't compile), and DateTimeStr moved from gui.cpp to util.cpp since it's not actually GUI-specific. Adapted rather than copied: our src/Makefile carries extra objects (nostr/compactblock/update) and the mandatory REPO/UPDATE_PUBKEY (or NO_UPDATE=1) machinery upstream doesn't have. Caught one real bug while wiring the root Makefile's linux-node-build target: forwarding NO_UPDATE unconditionally (even as an empty value) would have made `ifdef NO_UPDATE` true in the child invocation regardless -- GNU Make's ifdef checks whether a variable was set at all, not whether it's non-empty -- silently disabling the updater in every node build, including regular (non-self) ones. Branched explicitly instead. ## /gen not mining (Bitflash-sh#42) nMineMode defaults to MINE_RELAY and nothing in ParseStartupArguments ever changed it for the plain /gen case (only /operator and /participant set their own mode) -- so a node started with /gen alone spawned ThreadBitcoinMiner, hit BitcoinMiner()'s relay guard, and returned immediately. The node ran indefinitely looking completely healthy -- synced, held peers, no errors -- while mining nothing, with the only hint being nMineMode = 0 in a startup line that reads like a status echo. This affects every headless miner, since the GUI was the only other way to select a mining mode. /gen now implies MINE_SOLO unless /operator or /participant asked for something else.
…RM64 build (port of Bitflash-sh#41); gitignore: cover key files (port of Bitflash-sh#43) ## Bootstrap seeds (Bitflash-sh#36) A node with an empty cache has only the Nostr relays for discovery -- if those are down, blocked, or just slow, a first run has no way into the network at all, which is exactly the moment a user has the least patience. Seeds are the floor under that: a compiled-in list of long-lived peers, tried automatically only when the peer cache is empty. A seed entry is deliberately just an address and an encryption key, no meeting node -- the rendezvous relay pairs on the service's public key, and a .btf address is that key, so a seed is findable by walking the known relays. Recording which relay a seed was on would rot the moment it failed over, which is the exact "advertising a rendezvous nobody is registered at" failure this project has chased before. Seeds carry no special authority: they hand out the same signed, self-certifying descriptors any peer does, so a hostile seed can stall a bootstrap but can't forge a peer or feed a false chain. The compiled list here is empty, deliberately -- upstream shipped their own dedicated bootstrap node's address in their version of this commit, but that's their infrastructure, not something this fork has any relationship to or authority to point users at. /btfseed= ADDRESS:ENCHEX (repeatable, CLI-only) covers testing and private networks without needing a compiled entry. Adapted the hex-decode call to our own BtfPeerCacheHexToBytes rather than a generic HexToBytes upstream doesn't have under that name. ## ARM64 build failure (Bitflash-sh#41) ByteReverse(word32) in sha.h used the x86 bswap instruction via inline asm, guarded only by #if defined(__GNUC__) -- true on every architecture GCC/Clang target, so on aarch64 the assembler rejected the instruction outright. The 64-bit ByteReverse right below it already had the correct guard (&& defined(__x86_64__)); the 32-bit one just never got it. Fails the build on Apple Silicon, ARM servers, or a Raspberry Pi -- all plausible headless-node hosts. ## .gitignore gaps (Bitflash-sh#43) *.dat/wallet.dat were covered, but btf_enc.key (this node's .btf identity) and nostr.key (its Nostr signing key) are not .dat files and are exactly as sensitive -- already listed explicitly in our .gitignore from an earlier session, but walletrpc.token (bearer authority to spend from the wallet over the wallet API) was missing entirely, along with a general *.key catch-all and patterns for key backup archives, mempool dumps, and stray .patch/.rej/.orig files. A node run with its data directory inside a working tree -- the obvious thing to do while developing -- would otherwise leave all of this untracked but committable.
StartUpdateCheck only ever ran from inside RunGUI, so a node started with -nogui/-daemon (or the new headless bitflash-node binary) never checked for an update at all -- and that's exactly the deployment most likely to run unattended for a long stretch, which is when missing a critical update matters most. No modal to show in a log-only context, so it's a plain log line instead: same 30-minute interval the GUI uses, checked inline in the existing sleep loop rather than a new thread. A no-op on a -self build, same as everywhere else Update_CheckLatest is called (it's a stub there).
…ith the GUI Both settings were only ever changeable from the GUI's Options dialog -- a headless node was stuck with a randomly generated nickname forever and a transaction fee of 0 with no way to override either without a window it doesn't have. Persisted the same way the GUI does (CWalletDB::WriteSetting), so a later GUI session on the same wallet.dat picks up whatever was set here.
…ash-sh#40) BitcoinMiner() called key.MakeNewKey() once per mining session and paid every block's reward to that key -- generated fresh, with no backup covering it until AddKey() ran, and that only happened *after* a block was successfully found. Two real ways to lose funds: 1. A wallet.dat backup taken before mining starts doesn't contain this key. Mine blocks, restore that backup later, and the reward is unrecoverable even though the coins are still on-chain. Reported exactly this way against this codebase in Bitflash-sh#40. 2. AddKey() only ran after a block was found and about to be processed. A crash between broadcasting the block and that save meant the reward's private key existed only in memory and was simply gone -- no backup/restore needed to trigger this one. Both are the same class of bug the keypool already exists to prevent, just never applied here. vchMinerPubKey = GetKeyFromPool() instead: the key is durably in wallet.dat (via TopUpKeyPool's AddKey, already run before the key is ever handed out) from the moment mining starts, not from the moment a block happens to be found. The old save-then-generate-next block after a found block is now just draw-the-next-one, since there's nothing left to save at that point.
Was badly stale: referenced v1.1.0 filenames, a hardcoded 'Bitflash-*' naming pattern that hasn't been true since release automation shipped, an 'Options' menu that's been renamed to 'Mine', and build instructions (plain 'make linux'/'make windows') that fail outright now that REPO/UPDATE_PUBKEY are mandatory -- someone following this doc today would hit a wall immediately. Also added real content that was never documented at all: the headless bitflash-node binary (no libGL dependency, the actual recommended path for a server), the periodic signed-update-check system and how it fails closed, the address book/peers panel, node nicknames, and current CLI flags. And a prominent backup warning (Bitflash-sh#40): wallet.dat alone isn't a safe backup without its database/ subfolder, and a backup is a point-in-time snapshot, not something that stays valid forever.
Cut from 173 to 111 lines. Kept: what it works like, how to build, mining modes, headless deployment, the backup warning. Dropped the separate Peers/Updates sections as their own headers -- folded the one-line versions of each into Quick start and Headless instead.
Chose the lower-risk path over adopting a TUI library (FTXUI/ cpp-terminal were the two real candidates, both cross-platform and well-maintained, but pulling either in means integrating a new dependency into two separate build systems for a display-only feature). Raw ANSI escape codes instead: cursor-home + per-line clear each redraw, no full-screen flicker, works on any terminal that understands ANSI (which is everywhere this app already targets -- Linux, macOS, and Windows 10+ consoles once VT100 processing is enabled per-handle, done here via SetConsoleMode). Shows height, peer count, mining mode/status, and -- for participant mode -- owed balance, round shares, and the overdue-payout warning already built earlier this session; for operator mode, miner count and blocks found this session. Update-check status folds in too, same 30-minute background check as before, just displayed instead of only logged. Auto-detects whether stdout is an actual interactive terminal (isatty/_isatty) and falls back to the existing plain scrolling log lines otherwise -- a systemd service, a pipe, anything redirected to a file gets exactly the old behavior, since ANSI redraw codes in a log file would be either noise or literally corrupted-looking text, and nobody's watching a redirected stream live to read a dashboard anyway. Real risk this could have introduced: background threads' printf/ LogPrint already echo to the console on non-Windows (confirmed in util.h), which would interleave with and corrupt an in-place- redrawing display. Added g_fSuppressConsoleEcho, checked right at that echo site -- set only while the dashboard owns the screen, so debug.log keeps getting every line as always, console output just comes from the dashboard alone during that time.
…ndored, MIT) The previous version was raw ANSI escape codes with no real cross-platform testing behind it -- exactly the kind of one-off terminal-handling code that looks fine in review and breaks on a real Windows console (a failed VT100 enable had no fallback, would have shown literal escape-code garbage on screen instead of a dashboard). Correctly called out as the wrong trade-off: I'd avoided a new build dependency at the cost of correctness on the actual thing being built. Vendored FTXUI's official amalgamated release (v7.0.1, MIT, from GitHub Releases -- same integration shape as ImGui already gets in this repo: two files, no CMake, no package manager) instead. It's packaged for Debian/Ubuntu/Arch/vcpkg/Conan and used in exactly this category of app already (resource monitors, CPU meters), which is the actual point: battle-tested where the hand-rolled version wasn't. Confirmed by actually building and running a test program against the vendored files before committing to this, not just reading docs. src/dashboard.cpp uses FTXUI's real DOM API (vbox/hbox/text/ separator, Screen::ResetPosition for in-place redraw) instead of raw escape sequences, and needs none of the manual Windows console-mode code the old version had -- FTXUI handles that internally (confirmed in its vendored source, and by the test program running correctly with zero setup). FTXUI requires C++17; the rest of this codebase stays gnu++14. FTXUI_CXXFLAGS derives a C++17 variant of the existing flags (same defines/includes, just the standard swapped) so only ftxui.cpp and dashboard.cpp build under it -- nothing else changes standard. Wired into both Makefiles identically (Linux and Windows/MSYS2), added to NODE_OBJS so both the GUI binary (-nogui/-daemon at runtime) and the headless bitflash-node binary link it. Actually compiled this, not just inspected it -- installed the missing dev packages in this sandbox to get a real build, which caught a real bug before it could ship: `using ftxui::Dimension;` is invalid (Dimension is a namespace, not a type), which would have failed to build outright. Fixed by qualifying ftxui::Dimension::Full() directly at its one call site. Confirmed clean compiles across all three build variants (with-updater, -self, and BITFLASH_NO_GUI) plus spot-checked that untouched core files (main.cpp, net.cpp, rpc.cpp, util.cpp) still compile cleanly after the util.h change from the previous pass (g_fSuppressConsoleEcho, still needed: FTXUI doesn't know about this codebase's own console-echo path in OutputDebugStringF, so background threads' log lines would still interleave with and corrupt the redraw without it).
… duration format Went back and actually compared against the GUI instead of assuming the dashboard's original scope was right. Real gap: the GUI's header shows the wallet address and balance first, before anything else -- same place here now, for the same reason (it's what anyone checking on a running node wants to see first). Nickname added too, small but free given it's already loaded. Deliberately still not mirroring the GUI's full surface: the transaction list, peer/cached-peer/address-book tables, and pool selection are genuinely list/management views that don't fit a fixed non-scrolling frame the way a scrollable GUI panel does -- peer count stays a count, not an attempt to cram a table into 80 columns. That's a real design choice, not the same kind of oversight the missing balance/address was. Also matched the GUI's uptime/duration format (seconds under a minute, minutes under an hour, hours after) instead of the dashboard's own minutes-only format, which read as '150m ago' instead of '2h ago' on anything long-running -- same FormatDuration used for uptime, the update-check age, and the overdue-payout warning, so all three read consistently instead of each doing their own thing. Recompiled against the real headers after these changes (same process as the initial FTXUI port) -- clean, no new errors.
Previously the dashboard could only tell you an update existed -- 'run the GUI build to apply it there' was a dead end for anyone running bitflash-node unattended on a box with no GUI anywhere on it. Rebuilt on FTXUI's component module (Renderer + CatchEvent) instead of the previous static print loop, using ScreenInteractive/Loop -- already-vendored, already-tested library code, not hand-rolled keyboard input handling. Verified by compiling and partially linking against the real headers again (same process as the initial FTXUI port): confirmed every component-module symbol used here (Renderer, CatchEvent, ScreenInteractive, Loop, Event) resolves cleanly from the vendored ftxui.cpp, with zero FTXUI-related link errors. Ctrl-C is deliberately left to FTXUI's own default handling rather than mixing in a custom signal() call for this path -- confirmed via FTXUI's own issue tracker that combining an external SIGINT handler with the library's internal terminal/signal management is a known way to end up with a hung terminal on exit. Periodic redraw and the update check itself run on a background thread using screen.PostEvent(Event::Custom), the same pattern FTXUI's maintainers point people to across multiple discussions for exactly this 'redraw on a timer, not just on input' case. Pressing 'u' only does anything once an update is actually shown as available (a no-op otherwise); it hands off to Update_ApplyAndRelaunch on its own thread, same signature-verification path as the GUI's Update Now button, same fail-closed behavior.
…date modal g_showUpdateDialog was doing two jobs -- whether the modal is open, and whether the status bar thinks an update exists -- so clicking Skip (which correctly only closes the modal) also incorrectly made the status bar forget the update was ever found. Split into two flags: g_showUpdateDialog controls only the modal now, g_updateAvailable is set once a check finds something newer and stays true regardless of dismissing the modal, until an actual update succeeds (which relaunches the process, making the state moot) or the app restarts. Status bar's Update button now also re-opens the modal when clicked, giving a way back to it after a dismiss.
GetDepthInMainChain() returns 0 for both a genuinely unconfirmed tx and an orphaned coinbase (block not in main chain) -- same value, so every depth-0 row had no defined order relative to each other and fell back to mapWallet's raw hash ordering, which has nothing to do with time. Visible in a screenshot as orphaned 'Generated' rows with dates jumping around non-chronologically. Added time as a tiebreaker within equal depth (most recent first), kept depth as the primary key exactly as before -- a resync re-stamping many txs with the same time just means those particular ties fall back to hash order again, no worse than the prior behavior, but the common case (normal operation, not mid-resync) now sorts correctly.
…ional auto-return Participant mode retried a dead pool forever with no way out short of manually switching modes. New: /fallbacktosolo=MINUTES (GUI: "Fall back to solo mining after") — after the pool's been unreachable that long, switch to solo automatically. 0/unset keeps the old behavior (retry forever). /noautoreturn (GUI: unchecking "Automatically go back to the pool") controls whether it then periodically retries the original pool, or just stays solo until the user intervenes. Settings are CLI-flag/GUI-session only, not persisted to wallet.dat — same reasoning already established for nMineMode/strParticipantPool in db.cpp's LoadWallet: a saved value would silently override whatever was just requested on the command line, with no error. Thread-safety was the real design constraint here, not the settings. BitcoinMiner() is invoked once per thread launch (ThreadBitcoinMiner doesn't loop), and the solo-mining hot loop's exit condition is only `while (fGenerateBitcoins)` — it never re-checks nMineMode. Naively flipping nMineMode out from under a running thread would leave it mining solo forever regardless, while a second thread started alongside it in participant mode — two mining threads at once. Both transition directions instead follow the same shape: the thread that's currently running is the one that decides to stop, spawns its own successor (via the existing _beginthread(ThreadBitcoinMiner, ...) call the GUI's Start Mining button already uses) with the new mode already set, and only then returns, ending itself. No other thread is ever running when the mode changes, in either direction. The auto-return check lives at the very top of the solo loop's outer iteration, before that iteration's RandomX VM is created — the one place in the loop where returning early needs no cleanup at all. g_fAutoFallenBackToSolo (whether the current solo session exists *because of* an automatic fallback, vs. the user picking solo deliberately) is cleared on any manual mode change from the GUI — otherwise a stale flag from an old auto-fallback could later hijack a solo session the user chose on purpose, switching it back to participant mode against their explicit choice. Compiled all three touched files for real against the actual headers (main.cpp, gui.cpp, main_gui.cpp) before committing, given this touches mining thread control flow directly -- clean across all three.
Prompted by a maintainer's root-cause of Bitflash-sh#5 (the segfault at block 1062): %I64d, an MSVC-only specifier inherited from the original Bitcoin 0.1 codebase, desyncs vararg consumption on glibc -- the compiler had been warning about it the whole time, and -w (used project-wide, here too) silently threw the warning away. We don't have %I64d anywhere in our tree (checked exhaustively, zero hits outside properly-guarded vendored ImGui code) -- but the lesson is about the suppression, not that specific bug: -w hides exactly the warning class that would have caught it, project-wide, permanently. -Wformat -Wformat-security -Werror=format added back after -w on both Makefiles, narrow enough to not fight the blanket suppression everywhere else. Verified clean on Linux by actually compiling every major source file (main/net/gui/main_gui/util/db/rpc/dashboard.cpp) plus the vendored FTXUI source with these flags live -- zero hits. Windows verified more narrowly: installed a real mingw-w64 cross compiler in this sandbox and confirmed %lld (what this codebase actually uses throughout) doesn't false-positive under it, but a full cross-compile of the real Windows build wasn't feasible here (would need a mingw-built OpenSSL/websockets/curl chain this sandbox doesn't have) -- lower confidence than the Linux side, worth watching the first real Windows CI run after this lands.
…r no-op fix) My previous attempt (adding -Wformat -Werror=format after -w in CXXFLAGS) did nothing. Verified by direct test: a genuine format bug (%d given a long long) still compiles clean with -w present in the command line, regardless of where -Wformat/-Werror=format are placed relative to it. -w overrides them unconditionally. Ported and adapted Bitflash-sh#44 properly instead, which diagnosed this exact problem against the same root cause (the Bitflash-sh#5 segfault) and fixed it correctly: -w removed entirely, replaced with -Wno- deprecated-declarations/-Wno-write-strings/-Wno-narrowing/-Wno- unused-result (the categories this tree's own code legitimately triggers) plus -Wformat -Wformat-security -Werror=format. Also required BF_FORMAT annotations on strprintf/my_snprintf/ErrorImpl/ OutputDebugStringF -- printf itself is #defined to the last of those, and GCC only checks format strings for functions it knows are printf-like, so without the attributes -Wformat had nothing to inspect regardless of -w. gnu_printf archetype specifically: MinGW's alternative, ms_printf, models legacy msvcrt and would reject %zu (used here, supported by UCRT) while accepting %I64d (the specifier that actually crashes on glibc) -- exactly backwards for what this needs to catch. Collapsed PRId64/PRIu64/PRIx64 to one spelling (dropping the MSVCRT branch that produced %I64d in the first place), needed for both platforms to share one format archetype. Adapted rather than copied where our fork has diverged: PrintBlockTree doesn't exist here (removed earlier this session as dead code, so its fixes in the upstream diff don't apply), and `error` here is a macro over ErrorImpl (ours), not a plain function (theirs). Removing -w surfaced real bugs beyond what the upstream PR's diff covers, since this fork carries additional code: a SECP256K1_STATIC macro-redefinition warning in nostr.cpp/btfaddr.cpp (guarded the same way upstream did), and one more size_t-into-%d mismatch in util.cpp's AddTimeData logging that wasn't in their tree. Verified by actually compiling every source file in the tree with the real flags (not just the ones this diff touches) -- main/net/gui/ main_gui/util/db/rpc/script/update/market/dashboard.cpp all clean. nostr/btfaddr/compactblock/randomx_pow.cpp couldn't be verified this same way here (their C dependencies -- secp256k1, libsodium, RandomX -- aren't vendored in this sandbox, unrelated to this change), but nostr.cpp and btfaddr.cpp got the one warning each that could be checked without those headers (the macro redefinition, both fixed).
…itflash-sh#46) Issue Bitflash-sh#40's first half: a wallet.dat copied out on its own doesn't open anywhere else, even though it's byte-identical and holds the same keys. Root cause chain, from the bottom: _endthread() is pthread_exit() on POSIX, which glibc implements by throwing abi::__forced_unwind through the stack -- not a real exception, and catching it without rethrowing is a fatal error by contract. CATCH_PRINT_EXCEPTION's bare catch(...) did exactly that, so every Linux shutdown aborted mid-way rather than completing. That mattered because Berkeley DB stamps every page with a log sequence number tied to the environment that wrote it; DBFlush(true) is what clears that (via lsn_reset) before the file is safe to move, and DBFlush has been declared since 2009 and never actually called on either shutdown path. So the wallet was never released from its directory, and the one file anyone would think to back up was the one file that couldn't travel. Three fixes: - Rethrow abi::__forced_unwind rather than swallowing it (util.h), and call the now-effective DBFlush(true) on both shutdown paths (headless and GUI) in main_gui.cpp. - Adopt orphaned wallet.dat/blkindex.dat on open: a freshly created database/ environment beside pre-existing .dat files means those files came from elsewhere, so their sequence numbers get reset. Recovers wallets already stranded by older builds. - BackupWallet(), reachable as /backupwallet=FILE: checkpoints, copies, clears the copy's sequence numbers. Bitcoin 0.1.0 had a backup command; this tree hasn't had one since the wxWidgets removal. DBFlush's own lsn_reset call is now wrapped too (its txn_checkpoint already was, from this fork's earlier BDB-exception fix) -- letting it throw on the way out is how this whole chain started. Adapted rather than copied: this fork's dead-code sweep earlier this session correctly removed FileExists as unused at the time -- it has a real caller again now (both the orphan-adoption check and BackupWallet), restored scoped to db.cpp rather than re-exposed in util.h, since that's the only file that needs it. DBFlush's txn_checkpoint wrapping already existed here from a prior fix; only lsn_reset needed adding. Updated the README's backup guidance to point at /backupwallet as the correct way to do this, rather than copying the data directory by hand. Still not addressed, and the larger half of Bitflash-sh#40: keys are generated on demand rather than derived from a seed, so a backup is a snapshot and coins paid to addresses created after it aren't in it. That's a wallet-format change and its own piece of work, matching what upstream's own PR description says about scope. Verified by compiling every touched and dependent file for real against the actual headers, with the format-checking flags from the previous commit live -- clean throughout.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This picks up from the earlier PR (closed), rebased on current
main— it now includes the 1.2.1/1.2.2 hardening series (orphan cap, sigops cap, Stratum bounds, string-resize fix), which the earlier version predated. Grouped by area below; happy to split into separate PRs per area if that's easier to review, per your earlier offer.Consensus / security
mainnow.[1]instead of["EVENT",...]crashed the read thread (j[0].get<string>()with no type check) — same bug class as nostr: bound what a relay can make the node allocate #17, ported to our libwebsockets-based reader since the original frame-size-cap fix doesn't apply after the WebSocket rewrite below.Pool / relay stability (the "broken before" work)
Networking
0.0.0.0instead of agethostname()-derived address.Compact block relay
Auto-update + release automation (new since the earlier PR)
REPOand the signing public key are mandatory build-time arguments now, not defaults — and the key is never hardcoded.tools/generate_update_key.shgenerates a keypair locally; whoever merges this should run it themselves and use their own key, not carry mine over — that was your first concern last review, and this is the actual fix for it, not just a promise.make linux-self/make windows-selfbuild the identical app with the updater compiled out entirely (BITFLASH_NO_UPDATER) — no repo tie-in, no key required, no GitHub call ever made, for anyone who doesn't want auto-update at all.preflightjob validates the signing key/pubkey pair before either build runs, so a bad secret fails in seconds instead of after a 5-minute build.