[vincenzopalazzo]

IBD teardown sets the confirm cancel and then joins the confirm engine. The lookup thread only checked for a stop between waves. A single wave resolves its TipOnly need, up to BQ_RESOLVE_WAVE_MAX_INPUTS (64k) parent txids, in one get_fk_by_txid_batch. On a busy HDD that one resolve takes most of an hour, and stop, SIGTERM and a catch-up give-up all wait for it to finish.

What we saw

This was a mainnet node with --datadir-hdd --sh-index, about block 512,900, on one 7200 rpm disk that a Bitcoin Core IBD was also using. A 30-minute internet outage made a build from before 2dcf5b8 give up catch-up after four empty redials. 2dcf5b8 now keeps redialing in Direct mode. The teardown then logged still waiting for confirm engine for 58 minutes before the process exited.

Change

• Store. get_fk_by_txid_batch_cancellable is the same TipOnly resolve as get_fk_by_txid_batch (one open probe, one sealed walk, one create.loc batch).
  • It asks cancelled before the resolve, before each sealed segment, and before each identity shot, and returns StoreError::Cancelled("txid batch") when the cancel is set.
  • A stop waits for the in-flight segment or shot. The need is not split, so a live wave does not repeat the sealed walk.
  • get_fk_by_txid_batch_mode uses the same path with a callback that is never cancelled, so this adds no second resolve path.
• Query. confirm_bq_resolve_wave_capped resolves its need through the new function and checks query.confirm_cancelled().
• IBD. LookupFaultPolicy::on_err maps a cancelled error to the new LookupFaultAction::Stop. The lookup thread then logs ibd: confirm lookup cancelled mid-wave and leaves its loop. A cancelled wave no longer counts toward LOOKUP_FAULT_HALT_AFTER, so it can't emit an engine-fault reject on the way out.

request_confirm_cancel is only called from IBD teardown, and clear_confirm_cancel runs at IBD start. A running sync uses this resolve and the callback stays false, so it never takes the cancelled return.

Measured on the same mainnet datadir

I sent SIGTERM while IBD was running and the confirm lookup was busy, under the same shared-disk load each time.

Build	SIGTERM to exit
master 4c566fc	2,605 s
this branch	221 s

The patched teardown logged confirm lookup cancelled mid-wave, then confirm lookup exit, then confirm engine stopped (94.8s). The rest of the 221 s is the node's shutdown after the confirm engine stopped, on the same disk.

Tests

• rbitcoin-query confirm_run::bq_resolve::wave_under_confirm_cancel_returns_cancelled: a wave started under the confirm cancel returns a cancelled error and doesn't resolve its need.
• rbitcoin-query confirm_run::bq_resolve::txid_batch_asks_cancel_before_the_identity_shot:
  • Without a cancel, the one resolve returns the same rows as the one-shot batch.
  • A cancel set on the first ask returns cancelled before any identity shot.
• rbitcoin-ibd ibd::confirm::lookup_fault_policy_stops_on_cancel: a cancelled error is Stop every time and never adds to the halt count.

Local gates

• cargo fmt --all -- --check: passes.
• cargo clippy --workspace --all-targets -- -D warnings: passes on macOS, with rbitcoin-net limited to --lib, because its test targets need libc and only build on Linux.
• ./scripts/ast-grep.sh (ast-grep 0.45.1, Linux): passes.
• cargo test --workspace --no-fail-fast (Linux, rustc 1.95.0):
  • 13 test targets pass outright, about 2,700 tests. The three new tests pass.
  • 4 existing tests fail. Each one also fails without this change or passes when run alone. They're listed below.
• ./scripts/ci-os-smoke.sh (macOS): passes.
• I didn't run cargo deny, because no dependencies changed. Coverage runs in CI only.

The four failing tests:

• ibd::events::confirm_reject_tests::ibd_mutated_body_is_refetched_not_blacklisted (repeated tail: sender dropped) and ibd::events::confirm_reject_tests::ibd_spend_of_an_unknown_txid_is_invalid (pump stall).
  • These are flaky with or without this change.
  • Over four full rbitcoin-ibd suite runs each on macOS, master 8e62e4c failed three, 4c566fc failed three and this branch failed two.
  • Both also fail on 4c566fc on Linux.
• cli::trust_peer_addr passes alone. It races with another test over the RBITCOIN_P2P_MAX_INBOUND environment variable.
• end_of_ibd::end_of_ibd_work_fork hit its 30 s wall during the full parallel run.
  • Alone it takes 12 to 15 s on both master and this branch.
  • The end_of_ibd:: group passes 3 of 3 on Linux on both commits.

🤖 Generated with Claude Code

[rearden-grok]

Verdict: COMMENT

The cancel path is correct and I would not block on it. One correction to the write-up: a running confirm lookup always uses the chunked resolver, not only teardown.

Findings

Steady-state waves always chunk (not blocking). confirm_bq_resolve_wave_capped always calls get_fk_by_txid_batch_cancellable with 4096. confirm_cancelled() is false for a live sync (request_confirm_cancel is only IBD teardown, after request_stop). Each piece is a full resolve_fk_and_range_batch_with_tip (open probe, sealed walk, txid.body, own create.loc batch) under one fence snapshot. get_fk_by_txid_batch_mode is the one-shot path (chunk = txids.len()). The lookup wave is not. docs/heads.md and docs/concurrency.md still say one get_fk_by_txid_batch. The published timing is SIGTERM-to-exit (2605s → 221s), not confirm rate.

What I checked and accept:

• Cancel is polled before each piece; the in-flight piece finishes; partial rows are dropped; ? is before promote, so a cancelled wave does not promote or dequeue.
• LookupFaultAction::Stop does not increment consecutive and breaks the lookup loop the same way feed.stopped() does, then notifies and joins load. Load/scripts already poll the flag. No engine-fault reject on the way out.
• is_cancelled() does not overlap uring-session or SQ-backpressure. The unsynced-head Cancelled is a different function.
• Resolve is per key, so a chunk boundary cannot change which connected sibling wins. One fence snapshot is shared. chunk == 0 becomes 1. Empty input still clears the thread-local leftover miss.
• No consensus/validation change, no new unwrap on peer/file input, no dependency change.

Nit. wave_under_confirm_cancel_returns_cancelled only asserts the error, not that the height stays queued.

Nit. Decode and key collection still run before the first cancel check. That is not the HDD wait in the report.

Built and tested

Head e69ba3dba7b1a1f73b40c3a3122cbded9ab0dfd1, CARGO_BUILD_JOBS=1.

• cargo fmt --all -- --check: exit 0
• cargo test -p rbitcoin-query --test suite confirm_run::bq_resolve::: 20 passed
• cargo test -p rbitcoin-ibd --test suite ibd::confirm::lookup_fault_policy: 2 passed
• cargo test -p rbitcoin-store --test suite get_fk_by_txid_batch: 3 passed
• cargo clippy -p rbitcoin-store -p rbitcoin-query -p rbitcoin-ibd --lib --tests -- -D warnings: exit 0

[coreyphillips]

Two independent reviews.

nits

• Docs still describe the lookup head read as one batch (crates/rbitcoin-ibd/src/ibd/perf_log.rs:374). Some docs still describe the wave's head read as a single batch: - perf_log.rs:374 still documents lookup_wave_head_ms as "TipOnly get_fk_by_txid_batch". - perf_log.rs:383 says the create.loc fill is "One batch after identity". - bq_resolve.rs:145 says the same: "one batch after identity waves". With pieces, the wave now runs one create.loc batch per piece. The PR updated the neighbouring head_ns doc and docs/concurrency.md but missed these lines.
• Changelog cites a 2605s to 221s figure the note never gives (changelog.d/lookup-wave-cancel.md:10). The last sentence of the fragment reads "The 2605s to 221s figure is SIGTERM-to-exit, not confirm rate." The note never states that figure, so a reader of CHANGELOG.md after the release cut has no context for it. Either state the measurement (for example, "SIGTERM to exit went from 2,605 s to 221 s on one mainnet HDD datadir") or drop the sentence.

the reviewers disagree, your call

• Every live lookup wave now resolves in pieces, and confirm rate is unmeasured (crates/rbitcoin-query/src/confirm_run/bq_resolve.rs:369). Objection: Live waves do run complete resolves per 4,096-key piece, but that does not establish the predicted HDD slowdown. The performance regression remains unmeasured, and I could not independently verify the PR-body wording. Answer: I checked both parts. The PR body still says "request_confirm_cancel is only called from IBD teardown ... A running sync never hits the new path" (gh pr view 1189), but bq_resolve.rs:369 sends every live wave through resolve_txid_pieces, and store.rs runs resolve_identity_core (open probe, each sealed segment, create_loc.range_batch) once per 4,096-key chunks() piece. So the body tells a reviewer to skip the hot-path question, even though the second commit's own title concedes the always-chunked behavior. I am not claiming a measured regression. The claim is that the description is wrong and a lookup-batch shape change went in without the operator-host A/B that AGENTS.md requires for perf, which is worth one line of follow-up before merge.

[rearden-grok]

REQUEST_CHANGES. I would not merge 723e892d while every live lookup wave pays for a full resolve per 4096 txids.

Earlier rearden-grok findings:

• Live waves always resolve in 4096-txid pieces, and confirm cost is unmeasured: still open. docs/heads.md, docs/concurrency.md, and the head_ns doc now say 4096. The wave still chunks, and lookup_wave_head_ms is still unmeasured.
• The cancel test did not assert the height stays queued: fixed (block_queue_has_height(1)).
• Decode runs before the first cancel check: still open. Nit. confirm_cancelled is read only inside get_fk_by_txid_batch_cancellable, after decode and key collection.

Corey:

• Stale one-batch docs: agree. perf_log.rs:374 still documents lookup_wave_head_ms as get_fk_by_txid_batch. perf_log.rs:383 and bq_resolve.rs:145 still say one create.loc batch after identity. Each piece calls range_batch on its own. Nit.
• Changelog cites 2605s to 221s: agree. changelog.d/lookup-wave-cancel.md:10 uses the figure and says it is SIGTERM-to-exit, and never states the measurement. Nit.
• Piece-count on the live path: agree on the mechanism. confirm_bq_resolve_wave_capped always calls get_fk_by_txid_batch_cancellable with 4096, and each piece is a full resolve_identity_core plus its own create.loc batch. A 64k-key wave is 16 pieces; the 256k key cap is about 64. The PR body still says a running sync never hits the new path. Disagree that AGENTS.md requires a host A/B before merge: that line says a perf A/B is operator-host only. The +20% head ms/blk fail-ship bar lives in docs/io-modality.md, and the must-measure case there is a demap.

Required change is the inline on resolve_txid_pieces. G-page loads for a segment run again for every piece that still touches it. Body and loc reads are partitioned and lose page coalescing. Principle 9 wants that cost named; the changelog names the piece size and says a live sync never sets the flag. A cancel check in resolve_identity_core's sealed-segment loop, on one resolve of the whole need, keeps one g-page sweep per segment. A single id shot can still be the long HDD wait, so split that sorted pread if one shot is too long to stop. Keeping the 4096-key resolves needs an operator-host lookup_wave_head_ms (or blocks/s) against the merge base on the --datadir-hdd setup. docs/io-modality.md fails a ship at +20% head ms/blk with no tip-rate win.

0 bugs, 1 required change, 2 nits.

[rearden-grok]

APPROVE. I would merge 05717187. The 4096-piece resolve is gone, and a live wave is one open probe, one sealed walk, and one create.loc batch again.

The 723e892 required change is fixed. resolve_txid_pieces is gone. get_fk_by_txid_batch and get_fk_by_txid_batch_mode both call resolve_txid_batch, and the mode path passes a callback that returns false. The lookup wave calls get_fk_by_txid_batch_cancellable with confirm_cancelled. That is the same resolve_fk_and_range_batch_cancellable of the whole need. The extra work on a live wave is the callback: once before the resolve, once per sealed segment, and once per identity shot. It does not repeat g-page loads or split body and loc reads. The range-diff shows the first two PR commits unchanged by the rebase and this behavior only in 05717187.

Vincenzo's three replies match the tree, with two limits. The cancel is checked at store.rs:1179, at the top of the sealed-segment loop, and at the start of each identity shot. It is not checked again before attach_loc_to_identity, so a cancel that lands during the last shot still runs the full create.loc batch. The PR body now says a running sync uses this resolve and the callback stays false. The 2,605 s to 221 s table is still in the PR body and is the old piece-sized run, not this head.

Still-open nit from 723e892: decode and key collection still run before that first cancel check. The other two nits are fixed: perf_log.rs:374 and :383 and bq_resolve.rs:145 now say one cancellable resolve and one loc batch after the sealed walk, and changelog.d/lookup-wave-cancel.md no longer cites 2605 s to 221 s.

Corey Phillips: (a) stale one-batch docs, fixed. (b) changelog figure with no measurement, fixed. (c) live waves in 4096 pieces, moot. That path is gone, and the between-segment check he suggested is what landed, plus a check before each identity shot.

Nit: txid_batch_asks_cancel_before_the_identity_shot returns true on the first ask, which is the check in resolve_txid_batch before the resolve. Removing the checks in the segment loop and in id_idx_wave would still pass. The three new tests are a 1-block wave, a 3-coinbase batch, and a policy loop. No sleeps, timeouts, or large fixtures, so they sit inside the 2 s budget. The CI suite deltas are noise.

0 bugs, 0 required changes, 2 nits.