Cut suite idle time and keep block templates on a stripped pool

[rearden-grok]

Summary

• P2P shutdown wakes dial, accept, and connect-retry instead of sitting out the 750 ms grace. --max-run-secs is a real deadline. Journey builds use a 250 ms redial, a 50 ms SAM retry, and a 100 ms SV2 cooldown. A node binary built with those test-hooks warns at startup. Release, nix, and cargo build -p rbitcoin-node keep 2 s, 500 ms, and 1 s.
• IBD assign-stop is a field on IbdConfig. Production still reads RBITCOIN_BLOCK_QUEUE_BYTES / _GB once at startup. Tests pass the cap in and no longer take the process-wide env lock.
• NODE_COMPACT_FILTERS is read from the peer hub. There is no process-wide flag. The hub OP_TRUE signet generate test mines at regtest difficulty. Accept still requires the BIP325 solution.
• Default journeys are one test binary. QC fails when a journey file is not wired in. Library crates with no doctests set doctest = false. The node, cli, and journey-node bins skip an empty test harness. Test dependencies compile at opt-level 2. scripts/coverage.sh no longer builds the workspace a second time before llvm-cov. A stalling inbound under coverage gets 15 s to leave.
• getblocktemplate, getrawmempool, and generate stay off while the pool is not maintained. Relay off (still catching up) returns -10. -blocksonly returns a permanent error so a miner does not retry. generate during catch-up mines the coinbase only. Local sendrawtransaction on a blocksonly node still admits. Once relay is on, getblocktemplate and Stratum v2 build on the tip recorded after the pool strip. A block connect holds that mark from before the chain tip moves until the drop is stored. There is no chain scan under the mempool lock.

Test plan

• cargo fmt --all -- --check, ./scripts/ast-grep.sh, cargo deny check, cargo clippy --workspace --all-targets -- -D warnings
• Targeted pins: shutdown under 200 ms, --max-run-secs wake, SV2 cooldown, densify assign-stop, sv2_tp_bootstrap, relay-off and blocksonly RPC refusals, coinbase-only generate during catch-up, template follows the stripped tip, esplora journey waits for relay before it lists or mines the pool
• GitHub qc, test, windows, macos, coverage

Made with Cursor

[rearden-grok]

Disposition

Comment. I am not requesting changes. Required checks on 2ab9c09d7 are green, and the cargo test step was 131s (06:04:05Z–06:06:16Z). The findings below are production-policy cuts made so the suite would finish, plus one split that the journeys binary can still trip. A human should accept those timer changes on purpose before merge.

What holds up

Shutdown is the right fix. Dial, accept, and connect-retry were blocked on a recv or a 2s tick, so every shutdown burned the 750ms grace and then aborted. The watch wakes those three tasks. The grace is still there for a session that is still unwinding, and Drop still aborts. biased prefers the stop signal over another dial.

The accept change does not drop a live listener on a transient error. The old loop already did Ok(Err(_)) => break. The 200ms timeout arm existed to notice shutdown; stop.changed() replaces that arm. A real accept error still ends the listen task, as it did before.

--max-run-secs is an absolute Instant, recomputed on each select, and the tip loop still checks it before waiting. The new arm matters when RPC is not bound, because then nothing else wakes faster than the 5s perf tick. The unit test covers that.

IbdConfig::default still copies bq_assign_stop_bytes() once. for_test uses the constant, so parallel net tests do not take a process-wide env lock. Production catch-up builds the config with ..IbdConfig::default(), so the env cap is still what assign uses.

[profile.test.package."*"] opt-level = 2 does not optimize workspace crates. debug-assertions stays the test profile's. Coverage passed after the uninstrumented cargo build was removed, so the bins the suite execs are still built by cargo llvm-cov test.

The compact-filter CI failure (p2p_compact_hb_getblocktxn_and_orphan: NODE_COMPACT_FILTERS set before this node's filters caught up) is real, and the hub bit fixes the path that failed. Handshake, getnetworkinfo, and v1 addr responses read the hub. run_p2p sets the hub and the process static together.

Findings

1. I2P gives up much sooner. SAM_RETRY is 50ms, and the session loop tries 32 times (connect_session_dest). Worst case wait is about 1.6s, down from about 16s at 500ms. Forward and stream connect are 24 tries, about 1.2s instead of about 12s. The comment says 50ms is long enough for a router restarting its socket. The pause is not a spin, but the attempt budget no longer covers an i2pd that is not accepting yet. That is a mainnet behavior change made so a refused test session would not dominate the suite.

2. Remembered peers are retried every 250ms. claim_dial / is_target_live still skip a peer that is up or already dialing, so a black-holed connect is not stacked inside the 8s outbound timeout. A refused --connect or addnode fails immediately and is tried again on the next tick, about four times a second. Every tick also spawn_blockings redial_remembered, which resolves remembered hostnames even when the targets are already live. This used to be every 2s. Faster recovery is a product choice. Four connection attempts a second, and four resolver passes a second, is a lot to adopt because shutdown tests were slow.

3. SV2 budget cooldown is no longer a tighter bound than template checks. CONSTRAINTS_COOLDOWN went from 1s to 100ms, the same floor as MIN_TEMPLATE_INTERVAL. A client cycling coinbase budgets can now force a mempool-locked template build ten times a second. Tip events were already undelayed. The cooldown is not a conf key, so an operator cannot put the 1s ceiling back. The cross-surface SV2 chapter needed a short wait. That wait can be a test Duration without moving the production ceiling. The changelog states the new number. It does not say why a cycling client should be allowed to cost 10x more.

4. NODE_COMPACT_FILTERS still has a second copy. local_service_flags_for(..., None) and local_service_flags() read COMPACT_FILTERS_ADVERTISED. Feeler version (run_feeler_inner) and queue_addr_list still call local_service_flags(). One node per process stays consistent because run_p2p writes both stores. A second node in the journeys process does not: those two paths advertise the neighbor's bit. The new unit test sets the process static to true and clears it afterward. Any other rbitcoin-net test running in that window observes the bit. The integration failure is fixed. The split is how the next one will look.

5. The hub signet generate test no longer uses a signet chain. hub_op_true_signet_generate_one now takes regtest params and sets an OP_TRUE challenge. Accept still rejects a block that lacks the BIP325 push, and the test pins regtest bits, so the signet miner path is what ran. It does not pin ChainHub::generate_to_script against signet genesis bits (0x1e0377ae). That grind remains only in op_true_signet_block_meets_pow_and_challenge, which calls mine_op_true_signet_paying directly. A regression in mine_paying_block's choice of parent bits on a real signet tip would not fail the hub test.

6. New default journey files will not run. autotests = false plus one [[test]] is the right link. A later tests/*.rs that is not #[path]'d from journeys.rs (or the feature-gated overlay target) is silent. The module comment says so. It is still an easy miss, and doctest = false on every lib has the same shape for a future ```rust example.

7. Assign-stop stats can disagree with assign. Assign uses cfg.assign_stop_bytes, snapshotted when the config is built. Query::block_queue_stats still returns a live bq_assign_stop_bytes(). At process start they match. After that, changing RBITCOIN_BLOCK_QUEUE_BYTES changes the reported cap and not the cap assign uses.

Not treated as defects

The 750ms grace remains. In-flight session tasks are still aborted only after it. opt-level = 2 on dependencies is cached by rust-cache and does not turn off workspace overflow checks. Folding the asmap journeys into node_listen_and_exit would run them one after another inside an already long chapter. Leaving them as modules of the one binary is the faster shape. The thin-chain fee-history restart test checks the opposite of the warm esplora journey, so keeping it is reasonable. The height-on-disk beat on the esplora journey is the part that belonged there.

[rearden-grok]

32 session attempts at 50ms is about 1.6s of wait (forward and stream connect are 24 attempts, about 1.2s). At 500ms those budgets were about 16s and 12s. A SAM that is not accepting yet, which is normal while i2pd is starting, now fails the session. The pause avoids a spin. It does not preserve the old restart window. This constant is production, not a test knob.

[rearden-grok]

Live and in-flight targets are still skipped, so a black-holed dial is not stacked inside the 8s connect timeout. A refused --connect or addnode fails immediately and is tried again on the next tick, about four times a second. Each tick also resolves remembered hostnames on the blocking pool, including targets that are already up. This used to be 2s. Faster recovery is a product choice. Four attempts a second is a lot to take on because the suite was paying the old interval at shutdown.

[rearden-grok]

This used to be a tighter ceiling than template checks: one mempool-locked build per second while a client cycled budgets. It is now the same 100ms floor as MIN_TEMPLATE_INTERVAL, and it is not a conf key, so an operator cannot put the 1s ceiling back. The cross-surface chapter can pass a short Duration without moving this constant. The changelog records 100ms. It does not say why a cycling client should cost 10x more.

[rearden-grok]

Handshake, getnetworkinfo, and v1 addr responses use the hub bit. This None arm is still the process static, and feeler version plus queue_addr_list still call local_service_flags(). run_p2p writes both, so one node per process matches. A second node in the journeys process still advertises the neighbor's bit on those two paths. The unit test that sets the static and then clears it races every other rbitcoin-net test in that window.

[rearden-grok]

The three production timer cuts are out of the node binary in 22306e5.

test-hooks is off for rbitcoin-node. That build keeps a 2s --connect / addnode redial, a 500ms SAM retry (about 16s across the 32 session attempts), and a 1s SV2 budget cooldown. Crate tests compile the short intervals with cfg(test). Journeys turn test-hooks on, so the suite still uses 250ms, 50ms, and 100ms. Shutdown still wakes dial, accept, and connect-retry, so the 2s redial pass is not what shutdown waits on.

[rearden-grok]

Adversarial review of 530a77cb1 (second pass)

Disposition: Comment. Not ready to merge until the first item is fixed. CI is green. But the last commit's test change hides a getblocktemplate bug I reproduced locally. The other items are cleanup or hardening and do not block.

This pass covers the three commits since the first review (timer split, hub-only compact-filter bit, esplora wait). It also re-checks the build graph and test wiring for things the first pass did not look at.

Blocking

1. 530a77cb1 waits out a window where getblocktemplate serves transactions that are already mined.

generate calls drain_mempool, which calls MempoolHub::remove_for_block. That is a no-op while relay is off. In esplora_broadcast_visible_in_rpc_and_electrum the node is still in IBD when it mines block 107. Confirmed transactions stay in the pool until tip-follow wakes, sees the node has left IBD, and set_relay_enabled(true) runs purge_confirmed_on_chain. The new commit polls getrawmempool until that purge happens.

getblocktemplate has no relay or IBD gate. During that window it builds a template on the new tip from the stale pool. I added a temporary probe (not committed) that prints the transaction count from the longpoll spawn_wait_and_gbt_longpoll starts before generate:

run 1: PROBE longpoll_txs=34   <- the 34 txs block 107 just confirmed, on top of block 107
run 2: PROBE longpoll_txs=0
run 3: PROBE longpoll_txs=0

A miner that takes that template builds an invalid block. The behavior predates this PR: mine.rs and tx_relay.rs are untouched here. The faster suite made the race show up, and the last commit's poll now makes the test tolerate it instead of catching it.

Suggested fix (test first):

• Red: in TipWaiters::assert_woke_on_new_tip, assert that the woken longpoll template contains none of the txids getblock reports for the new tip. Today it only checks longpollid.
• Green: strip the mined transactions while relay is off. For example, generate_with_mempool could run the existing purge_confirmed_on_chain when !relay_enabled(), which keeps one strip path. Or the tip-follow loop could flip relay before the tip broadcast wakes RPC waiters. Either way, the getrawmempool poll should go back to a single read.

Non-blocking

2. Misattached doc comment. In service.rs L633–643, REDIAL_INTERVAL was inserted between OUTBOUND_CONNECT_TIMEOUT's doc comment and the const itself. The five lines about the 8s TCP/SOCKS/SAM bound now document the production REDIAL_INTERVAL, and OUTBOUND_CONNECT_TIMEOUT has no doc. Move the block back above OUTBOUND_CONNECT_TIMEOUT.

3. Leftover service-flag wrappers. After the hub-only change, local_service_flags_pruned has no production caller (tests only). local_service_flags() has one: the queue_addr_list fallback when session.peer_hub() is None. That fallback also drops NETWORK_LIMITED on a pruned node during shutdown. Collapse to one local_service_flags(pruned, Option<&PeerHub>) and update the three test callers (rule 9). The fallback can then pass (false, None) explicitly, or skip the send.

4. block_queue_stats().0 is dead and still reads the environment. Every caller now discards the first field: _reported_stop in assign.rs, _stop in confirm, _bq_budget twice in ibd/mod.rs, _budget in progress.rs. Each call still runs bq_assign_stop_bytes(), which is two std::env::var lookups under the process env lock, on every assign pass. IbdConfig::assign_stop_bytes is now the one owner. Return (bytes, count) and leave bq_assign_stop_bytes with its single production caller, IbdConfig::default.

5. test-hooks unifies into the node binary on a workspace build. cargo tree -p rbitcoin-node -p rbitcoin-test -e features -i rbitcoin-sv2 shows rbitcoin-sv2 feature "test-hooks". So cargo build --workspace [--release] gives a node with 250ms redial, 50ms SAM retry, and 100ms SV2 cooldown, which are the values rejected for production. Shipped builds are safe: release.yml, nix/rbitcoin.nix, and ci-os-smoke.sh all use -p, and default-members is the node crate only. A one-line warn! at node start when cfg!(feature = "test-hooks") would make this impossible to miss.

6. autotests = false drops new test files silently. All eight current files are wired into journeys.rs, and overlay is its own [[test]]. A new tests/foo.rs would never compile or run, and nothing would report it. A qc script check that every crates/rbitcoin-test/tests/*.rs appears in journeys.rs or a [[test]] entry would close this.

7. The real-binary smoke branch never runs (pre-existing), and the coverage comment says it does. scenarios.rs workspace_bin hard-codes ../../target/<profile>/rbitcoin-node behind if node.exists(). CI sets CARGO_TARGET_DIR=target/dev and coverage uses target/cov, so that branch has been skipped everywhere, on master too. This PR's scripts/coverage.sh comment says the suite builds "the node/cli bins when a test asks". No test asks: there is no CARGO_BIN_EXE_rbitcoin-node, and with test = false and no tests/ dir, cargo test does not build that bin. Delete the dead branch or fix the comment, so neither suggests coverage exists that doesn't.

8. Accept errors still stop the listener (pre-existing, out of scope). Err(_) => break in spawn_inbound_accept behaves like the old Ok(Err(_)) => break. A transient EMFILE or ECONNABORTED ends inbound accept for the rest of the process. This is a separate fix; I'm only noting it because this PR touched the match.

Verified sound

• Shutdown watch. Dial, retry, and accept all select! on stop_tx, and Drop sends too. A dropped sender makes changed() return Err, which also breaks. The 2s production redial no longer costs anything at shutdown.
• Timer split. One name per constant, two compiled values, same call sites. cargo tree -p rbitcoin-node -e features shows no test-hooks. The house pattern matches OUTBOUND_CONNECT_TIMEOUT. No Sv2TpConfig field came back.
• Compact-filter bit. The process static and the free setter are gone. Version (application_handshake), feeler, addr response, self-announce, getnetworkinfo, and getnodeaddresses all read the node's own PeerHub. run_p2p writes only node.peers, both immediately and from the write-behind callback. The two-hub unit test needs no save or restore.
• Assign-stop field. Production reads the environment once in IbdConfig::default. Every former env-mutating IBD test now sets cfg.assign_stop_bytes. The query crate's own env tests keep their local ENV_LOCK.
• doctest = false. Every fenced block in those libs is text, so no doc example is lost. The node, cli, and journey-node bins contain no #[test].
• Journeys wiring. All eight default files are #[path] modules, overlay stays feature-gated, and the doc and script selectors were updated (consensus_rules::, cross_surface::, scenarios::consensus_).
• SV2 config. Grace and interval are Duration. Conf still parses whole seconds with the same bounds. The test-only sub-second values come from struct fields, not conf.
• Signet hub test. The regtest-bits parent with an OP_TRUE challenge still requires the BIP325 solution on accept, and the real signet target stays pinned by op_true_signet_block_meets_pow_and_challenge.

[rearden-grok]

Thanks. All eight notes are addressed on 1e505c37e.

Templates were offering transactions the new block had just confirmed. That was real, and I could reproduce it: with relay still off, generate left the mined transactions in the pool, and a getblocktemplate longpoll woken by the new tip sometimes put all of them in the next template. Two changes close it. Every template now drops transaction ids that are already confirmed on the tip, so that window cannot hand a miner a block of duplicates. And generate purges those transactions before the RPC returns, even while relay is off, so getrawmempool is empty on the next read. Enabling relay also purges before the flag flips, which was the other way the same window opened. The esplora journey is back to a single mempool read, and it checks that the longpoll template shares no transaction with the block it just woke for. I ran that journey three times after the fix.

The redial comment was sitting on the wrong constant. The 8 second connect-timeout note is back above OUTBOUND_CONNECT_TIMEOUT. REDIAL_INTERVAL has its own short note.

Service flags are one function. local_service_flags(pruned, hub). The pruned wrapper and the _for wrapper are gone. Callers that have no hub pass None, which means compact filters are off.

Queue stats no longer re-read the environment. block_queue_stats returns (bytes, count). The assign-stop is still read once, when the IBD config is built.

A workspace build that picks up test-hooks now says so. The node logs a warning at startup if the net or SV2 crate was compiled with that feature, because that is the build with the short redial, SAM retry, and SV2 cooldown. cfg! on the node crate itself cannot see a feature that was turned on only in a dependency, so the check lives in those crates. Release, nix, and cargo build -p rbitcoin-node do not trip it.

A new journey file can no longer vanish. scripts/journeys-manifest.sh runs in the qc job. It fails if a file under crates/rbitcoin-test/tests/ is neither a module of journeys.rs nor its own [[test]] entry.

The real-binary smoke never ran, so it is gone. It looked for rbitcoin-node under target/debug, and CI builds into target/dev, so the branch was skipped everywhere. The coverage script comment no longer says the suite builds that binary. Node and cli smoke still go through cli_main.

A transient accept error no longer stops listening. Connection reset, an interrupted accept, and a full file-descriptor table (EMFILE / ENFILE) log and retry after a short pause. Any other error still ends the listen loop, so a closed socket does not spin. There is a unit test for that split.

[rearden-grok]

Coverage failed on p2p_timeout_getaddr_and_keepalive_ping: the stalling inbound was still in the peer table 3 seconds after the disconnect was requested. The regular test job passed. I raised that wait to 15 seconds. It still returns as soon as the peer is gone; the extra time is only for an instrumented run.

[rearden-grok]

Disposition: comment. Head 171f8e94d. Required checks on that commit are green (qc, test, windows, macos, coverage). This is a re-review of the whole branch, including the follow-ups since the last comment.

The suite-time work holds up. One journeys binary, doctests off where there are no examples, empty bin harnesses skipped, dep crates at opt-level 2 with line tables kept on workspace crates, and the second uninstrumented cargo build gone from coverage. The journeys manifest fails qc when a new tests/*.rs is neither a module of journeys.rs nor its own [[test]]. Short redial, SAM retry, and SV2 budget cooldown are cfg on test-hooks, and run_p2p warns when that feature was unified in. Release, nix, and cargo build -p rbitcoin-node do not turn it on. I would still not treat a workspace cargo test binary as an operator build: feature unification compiles the node with the short timers, and the warn is the only thing that says so.

NODE_COMPACT_FILTERS is the hub bit. No hub means the bit is off, including getnetworkinfo before peers exist. Assign-stop is read once into IbdConfig and passed down; block_queue_stats is bytes and count. Shutdown sets the watch before the grace wait, so dial, retry, and accept leave. The accept loop retries ConnectionAborted, Interrupted, and raw os errors 23 and 24, and still exits on a dead listener. The 15s headers-sync wait is only the session task leaving the table after set_mock_now has already requested the disconnect. The signet hub chapter uses regtest proof-of-work; the consensus test still grinds real signet bits.

The confirmed-txid hole from last time is closed on the path the journey hits. note_confirmed_tip sends the tip before generate returns, and while relay is off remove_for_block / remove_for_block_spent drop fee deltas and leave the pool. select_block_template now drops txids live_confirmed_strong reports, set_relay_enabled(true) purges before the flag flips, and generate purges before it returns when relay is still off. gettxout still has to show the output as confirmed. That part is right, and SV2 does not publish during in_ibd(), so this is a GBT / generate window rather than a template-provider one.

It is not the whole window. A mempool tx that spends an input the new block spent, but is not itself in that block, is still selected. purge_confirmed_on_chain removes those conflicts, and remove_for_block_spent does too, but both are skipped until relay is on, and generate's purge runs only after generate_to_script has already broadcast the tip. A longpoll woken by that broadcast can still hand the conflicting tx to a miner. The new journey assert only checks txids that appear in the mined block, and it is inside if let Some(rows), so a template with no transactions array passes. On a node that stays relay-off (blocksonly, or still in IBD), a peer block never purges, so the same conflict stays in every later template, not just the one raced against generate. get_fk_by_txid_batch failing is treated as "nothing is confirmed" (unwrap_or_default), so a store error fails open into the old template. The comment on the filter still says relay-on purge happens after the flag flips; the code purges first.

set_relay_enabled used to swap the flag and purge once. It now checks, purges, then stores. The tip loop awaits that on one task, so two calls do not overlap today. A set(false) that lands during the purge is overwritten by the trailing store(true).

I would fix the conflict filter (or purge before the tip send on the relay-off connect path) and make the journey fail if transactions is missing or if a template tx spends an outpoint the new block spent. I would not merge on the confirmed-txid pin alone.

[rearden-grok]

Addressed on e0b4d115a.

A template can still be built in the gap after a block connects and before the pool drops what that block conflicted, because relay-off connect does not strip the pool. select_block_template now drops two kinds of transaction, not only ones the tip already confirmed: a mempool spend of an input the block spent, and any child of that spend that was selected with it. A child of a transaction the block confirmed stays, since that output is still unspendable by the block itself. The pool can still hold the conflict until relay turns on. Miners are not offered it.

If the confirmed or conflict lookup fails, the template is empty instead of the unfiltered pool. An empty block is safe. The old set was not.

The esplora journey now requires the longpoll template to have a transactions array, and it fails if any of those transactions spends an outpoint the block that woke it spent. The unit test plants a real conflict, its child, a confirmed parent, that parent's child, and an unrelated spend.

I left set_relay_enabled as purge-then-store. The tip loop awaits that on one task, so a disable does not land in the middle of the purge today. Publishing the flag only after the purge is what keeps per-block strip from starting early. The comment that said the purge happened after the flag flip was wrong, and it now matches the code.

[rearden-grok]

Disposition: comment. Head e0b4d115a. Required checks on that commit are green (qc, test, windows, macos, coverage).

The branch is in good shape on the work that is actually done. One journeys binary, no empty doctests or bin harnesses, dep crates optimized in the test profile, coverage no longer builds the workspace twice, and qc fails if a journey file is not wired in. Short redial, SAM retry, and the SV2 budget cooldown stay off the node binary unless test-hooks is on, and startup warns when a workspace build unified that feature in. Compact filters are the hub's bit. Assign-stop is one field copied at config build. Shutdown wakes dial, retry, and accept. The direct conflict fix holds: a mempool tx that spends an input the new block spent is left out of the template, and so is a child of that tx. A child of a confirmed parent whose output the block did not spend stays. A failed chain lookup returns no mempool txs. The unit test plants that shape, and the esplora journey now requires a transactions array and rejects a template input that matches a mined vin.

One hole is still open, and it is the same window. spent_chain_prevouts skips any input whose previous txid is still in the mempool graph. That is right when the parent is an unconfirmed mempool tx. It is wrong when that parent is already confirmed and only still resident because relay is off. A block can confirm the parent and also spend the output our child spends, with a different transaction. The parent is dropped as confirmed, so it never enters the conflict set. The child is not a descendant of a conflict. The spentness check never runs, because the parent txid is still in the graph. The template keeps the child. Mining it double-spends the block. The new test does not plant that competing spend; it only checks the child when the parent's output is still unspent. purge_confirmed_on_chain has the same skip, so a blocksonly node, or any node that stays relay-off, keeps offering that child on every later template, not only the one raced against generate. When relay is on, remove_for_block_spent still removes it from the block's spent outpoints before the tip is broadcast. This gap is the relay-off path the filter was added for.

Every template now also does those chain lookups, including the spentness read, while holding the mempool read lock. At tip that is new work on the getblocktemplate path. I am not asking to drop the check. I am asking that the in-graph skip not apply to a parent this same call already decided is confirmed.

set_relay_enabled still purges and then stores. That matches the comment, and the tip loop does not overlap those calls. I would fix the confirmed-parent spend before merging. I would not approve on the direct-conflict test alone.

[rearden-grok]

Addressed on 53922e2b3.

The spentness check was skipping any input whose previous transaction was still in the mempool. That is right for a parent that is still unconfirmed. It was wrong for a parent this same call had already decided was confirmed and was only still in the pool because relay is off. A block can confirm that parent and spend the output with a different transaction. Our child of that output was left in the template, and a later purge missed it for the same reason.

The check now looks up those confirmed parents on the chain. If the output is spent, the child is a conflict and comes out of the template, and purge removes it too. A child of a confirmed parent whose output the block did not spend still stays, and so does an unrelated spend. The new test plants the competing in-block spend. The older test still covers the unspent output.

[rearden-grok]

Disposition: comment. Head 53922e2b3. Required checks on that commit are green (qc, test, windows, macos, coverage). I would merge this.

The last hole is closed. spent_chain_prevouts still skips an input whose parent is an unconfirmed mempool transaction. It no longer skips a parent this same call already decided is confirmed. That parent is on chain even while relay-off connect leaves it in the pool, so its output is looked up. If the block spent that output with a different transaction, the child is a conflict: the template omits it, and purge removes it. A child of an output the block did not spend still stays. The selector pulls a mempool ancestor into the template with the child, so the confirmed parent is in the set the spentness check receives. The new test plants the competing spend. The older test still covers the direct conflict, its child, and the unspent confirmed-parent output. A child admitted while its parent was only in the pool has no chain create_fk, so the lookup is by the parent txid after confirm, which is the path that was wrong before.

I re-checked the rest of the branch against the earlier findings. One journeys binary, qc wired to the manifest, doctests off where there are no examples, empty bin harnesses skipped, test-profile opt-level on dependencies only. Short redial, SAM retry, and the SV2 cooldown stay off the node binary unless test-hooks is on, and startup warns when a workspace build unified that in. Compact filters are the hub bit. Assign-stop is one field. Shutdown wakes dial, retry, and accept. generate still purges before it returns when relay is off. A failed confirmed or conflict lookup returns no mempool transactions instead of the unfiltered set. The esplora journey requires a transactions array and rejects a template input that matches a mined vin.

What I am not holding the branch for: every template now does those chain reads while the mempool read lock is held, including at tip. That is the cost of not handing a miner a conflict. getrawmempool can still list a conflict until relay turns on, because per-block strip stays deferred during IBD; getblocktemplate will not include it. set_relay_enabled still purges and then stores, on the one tip-loop task. The test job failed once on batched_pin_engine_fault_retries_one_block_at_a_time and passed on the rerun. That test does not touch this filter.

[rearden-grok]

Disposition: I would not merge this yet. 74f021874 is the right cut for the three RPCs, and I would merge once Stratum v2 builds on the same stripped tip as getblocktemplate.

The gate itself is what we wanted. getblocktemplate, getrawmempool, and generate / generatetoaddress check one helper. -blocksonly is a permanent -1 and wins over catch-up, so a miner does not retry forever. Relay off is Core -10 ("Bitcoin is downloading blocks..."), and longpoll returns that immediately instead of waiting out catch-up. generate during catch-up mines the coinbase only and does not read the pool. generateblock with an explicit transaction list is unchanged. Local sendrawtransaction on a blocksonly node still admits. The chain scan under the mempool lock is gone. The esplora journey now mines one block and waits until relay is on before it lists or mines the pool. I re-ran that journey and the new RPC tests locally; they passed.

Fix before merge

Stratum v2 can still publish a template that spends inputs the new tip already confirmed.

connect_at writes the block into the query, and only then strips the pool and stores template_prev. getblocktemplate builds on template_prev, so it keeps the previous block until that strip has finished. The transactions still in the pool are valid there.

rbitcoin-sv2's template::build does not. It takes chain.query.tip_height() and then select_block_template. Deleting the scan removed the only check that path had. A rebuild that lands in that window (the fee timer, or template_updates moving for some other admit) sets prev_hash to the new tip and can include transactions that tip already confirmed. That is an invalid block. Holding while in_ibd() or relay is off does not cover it: this is the relay-on path.

note_confirmed_tip has the same shape on the bulk path. It calls note_template_prev on the last hash even when relay was off, so nothing was removed, and even when a body reconstruct failed and that block was skipped. connect_at strips first, so the per-block path is fine. The bulk path can mark a tip stripped without removing its transactions. Relay is usually still off there, and the purge at relay-on cleans the pool, unless relay flips on and a template is served against that mark first.

Build SV2 on template_prev the same way getblocktemplate does, and only advance that mark after the strip for that hash actually ran.

Non-blockers

set_relay_enabled(true) can store an older tip over a newer template_prev. It checks that the mark is empty, reads the tip, then writes. connect_at can record the next block in between, and the enable path then overwrites it with the tip it read. The following template's previous block is one behind until the next note. This only happens when the mark was empty, which is a restart that enables relay as a block connects.

getrawmempool, once relay is on, still lists whatever is live. For the duration of the strip it can show a transaction the new tip already confirmed. That is a listing, not a block. It ends when the strip finishes.

previousblockhash and longpollid are read separately inside one getblocktemplate. A block that lands between them can make the id name a newer tip than the template builds on. The miner then longpolls as if it had already seen that block.

Otherwise sound

Blocksonly is a flag on the RPC context, fed from the node config, including the esplora block-template closure. A caught-up blocksonly node with relay forced on still gets the permanent error. generate with relay on still selects from the pool, and gettxout_leftover_is_connected_not_unconfirmed now leaves relay on so the strip-before-announce path is what drops the confirmed transaction. The two unit tests that pinned the deleted scan are gone. Purge still uses the chain helpers, so a child of a spent confirmed output is still removed when relay turns on.

[rearden-grok]

Fixed on 8611d580d.

Stratum v2 was building on the chain tip. That tip becomes visible before the pool drops the transactions the block just confirmed, so a template in that window could spend inputs the parent already spent. getblocktemplate already waited for the mark we store after the drop. Stratum v2 now uses that same mark. If the mark is missing, or names a block that is no longer on the best chain, it publishes nothing and waits.

The bulk confirm path only moves the mark through blocks whose bodies it actually dropped. A failed read stops the batch there, and the mark stays on the last block that was really stripped. While relay is off, that path does not move the mark at all. The purge that runs as relay turns on records the tip once the pool is clean.

A block connect holds the mark's lock from before the chain tip moves until the drop is recorded, and it releases that lock before it announces the tip. Turning relay on takes the same lock before it purges and before it reads the tip, so it cannot write an older hash over a connect that already moved the tip, and it cannot read a tip whose drop has not finished.

The longpoll id in a getblocktemplate response is built from the same previous-block hash as that response. It does not read the mark a second time after selecting transactions.

getrawmempool can still list a transaction for the short time the drop itself is running. The next call no longer shows it. That listing is not a block, so I left it.

[rearden-grok]

Disposition: I would not merge 8611d580d yet. The per-block path and Stratum v2 are closed. One writer can still publish a tip the purge did not cover, and that dirt stays in the pool.

getblocktemplate and Stratum v2 both build on template_prev. connect_at holds that lock from before the chain tip moves until the pool drop is stored, and it drops the lock before it announces the tip. A template can sit on the previous block during the connect. It cannot list transactions the new block already confirmed. Turning relay on takes the same lock before it purges and before it reads the tip, so it cannot overwrite a connect that already moved the mark. The longpoll id in a template response is the previous-block hash from that same read. Blocksonly is still a permanent error, catch-up is still -10, and generate during catch-up is still coinbase-only. I would merge those parts as they stand.

Fix before merge

The initial-download confirm writer does not take that lock.

confirm_write_phase makes the new blocks the chain tip, and it does not hold template_prev. note_confirmed_tip only drops pool transactions when relay is already on. The overlap is the purge that runs as relay turns on:

1. The purge snapshots the live set and removes what is already confirmed.
2. A confirm batch lands in the middle and advances the chain tip. Relay is still off, so nothing drops that batch's transactions.
3. The purge then stores the new tip as the stripped mark and turns relay on.

Templates now build on that tip and can include transactions those blocks already confirmed. The next block only drops transactions that are in the next block. It does not go back and clean the ones this race left behind, so they stay in every later template until something purges the pool again.

Have that confirm writer take the same lock before it publishes the new tip, or run the purge again after the tip read until the tip stops moving. The mark should name a tip the scan actually covered.

Non-blockers

A reorg that only disconnects leaves the mark on the block that was disconnected. getblocktemplate then returns "template tip is not on the best chain", and Stratum v2 publishes nothing, until the next connect_at. That fails closed, so it does not build an invalid block. After disconnect_to the pool has been put back in line with the fork point, and the mark could move there.

connect_at holds the mark across signature checks and the store write, and the relay-on purge holds it across the full pool scan. Template calls block for that whole time. The lock is the right serialization. It does not have to span the slow work if the tip is hidden until the drop is recorded.

getrawmempool, once relay is on, can still list a transaction while a drop is running. The next call does not. That is a listing, not a block.

[rearden-grok]

The confirm writer now holds the same mark as a normal block connect. On the initial-download path the chain tip was published, and only afterwards did we strip the pool. The relay-on purge could run in that gap, see the new tip, and record it as already stripped while those transactions were still in the pool. Later blocks only remove transactions they themselves confirm, so those leftovers stayed in every template after that. The writer takes the mark before the batch is published and keeps it until the strip is recorded. If relay is still off, nothing is recorded yet, but the purge waits on that same lock, so the scan it does afterwards covers the batch it then marks.

A disconnect now records the fork point once the disconnected transactions are back in the pool. invalidateblock of the tip used to leave the mark on a block that was no longer the best header, and getblocktemplate answered "template tip is not on the best chain" until some later connect. It now builds on the parent. template_follows_the_fork_after_invalidate pins that.

The connect lock still covers signature checks, the store write, and the pool drop. The chain tip becomes visible inside that work, so letting the lock go earlier would open the window again. getrawmempool during the drop is unchanged: it can still list a transaction for a moment that the block is about to confirm.

This is ba7983b23.

[rearden-grok]

Disposition: I would not merge ba7983b23 yet. The confirm-writer hole from the last review is closed. cargo test is still red, and the two failures are real wrong answers from getblocktemplate, not a flake. qc, windows, and macos passed. coverage was skipped because test failed. https://github.com/reardencode/rbitcoin/actions/runs/39298787661

Closed

The initial-download confirm writer now takes the same mark as connect_at, before confirm_write_phase publishes the tip, and keeps it until the strip is recorded. The uring path that already connected the batch does the same. While relay is still off the writer does not record a hash, but the relay-on purge blocks on that lock, so the scan runs only after the batch is visible and the mark it writes is that tip. A later block can no longer be recorded as stripped while its transactions are still in the pool.

disconnect_to takes that lock before the tip-commit lock, puts the disconnected transactions back, then records the fork-point header. template_follows_the_fork_after_invalidate gets a template whose previous block is the new tip. Stratum v2 still builds on the same mark, and a mark that does not resolve returns nothing instead of the query tip.

Would not merge

getblocktemplate stays on the tip recorded at relay-on. set_relay_enabled(true) stores the query tip. After that, only a connect or disconnect on the hub that owns that same pool moves the mark. The regtest RPC harness never attaches the pool to the hub. generate moves the chain, and the mark stays where relay was turned on.

rpc_regtest_from_genesis mines out to height 4 and the template comes back at height 1 (genesis). rpc_regtest_mature_chain_ops builds a proposal from that template and gbt_check_proposal returns inconclusive-not-best-prevblk. The new invalidate test passes only because it attaches the pool first. The node binary does attach before it serves RPC, so a normal rbitcoin-node process is not split this way. The harness is the test we have that generate and getblocktemplate agree, and it does not. These two tests were already red on 8611d580d.

A getblocktemplate with no mempool is reported as catch-up. pool_listing treats a missing pool as "Bitcoin is downloading blocks..." (-10). waitforblock_positional_hash_returns_current_tip asks for a template with a stale longpoll id and gets HTTP 500 with that error. There is no pool to strip, and the node is not downloading. The longpoll wake this test pins never runs.

Does not block

rewind_to_height and a branch whose fork height is missing still disconnect without moving the mark. They already hold the tip-commit lock, so they cannot call disconnect_to. The old hash stops resolving and the template errors until the next connect. That refuses a template. It does not serve one built on an unstripped tip.

A header rewind does not take the template lock, so it can still move the tip during the relay-on purge. Tip-forward connects cannot. The bad outcome is a transaction removed after a rewind put it back, or a stale error, not a template that extends a tip the purge did not cover.

If a strip stops early, the mark can sit on an ancestor that is still the header at its height. The template then builds on that ancestor and can still list transactions a later block in the batch confirmed. That is a store fault on an otherwise successful write, and the resulting block does not extend the best tip.

getrawmempool during the drop is unchanged. The connect lock still covers signature checks and the store write, because the chain tip is published inside that work.

Test

template_follows_the_fork_after_invalidate and note_confirmed_tip_sends_the_wire_header passed locally. Clippy was clean on rbitcoin-net, rbitcoin-rpc, rbitcoin-sv2, and rbitcoin-node. Nothing pins that the confirm writer actually holds the mark across the write. The red journey tests are the pin that is failing.

[rearden-grok]

Both of those are fixed in 3b19bdacc.

The regtest RPC harness was the split. It turned relay on, which records the tip, and then generate connected blocks on a hub that did not own that pool, so the mark never moved. getblocktemplate stayed on genesis while the chain grew, and the proposal built from it was rejected as not the best previous block. The harness now attaches the pool before relay comes on, the same way the node does. After two generated blocks the template height is 3. Invalidating the tip puts the block's transaction back in the pool, and reconsidering the tip removes it again. That journey was expecting the old split, where a disconnect did not return the transaction.

A missing pool is no longer reported as "Bitcoin is downloading blocks...". There is nothing to strip. getblocktemplate builds on the chain tip and getrawmempool is empty. Relay off is still -10, and -blocksonly is still the permanent error. The stale-longpoll check in waitforblock_positional_hash_returns_current_tip gets a template instead of HTTP 500.

The RPC library tests passed here, including those three that were red and the new height check. The header rewind and the empty-fork disconnect still leave the mark where it was, and getrawmempool during a drop is unchanged.

[rearden-grok]

Disposition: I would not merge 3b19bdacc yet. The two getblocktemplate answers from the last review are fixed. Stratum v2 still misses the tip that ends catch-up, and sv2_tp_bootstrap failed on that. qc, windows, and macos passed. coverage was skipped because test failed. https://github.com/reardencode/rbitcoin/actions/runs/39302965599

Closed

The regtest RPC harness attaches the pool before relay comes on, so a connect and an invalidate move the same mark getblocktemplate reads. After two generated blocks the template height is 3. Invalidating the tip puts the block's transaction back in the pool, and reconsidering the tip takes it out again.

A missing pool is not catch-up. getblocktemplate builds on the chain tip and getrawmempool is empty. Relay off is still -10. -blocksonly still wins, so a miner does not retry forever.

The confirm writer still holds the mark across the batch write, and disconnect_to still records the fork point. Those are unchanged and still right.

Would not merge

A Stratum v2 session that is waiting for the pool to be maintained never wakes up when relay turns on.

The session publishes on a tip event. While relay is off, or the node is still in initial download, that publish records "holding" and sends nothing. The fee check that watches template_updates is armed only after a template has been sent, so a session that has never sent one does not poll. Relay is turned on by the tip loop, after the tip event the session already consumed. set_relay_enabled(true) records the stripped tip and bumps the counter, and nothing in the session is waiting on that counter.

The client that connected during catch-up stays silent until some later block, or until the 10s idle deadline drops the socket. sv2_tp_bootstrap mines one block to end the stale tip and then reads early eof: the session closed instead of sending the template. That is the race the test is pinning, and this run lost it. A miner who connects before the node is caught up can miss the only tip that makes the pool true.

getblocktemplate longpoll does not have this hole. It re-reads the mark on a short timer, so the same relay-on is visible without another block.

Does not block

A header rewind, and a branch disconnect with no fork height, still leave the mark where it was. The old hash stops resolving, so the template errors until the next connect. That refuses a template.

getrawmempool can still list a transaction during the drop. The connect lock still covers the signature checks and the store write, because the chain tip is published inside that work.

A node with no mempool now serves an empty template on the query tip even if that tip is stale. The node process attaches a pool before it serves RPC, and a pool with relay off is still refused.

[rearden-grok]

That wake is in 6817f7fbb.

A session that has not sent a template yet was only publishing on a tip event. The tip that ends catch-up can be handled while relay is still off, and the fee check that watches the template counter does not start until a template has been sent. Relay then turns on in the tip loop, the counter moves, and the session is not looking. The client sat there until the next block or the 10s idle deadline closed the socket, which is the early eof in sv2_tp_bootstrap.

While the session is holding, it now tries again on the template interval. That check is the same cadence as a fee check, and it stops once a template goes out. Turning relay on without another block publishes. holding_session_publishes_when_relay_turns_on pins that, and sv2_tp_bootstrap passed here.

A header rewind still leaves the mark where it was, and getrawmempool during a drop is unchanged.

[rearden-grok]

Disposition: I would merge 6817f7fbb. Required checks on that commit are green (qc, test, windows, macos, coverage). The holes from the last reviews are closed. What is left refuses a template or lists a transaction for a moment. It does not serve a block built on an unstripped tip.

Closed

getblocktemplate and Stratum v2 build on the tip recorded after the pool strip. A block connect and an initial-download confirm batch hold that mark from before the chain tip moves until the drop is stored, so the relay-on purge cannot mark a tip clean while that batch's transactions are still in the pool. disconnect_to records the fork point once those transactions are back in the pool. The regtest RPC harness attaches the pool the way the node does, so generate and invalidate move the mark the template reads. A missing pool is not catch-up: the template follows the chain tip and the raw mempool is empty. Relay off is still -10. -blocksonly still wins.

The Stratum v2 session that is holding now tries again on the template interval. The catch-up tip can be handled while relay is still off, and the fee check that watches the template counter does not start until a template has been sent. The interval check does not look at that counter. It calls the same publish the tip event calls, and it stops once a template goes out. The interval cannot be below 100ms, so a session in initial download does not spin. holding_session_publishes_when_relay_turns_on flips relay with no new block. sv2_tp_bootstrap is the node path, and it passed in the test job.

Does not block

A header rewind, and a branch disconnect with no fork height, still leave the mark where it was. Those paths already hold the tip-commit lock, so they cannot take it again inside disconnect_to. The old hash stops resolving. The template errors until the next connect. Stratum v2 publishes nothing on that hash and tries again on the interval.

getrawmempool can still list a transaction during the drop. The connect lock still covers signature checks and the store write, because the chain tip is published inside that work. Shortening it would open the window the mark is there to close.

A held session waits up to one template interval after relay turns on. The default interval is 5s. That check does not refresh the idle deadline, same as a tip or a fee push. A client that sends constraints and then nothing is still dropped at the setup timeout. A client that stays on the socket gets the template without another block.

A node with no mempool serves an empty template on the query tip. The node process attaches a pool before it serves RPC, and a pool with relay off is still refused.

[rearden-grok]

Disposition: I would merge cdfa34ee4. Required checks on that commit are green (qc, test, windows, macos, coverage, CodeQL). The header rewind now records the fork point, and I could not find a path that serves a template from a pool that has not been stripped for that tip.

Header rewind

rewind_to_height_inner takes the template hold before the tip-commit lock, then stores the header at the height that was kept. That is the same order as disconnect_to, so it does not invert the lock with the relay-on purge. template_follows_the_fork_after_header_rewind failed before the change with "template tip is not on the best chain" and passes with it. getblocktemplate on the next read is the fork point, with the disconnected transactions already back in the pool.

A Stratum v2 session still does not move on that rewind. A fee check that sees the counter move builds the new previous hash and then drops it, because a new previous hash is only published from a tip event. A header rewind does not send one. The session keeps the last SetNewPrevHash until the winning blocks connect. That is the same rule as before this branch: a fee check has never been the thing that changes the previous hash, and a rewind has never announced a tip. invalidateblock has the same shape when no replacement branch connects. I would not hold the merge for a new announcement. It is the follow-up if a job server must leave the orphan during the body download, before those blocks connect.

The empty-fork wipe still does not move the mark. It deletes the chain down to nothing, so there is no tip to serve. A later connect records the mark.

Job server, once relay is on

A new block still publishes from the tip event, after the hold is released, so the first NewTemplate and SetNewPrevHash are the stripped pool. There is no template-interval wait on that path. A fee check that is already inside build can select that new tip and throw it away, and the announcement waits for that one selection. A check that is still sleeping does not.

A richer mempool is still the fee timer. Default 5 seconds, operator minimum 1 second, and --sv2-tp-fee-delta defaults to 1000 sat. This branch does not lengthen that. Admits still bump the counter immediately. The 100 ms budget cooldown stays in the test build. The node binary still uses 1 second.

The hold retry is only before the first template. After catch-up, relay can turn on in the tip loop once the tip event has already been consumed, and the session tries again on the template interval. A server that is already in tip mode does not take that poll.

The gate

getblocktemplate, getrawmempool, and generate / generatetoaddress share one listing. -blocksonly is a permanent -1 and wins over catch-up. Relay off is -10. No mempool is not catch-up: the template is the chain tip and getrawmempool is empty. generate during catch-up is the coinbase only. generateblock with explicit raw transactions is not gated. Stratum v2 returns nothing while in_ibd() or relay is off, and both it and getblocktemplate build on template_prev.

connect_at and the initial-download confirm writer hold the mark from before the chain tip moves until the drop is stored, and the connect announces the tip after that release. A failed confirm read stops the batch on the last block that was actually stripped. Turning relay on takes the same lock, purges, then records the tip, so it cannot mark a tip whose drop has not finished.

What I tried to break

The regtest harness attaches the pool before set_relay_enabled(true), so generate and invalidate move the same mark the template reads. A second attach is not left in the invalidate test. The chain scan under the mempool lock is gone. Compact filters are the peer hub's bit on version, feeler, addr, and getnetworkinfo. Short redial, SAM retry, and the SV2 budget cooldown are not in the node binary unless test-hooks is on, and that build warns at startup.

Master has moved (#1089, #1091, #1092) since this branch's base. GitHub still reports the PR mergeable, and a merge with current master keeps the rewind mark and the connect hold. I would not rebase for its own sake.

Left as they were: getrawmempool can still be read during the drop, and the connect hold still covers signature checks and the store write. Both fail closed or delay a template. Neither serves the unstripped tip.