Skip to content

fix(dash-spv): stop losing derived scripts, and close the loop on wallet state - #989

Draft
ZocoLini wants to merge 1 commit into
devfrom
fix/sync-determinism-script-routing
Draft

fix(dash-spv): stop losing derived scripts, and close the loop on wallet state#989
ZocoLini wants to merge 1 commit into
devfrom
fix/sync-determinism-script-routing

Conversation

@ZocoLini

Copy link
Copy Markdown
Collaborator

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, and is replaced by a trace. Those scripts are swept by the next batch commit, which is a delay where the old routing simply dropped them.

Three 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; and a rescan of an empty batch, which must still record what it was handed.

Claude-Session: https://claude.ai/code/session_015NhHBDGiKfiGpy7FwooyfS

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.70588% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.95%. Comparing base (93260bf) to head (974c2c3).

Files with missing lines Patch % Lines
dash-spv/src/sync/filters/manager.rs 94.15% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #989      +/-   ##
==========================================
- Coverage   77.10%   76.95%   -0.16%     
==========================================
  Files         329      329              
  Lines       83511    83679     +168     
==========================================
  Hits        64394    64394              
- Misses      19117    19285     +168     
Flag Coverage Δ
core 78.25% <ø> (ø)
ffi 49.36% <ø> (-1.56%) ⬇️
rpc 20.00% <ø> (ø)
spv 92.11% <94.70%> (+0.04%) ⬆️
wallet 79.62% <ø> (ø)
Files with missing lines Coverage Δ
dash-spv/src/sync/filters/batch.rs 97.87% <100.00%> (+0.27%) ⬆️
dash-spv/src/sync/filters/sync_manager.rs 100.00% <ø> (ø)
dash-spv/src/sync/filters/manager.rs 97.71% <94.15%> (-0.23%) ⬇️

... and 16 files with indirect coverage changes

…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, and is
replaced by a trace. Those scripts are swept by the next batch commit, which
is a delay where the old routing simply dropped them.

Three 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; and a rescan of an empty batch, which must still record what it was
handed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015NhHBDGiKfiGpy7FwooyfS
@ZocoLini
ZocoLini force-pushed the fix/sync-determinism-script-routing branch from eeddc4b to 974c2c3 Compare September 2, 2026 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant