fix(dash-spv): avoid filter sync getting stuck on the first new block after a restart#835
Open
xdustinface wants to merge 2 commits into
Open
fix(dash-spv): avoid filter sync getting stuck on the first new block after a restart#835xdustinface wants to merge 2 commits into
xdustinface wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughRestart and syncing filter paths now anchor scan cursors from the stored filter tip, create lookahead batches even while active batches exist, and cover the restart/rescan boundary cases with updated regression tests. ChangesFilters commit boundary fix
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SyncManager
participant FiltersManager
participant Pipeline
participant Peer
Client->>SyncManager: start_sync()
SyncManager->>SyncManager: set filter header tip to stored_filters_tip
SyncManager->>FiltersManager: start_download()
FiltersManager->>FiltersManager: compute download_start from stored tip
FiltersManager->>Pipeline: init/park at download_start
Peer-->>FiltersManager: new filter headers advance tip
FiltersManager->>FiltersManager: handle_new_filter_headers()
FiltersManager->>Pipeline: try_create_lookahead_batches()
Possibly related PRs
Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dash-spv/src/sync/filters/sync_manager.rs (1)
90-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCursor anchoring on the fully-synced restart path is correct.
Setting
next_batch_to_store/processing_heighttostored_filters_tip + 1here mirrors the frontier anchoring now done inmanager.rs::start_download, closing the reconnect-after-disconnect variant of the#824wedge. Directly validated bytest_start_sync_synced_restart_then_boundary_block_commitsinmanager.rs.Since this "anchor cursors to the frontier" pattern now exists in two places (here and
start_download's early return), consider extracting a small shared private helper (e.g.fn anchor_frontier(&mut self, height: u32)) to reduce the chance of a third restart path drifting out of sync with this fix in the future, similar to how this bug originated from two independently-evolving restart paths.♻️ Possible shared helper (illustrative)
+ /// Anchor the store/processing cursors at `height` so a block arriving + /// before the next explicit batch creation doesn't wedge the in-order + /// store loop or create a lookahead batch from a stale frontier. + fn anchor_frontier(&mut self, height: u32) { + self.next_batch_to_store = height; + self.processing_height = height; + }🤖 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/sync_manager.rs` around lines 90 - 97, The cursor anchoring logic is correct, but it is duplicated between the fully-synced restart path in sync_manager.rs and the frontier-anchoring early return in manager.rs::start_download. Extract that shared “anchor cursors to the frontier” behavior into a small private helper such as anchor_frontier on the relevant manager type, and call it from both places so next_batch_to_store and processing_height stay synchronized and future restart paths cannot drift apart.
🤖 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.
Nitpick comments:
In `@dash-spv/src/sync/filters/sync_manager.rs`:
- Around line 90-97: The cursor anchoring logic is correct, but it is duplicated
between the fully-synced restart path in sync_manager.rs and the
frontier-anchoring early return in manager.rs::start_download. Extract that
shared “anchor cursors to the frontier” behavior into a small private helper
such as anchor_frontier on the relevant manager type, and call it from both
places so next_batch_to_store and processing_height stay synchronized and future
restart paths cannot drift apart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b33ee98d-66cc-45a1-83b7-8bea96bbde52
📒 Files selected for processing (3)
dash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/tests/dashd_sync/tests_restart.rs
xdustinface
force-pushed
the
fix/filters-tip-boundary-stall
branch
from
9b5a8af to
17fe13c
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/sync/filters/sync_manager.rs (1)
85-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStart downloading when persisted filter headers are already ahead.
If a restarted manager restores
filter_header_tip_height = 101from storage whilestored_filters_tip == committed_height == 100, this branch anchors cursors but then falls through toWaitForEventsat Line 110. No newFilterHeadersStoredevent is guaranteed, so the boundary filter body is never requested and sync can remain one block behind.Proposed fix
if stored_filters_tip > 0 && stored_filters_tip == self.progress.committed_height() { self.progress.update_filter_header_tip_height(stored_filters_tip); // Initialize the pipeline at the current tip. On full disconnect in-flight state gets // reset, so we need to initialize the pipeline otherwise it would re-queue from height 1. self.filter_pipeline.init(stored_filters_tip + 1, stored_filters_tip); @@ self.next_batch_to_store = stored_filters_tip + 1; self.processing_height = stored_filters_tip + 1; + + if self.progress.filter_header_tip_height() > stored_filters_tip { + let mut events = vec![SyncEvent::SyncStart { + identifier: self.identifier(), + }]; + events.extend(self.start_download(requests).await?); + return Ok(events); + } + // Only emit SyncComplete if we've also reached the chain tipPlease add a regression where
FiltersManager::newrestores filter headers past the stored filter tip beforestart_syncruns. As per coding guidelines,Write unit tests for new functionality.🤖 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/sync_manager.rs` around lines 85 - 97, The restart path in FiltersManager::start_sync leaves the manager in WaitForEvents even when update_filter_header_tip_height has restored filter_header_tip_height ahead of stored_filters_tip, so the boundary body download is never triggered. Update the start_sync logic to detect this persisted-ahead state and immediately begin requesting the missing filter bodies instead of relying on a future FilterHeadersStored event. Add a regression test around FiltersManager::new and start_sync that restores filter headers past the stored tip and verifies the manager starts downloading the next boundary block.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@dash-spv/src/sync/filters/sync_manager.rs`:
- Around line 85-97: The restart path in FiltersManager::start_sync leaves the
manager in WaitForEvents even when update_filter_header_tip_height has restored
filter_header_tip_height ahead of stored_filters_tip, so the boundary body
download is never triggered. Update the start_sync logic to detect this
persisted-ahead state and immediately begin requesting the missing filter bodies
instead of relying on a future FilterHeadersStored event. Add a regression test
around FiltersManager::new and start_sync that restores filter headers past the
stored tip and verifies the manager starts downloading the next boundary block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d8b4240-e176-4579-9228-a607341c5687
📒 Files selected for processing (3)
dash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/tests/dashd_sync/tests_restart.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/tests/dashd_sync/tests_restart.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #835 +/- ##
==========================================
+ Coverage 73.44% 73.57% +0.13%
==========================================
Files 324 324
Lines 72577 72758 +181
==========================================
+ Hits 53306 53534 +228
+ Misses 19271 19224 -47
|
… after a restart Fixes #824. A fully synced client that restarted, or reconnected after losing all peers, came back with its filter download state uninitialized. The next new block then re-requested every filter from height 1 and wedged the in-order store loop, so `committed_height` froze one block below the tip and the client never reported synced again. `start_download` now anchors `next_batch_to_store` and `processing_height` and parks the pipeline at the download frontier before its already-synced early return. The already-synced reconnect branch of `start_sync` delegates to `start_download` instead of duplicating that logic, which also downloads the boundary filter body when the restored filter header tip is ahead of the stored bodies. `handle_new_filter_headers` now gives a new boundary block a processing batch even while a historical rescan is still draining `active_batches`. Covered by unit regression tests for both restart paths, a reconnect test with filter headers ahead of stored filter bodies, and a dashd integration test that mines a block right after a fully synced restart.
xdustinface
force-pushed
the
fix/filters-tip-boundary-stall
branch
from
a45445f to
f2b4c18
Compare
Contributor
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/sync_manager.rs`:
- Around line 87-89: The SyncManager::start_download path is overwriting a newer
filter-header tip with stored_filters_tip when committed_height matches, which
can regress progress and skip the missing boundary body request. Update the
logic in SyncManager::start_download (or the surrounding filter-tip update path)
to preserve the already-known frontier by taking the max of the existing
filter_header_tip_height and stored_filters_tip before calling
update_filter_header_tip_height, so the later early-return check does not
short-circuit the needed download.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7db2055-4106-4e78-b7c2-0916f1539034
📒 Files selected for processing (3)
dash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/tests/dashd_sync/tests_restart.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv/tests/dashd_sync/tests_restart.rs
- dash-spv/src/sync/filters/manager.rs
github-actions
Bot
added
ready-for-review
ZocoLini
approved these changes
Jul 3, 2026
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
3 participants
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.
Fixes #824. When a fully synced client restarted and then a new block arrived, the block's filter was downloaded but never processed. The client stayed one block behind forever and never reported synced again.
start_downloadnow anchors the store and processing cursors and parks the pipeline at the frontier before returning early on an already-synced wallet.start_syncnow delegates tostart_downloadinstead of duplicating that logic, which also downloads the boundary filter body when the restored filter header tip is ahead of the stored bodies.Summary by CodeRabbit