[rearden-grok]

Closes nothing; cites #421 (IBD stall / recover resume). Do not merge until required checks are green.

Why

v0.6.0 → master dropped a few coverage pins and left two recover holes that dpc’s review of #425 named as P1:

1. After Class C, height_of_hash / IBD has_block treated the write as finished. A session fault on spend annotate or tx.head drain skipped retry of those steps (connect_at’s fault loop no-op’d).
2. Load note_lookup_ok advanced next_tx_start and in_flight before confirm_wire_load_from_plan. Session-fault recover requeued hashes without clear_all; sync_body_hwm uses max and will not roll a speculative HWM down.

Also restore: Class A one ≥3-op write batch observer, mid-wave session-false must not store URING_MODE=2, DrainOnDrop matches session Drop (no abort), 1p1c child-fail rollback evicts spenders of the parent, owned-wave first-error, scripts publisher thread, RPC maxfeerate overflow fail-closed.

What

• Class A: UringSession max-batch pwrite SQEs; put_full_batch_one_body_write_wave asserts ≥3 when uring is on. No test_take_pwrite_waves.
• Mid-wave false: poisoned begin_batch is this-batch fallback only.
• DrainOnDrop: drain_all_inner(false) like UringSession::Drop. Explicit drain_all stays fail-closed.
• IBD stuck #421 P1-1: already-at-height still runs idempotent post_commit + head drain. IBD write session-fault requeues even when has_block.
• IBD stuck #421 P1-2: load uring recover calls LoadAheadState::clear_all inline before requeue.
• 1p1c: remove_txid_tree evicts the parent and mempool txs that spend it.
• Scripts: start_for_each_owned first-error pin; publisher / confirm_scripts_phase are not rbtc-scripts-*.
• RPC: checked prevout-sum; overflow is over-cap; missing prevout still skips the cap.

Compact fill/duplicate reconstruct was already in rbitcoin-net. DrainBudget trickle-vs-stall was already pinned on master.

Out of scope

Clippy allow-list batches, recover credit gap (1000), leftover index refuse (#422), --sptweaks-dust default, --rpcworkqueue default, musl, merge.

[rearden-grok]

Verdict: request changes (posted as a comment — the App token authored this PR and cannot formally request changes on it).

Reviewed at b7efcde6 against origin/master (27103190). Required checks are green. Two of the changes need to come out or be reworked before this merges; the rest is fine.

Blocking

1. DrainOnDrop fail-open reintroduces the use-after-free #425 closed

crates/rbitcoin-store/src/uring_session.rs:1014-1018 switches DrainOnDrop::drop to drain_all_inner(false). The PR body justifies this as matching UringSession::Drop, but the two guards protect different things.

DrainOnDrop exists to drain before SQE buffers drop (its own doc, and drain_all's doc at 730-735). Both users declare it after the buffers:

• spend_annotate_uring.rs:546-547 — slots: Vec<Option<PageSlot>> (each holds buf: Vec<u8>) then let mut session = session.drain_guard();
• spend_annotate_uring.rs:85-86 — same shape with Slot.

Drop order is reverse declaration: the guard drops first, slots immediately after. On master a zero-CQE hard cap inside that drain calls abort_uring_unusable while the buffers are still pinned. With this PR it sets poisoned, logs, returns Err (swallowed by let _), and slots free the buffers while the kernel still owns the SQEs pointing at them. Then with_thread_local at 1138-1143 sees in_flight() != 0 and calls fail-closed drain_all(), which hits the hard cap and aborts anyway — roughly 120 s after the UAF window opened. So the change does not remove the abort; it moves it past the point where it was protecting memory.

UringSession::Drop is fail-open because the machines that own buffers have already drained (or aborted) by the time the TLS slot is torn down; the session does not own any buffers. That asymmetry was deliberate in #425 and is what docs/io-modality.md said before this PR edited it.

The new test drain_guard_hard_cap_does_not_abort does not pin the change: apply_drain_window (789-801) sets poisoned = true before finish_drain_hard_cap, and under cfg(test) finish_drain_hard_cap returns Err instead of aborting. I ran the test against master's DrainOnDrop (drain_all()) with everything else from this branch: test uring_session::tests::drain_guard_hard_cap_does_not_abort ... ok. It is not a Red test.

Fix: revert DrainOnDrop::drop to drain_all(), drop the test, and revert the docs/io-modality.md and CHANGELOG lines that describe the new behaviour. If "no abort from a guard" is a real requirement, the only memory-safe alternative is to leak the slot buffers on poison, which is a design change that belongs in its own PR.

2. IBD half of #421 P1-1 is not fixed; the new branch is unreachable and the requeue is dropped downstream

crates/rbitcoin-net/src/ibd/confirm/mod.rs:1532 adds confirm_write_err_action so a session fault wins over has_block. Two things stop it from doing anything in the scenario dpc described (Class C committed, then spend annotate or tx.head drain faults):

1. The stale gate runs first. In the Err arm, line 1520 write_batch_is_stale_plan(&hub_wb, &feed_wb, height) executes before the new match. After Class C commits, tip ≥ every height in the batch, so first_h != tip + 1 and the batch is dropped as stale (feed.finish, continue). The UringRecover / AlreadyCommitted arms are never entered for a post-Class-C fault. No recover credit is taken, no note_confirmed_tip, no block_queue_dequeue_height, no Accepted events.
2. Even if reached, the requeue is discarded. requeue_hashes puts the heights back in ready. The load thread at 1753-1757 does if hub_load.has_block(&hash) { g.ready.remove(&h); continue; }, and the write thread at 1439 gates on write_batch_is_stale_plan before calling confirm_write_phase. So the AllOld → finish_post_commit_hashes addition in write.rs:60-64 is never reached from the IBD pipeline. The only pipeline callers that can hit AllOld are confirm_wire_run_preverified and ChainHub::confirm_write.

confirm_write_session_fault_recovers_even_when_has_block tests the pure decision function, not the pipeline, which is why it is green.

What actually happens today for a post-Class-C fault in IBD: tx.head self-heals (the head_note_pending restore is picked up by the next batch's take_pending_queued), but spend annotations for that batch are lost in tip index mode, and the body-queue entries for those heights are never dequeued.

Fix: the write thread should own the retry, the same way connect_at does for tip. In the Err arm, classify is_uring_session_fault before the stale check. If Class C already committed (is_connected on every hash — not has_block, which is RAM-cache-or-confirmed-set and documented at chain.rs:606-610 as not meaning "on chain"), take recover credit and call a pub finish_post_commit_hashes(query, &hashes) in place, looping on further session faults; then run the normal Ok bookkeeping (note_confirmed_tip, dequeue, Accepted, finish). Requeue through lookup/load cannot work for already-connected heights. Pin it with a pipeline test that injects a post-Class-C session fault and asserts annotate completes, Accepted is emitted, and the BQ entry is dequeued — not a table test on the enum.

Non-blocking

• write.rs:299 annotate_jobs_from_connected_hash does not check that hash is connected at its height; it trusts get_header_by_hash + header_txs.get_list. Reads are still safe because is_confirmed_strong_at is the authority, but a stale-fork hash reaching AllOld would stamp a non-active spender and flip a sole spender to MULTI_SPENDER when the real spend lands. Take (height, hash) and require height_of_hash(hash) == Some(height) so the name is honest.
• decide_annotate (spend_annotate_uring.rs:325-327) is idempotent for the sole-spender case (field == spend_fk → Skip) but appends unconditionally in the multi case. A repeated finish_post_commit on an already-multi output appends a duplicate list node. Rare (multi needs a fork double-spend), worth a Skip when spend_fk is already in the list.
• finish_post_commit_hashes always submits a head-drain job even when queued is empty and the spend index is off. Cheap, but it is on the tip retry path; an early return when both are empty is one line.
• LoadAheadState::on_uring_recover doc says "before requeue"; the call at 1995 is after requeue_on_uring_recover. Same thread, same effect, but the comment should match.
• batch_pwrite_n / max_batch_pwrite_n are production fields bumped on every push_pwrite with only a cfg(test) reader. There is precedent (max_pwrite_len, sqe_n), so not blocking, but it is a test observer on the hot path.
• CHANGELOG "IBD session-fault resume" claims the has_block requeue "retries annotate+drain"; per (2) it does not. Reword once the write-thread retry lands.

Verified as claimed

• accept_and_connect_block_preverified retry now runs finish_post_commit (tip side of P1-1). already_at_height_retries_post_commit_spend_annotate is Red on master (left: Fk(0) right: Fk(2)), Green here. connect_at's recover loop at chain.rs:2042-2057 re-enters it correctly.
• P1-2: on_uring_recover → clear_all sits after note_lookup_ok (1889) and before the requeue-driven retry; the stamp-fault path (1833) does not need it because note_lookup_ok has not run yet.
• Mid-wave begin_batch false, Class A ≥3-SQE batch pin, owned-wave first-error, scripts publisher thread, 1p1c remove_txid_tree, RPC maxfeerate overflow: test additions read correctly and the production deltas are small and match their tests.

[rearden-grok]

Addressed in 988fc62.

Blocking 1 (DrainOnDrop): reverted to fail-closed drain_all(). The guard drops before SQE buffers; fail-open opened a UAF window and the abort still happened later on TLS teardown. Session Drop stays fail-open. Test and the io-modality/CHANGELOG sentences are gone.

Blocking 2 (IBD P1-1): the write thread now classifies is_uring_session_fault before the stale gate. If every hash is is_connected (not has_block), it takes recover credit and calls finish_post_commit_hashes in place, looping on further session faults, then the normal Ok bookkeeping (note tip, BQ dequeue, Accepted, finish). Requeue is only for session faults that are not yet connected. Pin: write_session_fault_after_class_c_finishes_annotate_in_place (stale would drop; annotate + BQ dequeue).

Non-blocking: annotate jobs require height_of_hash == height; multi-list repeat of the same spend_fk is Skip; empty queued + spend-index off returns without a drain job; load recover comment matches call order; CHANGELOG matches the write-thread retry.

CodeQL on write_idempotent_tests.rs HeaderRecord nonce is #[cfg(test)] only. AGENTS.md: App token cannot dismiss test-only alerts — please dismiss 244/245/246.

[rearden-grok]

Summary

PR #441 restores post-v0.6.0 coverage pins and tries to close #421 recover holes: Class A max-batch pwrite observer, mid-wave session-false, DrainOnDrop fail-open, already-at-height finish_post_commit, IBD write has_block requeue, load clear_all after note_lookup_ok, 1p1c spender eviction, scripts first-error / publisher thread, and RPC maxfeerate overflow fail-closed. Prior review already covers the two blockers (DrainOnDrop UAF vs #425, and IBD P1-1 confirm_write_err_action unreachable behind the stale-plan gate / has_block skip). This pass found one additional issue: 1p1c rollback now evicts already-published concurrent spenders from the graph but does not run the hub unindex / template path that every other live eviction uses.

Issue counts by severity

• bugs: 1
• suggestions: 0
• nits: 0

[rearden-grok]

[bug] rollback_1p1c_parent now calls remove_txid_tree, which evicts live graph txs that spend the below-min-relay parent. That is the right graph fix for the window described in admit_1p1c (parent is committed and the write lock is dropped before child script/commit; a concurrent accept of a spender can publish_admitted in that window). Those concurrent spenders are already in hub relay maps, sh_index, unbroadcast, fee-deltas, and the template. remove_for_block_spent (tx_relay.rs:1734-1742) and expiry (tx_relay.rs:872-880) both unindex_txid + note_template_update after evict_conflicts_with / remove_txid. This rollback does neither. remove_txid_tree also drops the Vec<Txid> that evict_conflicts_with returns (accept.rs:1099), so the hub cannot unindex even if it wanted to. Result: INV/getdata and gettxspendingprevout can still name a tx that is gone from the mempool graph; the template can keep the evicted spender. The new test only asserts graph.contains. The parent itself was never published, so unindexing the parent is a no-op — the leak is the newly evicted published spenders.

Suggestion: Return the evicted txids from remove_txid_tree (or unroll evict+remove at the hub). In rollback_1p1c_parent, drop the mempool write lock, then for each gone spender (not the unpublished parent) call unindex_txid, remove fee-deltas, and note_template_update — same shape as remove_for_block_spent. Extend the pin so a hub-level 1p1c child-fail with a concurrent spender leaves wtxid_by_txid / sh_index empty for that spender.

[rearden-grok]

Addressed in f43faf8. remove_txid_tree returns the dropped txids. rollback_1p1c_parent drops the mempool write lock then unindex_evicted, the same helper remove_for_block / remove_for_block_spent use (relay maps, sh_index, fee-deltas, template). Pin: rollback_1p1c_unindexes_published_spenders (paying sibling of a live 1p1c parent; relay_seq_of / accept_time_txid empty after rollback).