#35901 wallet: Fix ScanForWalletTransactions missing tx when look-ahead pool expands mid-block
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35901 wallet: Fix ScanForWalletTransactions missing tx when look-ahead pool expands mid-block | Ready | 0 (+1) | Strong | 26 + 728 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Fix a descriptor wallet bug where block rescans miss transactions when a transaction paying just outside the look-ahead pool precedes a pool-expanding transaction in the same block. In `ChainScanner::ScanBlock`, the scan now maintains a list of unmatched transaction indices and retries them until a full pass matches no new transactions or all transactions are matched. It also checks for abort and shutdown requests between retry passes.
Problem: Descriptor wallet rescans (`rescanblockchain`, `importdescriptors`, `restorewallet`) evaluate block transactions strictly in block order. If a transaction paying to an address just beyond the current look-ahead pool appears before a transaction that triggers a pool top-up in the same block, the earlier transaction is ignored; the wallet reports an incorrect balance without warning unless an explicit second rescan is performed.
Evidence of importance:
- Commit message for 54cd292407 provides a reproduction showing that initial block scan misses lookahead transactions and produces an incorrect balance, while a second rescan succeeds.
- PR description details gaps found during review in earlier prefix-rescan implementations: multi-level top-up cascades and intra-block spends of look-ahead outputs were missed unless unconfirmed transactions were retried iteratively.
Scope: The core fix is confined to a compact retry loop in `src/wallet/scan.cpp`, accompanied by comprehensive unit, functional, and benchmark tests.
Risk of a mistake: medium (wallet). A flaw in wallet block rescanning logic could cause infinite loops, hang wallet imports, or misclassify transaction presence and confirmations.
Changed since the previous assessment: Replaced the previous shrinking-prefix rescan approach with a bounded retry-until-stable loop over unmatched transactions, added mid-block abort/shutdown checking, and added a rescan cascade benchmark.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: review of the updated retry-until-stable loop and abort handling from w0xlt, polespinasa, molnard, and jeanpablojp 2026-10-07
Agreement
Strong
substantive support, no open objection (achow101, molnard, polespinasa)
Thread notes: The author replaced the prefix-scan approach with a retry-until-stable loop, resolving the previous objections from molnard (r:5041935034) and jeanpablojp (c:5459727026). The author answered polespinasa's questions and implemented w0xlt's suggestion to check for shutdown/abort between passes.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp (contributor) | objection, correctness | -walletnotify is invoked a second time for unchanged transactions during full second scan passes | accepted · fix pushed · accepted the prefix-bounded re-scan in commit 278564b | answered | yes | 2026-08-10: 'The second walk also re-runs -walletnotify. AddToWallet notifies on every call, so the script fires again for transactions that did not change' Status from: 2026-08-12: jeanpablojp: 'ACK 278564b95c0607fd54892d42a60a001e6b32a236' |
| molnard (none) | objection, correctness | If a child tx spends a lookahead parent later in the block, prefix rescan misses the child and leaves the parent output classified as unspent | fixed · fix pushed · fixed by replacing shrinking-prefix rescan with retry-until-stable loop covering same-block child spends (Part 6) | answered | yes | 2026-08-27: 'The parent output can remain classified as unspent, leaving the wallet balance incorrect until another rescan.' Status from: 2026-10-07: pablomartin4btc: 'a same-block spend of a look-ahead receive only recognized later in the same pass' |
| molnard (none) | suggestion, approach | Avoid calling collect_range_ends() when SyncTransaction() returns false | fixed · fix pushed · implemented in 1d9de0e6 | answered | yes | 2026-08-27: 'We can avoid some calls to collect_range_ends() by checking the return value of SyncTransaction().' Status from: 2026-08-28: pablomartin4btc: 'Done.' |
| molnard (none) | suggestion, maintenance | Use std::move when assigning cur to range_ends | fixed · fix pushed · implemented in 1d9de0e6 | answered | yes | 2026-08-27: 'range_ends = std::move(cur);' Status from: 2026-08-28: pablomartin4btc: 'Done.' |
| molnard (none) | suggestion, approach | Combine the initial scan and prefix rescans into a single loop using scan_end as the bound | fixed · fix pushed · consolidated into a single loop in 1d9de0e6 | answered | yes | 2026-08-27: 'The initial full-block scan and the subsequent prefix rescans can be combined by using scan_end as the exclusive upper bound' Status from: 2026-08-28: pablomartin4btc: 'Done, thanks!' |
| jeanpablojp (contributor) | objection, correctness | A cascading receive past scan_end that becomes wallet-relevant only after the prefix pass recovers an earlier tx is missed, leaving the wallet balance short | fixed · fix pushed · fixed by replacing prefix rescan with retry-until-stable loop (Part 5) | answered | yes | 2026-08-29: 'A receive only becomes the wallet's once the prefix pass recovers an earlier one, and it sits past scan_end. The balance comes out 0.00050000 short.' Status from: 2026-10-07: pablomartin4btc: 'a prefix-recovered receive triggering a further pool expansion past where the prefix had already shrunk to;' |
| polespinasa (member) | question, approach | Why only rescan the block prefix rather than entire or multiple previous blocks, and what is the accepted performance-loss metric? | answered · author replied in detail explaining the full-block retry loop and performance characteristics | answered | yes | 2026-10-06: 'This PR proposes the prefix of the block, why not 1 entire block? Why not N previous blocks? Which is the accepted performance-loss metric?' Status from: 2026-10-07: pablomartin4btc: 'The PR description needs updating — the current approach goes through the entire block, not a prefix.' |
| w0xlt (contributor) | suggestion, safety | Check for shutdown and abort requests between passes in the retry loop | fixed · fix pushed · implemented and tested in commit pushed with c:6044809901 | answered | yes | 2026-10-07: 'Since you’re changing this code and the loop now retries transactions, you could also check for shutdown and abort requests between passes.' Status from: 2026-10-07: pablomartin4btc: 'Done.' |
Support:
- achow101 (member) (Concept ACK, 2026-08-10) [read]: Agrees with the concept of fixing the lookahead miss despite scanning blocks twice when a lookahead output is found
- jeanpablojp (contributor) (ACK, 2026-08-12): (no reason given) [not substantive]
- molnard (none) (Concept ACK, 2026-08-27) [read]: Agrees the bug is valid while noting the unhandled child-spend case
- polespinasa (member) (Concept ACK, 2026-10-06) [read]: I think this is a nice patch and the only re-check the pre-fix is clean and optimum.
- vicjuma (contributor) (Concept ACK, 2026-10-06): (no reason given) [not substantive]
- w0xlt (contributor) (Concept ACK, 2026-10-07): (no reason given) [not substantive]
Participants:
- achow101 (member, support, 2 statements 2026-08-10 to 2026-08-10): Concept ACK despite double-scan performance trade-off
- jeanpablojp (contributor, objection, 3 statements 2026-08-10 to 2026-08-29): demonstrated that cascading receives past scan_end are missed, leaving wallet balance short
- molnard (none, objection, 4 statements 2026-08-27 to 2026-08-27): Concept ACK on bug fix, but noted child transactions spending lookahead outputs after expansion point are missed
- polespinasa (member, support, 1 statements 2026-10-06 to 2026-10-06): Concept ACK; questioned rescan scope beyond the block prefix and performance metrics
- vicjuma (contributor, support, 2 statements 2026-10-06 to 2026-10-06): Concept ACK and suggested updating the PR description
- w0xlt (contributor, support, 2 statements 2026-10-07 to 2026-10-07): Concept ACK; suggested checking for shutdown/abort requests between retry passes
Files
729 lines under test/bench/ci.
- test/functional/wallet_rescan_intrablock_ordering.py +444/-0
- src/bench/wallet_rescan_cascade.cpp +193/-0
- src/wallet/test/rescan_tests.cpp +89/-0
- src/wallet/scan.cpp +26/-5
- src/bench/CMakeLists.txt +1/-0
- src/wallet/test/wallet_tests.cpp +0/-1
- test/functional/test_runner.py +1/-0
Card
The short text the ranking pass compares across PRs.
This pull request fixes a descriptor wallet rescan bug where transactions paying to addresses outside the current look-ahead pool are missed if a pool-expanding transaction appears later in the same block. When this happens during rescans or wallet imports, the wallet displays a lower-than-actual balance with no indication that transactions were skipped. The fix iteratively retries unmatched transactions in the block until no new wallet transactions are detected, ensuring cascading pool expansions and same-block spends of look-ahead outputs are discovered. It also adds a cascade rescan benchmark and allows mid-block aborts during multi-pass rescans.
Data
thread read through 2026-10-07 (35 statements), head 5f62bf08a0
labels: Wallet
Categories
Wallet
P1 · bug fix
Fixes a silent failure to detect transactions during descriptor wallet rescans and restores when an intra-block output falls just outside the look-ahead pool. Leaving transactions and wallet balances incorrect without warning during `restorewallet` or `importdescriptors` is a serious recovery and balance correctness bug that should not persist across releases.
Membership: The PR carries the Wallet label and fixes transaction scanning logic during block rescans in `src/wallet/scan.cpp`.
Factors: security/stability 1, bug 2, performance 0, user value 2, leverage 0