fix(dash-spv): re-open committed ranges for gap-limit scripts derived after commit (#846)#873
fix(dash-spv): re-open committed ranges for gap-limit scripts derived after commit (#846)#873bfoss765 wants to merge 3 commits into
Conversation
Port the dash-spv portion of the platform#3549 crate-level bug-pin repro onto the current `dev` architecture (post-dashpay#820: FiltersManager + BlockMatchTracker). Three tests drive the real filter -> block -> wallet pipeline with a deterministic seed, synthetic blocks and real BIP-158 filters, no network: - coinjoin_gap_limit_dense_same_batch_recovers — GREEN (dashpay#820 guard) - coinjoin_gap_limit_inversion_within_batch_recovers — GREEN (dashpay#820) - coinjoin_gap_limit_stall_across_committed_batch — RED: gap-window outputs in an already-committed batch are never recovered because the new-script rescan only reaches `active_batches`. Ported from dashpay/rust-dashcore@c24912c1 (repro/pr3549-rdc); the key-wallet dashpay#763/dashpay#764 repros in that commit are out of scope here. Co-Authored-By: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y#846) Gap-limit scripts derived after a filter batch commits were never tested against the committed range, so outputs paying gap-window addresses in an already-committed block stayed permanently invisible. PR dashpay#820 fixed the in-flight (active-batch) half via the commit-time fixpoint rescan; this closes the cross-commit-boundary half. Root cause: discovery suppression was keyed by scan progress (which heights a wallet had committed), not by which scripts a block was tested against. `rescan_batch` only reaches `active_batches`, and commit removes the batch and prunes the tracker at/below the committed height, so a forward index<->height inversion — a low external index funded in a later block whose gap-window extension covers a higher index paid in an earlier, already-committed block — left the earlier block's outputs unrecoverable. Fix: retain committed batches' filters in a bounded sliding window (`committed_filters`, moved out of the batch on commit — no clone, so the common path is untouched). On every `BlockProcessed` that derives new scripts, queue them with the committed_height snapshot at derivation time; `try_process_batch` drains the queue, re-tests the fresh scripts against the retained committed filters at/below that ceiling, and re-queues any matched committed block through the existing `track_for_new_scripts` path to a fixpoint. Per-wallet `committed_tested_scripts` dedups so each script pays one pass; heights that commit later already saw the script via the normal per-batch scan. `FiltersSyncComplete` is held off while a re-opened below-frontier block is in flight, and the retained filters are released on full sync. Trade-offs: - Zero overhead on the common no-new-scripts path (queue empty, early return). Extra work is bounded to the CoinJoin-style derive-mid-sync path and to the retained window (`MAX_RETAINED_COMMITTED_FILTERS`). - Reach/memory are the same knob: inversions spanning more than the window are not recovered by the in-memory path; a birth-height rescan remains the backstop. - In-memory only. A wallet that syncs (or re-syncs) with this code self-heals within the pass; a wallet that already completed its sync and persisted the stall before this fix is not retroactively healed and still needs a rescan from birth. Makes `coinjoin_gap_limit_stall_across_committed_batch` green while keeping the two dashpay#820 guards green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 48 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesCommitted-range filter rescans
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WalletManager
participant FiltersManager
participant BlockMatchTracker
participant CommittedFilters
WalletManager->>FiltersManager: BlockProcessed with new_scripts
FiltersManager->>CommittedFilters: Match scripts against retained filters
FiltersManager->>BlockMatchTracker: Track reopened blocks
FiltersManager-->>WalletManager: BlocksNeeded
WalletManager->>FiltersManager: BlockProcessed for reopened blocks
FiltersManager-->>WalletManager: FiltersSyncComplete after rescans drain
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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 `@dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs`:
- Around line 313-376: Update the documentation around
coinjoin_gap_limit_stall_across_committed_batch and the related module-level
description to identify this as a GREEN regression guard for `#846`, removing
claims that recovery remains impossible or RED-by-design. Rewrite the
highest_used assertion failure message to describe the expected recovery of
indices 40..=51 and the regression where committed-range rescan fails, while
preserving the existing Some(51) expectation.
🪄 Autofix (Beta)
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
Run ID: 4ae9ed3e-a32a-47b2-91f7-235fc0a71b08
📒 Files selected for processing (5)
dash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/block_match_tracker.rsdash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/sync_manager.rs
… case is now a GREEN dashpay#846 regression guard Addresses the CodeRabbit review note on dashpay#873: the module header, doc comment, and assertion message still described the cross-commit stall as an open RED defect; they now document the recovered behavior and read as a regression guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #873 +/- ##
==========================================
+ Coverage 73.71% 73.74% +0.02%
==========================================
Files 325 325
Lines 73799 73907 +108
==========================================
+ Hits 54404 54502 +98
- Misses 19395 19405 +10
|
|
Gap-limit sizing evidence from on-device testing (testnet, Android) — same reference wallet as the earlier confirmation (12.08713251 DASH, 1,049 txs, 4,517 used CoinJoin keys), same device, freshly-created SDK wallet each run, identical code except
Interpretation, honestly bounded: the gap-30 failure is a record-persistence stall in the initial scan pipeline, not a counter-example to this PR — the scan stalls at a layer before the committed-range machinery would be exercised, so the fix never got the chance to act. This PR's deterministic red→green repro remains the correctness proof for the late-derived-scripts defect it fixes. But the practical conclusion for sizing is clear: at 30 the initial-scan pipeline is not currently viable for heavy CoinJoin wallets; at 100 it is fast and exact. We've reverted our local experiment and are staying on gap 100 (#868). |
Comparison with #866, with a field measurement bearing on the 20k windowAuthor of #866 here, so read with that disclosure — but the point of this comment is an empirical measurement that discriminates between the two designs, taken from a real testnet CoinJoin wallet of the same class as the Android wallet cited in this PR's description (1,047 txs, heavy mixing history; full run details in the #866 thread). Both PRs converge on the same mechanism — script-keyed backward re-test routed through The measurementIn #866's field run on that wallet, the deepest backward recoveries reached ~18,200 blocks below the committed frontier at recovery time (a cluster at heights 1484723–1484813, recovered when Cost comparison, measured rather than estimatedThe window's motivation is avoiding disk I/O and bounding memory, but on the same field run #866's full-history disk pass (~662k stored filters) completed in ~1 second, batched to one sweep per commit-with-new-scripts by deferring to forward-fixpoint quiescence. And on peak memory the comparison inverts: #866's transient working set is one 5,000-filter chunk, while the retained window holds 20,000 filters for the life of the sync. So the bounded window pays a correctness cliff to save an I/O cost that measures as negligible, while using more memory than the unbounded approach. Where this PR has the better ideasThe derivation-time Suggested resolution for the maintainers: pick the storage-backed re-test for its unbounded reach, and fold in this PR's ceiling optimization. Happy to do that folding on #866 if that's the preferred direction. 🤖 Generated with Claude Code |
2 participants
Summary
Gap-limit scripts derived after a filter batch commits were never re-tested against the committed range, so outputs paying gap-window addresses in an already-committed block stayed permanently invisible — a silent fund-integrity failure. PR #820 fixed the in-flight (active-batch) half; this PR closes the remaining cross-commit-boundary half.
Fixes #846.
Root cause
Discovery suppression was keyed by scan progress (which heights a wallet had already committed), not by which scripts a block was actually tested against:
rescan_batchbails for any batch not inactive_batches(only in-flight batches are reachable).active_batches.remove) and prunes theBlockMatchTrackerat/below the committed height.BlockProcessedand itssynced_heightadvances at commit.So a forward index↔height inversion — a low external index funded in a later block, whose gap-limit window extension covers a higher index paid in an earlier, already-committed block — leaves the earlier block's outputs unrecoverable.
check_transaction_for_matchonly ever sees already-generated addresses, so the SPV-side re-test is the only recovery path, and it never reached the committed range.This bites CoinJoin wallets hardest (dense, slightly-out-of-order index usage).
Fix
Give the new-script re-test backward reach across the commit boundary, keyed by the script set rather than by scan progress:
committed_filterskeyed by height. The common no-new-scripts path is untouched — this only changes whether a just-committed batch's filters are freed or parked.BlockProcessedthat carries gap-limit-derived scripts enqueues them together with thecommitted_heightsnapshot at derivation time. Heights above that ceiling were still in active batches at derivation and were tested against these scripts by the normal per-batch scan, so only the range at/below the ceiling is owed a re-test.try_process_batchre-tests the fresh scripts against the retained committed filters ≤ ceiling and re-queues any matched committed block through the existingtrack_for_new_scriptspath. A re-opened block that itself derives further scripts feeds the same queue, cascading to quiescence.committed_tested_scriptsensures each script pays exactly one pass over the committed range; a range that commits later already saw the script via the normal scan, so no re-test is owed for it.FiltersSyncCompleteis held off while a re-opened below-frontier block is still in flight (it has no owning active batch to account for it); retained filters are released on full sync.Trade-offs
MAX_RETAINED_COMMITTED_FILTERS(20 000 heights ≈ 4 batches). Filters are matched in memory — no disk I/O.BlockProcessedthat derives scripts, which happens whenever a funding block is (re)processed during a sync pass — so a wallet that syncs (or re-syncs) with this code present self-heals within that pass (this is the real-world fresh-sync recovery case). A wallet that had already completed its sync and persisted the stall before this fix does not re-derive scripts on restart (nothing below its persistedsynced_heightis reprocessed) and is not retroactively healed; such a wallet still needs a rescan from its birth height. New syncs never enter the stalled state. Persisting a per-wallet "scripts-tested-through" watermark to also retroactively heal pre-existing persisted stalls is a larger, schema-touching change left as follow-up.Real-world confirmation
Android testnet integration (dashj→Kotlin-SDK migration): a real long-lived wallet — 12.087 DASH, 1,047 txs, 4,517 used CoinJoin keys, birth 0 — restored byte-exact under dashj 22.0.3 but landed synced with 0 txs / 0 duffs under a fresh SDK wallet even at gap limit 100, no fault raised. Recovery required delete+recreate (which re-registers the full script set before the first commit). This is exactly Found-034 viewed from the other side: it is the re-scan gate keying, not the window size. See #846 (comment).
Test evidence (red → green)
Ported the deterministic crate-level repro (real
FiltersManager+BlockMatchTracker+WalletManagerwith a CoinJoin account, synthetic blocks, real BIP-158 filters, no network) fromrepro/pr3549-rdc:coinjoin_gap_limit_dense_same_batch_recovers(#820 guard)coinjoin_gap_limit_inversion_within_batch_recovers(#820)coinjoin_gap_limit_stall_across_committed_batchhighest_used=29highest_used=51cargo test -p dash-spv --lib→ 474 passedcargo test -p key-wallet-manager→ all passedcargo fmt --all --checkclean;cargo clippy -p dash-spv --all-targets --all-features -- -D warningsclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests