fix(dash-spv): rescan committed filter ranges for newly derived scripts#866
fix(dash-spv): rescan committed filter ranges for newly derived scripts#866HashEngineering wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough
ChangesCoinJoin gap-limit recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FiltersManager
participant DiskStorageManager
participant BlockMatchTracker
participant WalletManager
FiltersManager->>DiskStorageManager: load committed filter chunks
DiskStorageManager-->>FiltersManager: return persisted filters
FiltersManager->>BlockMatchTracker: match newly derived scripts
BlockMatchTracker-->>FiltersManager: emit BlocksNeeded
FiltersManager->>WalletManager: process requested blocks
WalletManager-->>FiltersManager: emit BlockProcessed
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #866 +/- ##
==========================================
+ Coverage 73.69% 73.97% +0.27%
==========================================
Files 324 324
Lines 73434 73513 +79
==========================================
+ Hits 54117 54381 +264
+ Misses 19317 19132 -185
|
![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
🧹 Nitpick comments (1)
dash-spv/src/sync/filters/manager.rs (1)
1002-1058: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate tracking/event-building logic vs.
rescan_batch.The
block_to_wallets→tracker.track_for_new_scripts→blocks_needed/new_blocks_count→pending_blocksaccounting block (lines 1002-1058) closely mirrors the equivalent section inrescan_batch(lines 705-747). Extracting a shared helper (taking the match map,batch_start, and mutable access totracker/progress/active_batches) would reduce the risk of the two paths silently diverging on future changes.♻️ Sketch of a shared helper
fn attribute_matches_for_new_scripts( &mut self, batch_start: u32, block_to_wallets: BTreeMap<FilterMatchKey, BTreeSet<WalletId>>, ) -> Vec<SyncEvent> { // shared body: track_for_new_scripts loop, pending_blocks bump, BlocksNeeded push }🤖 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 1002 - 1058, Extract the duplicated match-attribution and event-building logic from rescan_batch and the committed-range rescan into a shared helper, such as attribute_matches_for_new_scripts, on the containing type. Have it accept batch_start and the block_to_wallets map, perform tracker.track_for_new_scripts, update progress/pending_blocks/active_batches consistently, and produce the required blocks_needed, new_blocks_count, and SyncEvent results; replace both inline implementations with this helper.
🤖 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/manager.rs`:
- Around line 966-1030: Optimize rescan_committed_range to avoid rescanning the
same committed history for each fixpoint round. Add per-wallet high-water marks
tracking the earliest range already swept without matches, and use them to set
each wallet’s scan start so only previously unchecked history is loaded and
matched; update the marks after each successful sweep while preserving rescans
of ranges where matches require processing.
---
Nitpick comments:
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 1002-1058: Extract the duplicated match-attribution and
event-building logic from rescan_batch and the committed-range rescan into a
shared helper, such as attribute_matches_for_new_scripts, on the containing
type. Have it accept batch_start and the block_to_wallets map, perform
tracker.track_for_new_scripts, update progress/pending_blocks/active_batches
consistently, and produce the required blocks_needed, new_blocks_count, and
SyncEvent results; replace both inline implementations with this helper.
🪄 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: 7f2fbebe-28a8-4938-b90c-ac5eeab927c7
📒 Files selected for processing (4)
dash-spv/src/storage/filters.rsdash-spv/src/storage/mod.rsdash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rsdash-spv/src/sync/filters/manager.rs
|
Review feedback addressed in 5cef7c5:
All 3 acceptance tests and the full dash-spv suite (474) still pass; clippy/fmt clean. |
…ts (dashpay#846) Scripts derived by gap-limit maintenance mid-sync (CoinJoin index-height inversions being the concrete case) previously had zero backward reach across a batch-commit boundary: `rescan_batch` only reaches `active_batches`, commit prunes the tracker records, and the wallet's advanced `synced_height` filters those heights out of any later match. Outputs paying such scripts in an already-committed range stayed permanently invisible, and a fresh resync hit the same wall. BIP-158 filters are address-independent commitments and are all persisted, so the committed range can be re-tested from storage with no network traffic. `rescan_committed_range` runs at the same commit-time seam as the dashpay#820 forward rescan: it matches only the newly derived scripts against stored filters below the committing batch (chunked, best-effort per chunk), routes hits through the existing `track_for_new_scripts` re-download path, and attributes them to the committing batch so its pending-blocks accounting holds the commit open until the fixpoint drains — scripts derived from re-processed backward blocks feed the next round automatically. Adds `FilterStorage::filter_start_height` to bound the sweep, and adopts the cross-commit repro from `repro/pr3549-rdc` (with header/filter storage seeded to match the production invariant that heights at or below `stored_height` are persisted) as the regression gate; the two dashpay#820 guards stay green. Fixes dashpay#846 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…edupe match attribution Address review feedback on the committed-range rescan: - The backward sweep over stored history is the expensive direction, and the commit-time fixpoint could re-walk it once per derivation round. Scripts now accumulate on the committing batch (`FiltersBatch::backward_scripts`) while the forward fixpoint converges, and a single combined sweep runs only when the forward direction is quiescent. Blocks it finds re-enter through `collected_scripts`, so only genuinely new scripts get a follow-up sweep — each distinct script crosses the committed range exactly once. (A skip-swept-ranges high-water mark would be unsound here: a range swept for round N's scripts is not covered for round N+1's.) - Extract `queue_new_script_matches` as the shared tail of `rescan_batch` and `rescan_committed_range` (track_for_new_scripts routing, pending_blocks accounting, BlocksNeeded emission) so the two paths cannot silently diverge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HashEngineering
force-pushed
the
fix/committed-range-rescan-846
branch
from
5cef7c5 to
381a4fe
Compare
Field validation: real CoinJoin wallet on testnet, dev vs. this PR vs. DashJTested with the
The 10 transactions dev misses — and whyAll ten sit in the final ~280 blocks of batch [1480000–1484999] (batches commit at 850000 + 5000k), immediately below the 1485000 commit boundary: Their addresses were derived from transactions discovered in blocks at or above 1485000 — after batch [1480000–1484999] had committed — so on dev nothing ever re-tests those filters: the #846 mechanism, observed in the wild. All 10 are present in DashJ's set. Both runs used the same data dir (shared chain storage), and the wallet rebuilds from the mnemonic each run, so the miss is deterministic — a resync does not recover them on dev. Note the nine net-zero mixing rounds understate the damage: each spends a denominated input the wallet owns, so missing them leaves those denoms falsely unspent and silently corrupts the coins' mixing history/round counts. The recovery path, visible in the logThe fix run shows the committed-range sweep firing at successive batch commits as the discovery frontier climbs: The 10 transactions' detection timestamps land inside that sequence. Each full-history re-match completed in ~1s wall-clock (stored filters + GCS matching), supporting the deferred-sweep design being sufficient without further optimization. Known residual (separate issue)The fix run's final balance is 16.08717251 vs DashJ's 12.08713251 — exactly 4 × 1.00001 denominated coins held as falsely unspent. Identical tx sets with an inflated balance points at #649 (out-of-order UTXO spend), fixed by the open #851: the backward sweep re-applies old funding blocks after their spends were already processed, which without #851 re-inserts spent outputs as spendable. The two PRs are complementary — this one fixes which transactions are found, #851 fixes applying them out of order — and a combined-branch run is expected to reproduce DashJ's balance exactly. 🤖 Generated with Claude Code |
Follow-up: combined run with #851 closes the balance residualRe-ran the same wallet/data-dir with this PR merged together with #851 (plus a local CLI patch passing One nuance surfaced for completeness: the combined run records 1039 of the 1047 transactions — the 8 gap are out-of-order funding txs that #851's spend-first path currently skips from history (balance unaffected; blocks were downloaded and processed, so discovery by this PR worked). Details reported on #851. 🤖 Generated with Claude Code |
1 participant
What
Fixes #846: scripts derived by gap-limit maintenance mid-sync had zero backward reach across a batch-commit boundary.
rescan_batchonly reachesactive_batches; once a batch commits it is removed, its tracker records are pruned, and the wallet's advancedsynced_heightfilters those heights out of any later match. An output paying a script that didn't exist yet when its block's batch committed stayed permanently invisible — and a fresh resync from genesis hit the same wall deterministically. The concrete population is CoinJoin wallets, whose parallel mixing sessions produce index↔height inversions (the e2e ground truth in #846 shows a real one at index ~1767).How
BIP-158 filters are address-independent commitments and every height at or below
stored_heighthas its filter persisted — so the committed range can be re-tested from local storage, with no network traffic. This is the structural advantage over the BIP37/bloom approach (bitcoinj/DashJ must discard and re-download blocks on filter exhaustion because a stale client-supplied filter makes peers stop sending relevant txs; a stored BIP-158 filter never goes stale, only the query side does).rescan_committed_rangeruns at the same commit-time seam as the #820 forward rescan, whenever a committing batch has newly derived scripts:BATCH_PROCESSING_SIZE, best-effort per chunk (a chunk the storage can't serve is skipped with a warning rather than failing the commit);track_for_new_scriptsre-download path, so previously-processed blocks are re-applied against the extended pools;pending_blocksaccounting holds the commit open and scripts derived from re-processed backward blocks feed the next fixpoint round automatically.The sweep is bounded below by the earliest wallet birth height and the first stored filter; the latter comes from a new
FilterStorage::filter_start_heightaccessor (the underlyingSegmentCachealready tracked it).Testing
Adopts the cross-commit repro from
repro/pr3549-rdc(coinjoin_gap_discovery_tests.rs) as the regression gate. One harness change was needed: the original test injected batches without persisting anything, but in productionstored_heightonly advances after filters are written to storage — and stored filters are exactly what the recovery path reads. The test now seeds header/filter storage for the committed range to uphold that invariant. The assertion is unchanged.Before (dev + tests only) — the seeded harness alone does not make it pass:
After (dev + tests + fix):
Full
dash-spvsuite: 474 unit + 7 bin tests pass; clippy clean; fmt applied. (dashd_masternodeintegration tests requireDASHD_PATHand were not run.)Notes for review
new_scriptsfrom aBlockProcessedwhose block isn't in the in-flight tracker (sync_manager.rs— tip/mempool-confirmation paths) are still dropped without triggering any rescan.Fixes #846
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests