Pool/relay stability fixes, compact block relay, and signed auto-update + release automation - #31
Open
Vic-Nas wants to merge 240 commits into
Open
Pool/relay stability fixes, compact block relay, and signed auto-update + release automation#31Vic-Nas wants to merge 240 commits into
Vic-Nas wants to merge 240 commits into
Conversation
…ort forwarding needed
…icipant Stratum client
Transactions only ever showed a generic 'Sent'/'Received'/'Generated' label -- no counterparty at all. For a Sent tx, now shows the actual payee address(es) (excluding the change output, which is ours). For a Received tx, shows which of your own addresses got paid -- a UTXO transaction has no 'who sent this' field the way a bank transfer does, and deliberately not tracing inputs back to try to answer that: this app's whole point is anonymous .btf addressing, and that trace wouldn't reliably resolve to anything meaningful anyway. Uses the address book (AddressBookName) wherever a name exists, so a payee you've named shows as 'Alice (BTE9bEv9Tb...)' instead of a raw address -- the whole reason the book needed transactions wired to it in the first place, not just the peers panel.
Round-share accounting on the operator side was already correct -- m->roundShares (and gStats.roundShares) reset on every new block, whether the pool found it or someone else did, and always did. The actual gap: gParticipantSharesSent/Accepted in the GUI are lifetime session counters (never reset, not per block, not per pool switch), labeled just "Sent X shares" with nothing distinguishing them from a per-round count. Read as "shares this round" -- which it isn't -- the number looks wrong, even though it was working as designed. Split into two clearly-labeled lines: session totals (explicitly labeled "this session" now), and a separate "This round: X of Y shares" line sourced from the pool's actual round accounting. The round-share data rides the existing mining.get_owed query (already sent every 60s) rather than a new poll -- two more integer fields on a message already being exchanged, not an additional round trip. Lets a miner estimate their proportional share of an unmatured block's reward without waiting for payout, and without having to trust the operator's math after the fact: they can see the same numbers the payout split is computed from.
Corrects course from an earlier idea (checking a found block's coinbase against a "pool address") that turned out not to work: pool block templates use a fresh throwaway key per block (key.MakeNewKey()), not a stable operator address, so there's nothing persistent to check a coinbase against. The reward only ever reaches a miner through a later, separate payout transaction -- which this app already tracks on both ends. The actual buildable check: mining.get_owed already tells a participant an amount owed and the height it's expected to mature at. Comparing that across polls needs no new protocol and no new trust -- if the previous poll said a payout would mature at height H, the client's own already-synced chain has since passed H, and the owed figure hasn't moved, the pool either forgot or never sent it. That's directly falsifiable using numbers already flowing through the app, compared against each other and against chain height the client independently verified itself. Surfaced in the GUI as a visible orange warning once noticed, with how long it's been. Also bundles pool_miners (gStats.authorizedMiners, already computed) into the same get_owed response -- cheap context next to round-share counts, no extra round trip.
Generates a readable random nickname (adjective-noun-number, e.g. "brave-falcon-42") on first run, persisted via wallet.dat like other settings. Editable in Options, with a Randomize button to reroll. Deliberately kept OUT of the signed .btf descriptor rather than added as a new field there: that signature covers pubkey/enc/meeting_node/ created specifically (see DescriptorDigest), and every verifier -- including implementations we don't control -- would need to compute the digest the same new way to keep validating existing descriptors. Not worth that risk for something purely cosmetic. Instead, a new lightweight "nick" P2P message, sent once per connection right alongside the existing peer-exchange announcement. Unauthenticated by design: same trust level as an IRC nickname, not a cryptographic claim, and treated that way on receipt -- only ever used as a *suggested* address-book label (via AddressBookName/ SetAddressBookName, already wired up), only when the peer's .btf address is actually known (outbound connections only) and no name already exists for it. Never overwrites a name the user picked themselves.
…resses Pulled from Bitflash-sh#23 ("Deferred from the audit... change-key reuse"), which had already correctly diagnosed this and explicitly rejected the tempting shortcut: calling GenerateNewKey() directly for change or fresh addresses is unsafe without a keypool, because a key generated at the moment of use exists nowhere until that instant -- a wallet.dat backup taken one second earlier has no idea it exists. Restore that backup after such a transaction and the coins are still on-chain, but nothing in the restored wallet knows to look for them. That turns a privacy fix into a fund-loss bug, silently. Adds a real keypool (db.h: CKeyPoolEntry, wallet.dat record type "pool"; main.cpp: mapKeyPool/TopUpKeyPool/GetKeyFromPool): a target of 30 keys, pre-generated and written to wallet.dat *before* ever being handed out, topped back up immediately whenever one is consumed. Same resolution Bitcoin settled on for the same tension, not a novel design. Two things now draw from it instead of the old unsafe patterns: - CreateTransaction's change output. It used to reuse a pubkey pulled directly off one of the spent inputs ("Use the same key as one of the coins") -- which makes the change output trivially identifiable (whichever output *isn't* one of the input keys is obviously the real payment) and links every transaction that ever touches that key into one on-chain cluster. Exactly what this project's .btf addressing and no-IP-advertised network layer exist to prevent, undone right back on-chain by wallet behavior. Now GetKeyFromPool() instead. - Participant mining payout addresses. mining.authorize sends the payout address and the persistent .btf worker identity together, explicitly, to whichever pool is connected to -- so reusing the wallet's one default address meant every pool a miner ever used independently learned the exact same {.btf identity, payout address} pair, without needing to collude with each other. GetOrCreatePoolPayoutAddress() now draws a keypool key per pool (persisted in pool_payout_addrs.json, keyed by the pool's .btf address) -- stable across reconnects to the *same* pool (the operator needs that continuity to track an ongoing balance), but distinct across *different* pools. TopUpKeyPool() runs once at startup, right after LoadWallet(), so the pool is populated (and backed up) before anything could possibly need to draw from it.
…ort of Bitflash-sh#34/Bitflash-sh#38) Berkeley DB reports failure by throwing, and nothing on the load path caught it -- an unreadable blkindex.dat or wallet.dat (written by a different platform's Berkeley DB, truncated, a version mismatch) unwound out of main() into std::terminate and abort(). On Windows that surfaces as STATUS_STACK_BUFFER_OVERRUN (0xC0000409) inside ucrtbase.dll: no message, no log line past "Loading wallet...", and a faulting module that has nothing to do with the actual problem. This is directly relevant to the crashes reported on this fork this session -- same failure class, previously invisible. Three layers, matching upstream: 1. main_gui.cpp: try/catch around LoadBlockIndex() and LoadWallet(), printing the actual reason and exiting cleanly instead of terminating. 2. db.cpp CWalletDB::LoadWallet: try/catch around the whole record-reading loop, tracking the last record type read so the error message says what it choked on. Adapted rather than copied -- our loop also handles a "pool" record type (the keypool added this session) that upstream's doesn't have. 3. db.cpp cleanup paths: every dbenv.close(), pdb->close(), txn_checkpoint(), and log_archive() wrapped in try/catch so environment teardown after a failed open doesn't throw a second exception into std::terminate on top of the first. Deliberately a hard failure rather than skipping the bad record: a partially loaded wallet is worse than one that refuses to open, because the balance looks plausible while keys or transactions are silently missing.
… SockReadN Swept for functions with zero callers anywhere in the codebase. Found one small one (SockReadN in btftunnel.cpp -- the read side actually uses RvReadN/raw recv() instead, its write counterpart SockWriteN is real) and one much larger one: Satoshi's original, never-shipped Bitcoin 0.1 "marketplace" prototype (products, reviews-as-currency, publish/subscribe gossip), carried forward through every fork including this one without ever being reachable. Confirmed dead two ways before removing anything: - No P2P message handler ever existed for "product", "publish", "pub-cancel", "subscribe", or "unsubscribe" -- ProcessMessage has no case for any of them, so even a peer that tried to speak this protocol would get nothing back. - Nothing in the application ever calls AdvertInsert, Subscribe(), or any of the Advert*Publish templates. CNode::Subscribe -- the only thing that could ever set vfSubscribe true -- has zero callers, so IsSubscribed/CancelSubscribe/AnySubscribed never did anything meaningful either, confirmed independent of the network-side check. Removed: CProduct class and its mapProducts/mapMyProducts storage (market.h/market.cpp), the three Advert*Publish templates and PUBLISH_HOPS (net.h), CNode::Subscribe/IsSubscribed/CancelSubscribe and the vfSubscribe member (net.h/net.cpp), MSG_PRODUCT/MSG_TABLE and their ppszTypeName entries (net.h), the corresponding cleanup code in CNode::Disconnect() and the dangling MSG_PRODUCT case in main.cpp's AlreadyHave(). NOT removed: CUser, CReview, AddAtomsAndPropagate, and the "review" P2P message handler in main.cpp. Unlike "product", "review" has a real, reachable ProcessMessage case (CReview::AcceptReview() runs on actual untrusted network input, even though nothing in this client currently sends one) -- removing a live wire-protocol message type is a compatibility decision, not a dead-code cleanup, so left alone. 292 lines removed, zero added, no dangling references (checked exhaustively across every .cpp/.h for every removed symbol).
Second pass from the same sweep as c3a6ac2, smaller and independent of each other rather than one subsystem. Each confirmed to have exactly one mention anywhere in the codebase (its own definition) -- no callers, and none used as a function pointer/callback either (checked specifically, since that pattern doesn't show up as a normal call site and was the reason the earlier CrashHandler/PayoutThreadFn/ etc. false positives showed up in the initial grep pass). - ExtractHash160 (script.cpp/.h) -- unused address-extraction helper. - ParseString, FileExists (util.cpp/.h) -- FileExists is unrelated to FileReadable (a separate local helper in nostr.cpp actually used for the CA-bundle check); confirmed distinct before removing. - PrintBlockTree (main.cpp/.h) -- debug tree-printing helper, no caller and no CLI flag or hidden trigger found for it either. - AbandonRequests (net.cpp/.h) -- request-cancellation helper from the original Bitcoin 0.1 GUI's async-dialog pattern, which this codebase doesn't use. - ReadOwnerTxes (db.cpp/.h, CTxDB method) -- unused historical lookup by owner hash160. 197 lines removed, zero added. Exhaustive grep across every .cpp/.h for each removed symbol confirms no dangling references.
Contributor
Author
|
Another round since the last update. Summary: Wallet correctness/privacy — pulled directly from your own audit issue (#23), which had already correctly diagnosed the problem and rejected the tempting shortcut (calling a raw new-key generator directly is unsafe without a keypool, since a key created only at the moment of use isn't covered by a backup taken a second earlier). Added an actual keypool: pre-generated, backed-up-before-handed-out, target size 30. Used it to fix Crash fix — ported #34/#38 (BDB exceptions unwinding into Dead code — swept for zero-caller functions across the tree. Found and removed the entire never-shipped Bitcoin 0.1 marketplace subsystem ( Smaller stuff — random node nicknames (unauthenticated, kept deliberately out of the signed descriptor rather than extending that format), a Peers/Address Book panel, transaction list now shows actual counterparty addresses instead of just "Sent"/"Received", round-share visibility and an overdue-payout detector for participant miners (self-auditable against the pool's own numbers over time, no new trust required), and a top-level Windows crash handler (minidump + logged reason, where previously a crash just vanished with nothing to look at). On the rebase: I looked at it, and honestly it's more than I can take on right now -- ~200 diverged commits, several with real structural differences (the register/wait split adapted to concurrent multi-relay registration vs your single-relay round robin, the peer cache/pex work, etc.), and getting conflict resolution wrong there is worse than leaving the PR as an "adapted, not merged" diff. Understand if that makes review harder on your end. Happy to help with specific pieces of it if that's more tractable than the whole thing at once. |
CDB::Write/Erase are protected, only reachable from within a CWalletDB member -- same reason WriteName/EraseName exist as wrappers rather than being called directly. Missed this for the new keypool storage; added WritePool/ErasePool wrappers matching the existing pattern instead of reaching into the base class from main.cpp.
Update check itself is unchanged -- still the same one-shot background thread on launch (StartUpdateCheck in RunGUI), not a new thread and not a polling loop. What changed is presentation and what happens when nothing's found: - An available update now shows as a persistent inline band right under the menu bar instead of a modal popup -- visible without blocking anything, with "Update Now" / "Dismiss" right there. Same underlying apply path (StartUpdateApply, explicit click required, fails closed on a bad signature) as before. - Dropped the "up to date" popup entirely. It used to fire from a manual "Check for Updates" menu click, which no longer exists -- showing it on every automatic launch instead would just be a nag nobody asked for. Silence is correct when there's nothing to say. - A -self build (no UPDATE_REPO/UPDATE_PUBKEY compiled in) never shows anything here at all: Update_CheckLatest is a no-op stub in that build, so this is already exactly as unintrusive as it needs to be for someone who built it themselves and isn't concerned with updates -- nothing to add for that case, it already falls out of the existing -self design. Menu bar simplified to three flat items -- More, Mine, About: - File dropped entirely (Exit was the only other reason it existed, and the window's own close button already does that -- redundant menu entry for something the OS already provides). - View > More collapsed to a single top-level "More", not a one-item submenu. - Options renamed to Mine (menu item and dialog title both), matching what's actually in it -- mining mode and pool settings are the primary content, transaction fee and nickname ride along.
…bar" This reverts commit 4db1c6e.
…the launch check/modal Reverted the previous change entirely -- the startup check + modal box stays exactly as it was. Adds what was actually asked for: an 'Up to date' / 'Update available' segment in the status bar, which already redraws every frame regardless (an operation that already happens). Reads the same g_showUpdateDialog/g_updateInfo state the one existing launch-time background check already populates -- no second check, no new thread. Turns into 'Update available: vX.Y.Z' with an inline Update button when the existing check found something; otherwise reads 'Up to date' once the check has finished, or 'Checking for updates...' in the brief window before it has.
Reverting the update-band change wholesale also threw out the unrelated menu bar work (More | Mine | About, no File, no View submenu) bundled in the same commit -- should have reverted only the update-flow part. Reapplied here on top of the current state: modal + status-bar indicator both untouched, menu bar back to flat More/Mine/About.
Previous fix only redisplayed the one launch-time check's result somewhere new -- missed the actual point, which was periodic re-checking so a node running for days still notices something critical without needing a restart. Gated inside the render loop's existing per-frame tick (same place RefreshWallet's 120-frame gate already lives) rather than a new timer or thread: a cheap GetTime() comparison every frame, and only every 4 hours does it actually call StartUpdateCheck() -- the exact same one-shot detached-thread function the launch check already used, not a second persistent thread. Negligible cost between actual checks, no more than a plain integer subtraction and comparison.
Same path OutputDebugStringF (util.h) already writes to. ShellExecuteA on Windows (shell32 was already linked), xdg-open on Linux -- opens in whatever the system's default handler for a .log file is rather than this app trying to render/tail it itself.
Replaced with colons, semicolons, or commas depending on context. No wording changes beyond the punctuation itself.
…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.
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.