fix(key-wallet): hold back catch-up receives from coin selection - #937
fix(key-wallet): hold back catch-up receives from coin selection#937QuantumExplorer wants to merge 3 commits into
Conversation
A wallet catching up on history applies blocks in ascending order, so a receive it discovers says nothing about whether some higher, not-yet-scanned block already spends it. Coin selection only ever checked `is_locked` and coinbase maturity, against `last_processed_height` — a matched-block cursor, not the scan frontier — so a restored wallet would fund a transaction from an outpoint the network had settled as spent long ago. Peers drop that broadcast silently (Core has not sent BIP61 rejects by default since 0.17), and whatever it was funding is stranded with no error anywhere. Observed on testnet: a restore found a 1000 DASH receive in block 758983 and funded an identity top-up asset lock from it three seconds later; the block that had already spent that outpoint, 1510203, was still six minutes of scanning away. Track the height the scan is working toward (`WalletMetadata::scan_target_height`, published by the dash-spv filter manager from the filter-header tip) alongside where it has reached. A receive applied below the target is marked `Utxo::spend_scanned = false` and excluded by `Utxo::is_spendable`; reaching the target releases every survivor in one pass, since a spend would already have removed it. Receives at or above the target — live blocks and mempool — are never held back, so a caught-up wallet is unaffected. The gate stays open until a scanner reports a target, and UTXOs deserialized from persistence written before this field default to scanned, so consumers that never scan keep the previous behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe wallet now tracks a spend-scan target and holds confirmed UTXOs discovered below that target. The SPV filter manager publishes nonzero filter-header tips through the wallet manager. UTXOs become spendable after synchronization reaches the target. ChangesSpend-scan frontier gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FilterManager
participant WalletManager
participant ManagedWalletInfo
participant CoinSelection
FilterManager->>WalletManager: Publish nonzero filter-header tip
WalletManager->>ManagedWalletInfo: Update scan target height
ManagedWalletInfo->>ManagedWalletInfo: Hold UTXOs below target
ManagedWalletInfo->>CoinSelection: Exclude unscanned UTXOs
WalletManager->>ManagedWalletInfo: Update synced height
ManagedWalletInfo->>CoinSelection: Expose promoted UTXOs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Companion on the Platform side: dashpay/platform#4356 makes a lock that was already built this way fail with a typed terminal error so the app can offer to discard it. This PR is the prevention; that one is the cleanup. Platform's pin bump to pick this up is blocked on this merging. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet/src/tests/spend_scan_frontier_tests.rs`:
- Around line 67-78: Update restored_wallet_mid_catch_up and the additional
wallet setup sites to use the deterministic fixed-seed TestWalletContext fixture
instead of TestWalletContext::new_random(), preserving the existing wallet state
and transaction behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51f7a7a8-9b6d-48cc-a01d-3ab93703d8df
📒 Files selected for processing (12)
dash-spv/src/sync/filters/manager.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/wallet_interface.rskey-wallet/src/tests/mod.rskey-wallet/src/tests/spend_scan_frontier_tests.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/utxo.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/metadata.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #937 +/- ##
==========================================
+ Coverage 75.18% 75.21% +0.03%
==========================================
Files 328 328
Lines 78194 78261 +67
==========================================
+ Hits 58792 58867 +75
+ Misses 19402 19394 -8
|
- unnecessary_get_then_check in the already-spent-coin test - qualify the WalletMetadata::scan_target_height intra-doc link, which is not in scope from the interface module Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review asked for reproducible fixtures: a failure now replays with the same keys and addresses every run. Adds TestWalletContext::new_with_seed for any test that wants determinism. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Decided this is wrong |
1 participant
Issue being fixed or feature implemented
Coin selection can build a double-spend while a restored wallet is catching up on history.
Blocks are applied in strictly ascending order (
BlocksPipeline::take_next_ordered_blockstalls if any pending download is lower), so a receive discovered at heightHenters the UTXO set and becomes fully selectable long before blockH' > H— the one that spends it — has been downloaded or scanned. Nothing in the selection path consulted the scan frontier:Utxo::is_spendablechecked onlyis_lockedand coinbase maturity, and the height it was given islast_processed_height, a matched-block cursor rather thansynced_height.The resulting transaction is a double-spend of an outpoint the network settled long ago. Peers drop it at the mempool boundary and relay nothing back — Core has not sent BIP61
rejectby default since 0.17 — so it never confirms and never errors.Observed on testnet: a restore found a 1000 DASH receive in block 758983 and funded an identity top-up asset lock from it three seconds later. The block that had already spent that outpoint, 1510203, was still six minutes of scanning away.
The existing out-of-order guard in
update_utxosonly covers the mirror case (spend seen before the creating tx), so it never applied here.What was done?
Track where the scan is going alongside where it has reached:
WalletMetadata::scan_target_height— the filter-header tip the scan has committed to covering. Published by the dash-spv filters manager instart_download(so the gate is armed before any block is applied) andhandle_new_filter_headers(as the chain grows), via a new defaultedWalletInterface::update_scan_target_height.Utxo::spend_scanned— whether every block that could already have spent this output has been scanned. A receive applied from a block below the target is held back and excluded byUtxo::is_spendable.promote_spend_scanned_utxosreleases every survivor in one pass: at that point a spend would already have removed it, so what remains is genuinely unspent.WalletInfoInterface::spend_scan_complete()so consumers can distinguish "still syncing" from "insufficient funds".Two deliberate choices:
Receives at or above the target are never held back. A wallet-level "am I caught up?" gate would block spending for the seconds after every new block while the frontier lags the header tip — roughly 3% of the time on a live chain, surfacing as a confusing insufficient-funds error. Holding back per-UTXO leaves a caught-up wallet completely unaffected, and mempool receives (at the tip by definition) stay spendable even during catch-up.
The gate fails open until a scanner reports a target, and
spend_scannedserde-defaults totrue. Consumers that never scan, and UTXOs already in persistence, keep the previous behavior exactly. Fail-closed would have made every hand-built wallet and test fixture unspendable.Balance display is untouched:
update_balancereadsis_mature/is_lockeddirectly, notis_spendable, so a mid-catch-up wallet still reports a balance. Whether the UI should say "syncing" instead is a consumer-side decision, whichspend_scan_complete()now enables.How Has This Been Tested?
Seven tests in
key-wallet/src/tests/spend_scan_frontier_tests.rs, using the incident's actual heights (758983 / 1510203): receive held back below the target, released when the scan reaches it, never selectable when a higher block spent it, immediately selectable at the target, mempool unaffected, gate open with no target reported, and a legacy row missing the serde field deserializing as scanned.Mutation-checked rather than assumed: removing the
spend_scannedclause fromis_spendablemakes exactly the two incident tests fail and no others.Full
key-wallet,key-wallet-manageranddash-spvsuites pass;cargo clippy --all-features --all-targetsandcargo fmt --all --checkclean.Breaking Changes
None. New field on
Utxo(serde-defaulted) and onWalletMetadata; the newWalletInterfacemethod is defaulted. Behavior is unchanged for any consumer that does not publish a scan target.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests