fix(dash-spv): stop losing derived scripts, and close the loop on wallet state - #989
Open
ZocoLini wants to merge 1 commit into
Open
fix(dash-spv): stop losing derived scripts, and close the loop on wallet state#989ZocoLini wants to merge 1 commit into
ZocoLini wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughFilters now track scripts tested by each batch, route newly derived scripts by height, reconcile missed wallet scripts, and defer synchronization completion until backward scripts and in-flight blocks are processed. ChangesFilter synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This update preserves derived wallet scripts through repeated and out-of-order synchronization, reconciles untested scripts before commit, and delays completion until matched blocks are processed. The covered synchronization scenarios show no remaining merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant SyncManager
participant FiltersManager
participant FiltersBatch
participant BlockMatchTracker
SyncManager->>FiltersManager: collect_new_scripts(height, new_scripts)
FiltersManager->>FiltersBatch: route scripts by batch height
FiltersManager->>FiltersBatch: reconcile_untested_scripts(batch_start)
FiltersBatch->>BlockMatchTracker: rescan missing scripts
FiltersManager->>BlockMatchTracker: has_blocks_in_flight()
BlockMatchTracker-->>FiltersManager: pending block status
FiltersManager-->>SyncManager: FiltersSyncComplete when processing finishes
Suggested reviewers: 🚥 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 #989 +/- ##
==========================================
+ Coverage 77.18% 77.25% +0.07%
==========================================
Files 329 329
Lines 83603 83836 +233
==========================================
+ Hits 64528 64771 +243
+ Misses 19075 19065 -10
|
ZocoLini
force-pushed
the
fix/sync-determinism-script-routing
branch
from
September 2, 2026 18:08
eeddc4b to
974c2c3
Compare
6 tasks
…let state Two syncs of the same wallet against the same chain returned balances 2 000 020 sat apart, about half the time each. The money was one block, 2 429 667, which the runs that ended high never processed. Scripts derived while applying a block were handed to the batch named by the block's in-flight record. That record is keyed by block hash and consumed by the first delivery, and a block is delivered more than once — a rescan re-queues what the forward scan already handed over, and on a mainnet restore 2 904 of 3 039 relevant heights arrive twice or more. Every later delivery found no record, so its scripts reached no batch, no later batch, and no backward sweep. What made it invisible is that the derivation itself was not lost: the wallet kept the addresses. So no later block reported them as new, no rescan carried them, and the filter layer went on matching a query it did not know was incomplete. Measured: a second delivery of the block at 2 429 637 derived 27 scripts covering the mixing session that owns 2 429 667, the batch holding that block rescanned four times without them, and the block was never matched. `collect_new_scripts` now routes by height instead — to the batch whose range contains the block, which still holds that range's filters, falling back to the backward accumulator only when no active batch covers the height. That alone would still rest on a one-shot notification arriving, and 23.6% of blocks are applied out of order, so notification-shaped invariants are not worth much here. `reconcile_untested_scripts` therefore closes the loop on state: each batch records which scripts have been matched against its filters, and before committing it asks the wallet what it watches now and re-tests the difference. It also gives the lower active batches the scripts a higher one derived, which nothing did before. Ten full mainnet restores across five configurations now return the same 13 876 outputs and the same balance, where the same wallet previously split roughly 50/50 between two answers. Two of the first three runs exercised the routing path (92 and 27 scripts rescued), so the agreement is not luck. Two consequences of the new routing, both handled here. `rescan_batch` marks scripts tested before its empty-filters return, as `scan_batch` already did: otherwise a batch with no filters is handed the same set by every commit attempt and never converges. And `backward_scripts` can now be non-empty with no active batch, when a block is delivered after its batch committed — so the assertion in `try_process_batch` that it is empty no longer holds. Those scripts cannot be left to a next commit that may never come: the accumulator is in-memory only, nothing looks below the committed frontier again, and a shutdown at the tip loses them for good, since the restart resumes with `committed_height` already at the tip and no batch to reconcile them against. The completion branch therefore sweeps them itself over the whole committed range, and holds `FiltersSyncComplete` while the blocks that sweep found are still in flight. The gate is the tracker, not the commit gate: blocks a tip sweep queues are charged to no batch, so no batch can hold the completion for them. Their `BlockProcessed` re-enters the branch, and a round that derives no new scripts is the fixpoint. Processed records left by blocks applied after the last commit are pruned there too, since no commit will. Four regression tests, each failing without the fix: a `BlockProcessed` with no in-flight record whose scripts must reach the batch covering the height (and the backward accumulator when none does); a batch scanned with a query missing one address, whose commit must find that address's block without ever being told; a rescan of an empty batch, which must still record what it was handed; and a tip with scripts stranded in the accumulator, which must sweep them and withhold completion until the block it finds has been applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92DcuiKs8UdWkrhyfghqX
ZocoLini
force-pushed
the
fix/sync-determinism-script-routing
branch
from
September 7, 2026 17:33
974c2c3 to
e544cdf
Compare
ZocoLini
marked this pull request as ready for review
September 7, 2026 17:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Scripts derived while applying a block were routed via the block's in-flight record, which the first delivery consumes — and blocks are delivered more than once (2 904 of 3 039 heights on a mainnet restore). Every later delivery's scripts reached no batch and nothing matched them again. Two syncs of the same wallet ended 2 000 020 sat apart.
What was done
collect_new_scriptsroutes by height instead.reconcile_untested_scriptsre-tests, before each commit, whatever the wallet watches that the batch never matched — closing the loop on state, not on notifications. The completion branch sweeps scripts left with no batch to carry them and withholdsFiltersSyncCompleteuntil the blocks it finds are applied.On that last point: routing by height makes
backward_scriptsnon-empty with no active batch reachable (a block delivered after its batch committed), which the old code asserted could not happen. Leaving those scripts is a loss, not a delay — the accumulator is in-memory and a restart resumes at the tip with no batch to reconcile them. So completion sweeps them and waits on the tracker's in-flight blocks rather than on a batch's commit gate, re-entering on eachBlockProcesseduntil a round derives nothing new; processed records get pruned there since no commit will. Hang risk is unchanged. Caveat: the window never occurred in four bench syncs, so its only coverage is the unit test.Summary by CodeRabbit