fix(dash-spv): verify filter batches against the full script set before commit - #958
fix(dash-spv): verify filter batches against the full script set before commit#958PastaPastaPasta wants to merge 2 commits into
Conversation
…re commit A filter batch used to commit once the gap-limit chase's latest wave derived no new scripts. That signal is wrong twice over: scripts derived from blocks owned by other batches never enter this batch's collected set, and a wave whose visible transactions all pay already-derived indices (backfill - inevitable when dust waves are mined out of derivation order) derives nothing even though the batch still holds blocks paying indices past the window. Committing on it ends the chase early, and committed batches are never rescanned, so every transaction above the window at commit time is permanently lost and discovery flatlines: later activity pays underived addresses, filters stop matching, and the pool never extends again. Observed in production on a mainnet wallet with 345,261 sequentially used addresses: discovery froze at address index 2,400 after ~500 blocks of activity and silently missed 880k+ transactions while sync ran to tip. The manager now keeps a monotone script-derivation generation, bumped whenever block processing derives new scripts - including for blocks whose owning batch is already gone, whose scripts were previously dropped without ever being matched. Each batch records the generation it was last matched against the wallets' full script sets (initial scan or verification). At commit time, a batch whose recorded generation is stale is re-matched against the full current sets and may not complete its rescan while that verification still finds blocks. Quiet syncs never pay for this: with no derivations the generations match and the verification is skipped. The regression test drives the real FiltersManager + WalletManager over a synthetic dust restore whose payments land out of derivation order across a batch boundary; without this fix it loses 36 of 3,000 transactions, with it discovery completes (as does the in-order control). Co-Authored-By: Claude Fable 5 <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: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFilters synchronization now tracks wallet script generations. Batch commits wait for pending blocks and rescan when later derivations occurred. A dust-restore harness tests ordered and deterministic out-of-order mining across a batch boundary. ChangesWallet script generation tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The change prevents transactions from being missed during dense wallet restores, but commit-time verification can repeatedly re-download and re-match batch blocks, increasing restore CPU, network usage, and latency. This is a bounded performance risk that should remain with explicit owner awareness or follow-up optimization. Possibly related issues
Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant SyncManager
participant FiltersManager
participant FiltersBatch
participant WalletScripts
SyncManager->>FiltersManager: Process BlockProcessed scripts
FiltersManager->>FiltersManager: Increment script_generation
FiltersManager->>FiltersBatch: Record full-match generation
FiltersManager->>WalletScripts: Rescan with current full script set
WalletScripts-->>FiltersManager: Return newly matched blocks
FiltersManager->>FiltersBatch: Defer commit when matches or pending blocks remain
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dash-spv/src/sync/filters/manager.rs (2)
596-643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused in-module test for the generation gate.
run_dust_restorecovers the gate end-to-end only. A direct test would pin the contract and would fail fast if the gate regresses. The existing tests already build aFiltersBatchby hand and calltry_commit_batches, so the setup is small:
- Insert a scanned batch with
set_full_match_generation(0)and matching filters, setmanager.script_generation = 1, then assert the batch does not commit while the rescan finds blocks.- Set the batch generation equal to
script_generationand assert the batch commits without any rescan.As per path instructions for
dash-spv/**/{src,tests}/**/*.rs: "Implement comprehensive unit tests in-module for individual components using#[cfg(test)]".🤖 Prompt for 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. In `@dash-spv/src/sync/filters/manager.rs` around lines 596 - 643, Add a focused in-module #[cfg(test)] test covering the generation gate in try_commit_batches: construct a scanned FiltersBatch with matching filters and full_match_generation(0), set manager.script_generation to 1, and assert it remains uncommitted when rescan_batch finds blocks; then set the batch generation to the current script generation and assert it commits without rescanning. Reuse the existing test setup and batch-building helpers.Source: Path instructions
610-643: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider bounding the cost of the verification rescan.
The gate is correct and it terminates: each pass records
generation_now, so a pass that finds no blocks leaves the generations equal and the next pass commits.The cost is the concern. Every pass rebuilds the full scan script set for each scanned wallet and re-matches every filter in the batch. During a dense restore, one pass runs per derivation wave. For a wallet with a large scan set this is repeated O(waves × filters × scripts) work on the sync path.
Two options that keep the same guarantee:
- Pass only the scripts derived since the batch's recorded generation, instead of the full set. That needs a generation-keyed script log, so it is the larger change.
- Cache the per-wallet full set for the duration of one
try_commit_batchescall, so several batches in the same drain do not each rebuild it.Neither is required for correctness. Track the rescan duration before choosing.
🤖 Prompt for 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. In `@dash-spv/src/sync/filters/manager.rs` around lines 610 - 643, The verification rescan in try_commit_batches is correct but can repeatedly rebuild wallet script sets and rematch filters; add duration tracking around the needs_verification/rescan_batch work so the cost is observable before selecting an optimization. Preserve the existing generation tracking and rescan behavior, and use the duration to distinguish full-set construction from rescan matching where practical.
🤖 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.
Nitpick comments:
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 596-643: Add a focused in-module #[cfg(test)] test covering the
generation gate in try_commit_batches: construct a scanned FiltersBatch with
matching filters and full_match_generation(0), set manager.script_generation to
1, and assert it remains uncommitted when rescan_batch finds blocks; then set
the batch generation to the current script generation and assert it commits
without rescanning. Reuse the existing test setup and batch-building helpers.
- Around line 610-643: The verification rescan in try_commit_batches is correct
but can repeatedly rebuild wallet script sets and rematch filters; add duration
tracking around the needs_verification/rescan_batch work so the cost is
observable before selecting an optimization. Preserve the existing generation
tracking and rescan behavior, and use the duration to distinguish full-set
construction from rescan matching where practical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 390e1ccb-5507-4dac-82bb-55cb8fed7b1b
📒 Files selected for processing (3)
dash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/sync_manager.rs
…n flight A block in flight for a later batch can still derive new scripts when it lands, and those scripts can match an earlier batch's filters. Sealing the earlier batch during that window raced the delivery - the same knowledge-behind-the-watermark loss the verification rescan prevents, through a narrower window. Commits now wait for global block quiescence, so a seal implies no undelivered derivations exist anywhere and the verification fixpoint argument has no gaps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Validation update: an end-to-end mainnet resync of the production wallet (dash-spv binary from this branch, start height 2,440,000) reproduced the original outcome — 8,076 transactions discovered, discovery stopping at the same address frontier — despite this fix. The harness regression this PR fixes is real (the out-of-order test loses 36/3,000 on dev and passes here), but it is evidently not the (only) mechanism behind the production stall. An instrumented rerun is in progress to identify what actually terminates the chase on the real chain; holding this PR as draft-quality until that lands — the fix stands on its own merits, but the production claim in the description is overstated until the real mechanism is found. 🤖 Posted autonomously by Claude on behalf of pasta. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #958 +/- ##
==========================================
+ Coverage 76.47% 76.52% +0.04%
==========================================
Files 329 329
Lines 80353 80562 +209
==========================================
+ Hits 61453 61648 +195
- Misses 18900 18914 +14
|
|
Closing: end-to-end validation against the production wallet showed this change does not alter the real-wallet outcome (discovery still stops at the same frontier). The commit-termination issue the tests capture is real but is downstream of a more fundamental defect in the block re-application path that the harness masked; a corrected fix addressing the actual mechanism will supersede this PR. 🤖 Posted autonomously by Claude on behalf of pasta. |
1 participant
The bug
During an SPV restore of a wallet with many sequentially used addresses, discovery can permanently lose transactions and silently flatline while sync runs happily to tip.
Observed in production on a mainnet wallet with 345,261 used addresses: discovery froze at address index 2,400 after the first ~500 blocks of activity and missed 880k+ transactions. The wallet reported itself fully synced.
Why it happens
Filter sync processes compact filters in 5,000-block batches. When a matched block is processed and extends the wallet's address pools (gap-limit maintenance), the newly derived scripts are collected and re-matched against the batch's filters at commit time — the "gap-limit chase." The chase loops: rescan → download newly matched blocks → process → collect newly derived scripts → rescan again.
The loop's termination signal is "the latest wave derived no new scripts." That signal is wrong, twice:
highest_used, so no scripts are derived — even though the batch still holds blocks paying indices past the window. The chase ends, the batch commits.And commits are one-way: a committed batch is removed from the active set and
synced_heightadvances past it, so the missed blocks are never looked at again. Worse, the loss cascades: all later activity pays indices above the frozen window, so later filters match nothing, no blocks download, the pool never extends again, and discovery goes permanently silent.The fix
Track a monotone script-derivation generation on the manager, bumped whenever block processing derives new scripts (including for blocks whose owning batch is gone). Each batch records the generation it was last matched against the wallets' full script sets (its initial scan, or a verification rescan).
At commit time, a batch whose recorded generation is stale gets a verification rescan against the full current script sets, and may not complete its rescan while that verification still finds unprocessed blocks. The loop reaches a true fixpoint: a batch commits only when its filters genuinely contain nothing more for everything the wallet currently knows.
Quiet syncs pay nothing: with no derivations since the batch's scan, the generations match and verification is skipped entirely.
Proof via tests
run_dust_restoredrives the realFiltersManager+WalletManager<ManagedWalletInfo>over a synthetic 5,120-block chain: 3,000 dust payments to external addresses 0–2,999, mined 25 per block across aBATCH_PROCESSING_SIZEboundary (vs gap limit 30), with simulated block-delivery latency.dust_restore_discovers_all_txs_when_mined_in_index_order— control: in derivation order the chase keeps up block by block; passes with and without the fix.dust_restore_discovers_all_txs_when_mined_out_of_order— regression: payments shuffled deterministically (fixed-seed LCG, noranddependency) the way mempool waves actually land. Without the fix it loses 36 of 3,000 transactions (batch committed on a backfill-only wave); with the fix discovery completes.At production scale the same mechanism loses far more — a 200-block / 8,000-tx variant of this harness lost 44% of transactions before the fix and finds 100% after.
Validation
cargo test -p dash-spv --all-features— 550 unit tests green (incl. the 2 new ones)cargo test -p dash-spv --test dashd_syncagainst regtest dashd — 30/30 greencargo clippy --all-targets --all-features,cargo fmt— cleanNotes / follow-ups (intentionally out of scope)
track_for_new_scriptsdeliberately re-processes). Correctness first; a delta-tracking optimization can bound the re-downloads later.🤖 Generated with Claude Code
Summary by CodeRabbit