fix(dash-spv): collect the scripts derived by every application of a block, not only the tracked one - #1008
Conversation
…block, not only the tracked one A block is applied more than once during a scan: re-applied by a batch rescan for scripts derived after its first application, or delivered again for an in-flight re-request. Each application can recognise outputs the previous one could not, and derive further scripts through gap widening. The BlockProcessed handler collected new scripts only when the tracker still held the block's in-flight entry — which the first delivery consumes — so the scripts derived by every later application were dropped: never collected for the batch rescan and the committed-range sweep. On a CoinJoin-heavy mainnet wallet (fresh import, release build) 297 of the 3 645 gap-widened scripts were derived by re-applied blocks and lost. The blocks that spend the coins paid to those addresses match only on the coins' prevout scripts, so they were never found; the wallet ended the scan with 11 spent coins credited (#1006). Settle the in-flight entry and the pending count from the tracked delivery only, but collect new scripts from every delivery: into the tracked batch while it is active, else the batch covering the block, else the lowest active batch — whose commit rescans it, every later batch and the committed prefix. The rescan and sweep log lines now carry the script count. Regression test: a second BlockProcessed for an already-settled block must leave its scripts in the batch's collected scripts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe synchronization manager now collects scripts from repeated block applications, assigns them to active batches, and triggers processing when scripts arrive without a tracked delivery. A regression test covers late scripts from redelivered blocks. Rescan logs now include script counts. ChangesCoinJoin script cascade
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Late wallet scripts discovered from redelivered blocks now enter the rescan cascade, preventing affected historical spends from remaining credited. Regression coverage is included and no merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant BlockDelivery
participant SyncManager
participant ActiveBatch
participant RescanManager
BlockDelivery->>SyncManager: deliver block application
SyncManager->>ActiveBatch: collect and assign newly discovered scripts
SyncManager->>RescanManager: trigger processing for delivery or scripts
RescanManager-->>SyncManager: report rescan script counts
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1008 +/- ##
=======================================
Coverage 77.55% 77.55%
=======================================
Files 329 329
Lines 83624 83639 +15
=======================================
+ Hits 64851 64869 +18
+ Misses 18773 18770 -3
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
Reviewed at head 33ad4e26. The core fix is sound, and I traced the parts it rests on rather than taking them on trust: active_batches is a BTreeMap, so .keys().next() really is the lowest batch; the three-step target fallback (tracked → covering → lowest) keeps full coverage in every path I followed, because a script-carrying commit rescans its own range, every later scanned batch, and — deferred to drain — the whole committed prefix with min_synced = 0; collected_scripts is a HashSet, so a re-delivered block dedupes; and the new fixpoint terminates, since new_scripts comes from check_result.new_addresses and is empty on an identical re-application.
One inline comment: the None arm is the same silent-loss the PR fixes, moved to the last batch.
Non-blocking recommendations:
sync_manager.rs:252—if tracked_batch.is_some() || has_new_scriptsnow callstry_process_batch()on an untracked delivery even whenactive_batchesis empty, a state this handler never used to reach. There is nothing to commit, scan or look ahead there, but it does fall into the completion branch atmanager.rs:564and re-emitsFiltersSyncComplete—committed_height >= filter_header_tip >= targetall still hold after the final commit.tickgates onhas_pending_workinSyncedprecisely to avoid that. Downstream,BlocksManagerre-setsfilters_sync_completeand can flip itself toSyncedif its pipeline is momentarily empty, andMempoolManagerretriesactivate_all_peers.tracked_batch.is_some() || (has_new_scripts && !self.active_batches.is_empty())is strictly safe.coinjoin_gap_discovery_tests.rs:656—let late_script = addresses[35].script_pubkey();is a hardcoded index in a file whose header comment (lines 65-70) explains that every funded index is expressed relative toG = DEFAULT_COINJOIN_GAP_LIMITbecause the 30→100 move in #868 "would otherwise have silently turned the cross-commit repro into a trivially-green test". 35 is the worst available choice: the block pays 0-5, sokeep_until = 5 + G, and under the oldG = 30that is exactly 35 — the first, tracked delivery would have derived it itself and the test would pass with this fix reverted. It only isolates the second delivery becauseGis 100 today. Express it relative toG, or make it self-guarding by asserting the first delivery'sresult.new_scriptsdoes not already containlate_script.sync_manager.rs:224— the "re-applied outside its tracked delivery" debug line is gated ontracked_batch.is_none(), so it stays silent in the other re-homing case this PR introduces:tracked_batchisSome(b),bhas already leftactive_batches, the.filter(...)at line 214 drops it, and the scripts are charged to a different batch than the tracker named. Since the PR's own account says the added logging is what made the loss measurable, "target differs from the tracked batch" is the more useful condition. Related nit: thematch target.and_then(...)at line 237 is loop-invariant but sits inside the per-wallet loop, so the drop warning fires once per wallet per delivery rather than once per delivery.
🤖 Reviewed with Claude Code
| Some(batch) => { | ||
| batch.add_scripts_for_wallet(*wallet_id, scripts.iter().cloned()) | ||
| } | ||
| None => tracing::warn!( |
There was a problem hiding this comment.
This arm still drops the scripts, and its message is wrong about what recovers them — which reproduces the exact loss this PR fixes, at the tail of the scan.
The warning says the scripts "will only be re-tested by a later rescan". In the state that reaches this arm there is no later rescan:
rescan_committed_rangeruns only from a batch commit, andactive_batchesis empty here by definition.tickreturns early inSyncedwhenhas_pending_work == false(sync_manager.rs:303-311).- A restart does not help either: the committing batch already advanced
wallet_synced_heightto the tip, so neithertick's staleness check norstart_syncwill arm a rescan.
And this is reachable precisely where the PR's failure mode is most likely. The last batch commits once pending_blocks() == 0, but queue_new_script_matches re-emits BlocksNeeded for a block in the InFlight state without incrementing pending_blocks — so a duplicate delivery of an already-settled block can land after the final commit, with active_batches empty. The scripts derived from it are then lost the same way the tracked-batch-only collection lost them, just one batch later.
At minimum the message should stop naming a recovery that cannot happen. The real fix is to park these in backward_scripts so the next commit sweeps them — which is the direction #979 already takes, so this may be a matter of pulling that part forward rather than new design.
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
Which wallet are you using?? I am not able to replicate the issue in mainnet using job flower, do we have a test wallet with heavier ConJoin operations??
|
I was reading the PR and it looked similar to something I wrote before, and the issue attached was something I was working on, doesn't #989 already cover this? Same root cause — It goes further in three ways this leaves open: it routes scripts by height to the batch covering the block, falls back to I reproduced it on the bench today: four paired mainnet runs, Looks like a subset of #989 — worth checking whether merging that one makes this unnecessary. |
3 participants
Fixes #1006. Standalone on
dev; the same change is also stacked on #979 as #1007 (which this supersedes — #979's durable pending-sweep set additionally persists the collected scripts, and rebasing #979 over this is a one-line conflict in the same handler).Issue being fixed
A block is applied to the wallet more than once during a scan: re-applied by a batch rescan for scripts derived after its first application, or delivered again for an in-flight re-request. Each application can recognise outputs the previous one could not (the pools grew in between) and derive further scripts through gap widening. The
BlockProcessedhandler collected those scripts only when the tracker still held the block's in-flight entry — which the first delivery consumes — so the scripts derived by every later application were dropped on the floor: never collected for the batch rescan and the committed-range sweep.Measured on a CoinJoin-heavy mainnet wallet (fresh import, release build, iOS Simulator, with counters added temporarily to the handler): 3 645 scripts were derived during the scan; 3 348 reached the sweep and 297 — derived by 14 blocks that were applied 2–4 times each while their batch still had other blocks outstanding — were lost. The blocks that spend the coins paid to the lost addresses match only on the coins' prevout scripts (their outputs are not ours), so they were never found, requested or applied; the wallet ended the scan with 11 spent coins credited (0.11 DASH), and nothing in the engine could ever remove them: no spender record to attribute to,
observed_spent_outpointsnever saw the spend,spent_outpointsis only fed by matched transactions. The same coins came back after every restart because the persister mirrors the engine (dashpay/platform#4575, dashpay/platform#4638).What was done
sync_manager.rs: settle the in-flight entry and the batch's pending count from the tracked delivery only, but collect new scripts from every delivery — into the tracked batch while it is active, else the batch covering the block, else the lowest active batch, whose commit rescans it, every later batch and the committed prefix. With no active batch the scripts are only warned about; fix(key-wallet, dash-spv): re-emit late knowledge — record corrections, durable rescans, address-pool repair #979's durable pending-sweep set is the right home for them and will persist them once it lands.Rescan filtersandRescan committed filterslog lines now carry the script count, which is what made the loss visible.scripts_derived_by_a_redelivered_block_enter_the_cascadeincoinjoin_gap_discovery_tests.rs: a secondBlockProcessedfor an already-settled block must leave its scripts in the batch's collected scripts. Red before the fix, green after.How has this been tested
cargo test -p dash-spv --lib sync::filters: 115 passed (114 existing + the new one);cargo fmt --checkandcargo clippy --all-targetsclean fordash-spv.platform-walletFFI with this change applied on top of fix(key-wallet, dash-spv): re-emit late knowledge — record corrections, durable rescans, address-pool repair #979's compat rebase (the engine the field build uses):utxo_count=0for the CoinJoin account, the SwiftData store holds 0 unspent rows, a restart restores nothing. Repeated by hand through the app's own delete + import flow with the same result. This matches the only earlier run of the same wallet that had converged, by timing luck.Not addressed here
InFlightre-emitsBlocksNeeded). Harmless for correctness now that every application's scripts are kept, but wasteful; worth a separate look.prune_finalized_observed_spendsbounds by the committed filter height, which runs ahead of rescan deliveries during a historical scan; with the cascade complete this no longer bites on this wallet, noted in dash-spv: the initial scan's new-script cascade never re-tests blocks that only match on the prevouts of coins discovered mid-scan, so their spends are missed and the coins stay credited #1006.🤖 Generated with Claude Code
Summary by CodeRabbit