Regtest upgrade-activation controls and daemon fixes - #289
Open
MorningLightMountain713 wants to merge 10 commits into
Open
Regtest upgrade-activation controls and daemon fixes#289MorningLightMountain713 wants to merge 10 commits into
MorningLightMountain713 wants to merge 10 commits into
Conversation
generate, getblocktemplate, and the BitcoinMiner thread dereferenced the CReserveScript returned by ScriptForMining without a null check. When no handler sets the script -- no wallet and no -mineraddress (e.g. -disablewallet on regtest) -- the shared_ptr is null and the ->reserveScript.size() check crashes the daemon. Add the upstream null guard so these paths report a clean error instead. generate is regtest-only and the primary beneficiary: regtest block production (including PON) no longer crashes under -disablewallet. The other two paths carry the identical latent dereference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PON shares a branch id with several earlier upgrades, so -nuparams cannot target it, and regtest hardwires PON on (activation height -1, which IsPONActive treats as always-active). That leaves regtest unable to mine PoW blocks -- so under PON the coinbase is redirected to the dev-fund address and a test wallet can never be funded by mining. Add a regtest-only -ponactivation=height option that sets the PON upgrade's activation height directly. Passing a high value runs regtest in PoW mode, where the coinbase pays the wallet, so it can be funded normally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ACADIA gates whether createrawtransaction emits Overwintered transactions, whose signatures carry a consensus branch id. In regtest it defaults to no activation and shares branch id 0x76b809bb with several other upgrades, so -nuparams cannot single it out (the matcher stops at the first id match). This regtest-only flag sets ACADIA's activation height directly, mirroring -ponactivation, so consensus-branch-id signing can be exercised. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
blockToDeltasJSON pushed the output "address" key twice -- once inside the IsValidDestination guard and again unconditionally -- producing a malformed JSON object (and an empty-string address for non-standard outputs). Drop the unconditional push so each output emits "address" at most once, only for valid destinations, mirroring the input side. Output-only change to the insightexplorer getblockdeltas RPC; no consensus impact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The wallet's GetScriptForMining returns nothing when -mineraddress is set, and with -disablewallet no interface is registered, so the mining coinbase script came back empty and -mineraddress was silently ignored by generate/getblocktemplate/the internal miner. Fall back to GetScriptForMinerAddress in CMainSignals::ScriptForMining when no listener produced a script. Additive: paths that already produced a script (wallet enabled, -mineraddress unset) are unchanged. Not consensus -- coinbase payout policy only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every post-Sprout Flux upgrade shares branch id 0x76b809bb and the -nuparams parser broke on the first branch-id match, so a specific later upgrade (e.g. ACADIA, PON) could not be activated for testing. Accept an upgrade name ("PON") or UpgradeIndex value ("10") in addition to the branch-id hex; the selector is only treated as an index when it parses fully as an integer, so a branch id like 5ba81b19 still falls through to the (loud) invalid-upgrade error rather than silently matching a leading-digit index. regtest-only (the whole block errors on other networks); branch-id form unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CZMQAbstractPublishNotifier::nSequence had no initializer. The factory builds notifiers with new T(), which only zero-fills classes without a user-provided constructor, so the hashblock/hashtx/hashblockheight/chainreorg notifiers happened to start at 0 but the fluxnodelistdelta and fluxnodestatus notifiers (which declare their own constructors) started their per-topic sequence from indeterminate memory. Consumers relying on the documented upcounting sequence to detect dropped messages on those topics got a garbage base. A default member initializer fixes every notifier regardless of its constructors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AMQPSender::dispatch() threw "amqp connection is not active" whenever a publish happened while the proton connection was not yet active -- which occurs during startup and on transient reconnects. The message has already been queued by add_message(), so it would be sent normally once the link opens and credit arrives (on_sendable). The notification interface treats any failed send as fatal: a single false return from a notifier causes it to be Shutdown() and erased, silently stopping all further notifications of that type for the life of the daemon. Under load this manifests as a node that publishes every hashblock but, after one early transient blip, no further hashtx -- the hashtx notifier is gone. Leave the queued message to flush from on_sendable instead of throwing, so a transient inactive window no longer permanently disables a notifier. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A rescan (e.g. after z_importkey) persists each note's witness deque via CWalletTx::WriteToDisk, but nWitnessCacheSize is persisted only by SetBestChain, which a rescan does not call. A node restarted between those two writes reloads a stale, smaller nWitnessCacheSize alongside the larger note witness deques, and the next connected block aborts on the nWitnessCacheSize >= witnesses.size() assertion in IncrementNoteWitnesses (CopyPreviousWitnesses). Restore the invariant at load by raising nWitnessCacheSize to the largest reloaded witness deque. The witnesses are correct, so the spendable (front) witness is preserved; a healthy wallet (counter and deques persisted together by SetBestChain) is unaffected, and an already-affected wallet is recovered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Without this, a lone -connect=0 makes the open-connections thread dial the literal hostname "0" forever: harmless name-resolution failures normally, but with -proxy configured the name is handed to the SOCKS5 proxy on every attempt. Upstream Bitcoin special-cases the value to mean no automatic outbound connections at all; adopt the same rule (exactly one -connect entry equal to 0) and document it in the option help. Manual connections via addnode and inbound connections are unaffected. The regtest QA suite sets connect=0 on every node so that a test that isolates a node by restarting it cannot be broken by the daemon spontaneously redialing a peer it learned through an earlier addnode handshake (addrman persists such peers in peers.dat). Integration-covered by that suite; the open-connections thread has no unit-test seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Adds regtest flags for controlling network-upgrade activation and fixes a set of independent daemon defects. All changes are non-consensus; the ten commits are self-contained and reviewable individually.
Regtest activation controls
Regtest defaults every network upgrade to
NO_ACTIVATION_HEIGHT, so tests need a way to opt in per upgrade:-ponactivation=<height>sets the PON activation height on regtest. Setting it high leaves regtest in PoW, where the coinbase pays the wallet and tests can fund themselves by mining; setting it low activates PON rules at that height, and a test can cross the activation boundary.-acadiaactivation=<height>activates the ACADIA upgrade (Sapling and Overwintered transactions) at the given height on regtest.-nuparamsaccepts an upgrade name or index in addition to a branch id. Several upgrades share a branch id, so the branch-id form cannot target them individually.Fixes
DecrementNoteWitnesses. The size is clamped to the actual cache on load.getblockdeltasno longer emits a duplicateaddresskey in its output objects.-mineraddressis honoured when no other mining script is provided.-connect=0means no automatic outbound connections, matching upstream Bitcoin. Previously the value was treated as a hostname and dialed forever (and handed to the SOCKS proxy when one is configured). Manualaddnodeand inbound connections are unaffected; the option help documents the semantics.No ordering relationship with other open PRs.
🤖 Generated with Claude Code