Skip to content

fix(dash-spv): reprocess blocks on rescan against newly derived scripts#820

Open
xdustinface wants to merge 2 commits into
devfrom
fix/reprocess-blocks
Open

fix(dash-spv): reprocess blocks on rescan against newly derived scripts#820
xdustinface wants to merge 2 commits into
devfrom
fix/reprocess-blocks

Conversation

@xdustinface

@xdustinface xdustinface commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

The commit-time batch rescan re-matches scripts derived during block processing against the active batches, but BlockMatchTracker::track subtracted wallets that already had a block applied, so a block processed before those scripts existed was silently skipped. Scripts derived from processing a block could therefore never trigger a re-examination of any earlier block in the same batch, and partially recognized blocks stayed partial.

Add track_for_new_scripts, which keeps the in-flight accounting of track but ignores processed records, and use it in rescan_batch: every match there is by construction against scripts that did not exist at first processing. Re-application is safe because wallet-side block processing is idempotent (transaction dedup, outpoint-keyed UTXO insertion, balance recomputed from the UTXO set). Termination is preserved since the rescan only queries newly collected scripts, and re-processing a block whose outputs are already recognized derives nothing further.

The plain scan_batch path keeps the processed-record skip, late-added wallets still get their residual pass through plain track.

Summary by CodeRabbit

  • New Features

    • Wallet sync now detects payments to newly derived addresses during rescans, helping previously missed transactions appear after script/address discovery expands.
    • Historical syncing is more reliable for large batches of fresh addresses, even beyond the usual address gap window.
  • Bug Fixes

    • Fixed an issue where blocks already seen could be skipped during rescan, which could prevent new wallet matches from being counted and delay balance updates.

The commit-time batch rescan re-matches scripts derived during block processing against the active batches, but `BlockMatchTracker::track` subtracted wallets that already had a block applied, so a block processed before those scripts existed was silently skipped. Scripts derived from processing a block could therefore never trigger a re-examination of any earlier block in the same batch, and partially recognized blocks stayed partial.

Add `track_for_new_scripts`, which keeps the in-flight accounting of `track` but ignores processed records, and use it in `rescan_batch`: every match there is by construction against scripts that did not exist at first processing. Re-application is safe because wallet-side block processing is idempotent (transaction dedup, outpoint-keyed UTXO insertion, balance recomputed from the UTXO set). Termination is preserved since the rescan only queries newly collected scripts, and re-processing a block whose outputs are already recognized derives nothing further.

The plain `scan_batch` path keeps the processed-record skip, late-added wallets still get their residual pass through plain `track`.
One transaction pays a run of consecutive fresh addresses reaching past the gap window, mined before the client ever starts. The historical scan must climb the burst via fixpoint re-scanning and credit every output. Adds `DashCoreNode::send_many` so a regtest wallet can pay many addresses in a single transaction.
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds track_for_new_scripts to BlockMatchTracker, which re-queues blocks without an AlreadyProcessed path. Wires this into FiltersManager::rescan_batch for script-driven re-processing. Adds a send_many RPC helper to DashCoreNode and an integration test validating from-scratch SPV sync of a burst payment beyond the external gap window.

Changes

Gap-window rescan via track_for_new_scripts

Layer / File(s) Summary
track_for_new_scripts primitive and unit tests
dash-spv/src/sync/filters/block_match_tracker.rs
Adds BlockMatchTracker::track_for_new_scripts, returning NewlyTracked (inserting into blocks_remaining) or InFlight but never AlreadyProcessed. Extends track_state_machine test to assert both outcomes.
rescan_batch wiring and unit test
dash-spv/src/sync/filters/manager.rs
Replaces tracker.track(...) with tracker.track_for_new_scripts(...) in rescan_batch, removes the now-impossible InFlight arm, and adds test_rescan_batch_reprocesses_already_processed_block asserting a previously processed block is re-queued with pending_blocks set to 1.
send_many helper and burst-payment integration test
dash-spv/src/test_utils/node.rs, dash-spv/tests/dashd_sync/tests_transaction.rs
Adds DashCoreNode::send_many using serde_json::Map for multi-output RPC calls. Adds derive_external_addresses BIP44 helper and test_burst_payment_beyond_gap_window_synced_from_scratch asserting from-scratch sync discovers a pre-mined burst payment beyond the external gap window.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • dashpay/rust-dashcore#781: Directly related — that PR refactored the new_scripts/ScriptBuf pipeline through FiltersManager and rescan_batch, which is the same code path that track_for_new_scripts is now wired into.

Suggested labels

ready-for-review

Suggested reviewers

  • ZocoLini

Poem

🐇 Hop beyond the gap window's wall,
Where old "AlreadyProcessed" could stall,
track_for_new_scripts queues the block anew,
send_many coins to addresses true,
The burst transaction found by SPV's call —
No script derived fresh shall be missed, not at all! 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: rescanning blocks against newly derived scripts.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/reprocess-blocks

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 Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.02%. Comparing base (ceee4a9) to head (368456c).
⚠️ Report is 3 commits behind head on dev.

Files with missing lines Patch % Lines
dash-spv/src/sync/filters/manager.rs 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #820      +/-   ##
==========================================
+ Coverage   72.82%   73.02%   +0.20%     
==========================================
  Files         323      323              
  Lines       72000    72072      +72     
==========================================
+ Hits        52432    52633     +201     
+ Misses      19568    19439     -129     
Flag Coverage Δ
core 76.74% <ø> (ø)
ffi 47.34% <ø> (+1.33%) ⬆️
rpc 20.00% <ø> (ø)
spv 90.24% <98.27%> (+0.06%) ⬆️
wallet 71.80% <ø> (ø)
Files with missing lines Coverage Δ
dash-spv/src/sync/filters/block_match_tracker.rs 99.35% <100.00%> (+0.12%) ⬆️
dash-spv/src/sync/filters/manager.rs 97.44% <96.87%> (-0.06%) ⬇️

... and 18 files with indirect coverage changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/test_utils/node.rs`:
- Around line 322-327: The send_many helper is collapsing duplicate destination
addresses when building the serde_json::Map, which can silently change the
intended outputs. Update send_many to detect repeated address strings before
constructing the RPC object, and either reject them with an explicit error or
aggregate their amounts intentionally. Make the behavior clear in send_many and
the amount-to-map building logic so each `(address, amount)` pair is handled
deterministically.
🪄 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: e4d9b656-3aa2-423c-a62f-c0a3fcc1e0ff

📥 Commits

Reviewing files that changed from the base of the PR and between 95a3c8f and 368456c.

📒 Files selected for processing (4)
  • dash-spv/src/sync/filters/block_match_tracker.rs
  • dash-spv/src/sync/filters/manager.rs
  • dash-spv/src/test_utils/node.rs
  • dash-spv/tests/dashd_sync/tests_transaction.rs

Comment thread dash-spv/src/test_utils/node.rs
@github-actions github-actions Bot added the ready-for-review CodeRabbit has approved this PR label Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review CodeRabbit has approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant