wallet: Fix ScanForWalletTransactions missing tx when look-ahead pool expands mid-block #35901

pablomartin4btc commented at 5:29 PM on August 5, 2026: member

When ScanForWalletTransactions processes a block containing both a transaction to a key just outside the look-ahead pool and a pool-expanding transaction that triggers TopUp, and the former appears first in vtx order, the wallet misses the earlier transaction — leaving the balance wrong with no indication a second scan is needed. The fix tracks the last vtx position where the pool expanded and re-scans only the prefix [0, last_expansion_pos) — every transaction processed before the pool reached its final state.

The first commit demonstrates the incorrect behaviour; the second contains the fix.

<details> <summary>Bug details...</summary>

ScanForWalletTransactions iterates over a block's transactions in the order the miner placed them (vtx order). A descriptor wallet maintains a look-ahead pool of pre-derived keys so it can recognise incoming payments. When a payment arrives at a key near the pool boundary, MarkUnusedAddresses → TopUp extends the pool.

If within a single block:

Tx_lookahead pays to key index N (just outside the current pool [0, N-1]), AND
Tx_expand pays to key index N-1 (last in pool — triggers TopUp, extending pool to [0, 2N-1])

and Tx_lookahead appears at an earlier vtx position than Tx_expand, the scan misses Tx_lookahead:

Tx_lookahead processed: pool = [0, N-1] → IsMine returns false → missed
Tx_expand processed: key N-1 found → TopUp fires → pool = [0, 2N-1]

FastWalletRescanFilter::UpdateIfNeeded() fires only at the start of the next block iteration — there is no mechanism to re-examine Tx_lookahead within the same scan pass.

The transaction is not permanently lost — because Tx_expand extended the pool as a side effect, a second explicit rescanblockchain call recovers Tx_lookahead. But the wallet shows a wrong balance with no indication a second scan is needed.

This bug does not affect live blockConnected when both transactions pass through the mempool first: transactionAddedToMempool(Tx_expand) pre-extends the pool before the block arrives, so vtx ordering does not matter. It only manifests in ScanForWalletTransactions (rescan paths) for descriptor wallets.

</details>

<details> <summary>Affected callers...</summary>

Callers affected (all use ScanForWalletTransactions):

rescanblockchain
importdescriptors (via RescanFromTime)
restorewallet and wallet migration watchonly/solvable wallets (via AttachChain)

</details>

<details> <summary>Fix details and performance...</summary>

Fix: during the per-block vtx loop, snapshot range_end per HD descriptor after every transaction. Track the last vtx position where any descriptor's range_end increased (last_expansion_pos). After the full pass, re-scan only the prefix [0, last_expansion_pos) — transactions processed before the pool reached its final state. Transactions at last_expansion_pos and beyond were already seen with the fully-expanded pool and are not re-visited.

Using the last (not first) expansion position matters when multiple TopUp events occur in one block. With interleaved vtx order [lookahead_1, expand_1, lookahead_2, expand_2], stopping at the first expansion position (1) re-scans only [0, 1) and misses lookahead_2 at position 2. Stopping at the last (3) re-scans [0, 3) and finds both.

The re-scan repeats with the new last_expansion_pos if a transaction within the prefix itself causes a further pool expansion. The prefix strictly shrinks each iteration, guaranteeing termination without a safety cap.

Performance: the prefix re-scan fires for any block where TopUp fires during the vtx loop — effectively, any block containing a wallet-relevant transaction during a rescan. The re-visited prefix is [0, last_expansion_pos), proportional to where the last pool-expanding transaction sits within the block. Transactions at that position and beyond are visited exactly once.

</details>

<details> <summary>Test coverage...</summary>

Regression test: test/functional/wallet_rescan_intrablock_ordering.py

Part 1: loadwallet rescan — Tx_lookahead found after fix
Part 2: importdescriptors full rescan from genesis — Tx_lookahead found after fix
Part 3: second rescanblockchain recovers Tx_lookahead (pre-fix behaviour, kept as sanity check)
Part 4: two-level cascade — two successive TopUp events fire in one block, both lookahead txs found, validating the bounded multi-pass loop

</details>

<details> <summary>Manual reproduction (<code>regtest</code>)</summary>

bitcoind -regtest -keypool=5 -fallbackfee=0.0001 -daemon

bitcoin-cli -regtest -named createwallet wallet_name=funding
bitcoin-cli -regtest generatetoaddress 102 $(bitcoin-cli -regtest -rpcwallet=funding getnewaddress)

bitcoin-cli -regtest -named createwallet wallet_name=test
RECV_DESC=$(bitcoin-cli -regtest -rpcwallet=test listdescriptors \
  | jq -r '[.descriptors[] | select(.internal==false and .range!=null)][0].desc')
END_RANGE=$(bitcoin-cli -regtest -rpcwallet=test listdescriptors \
  | jq '[.descriptors[] | select(.internal==false and .range!=null)][0].range[1]')
ADDR_EXPAND=$(bitcoin-cli -regtest deriveaddresses "$RECV_DESC" "[$END_RANGE,$END_RANGE]" | jq -r '.[0]')
ADDR_LOOKAHEAD=$(bitcoin-cli -regtest deriveaddresses "$RECV_DESC" "[$((END_RANGE+1)),$((END_RANGE+1))]" | jq -r '.[0]')

bitcoin-cli -regtest unloadwallet test

TXID1=$(bitcoin-cli -regtest -rpcwallet=funding sendtoaddress "$ADDR_LOOKAHEAD" 0.001)
TXID2=$(bitcoin-cli -regtest -rpcwallet=funding sendtoaddress "$ADDR_EXPAND" 0.001)
bitcoin-cli -regtest generateblock "raw(51)" "[\"$TXID1\",\"$TXID2\"]"

# First scan — TXID1 missing, wrong balance
bitcoin-cli -regtest loadwallet test
bitcoin-cli -regtest -rpcwallet=test listtransactions

# Second scan — TXID1 recovered
bitcoin-cli -regtest -rpcwallet=test rescanblockchain
bitcoin-cli -regtest -rpcwallet=test listtransactions

</details>

Notes:

Originally noted by furszy in #31629 (comment); his "case 2" describes a related but harder inter-block variant: new blocks arriving via blockConnected during an active rescan with a not-yet-expanded pool. That problem remains a potential follow-up. This PR fixes the simpler intra-block sub-case where the miss happens entirely within a single block's vtx loop, with no concurrent block arrivals required.
Perhaps the issue is related to the symptom was previously reported in #19808 but could not be reproduced at the time and was closed without a fix.
#34681 refactors ScanForWalletTransactions into a ChainScanner class and touches the same code. If that PR merges first, this fix would need to move into ChainScanner::ScanBlock.
molnard found a related gap this PR doesn't close: if a parent tx pays a look-ahead key and a later tx in the same block spends that parent's output, and the spend sits at or after the block's last pool expansion, the prefix rescan recovers the parent but never revisits the spend — the parent output can remain classified as unspent. Fixing this properly needs a different approach (retry based on each tx's SyncTransaction() result until a full pass causes no change, rather than a shrinking-prefix rescan), which is a separate piece of work from this fix. Tracked as a follow-up, same as the furszy case above.

DrahtBot added the label Wallet on Aug 5, 2026

DrahtBot commented at 5:29 PM on August 5, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35901.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	achow101, molnard
Stale ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

pablomartin4btc force-pushed on Aug 5, 2026

DrahtBot added the label CI failed on Aug 5, 2026

DrahtBot commented at 5:35 PM on August 5, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/31030226987/job/92388765580 LLM reason (✨ experimental): CI failed because the lint-files test found a Python file with a shebang (test/functional/wallet_rescan_intrablock_ordering.py) but incorrect permissions (644 instead of executable 755).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

pablomartin4btc force-pushed on Aug 5, 2026

DrahtBot removed the label CI failed on Aug 5, 2026

jeanpablojp commented at 1:03 AM on August 10, 2026: contributor

This fixes a real bug. With the wallet.cpp hunk reverted the new test fails at Part 1 and the transaction is absent from listtransactions, and putting the hunk back makes it pass. Unit suite clean, wallet_* functional clean. The extra pass is also safe to repeat: CWalletTx::Update returns false when the wtxid and the state index are unchanged, so a second pass does not write again or count anything twice.

Two things came up when I was validating. Neither breaks anything, and both look cheap to tidy if you agree with them.

The extra pass runs more often than the description suggests. What the loop tests is whether range_end moved during the block, and any receive to a key at or past next_index moves it, so it isn't limited to the ordering you describe. With -blockfilterindex=1, 10 receives to 10 unused keys in 10 separate blocks, the rescan fetched 10 blocks and walked all 10 of them twice. Nothing goes wrong on the second walk, it just re-finds what it already had. The fast variant only fetches blocks the filter matched, which are largely the blocks that expand the pool, so "most rescans process each block exactly once" probably wants rewording even if the code stays as it is.

The second walk also re-runs -walletnotify. AddToWallet notifies on every call, so the script fires again for transactions that did not change: one invocation on master, two on this branch, same wallet and same txid, with debug.log logging the second as no-change Confirmed. The option is documented as "execute command when a wallet transaction changes", and on the second walk nothing changed.

Both go away if the re-walk covers only the prefix before the first expansion: keep the lowest vtx position where range_end moved and stop there. In the common case the expanding transaction is the only match in the block, so nothing gets walked twice. Does that miss a case I'm not seeing?

in src/wallet/wallet.cpp:1993 in a826c73d2d

1990 | +                    for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
1991 | +                        SyncTransaction(block.vtx[posInBlock], TxStateConfirmed{block_hash, block_height, static_cast<int>(posInBlock)}, /*rescanning_old_block=*/true);
1992 | +                    }
1993 | +                    auto new_range_ends = collect_range_ends();
1994 | +                    if (new_range_ends == range_ends) break;
1995 | +                    if (pass >= block.vtx.size()) break; // safety cap

achow101 commented at 7:32 PM on August 10, 2026:

In a826c73d2ddcbc8d2c8723a01093c94ff596434c "wallet: re-process block in ScanForWalletTransactions if pool expands mid-block"

nit: This can be in the for rather than an explicit break.

pablomartin4btc commented at 3:27 AM on August 12, 2026:

The overall structure changed since this comment — the old two-pass loop is replaced by a last_expansion_pos approach — but the spirit of the suggestion is there: the new inner for loop uses pos_in_block < *last_expansion_pos as its bound rather than an explicit break. Thanks!

achow101 commented at 7:36 PM on August 10, 2026: member

Concept ACK-ish

This does mean that every time we find a transaction with an output in our lookahead, we end up having to scan that block twice.

pablomartin4btc force-pushed on Aug 11, 2026

DrahtBot added the label CI failed on Aug 11, 2026

DrahtBot removed the label CI failed on Aug 12, 2026

test: Wallet misses tx when look-ahead pool expands mid-block

Demonstrates that ScanForWalletTransactions misses a transaction paying
to a key just outside the look-ahead pool when the pool-expanding
transaction appears later in the same block's vtx order.

Part 1: wallet reload triggers rescan of one block; tx_lookahead is
        missed on the first scan (wrong balance, no indication a
        second scan is needed).
Part 2: importdescriptors with timestamp=0 triggers a full rescan;
        same miss.
Part 3: a second rescanblockchain recovers tx_lookahead — the pool
        was extended as a side-effect of finding tx_expand, so the
        key is visible on re-scan.

TODO: Parts 1 and 2 assertions will be flipped in the fix commit.

c4f65f7e3b

pablomartin4btc force-pushed on Aug 12, 2026

pablomartin4btc commented at 4:25 AM on August 12, 2026: member

@jeanpablojp,

Thanks for the thorough review and for actually running the test!

The extra pass runs more often than the description suggests. What the loop tests is whether range_end moved during the block, and any receive to a key at or past next_index moves it, so it isn't limited to the ordering you describe. With -blockfilterindex=1, 10 receives to 10 unused keys in 10 separate blocks, the rescan fetched 10 blocks and walked all 10 of them twice. Nothing goes wrong on the second walk, it just re-finds what it already had. The fast variant only fetches blocks the filter matched, which are largely the blocks that expand the pool, so "most rescans process each block exactly once" probably wants rewording even if the code stays as it is.

On "runs more often" — still partially valid: the re-scan fires whenever range_end moves during a block, not only in the vtx-ordering scenario. What changed from the previous version is that the re-scan is now bounded to the prefix [0, last_expansion_pos) rather than the full block, so the overhead is smaller. Fair to say the PR description needs rewording there.

The second walk also re-runs -walletnotify. AddToWallet notifies on every call, so the script fires again for transactions that did not change: one invocation on master, two on this branch, same wallet and same txid, with debug.log logging the second as no-change Confirmed. The option is documented as "execute command when a wallet transaction changes", and on the second walk nothing changed.

On -walletnotify — you were testing the old version, which re-walked the full block and re-synced every already-known tx. In the current code the re-scan covers only [0, last_expansion_pos). In the common case (single expansion at position P), last_expansion_pos = P, so the prefix [0, P) contains only the transactions processed before the pool expanded — i.e. the missed lookahead, which is recorded for the first time in the re-scan, not a second time. No double notification. In the cascade case an expander from the first expansion lands inside the prefix and does get a second AddToWallet call, so one extra notification per cascade level. But for the common single-expansion case the double notification is gone.

Both go away if the re-walk covers only the prefix before the first expansion: keep the lowest vtx position where range_end moved and stop there. In the common case the expanding transaction is the only match in the block, so nothing gets walked twice. Does that miss a case I'm not seeing?

On first vs last — I didn't take that suggestion exactly. Part 4 of the test shows the case you asked about: with vtx order [lookahead_1, expand_1, lookahead_2, expand_2], stopping at the first expansion position (1) re-scans only [0, 1) — finds lookahead_1 but still misses lookahead_2 at position 2. last_expansion_pos (3) re-scans [0, 3) and finds both. For the single-expansion common case the two are equivalent (first_expansion_pos vs last_expansion_pos).

pablomartin4btc commented at 4:28 AM on August 12, 2026: member

This does mean that every time we find a transaction with an output in our lookahead, we end up having to scan that block twice. @achow101, the re-scan is bounded at last_expansion_pos, so it's only the prefix of the block — not the full block again. In the common single-expansion case the expansion transaction is at vtx position P; the re-scan covers [0, P), and the expansion tx plus everything after it is visited exactly once. You're right that any block where the pool expands mid-scan gets a partial re-walk; I'll tighten the performance language in the description.

pablomartin4btc commented at 4:58 AM on August 12, 2026: member

-<ins>Updates</ins>:

Addressed feedback from both @jeanpablojp and @achow101 (replies inline).
Updated the PR description to accurately reflect the current implementation:
- Intro and fix details now describe the last_expansion_pos approach (the description was still describing the old pass-counter/full-rerun version);
- Performance section corrected: the prefix re-scan fires for any block where TopUp fires during the vtx loop (any wallet-relevant block during rescan), not only in the specific vtx-ordering scenario @jeanpablojp tested.

jeanpablojp commented at 8:41 AM on August 12, 2026: contributor

ACK 278564b95c0607fd54892d42a60a001e6b32a236

DrahtBot requested review from achow101 on Aug 12, 2026

in src/wallet/wallet.cpp:1989 in 278564b95c

1986 | +                // The prefix shrinks each pass, guaranteeing termination.
1987 | +                auto range_ends = collect_range_ends();
1988 | +                std::optional<size_t> last_expansion_pos;
1989 | +                for (size_t pos_in_block = 0; pos_in_block < block.vtx.size(); ++pos_in_block) {
1990 | +                    SyncTransaction(block.vtx[pos_in_block], TxStateConfirmed{block_hash, block_height, static_cast<int>(pos_in_block)}, /*rescanning_old_block=*/true);
1991 | +                    auto cur = collect_range_ends();

molnard commented at 2:24 PM on August 27, 2026:

In 278564b95c0607fd54892d42a60a001e6b32a236

We can avoid some calls to collect_range_ends() by checking the return value of SyncTransaction(). If it returns false, the transaction did not involve the wallet, so TopUp() could not have been called and collect_range_ends() cannot return a different result.

pablomartin4btc commented at 8:24 PM on August 28, 2026:

Nice catch! Capturing the return value from SyncTransaction, especially when it's false, avoids wasted work on every non-wallet-relevant tx — which is the majority of transactions in most blocks during a rescan! I'll take this suggestion, thanks!

pablomartin4btc commented at 8:39 PM on August 28, 2026:

Done.

in src/wallet/wallet.cpp:1992 in 278564b95c

1989 | +                for (size_t pos_in_block = 0; pos_in_block < block.vtx.size(); ++pos_in_block) {
1990 | +                    SyncTransaction(block.vtx[pos_in_block], TxStateConfirmed{block_hash, block_height, static_cast<int>(pos_in_block)}, /*rescanning_old_block=*/true);
1991 | +                    auto cur = collect_range_ends();
1992 | +                    if (cur != range_ends) {
1993 | +                        last_expansion_pos = pos_in_block;
1994 | +                        range_ends = cur;

molnard commented at 2:35 PM on August 27, 2026:

                        range_ends = std::move(cur);

This is normally much cheaper.

pablomartin4btc commented at 8:12 PM on August 28, 2026:

I do agree, will take your suggestion, thanks!

pablomartin4btc commented at 8:39 PM on August 28, 2026:

Done.

in src/wallet/wallet.cpp:1995 in 278564b95c

1992 | +                    if (cur != range_ends) {
1993 | +                        last_expansion_pos = pos_in_block;
1994 | +                        range_ends = cur;
1995 | +                    }
1996 | +                }
1997 | +                while (last_expansion_pos.has_value() && *last_expansion_pos > 0) {

molnard commented at 2:48 PM on August 27, 2026:

In 278564b:

It may be simpler to keep the call to SyncTransaction() in one place. The initial full-block scan and the subsequent prefix rescans can be combined by using scan_end as the exclusive upper bound:

auto range_ends = collect_range_ends();
size_t scan_end = block.vtx.size();

while (scan_end > 0) {
    std::optional<size_t> next_scan_end;

    for (size_t pos_in_block = 0; pos_in_block < scan_end; ++pos_in_block) {
        SyncTransaction(
            block.vtx[pos_in_block],
            TxStateConfirmed{
                block_hash,
                block_height,
                static_cast<int>(pos_in_block),
            },
            /*rescanning_old_block=*/true);

        auto cur = collect_range_ends();
        if (cur != range_ends) {
            next_scan_end = pos_in_block;
            range_ends = std::move(cur);
        }
    }

    if (!next_scan_end.has_value()) break;
    scan_end = *next_scan_end;
}

The first pass scans the entire block. Each subsequent pass scans only the prefix before the last detected expansion.

pablomartin4btc commented at 8:33 PM on August 28, 2026:

I'll take it — merges the initial full-block for loop and the separate while-wrapped prefix rescan into one while (scan_end > 0) loop, shrinking scan_end to the last detected expansion each pass. Same termination guarantee, less duplication, and the natural single place for the early-skip from the other suggestion instead of repeating it in two loop bodies.

pablomartin4btc commented at 8:39 PM on August 28, 2026:

Done, thanks!

molnard commented at 3:13 PM on August 27, 2026: none

Concept ACK. The bug is valid, but I think the prefix-only rescan leaves the following case unresolved:

Scenario examined

A parent transaction pays key N, outside the initial look-ahead pool.
A later transaction pays key N-1 and expands the pool.
A subsequent child transaction spends the parent output to an external address.

Observed behavior

The first pass misses the parent.
The expander extends the pool.
The child is not recognized because the parent has not yet been added to the wallet.
The prefix rescan subsequently finds the parent, but it stops before the expander and therefore never revisits the child.

Consequence

The parent output can remain classified as unspent, leaving the wallet balance incorrect until another rescan.

Suggested coverage

Add a functional test case with the tx order:

- lookahead parent
- expander
- child spending the lookahead parent

Consider whether transactions after the expansion must also be reconsidered when the prefix pass discovers a previously missed wallet transaction.

Update

I was thinking about a solution... Briefly, a possible implementation is to iterate through the block and keep track of transactions for which SyncTransaction() already returned true, excluding them from later iterations (fix: walletnotify). Repeat this until an iteration causes no descriptor expansion.

This allows previously missed transactions to be retried in block order after the look-ahead pool expands.

wallet: Fix ScanForWalletTransactions when look-ahead pool expands mid-block

ScanForWalletTransactions iterates over a block's transactions in vtx
order. When a pool-expanding transaction appears *after* a transaction
paying to a key just outside the look-ahead pool, the earlier
transaction is missed: it was processed with a smaller pool (IsMine
returned false), and FastWalletRescanFilter::UpdateIfNeeded() only
fires at the next block boundary so there is no mechanism to
re-examine it within the same scan pass.

Fix: track the last vtx position where any HD descriptor's range_end
increased (last_expansion_pos). After the full forward pass, re-scan
only the prefix [0, last_expansion_pos) — every transaction that was
processed before the pool reached its final state. Transactions at
last_expansion_pos and beyond were already seen with the
fully-expanded pool and are not re-visited, so AddToWallet is not
called a second time for them (walletnotify does not double-fire).
The prefix strictly shrinks each iteration, guaranteeing termination
without a safety cap.

Using the *last* (not first) expansion position matters when multiple
TopUp events occur in a single block. With interleaved vtx order
[lookahead_1, expand_1, lookahead_2, expand_2], stopping at the first
expansion position (1) re-scans only [0, 1) and misses lookahead_2
at position 2. Stopping at the last expansion position (3) re-scans
[0, 3) and finds both.

Affected callers (all use ScanForWalletTransactions): rescanblockchain,
importdescriptors (via RescanFromTime), restorewallet and wallet
migration watchonly/solvable wallets (via AttachChain).

This bug does not affect the live blockConnected path when transactions
pass through the mempool first: transactionAddedToMempool(Tx_expand)
pre-extends the pool before the block arrives.

Extend the regression test with Part 4 to cover the two-level
cascade scenario, and flip Parts 1 and 2 from the bug-demonstrating
assertions in the previous commit to the fixed behavior.

1d9de0e62d

pablomartin4btc force-pushed on Aug 28, 2026

pablomartin4btc commented at 8:47 PM on August 28, 2026: member

I think the prefix-only rescan leaves the following case unresolved...

Thanks for working through this — traced it and agree, the parent-then-child scenario is a real gap. The prefix rescan recovers the parent (its position is before the last expansion), but if the child spends it at or after that boundary, the child is never revisited, so the spend is never recorded and the parent output stays classified as unspent.

Closing this properly needs a different approach — tracking per-tx SyncTransaction() success and retrying until a full pass causes no further change, as you suggested, which is a different algorithm with its own termination argument, not a small addition to this one. Given this PR already scopes out a comparable-complexity related case (@furszy's inter-block variant) as a follow-up rather than folding it in, I'd like to treat this the same way — documenting it as a second known limitation rather than expanding this fix. Added it as a note to the PR description.

Took your other three suggestions as-is, done in 1d9de0e62d237b4feea4d6ee5515312d7b2ba9fd, thanks!

pablomartin4btc commented at 8:52 PM on August 28, 2026: member

-<ins>Updates</ins>:

Addressed @molnard's feedback:
- Skip collect_range_ends() when SyncTransaction() returns false — if the tx doesn't touch the wallet, the look-ahead pool can't have expanded, so the check is guaranteed unchanged.
- range_ends = std::move(cur); instead of a copy.
- Consolidated the initial full-block scan and the shrinking-prefix re-scan into a single while (scan_end > 0) loop — same termination guarantee, less duplicated code, and the natural single spot for the SyncTransaction-return-value skip above.
- The additional "child spends parent" gap raised in the top-level review is real, but fixing it needs a different algorithm (retry based on each tx's SyncTransaction() result until a pass causes no change) — documented as a follow-up in the PR description, alongside the existing furszy inter-block carve-out, rather than folded into this fix.

jeanpablojp commented at 2:15 AM on August 29, 2026: contributor

On 1d9de0e6 there is a second case the prefix does not reach, with no spend in it. 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.

keypool 5, pool [0, 4], one block, vtx order A pays key 5, E pays key 4, B pays key 10. E is the only one found on the first pass. Its TopUp takes the pool to [0, 9], which leaves B one key outside, and puts scan_end at E's position. The prefix pass then recovers A, which tops the pool up past key 10, but B sits after scan_end and is never looked at again. Master misses A and B both, and the full re-walk this branch started with finds all three.

</details>

So the documented limitation is narrower than the real one. Two channels make a transaction relevant after it has already been walked, the pool growing again and IsFromMe, and once the prefix has shrunk past a position it reaches neither.

molnard's update already has the fix, and it covers this case as written. The variant I ran swaps the stopping condition for nothing new matched. collect_range_ends and the range_ends map both go away, and termination stops depending on expansions at all. It is not slower either, on an ordinary rescan or on a pathological one.

<details> <summary>The variant, and what I ran it against</summary>

std::vector<size_t> pending(block.vtx.size());
for (size_t i = 0; i < pending.size(); ++i) pending[i] = i;
while (!pending.empty()) {
    std::vector<size_t> unmatched;
    for (size_t pos_in_block : pending) {
        if (!SyncTransaction(block.vtx[pos_in_block], TxStateConfirmed{block_hash, block_height, static_cast<int>(pos_in_block)}, /*rescanning_old_block=*/true)) {
            unmatched.push_back(pos_in_block);
        }
    }
    if (unmatched.size() == pending.size()) break; // nothing new matched
    pending = std::move(unmatched);
}

Both versions find the two cases with the right balance and pass your test's four parts. On the variant above, walletnotify stays at one invocation and the unit and wallet_* functional suites are clean.

</details>

<details> <summary>Benchmark</summary>

Rescanning 50 blocks of 401 transactions with one wallet receive each, the branch and the variant are indistinguishable, 0.146s against 0.143s with overlapping ranges. On a block where each pass reveals one more transaction, 30 of them behind 300 filler, the branch takes 0.126s and the variant 0.084s, ranges apart, because a transaction that has matched is never walked again. Medians of seven runs and five.

</details>

pablomartin4btc commented at 4:47 PM on August 29, 2026: member

On 1d9de0e there is a second case the prefix does not reach, with no spend in it.

Thanks @jeanpablojp for working on this. I'm analysing how your fix behaves against the current state of the PR and @molnard's pending follow-up, plus his earlier feedback that's already addressed. Will report back soon.