fix(dash-spv): stop the filter rescan loop when the chain anchors above the wallet - #933
Merged
Merged
Conversation
…ve the wallet A wallet whose `synced_height` sits below the chain anchor could never be certified, and the resulting rescan never terminated. On mainnet `hd_wallet_sync_floor` pushes the anchor to the checkpoint at 200000 while the CLI creates wallets with birth height 0, so `synced_height` stayed at 0 for the whole run: 4846 restarts and 51784 batch scans produced zero sync-height advances and burned ~350% CPU indefinitely. Two independent gates jammed, both because neither knew that heights below the anchor are out of scope rather than pending work. `tick` restarted the scan whenever `wallets_behind` reported anyone below the committed frontier, wiping in-flight batches before any could commit. It now compares against the height a restart would actually resume from, which is floored at the wallets' birth heights and the stored headers' start. The commit-time contiguity guard required `synced_height + 1 >= batch_start`, which reads `1 >= 200000` under an anchor and never holds. It now substitutes `scan_floor - 1` for an unreachable `synced_height`. The floor deliberately excludes the `committed_height + 1` term of `scan_start`: tracking the scan frontier would reduce the guard to `batch_start >= batch_start` and delete the height check that `#649` relies on. A genuine gap above the floor still blocks the advance, and the account-generation guard is untouched. The floor comes from the stored header start rather than `hd_wallet_sync_floor`, so `--start-height` is covered too. That path bypasses the network clamp entirely and reproduces the same loop on testnet. Also warn when the anchor exceeds the wallet's birth height. That range is never scanned, and because BIP44 discovery is sequential, usage hidden in it can stop the gap-limit window from advancing and mask later addresses.
…llet `hd_wallet_sync_floor` is non-zero only for mainnet, so every existing test ran with a scan floor of 0 where the contiguity guard reads `0 + 1 >= 0` and passes. The suite was structurally unable to reach the stall. `create_anchored_test_manager` stores headers at a non-zero start height instead, which is where the floor is read from. That reproduces the mainnet shape without depending on `Network`, so it runs in normal CI. Four cases fail before the fix: a wallet below the anchor reaching `Synced`, the boundary sweep over `synced_height` in `test_tick_does_not_rescan_when_no_wallets_behind` (199998 included so a fix keyed on `synced_height == 0` does not pass), no re-request of in-flight filters after a tick, and a wallet added at runtime converging. The last one is the case that raising the birth height at construction would not have covered. Two more extend the existing `#649` guard tests and pass both before and after. They pin the behavior a wrong floor would break: a gap between the anchor and the batch must still block certification, and with contiguity satisfied at the floor the generation guard must be the only thing left holding the advance.
The multi-wallet test built a throwaway manager only to move three storage handles onto another one, which orphaned the storage `FiltersManager::new` had already read its progress from. `seed_anchored_storage` seeds whichever manager it is given, so both wallet types share one path. It also added wallet B before sync ever started, so it never exercised the runtime add its name claimed. B now joins after the frontier is reached, which covers the other half of the fix: a wallet that genuinely needs a rescan must still get one, since a floor that suppressed every restart would pass the other tests just as well. `drain_getcfilters` replaces the two copies of the request-drain destructure.
Contributor
📝 WalkthroughWalkthroughThe filter manager now uses the stored filter-header start as a scan floor. Wallet commits accept heights below that floor while retaining gap and generation checks. Rescan logic computes an effective restart height and anchored-chain tests cover wallet convergence and request handling. ChangesAnchored filter synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (3)
dash-spv/src/sync/filters/manager.rs (2)
216-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider limiting the repetition of this warning.
start_downloadruns on every restart, rescan trigger, andFilterHeadersStored-driven reinit. On mainnet, where the checkpoint floor is always above a birth height of 0, this emits the same warning on each call. The information is valuable once per wallet, so a repeatedwarn!reduces log signal.Options: emit it at
debug!for repeats, or track the last reported(header_start_height, wallet_birth_height)pair and warn only when it changes.🤖 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 216 - 229, Limit the warning in start_download to once per distinct (header_start_height, wallet_birth_height) pair. Track the last reported pair across calls, emit tracing::warn! only when the pair changes, and use a lower-severity log or no log for repeated occurrences while preserving the existing warning details for new pairs.
1569-1592: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the anchored scenarios into separate tests.
Both blocks append a second, independent scenario to an existing test. Each block builds a new wallet, a new manager, and a new anchored header store, so it shares no state with the first half. A separate
#[tokio::test]per scenario reports failures more precisely and keeps each test focused on one rule.Also applies to: 1787-1812
🤖 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 1569 - 1592, The anchored scenarios in the existing test should be split into separate focused #[tokio::test] functions because each creates independent wallet, manager, and header state. Extract the scenario around the visible wallet_b setup and the corresponding block near the second referenced range into standalone tests, preserving their setup, try_commit_batches call, and assertions unchanged.dash-spv/src/sync/filters/sync_manager.rs (1)
211-233: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider committing the floored restart height instead of the raw stale height.
Line 231 lowers
committed_heighttostale_min_synced, which can sit far belowscan_flooron an anchored chain.start_downloadnormally overwrites it withscan_start - 1, but its early-return path (scan_start > filter_header_tip_height) does not, so the reported progress can regress below the anchor until the next batch commits.restart_at.saturating_sub(1)matches the height the scan actually resumes from.♻️ Proposed change
self.reset_for_rescan(); - self.progress.update_committed_height(stale_min_synced); + self.progress.update_committed_height(restart_at.saturating_sub(1)); return self.start_download(requests).await;🤖 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 211 - 233, Update the restart handling in the stale-min-synced branch of the sync flow to call progress.update_committed_height with restart_at.saturating_sub(1) instead of stale_min_synced. Keep the existing restart_at calculation and start_download behavior unchanged so reported progress remains aligned with the actual floored scan start, including the early-return path.
🤖 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/manager.rs`:
- Around line 216-229: Limit the warning in start_download to once per distinct
(header_start_height, wallet_birth_height) pair. Track the last reported pair
across calls, emit tracing::warn! only when the pair changes, and use a
lower-severity log or no log for repeated occurrences while preserving the
existing warning details for new pairs.
- Around line 1569-1592: The anchored scenarios in the existing test should be
split into separate focused #[tokio::test] functions because each creates
independent wallet, manager, and header state. Extract the scenario around the
visible wallet_b setup and the corresponding block near the second referenced
range into standalone tests, preserving their setup, try_commit_batches call,
and assertions unchanged.
In `@dash-spv/src/sync/filters/sync_manager.rs`:
- Around line 211-233: Update the restart handling in the stale-min-synced
branch of the sync flow to call progress.update_committed_height with
restart_at.saturating_sub(1) instead of stale_min_synced. Keep the existing
restart_at calculation and start_download behavior unchanged so reported
progress remains aligned with the actual floored scan start, including the
early-return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85b494c6-c3d3-45ef-a848-b28f58a934a9
📒 Files selected for processing (2)
dash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/sync_manager.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #933 +/- ##
==========================================
- Coverage 75.52% 75.20% -0.33%
==========================================
Files 328 328
Lines 78017 78194 +177
==========================================
- Hits 58923 58805 -118
- Misses 19094 19389 +295
|
QuantumExplorer
approved these changes
Aug 8, 2026
QuantumExplorer
approved these changes
Aug 8, 2026
6 tasks
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
2 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.
Mainnet filter sync never progressed.
hd_wallet_sync_flooranchors the chain at checkpoint 200000 while the CLI creates wallets with birth height 0, sosynced_heightstayed at 0 and two gates jammed. Over 40 minutes: 4846 scan restarts, 51784 batch scans, zero sync-height advances, ~350% CPU.tickrestarted the scan wheneverwallets_behindreported anyone below the committed frontier, wiping in-flight batches before any could commit. It now compares against the height a restart would actually resume from, which is floored at the wallets' birth heights and the stored headers' start.The commit-time contiguity guard required
synced_height + 1 >= batch_start, which reads1 >= 200000under an anchor. It now substitutesscan_floor - 1for an unreachablesynced_height. The floor excludes thecommitted_height + 1term ofscan_start, since tracking the scan frontier would reduce the guard tobatch_start >= batch_startand delete the height check #649 relies on. A genuine gap above the floor still blocks the advance, and the account-generation guard is untouched.The floor comes from the stored header start rather than
hd_wallet_sync_floor, so--start-heightis covered too. That path bypasses the network clamp and reproduces the same loop on testnet.Also warns when the anchor exceeds the wallet's birth height. That range is never scanned, and because BIP44 discovery is sequential, usage hidden in it can stop the gap-limit window from advancing and mask later addresses.
Summary by CodeRabbit