key-wallet: committed-range rescans (#866) can redeliver funding after its observed-spent entry was pruned

[xdustinface]

Context

PR #851 fixes #649 with a wallet-level observed_spent_outpoints map: every block-observed spend is recorded, and update_utxos never inserts a UTXO whose outpoint is in the map. Entries are pruned at the finality boundary min(chainlock_height, synced_height).

The pruning rule is sound on current dev because every redelivery path is a full replay: a full rescan redelivers the spending block too (BIP158 filters match it via the prevout script), so the map repopulates before the funding is re-processed, in either order.

The gap

synced_height acts as a coverage certificate relative to the script set at scan time. #866 introduces rescans of already-committed filter ranges for newly derived scripts (gap-limit growth, late-added accounts). Such a rescan can redeliver a funding transaction for a script that was unknown when the range was first scanned, while the spend of that coin sits below the finality boundary and its observed-spent entry has already been pruned. The funding insert then resurrects a spent coin, reopening #649 for exactly the coins #866 recovers.

PR #851 originally tried to close the account-add slice of this with a synced_height rewind to birth plus a commit-time contiguity guard in dash-spv. That was dropped: the guard compared batch starts against absolute wallet birth instead of the scan anchor and bricked every checkpoint sync whose wallet birth lies below the anchor (infinite restart loop, 6421 restarts observed in field testing), and the rewind never covered #866's main redelivery source, gap-limit script growth.

What #866 (or a follow-up to it) must guarantee

For any committed-range rescan, one of:

• redeliver spends before or together with fundings for the rescanned range (full-replay semantics, the map repopulates itself), or
• pre-check redelivered fundings against recorded/finalized transactions AND the observed-spent map before inserting, or
• lower the effective prune boundary for the duration of the rescan (a scoped, anchor-aware version of the rewind idea).

Related direction for the map itself

The map currently records every input of every transaction in every matched block, since ownership of an unseen prevout is unknowable at spend time. For P2PKH inputs the spender's public key is in the scriptSig, so the prevout script can be derived and recording could be narrowed to spends of own coins. That would shrink the map to roughly one entry per real wallet spend and reduce the surface #866 has to reason about.

References

• bug: out-of-order block processing causes SPV wallet to miss UTXO spends #649, fix(key-wallet): out-of-order UTXO spend causes history divergence (#649) #851, fix(dash-spv): rescan committed filter ranges for newly derived scripts #866, dash-spv: build a reorg/disconnect pipeline so key-wallet can retract non-chainlocked observed-spent entries #854 (reorg retraction of observed-spent entries), key-wallet-manager: include watched_outpoints in compact-filter matching (defense-in-depth for #649-class bugs) #855 (watched-outpoint filter matching)

[xdustinface]

Closing: I filed this on a premise that does not hold, and the hazard is not real.

Why it is refuted. The claim was that #866's committed-range rescan could redeliver a funding transaction after its spend's observed_spent_outpoints entry had been pruned, resurrecting a spent coin. That assumed the rescan delivers the funding without the spend. It does not.

Dash Core's BasicFilterElements (src/blockfilter.cpp:206-212) adds every output scriptPubKey, the special-transaction elements, and every prevout scriptPubKey from the block undo data. So a block that spends a coin paying script S carries S as a filter element. #866's rescan_committed_range matches the newly derived scripts across the whole committed range, which means a sweep for a new script S matches both the funding block (S as an output script) and the spend block (S as a prevout script). Both are re-delivered, so the map repopulates.

Either delivery order is then correct: spend first repopulates the entry and the funding insert is skipped; funding first tracks the coin, and the spend is attributed through the ordinary matched-spend path off the now-live UTXO. track_for_new_scripts never returns AlreadyProcessed, so an already-processed spend block really is re-delivered. And for the forward-rescan-out-of-order case the entry cannot have been pruned anyway: the boundary is min(chainlock, synced_height) and synced_height is pinned below the batch by the pending_blocks barrier — the min() is what is load-bearing there, not the chainlock.

What survives is a review requirement on #866, not an issue here. Ascending-height delivery is what makes the above hold, and nothing states it. The sweep accumulates all chunks into one block_to_wallets and emits a single BlocksNeeded; the blocks pipeline queues all heights before draining and gates on the lowest pending height, which is why funding lands before spend. If that is later split per-chunk or per-wallet to bound memory, funding and spend land in different events and cross-event ordering is unenforced, at which point the hazard becomes real. Worth a comment at rescan_committed_range and a test covering a sweep that hits both a funding and its spend below the committed range. A narrower point for the same review: the sweep skips filter chunks it cannot load and continues, so a spend whose chunk fails to load while the funding's chunk succeeds would resurrect the coin.

I will raise both on #866 directly. The memory concern that motivated the record-only-own-spends idea mentioned here is now tracked properly in #899, including why that narrowing must wait for #866.