#35901 wallet: Fix ScanForWalletTransactions missing tx when look-ahead pool expands mid-block
https://github.com/bitcoin/bitcoin/pull/35901 · · +286/-2 in 3 files, 2 commits · labels: Wallet, Needs rebase
Goal
- Prevent descriptor wallets from silently missing transactions and showing wrong balances during rescans
- Ensures funds are found when look-ahead keypools expand mid-block during descriptor imports or recovery
This PR modifies `ScanForWalletTransactions` in `src/wallet/wallet.cpp` to prevent descriptor wallets from missing transactions during block rescans. When a transaction paying to a key just outside the look-ahead keypool appears before a pool-expanding transaction in the same block, the wallet currently fails to recognize the first transaction. The PR introduces a prefix re-scan pass over the block up to the last pool expansion point and adds a dedicated regression test.
Problem: Descriptor wallet rescans (`rescanblockchain`, `importdescriptors`, `restorewallet`) evaluate transactions in block order. If a transaction paying to a key beyond the current look-ahead pool precedes a transaction that expands the pool within the same block, the earlier transaction is ignored and the wallet reports an incorrect balance without warning.
Category: Wallet (#13 of 84)
P2 · bug fix
- P2 because it fixes a silent bug where block rescans miss transactions and compute incorrect balances
- Affects users recovering funds or importing descriptors when transactions order unfavorably mid-block
The PR fixes a silent wallet bug where transactions are skipped during rescan operations (`rescanblockchain`, `importdescriptors`), leaving users with an incorrect wallet balance without any diagnostic error. The issue is reasonably serious for descriptor wallet users recovering funds, though it is limited to specific intra-block transaction orderings.
Membership: Modifies `CWallet::ScanForWalletTransactions` in `src/wallet/wallet.cpp` to correctly handle look-ahead pool extensions during block scans.
Factors: security/stability 0, bug 2, performance 1, user value 2, leverage 0
Reviewability: Stale: Needs rebase
- Wait for rebase and revisions as PR has merge conflicts with master
- Author is evaluating an alternative algorithm proposal before pushing updates
The PR has merge conflicts with master following the merge of #34681, and the author is also reviewing alternative algorithm proposals before pushing an update.
Author status: Active; currently analyzing jeanpablojp's alternative loop implementation and test cases.
Open concerns:
- The shrinking-prefix re-scan does not revisit transactions located after the expansion point, meaning a subsequent child spend or cascaded receive in the same block remains missed and the balance is still short.
- Reviewer jeanpablojp demonstrated a concrete scenario where key 10 following key 4 and key 5 remains omitted, proposing an alternative loop tracking unmatched transactions instead.
Resolved concerns:
- Avoiding redundant `collect_range_ends()` overhead on transactions that are not wallet-relevant by checking `SyncTransaction()` return value.
- Consolidating the full pass and prefix rescan loops into a single bounded loop.
Agreement: Disputed
- Reviewers agree the bug exists, but proved the prefix re-scan approach does not fully solve it
- Unaddressed objection: receives past the expansion point remain missed, shorting the balance (jeanpablojp)
- Nonblocking objection: child spends after the expansion point are missed, leaving outputs unspent (molnard)
- Concept approval despite double-scan performance considerations (achow101)
Disputed: reviewers proved the prefix re-scan approach leaves intra-block spends and cascading receives missed, shorting the balance.
While everyone agrees the bug exists, jeanpablojp demonstrated that the proposed prefix-only scan does not fully resolve intra-block dependency chains and still leaves the balance short. The author acknowledged the issue and is evaluating an alternative iteration algorithm.
- 2026-08-27 molnard identified that child spends after the expansion point are missed by prefix rescanning.
- 2026-08-29 jeanpablojp proved with a concrete benchmarked example that a cascading receive past scan_end is also omitted.
- 2026-08-29 pablomartin4btc responded that they are analyzing the suggested fix.
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| molnard | correctness | If a child tx spends a parent output paying a lookahead key later in the block, the prefix rescan recovers the parent but never revisits the child, leaving the parent output classified as unspent. | open | no | yes | 2026-08-27: 'The parent output can remain classified as unspent, leaving the wallet balance incorrect until another rescan.' |
| jeanpablojp | correctness | A receive past scan_end that only becomes relevant after an earlier lookahead tx is found is never revisited, causing the balance to remain short. | open | yes | 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.' |
| jeanpablojp | correctness | -walletnotify fired twice for unchanged transactions during full second scan passes | resolved | no | 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' Settled: 2026-08-12: 'In the current code the re-scan covers only [0, last_expansion_pos)... for the common single-expansion case the double notification is gone.' |
Support:
- achow101: Agrees with concept despite double-scan performance consideration [not substantive]
Participants: achow101 (support), molnard (objection), jeanpablojp (objection)
State derived from the lists: blocking objection open, author engaging (jeanpablojp)
Review verdicts (DrahtBot): 0 (+1)
- Stale ACK: jeanpablojp
- Concept ACK: achow101, molnard
Files
251 lines under test/bench/ci.
- test/functional/wallet_rescan_intrablock_ordering.py +250/-0
- src/wallet/wallet.cpp +35/-2
- test/functional/test_runner.py +1/-0
Card
This pull request fixes a bug in descriptor wallet rescanning where ScanForWalletTransactions misses a transaction sent to a key outside the initial look-ahead pool if a pool-expanding transaction appears later in the same block. When this happens, the wallet displays an incorrect balance with no indication that another rescan is required. Reviewers demonstrated that the author's prefix re-scan approach still misses subsequent intra-block spends and cascading receives, leaving the balance short. The PR is currently stale due to merge conflicts with #34681 while the author evaluates a proposed multi-pass loop rewrite.