feat(dash-spv): force resync to re-anchor a running client at a lower checkpoint#856
feat(dash-spv): force resync to re-anchor a running client at a lower checkpoint#856xdustinface wants to merge 4 commits into
Conversation
Add `force_resync`, which wipes all chain storage and re-syncs from the checkpoint at or below the current minimum wallet birth height, plus `resync_needed`, which reports when a wallet sits below the anchored header start and a resync is required to scan its history. Wallet state (per-wallet synced heights and discovered outputs) lives outside chain storage and is preserved, so only chain data is refetched: an added wallet with a lower birth height drags the anchor down and gets its range scanned, while wallets already ahead keep their progress. Making an already-running client resync in place required teardown and rebuild across layers that were previously single-use: - The network manager is now self-arming across a reconnect. The request processor returns its receiver to the shared slot when it shuts down, so the request channel and the sender clones managers hold are never rebuilt and stay valid, and `connect` re-arms the shutdown token that `disconnect` cancelled. A second `connect` therefore behaves like a fresh start with no separate reset step. - `SyncCoordinator::reset` rebuilds the managers in place while keeping the progress and sync-event senders, so existing subscribers stay connected. Replacing the whole coordinator would drop those senders and silently freeze every progress and event stream. - `force_resync` tears down and restarts the coordinator and network directly instead of via `stop`/`start`, leaving the `running` flag set. Flipping it would make the `run` monitoring loop exit for good, freezing all outward progress even though sync continues internally. The block-headers manager also polls its tip once on the sync-completion edge: the peer only announces a new block when it connects to the tip it believes we already have, so a block that lands while we were still syncing is never pushed to us, and without the poll a freshly resynced client can miss it until the next block arrives. Covered by a node-free unit test for the re-anchor and detection logic and a dashd integration test that resyncs a running client, mines a boundary block, and asserts the client keeps tracking the chain with wallet state preserved.
Add `resync_if_needed`, which runs `force_resync` only when `resync_needed` reports a wallet now sits below the anchored header start, returning whether a resync actually ran. A caller that just added wallets can do the whole "rescan the older history if I need to" step in one call instead of pairing the check and the action itself. Add `EventHandler::on_resync_needed`, fired once by the run loop on the rising edge of `resync_needed` so a wallet added below the anchor is reported without the caller polling. The check runs on its own low-frequency interval, kept well above the sync tick so the extra storage lock it takes does not contend with active sync. The client only notifies: reacting is a policy decision (`force_resync` wipes and refetches chain data), so the app calls `force_resync` or `resync_if_needed` when it fits, or defers.
…gainst a real node Add `ClientConfig::with_checkpoints` to override the network's bundled checkpoints (regtest and devnet ship none), a `DashSpvClient::start_height` accessor, and a `DashCoreNode::checkpoint_at` test helper that builds a real checkpoint from the running chain. Regtest ships no checkpoints, so the anchor was always genesis and no integration test could exercise checkpoint anchoring against a real node, only mock unit tests. `test_checkpoint_anchoring_and_reanchor_to_lower_checkpoint` now injects checkpoints built from the running chain, anchors a client at a mid-chain checkpoint and syncs forward from it, then adds a wallet below the anchor and asserts `force_resync` re-anchors at the next checkpoint down and re-syncs. This also closes the pre-existing gap that checkpoint anchoring at startup had no real-node coverage at all. Production networks keep their bundled checkpoints, so the config override is a test-only affordance with no behavior change.
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds test-only checkpoint override support to ClientConfig, a checkpoint_manager() helper used during genesis anchoring, and new resync_needed/force_resync/resync_if_needed client APIs. Introduces an EventHandler on_resync_needed callback with periodic detection in SyncCoordinator, plus network reconnect fixes, sync-progress reseeding, and new tests. ChangesCheckpoint Override and Resync Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SyncCoordinator
participant DashSpvClient
participant NetworkManager
participant Storage
participant EventHandler
loop periodic tick
SyncCoordinator->>DashSpvClient: resync_needed()
DashSpvClient->>Storage: get_start_height()
Storage-->>DashSpvClient: anchored height
DashSpvClient-->>SyncCoordinator: true/false
alt rising edge detected
SyncCoordinator->>EventHandler: on_resync_needed()
end
end
EventHandler->>DashSpvClient: force_resync()
DashSpvClient->>SyncCoordinator: shutdown sync tasks
DashSpvClient->>Storage: clear chain storage
DashSpvClient->>DashSpvClient: build_managers()
DashSpvClient->>SyncCoordinator: reset()
DashSpvClient->>NetworkManager: connect() (re-arm shutdown_token)
NetworkManager-->>DashSpvClient: connected
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
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/block_headers/manager.rs (1)
194-219: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the post-sync
GetHeaderspoll before markingSynceddash-spv/src/sync/block_headers/manager.rs:204-217
Ifrequest_block_headers(*tip.hash())errors here,BlockHeaderSyncCompleteis dropped after the manager has already flipped toSynced, so the completion event never reaches downstream managers.🤖 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/block_headers/manager.rs` around lines 194 - 219, In the block header sync completion path inside the manager logic, the final post-sync GetHeaders poll is happening after the state is already switched to Synced and the completion event is queued, so an error there can prevent downstream notification. Update the synchronization flow around the pipeline completion handling in the block headers manager so the tip poll via request_block_headers(*tip.hash()) happens before setting SyncState::Synced and pushing BlockHeaderSyncComplete, and keep the finalize/event emission only after that request succeeds.
🧹 Nitpick comments (2)
dash-spv/src/network/manager.rs (2)
1426-1434: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit test coverage for the reconnect flow.
None of the three fixes in this diff (receiver reuse, peer-count reset, token re-arm) appear to have accompanying
#[cfg(test)]coverage in this module. Given the concurrency/lifecycle subtlety here (receiver handoff, cancellation token lifetime), a focused unit test exercisingshutdown()→connect()→shutdown()again would guard against regressions.As per path instructions, "Implement comprehensive unit tests in-module for individual components using
#[cfg(test)]and integration tests in thetests/directory" fordash-spv/**/{src,tests}/**/*.rs.🤖 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/network/manager.rs` around lines 1426 - 1434, Add in-module #[cfg(test)] coverage in manager.rs for the reconnect lifecycle around connect() and shutdown(). Create a focused unit test that exercises shutdown() → connect() → shutdown() again and verifies the receiver handoff, peer-count reset, and CancellationToken re-arm behavior through the NetworkManager methods involved (especially connect(), shutdown(), and start()) so regressions in the reconnect flow are caught.Source: Path instructions
1426-1434: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the token re-arm against a double
connect().
connect()unconditionally replacesself.shutdown_token. If it were ever called twice without an interveningdisconnect(), tasks spawned under the previous token (peer readers, maintenance loop, request processor) become permanently uncancellable — the field no longer references the token they're watching, so a latershutdown()can't reach them, andshutdown()'s task-drain loop would hang or leave stale tasks. All current call sites (e.g.force_resync) always pairdisconnect()/connect(), so this isn't triggered today, but nothing in the type enforces that discipline for future callers.🔧 Suggested guard
async fn connect(&mut self) -> NetworkResult<()> { // Re-arm the shutdown token so a reconnect after `disconnect` (which cancelled it) // spawns its tasks against a live token. The request channel is not rebuilt here: the // request processor returns its receiver to the shared slot on shutdown, so `start` // simply takes it again, keeping every manager's request sender valid across // reconnects. - self.shutdown_token = CancellationToken::new(); + if self.shutdown_token.is_cancelled() { + self.shutdown_token = CancellationToken::new(); + } self.start().await.map_err(|e| NetworkError::ConnectionFailed(e.to_string())) }🤖 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/network/manager.rs` around lines 1426 - 1434, Guard the token re-arm in connect so it does not blindly replace self.shutdown_token on a second call without a prior disconnect(). Update NetworkManager::connect to either reuse the existing token when already connected or return an error/early no-op if a live token is still in use, and keep the start() call tied to the same CancellationToken that spawned the existing peer readers, maintenance loop, and request processor.
🤖 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/client/lifecycle.rs`:
- Around line 234-278: The force_resync flow leaves the client partially torn
down if storage.clear() or Self::build_managers() fails after
shutdown/disconnect, since run state is preserved but the network is
disconnected and managers/storage are wiped. Update force_resync in lifecycle.rs
to either defer the shutdown/disconnect until after the rebuild succeeds, or
capture enough pre-resync state to restore the sync_coordinator, network, and
storage on error before returning. Use the existing force_resync,
sync_coordinator.reset, network.disconnect/connect, and Self::build_managers
paths to keep the client consistent on failure.
---
Outside diff comments:
In `@dash-spv/src/sync/block_headers/manager.rs`:
- Around line 194-219: In the block header sync completion path inside the
manager logic, the final post-sync GetHeaders poll is happening after the state
is already switched to Synced and the completion event is queued, so an error
there can prevent downstream notification. Update the synchronization flow
around the pipeline completion handling in the block headers manager so the tip
poll via request_block_headers(*tip.hash()) happens before setting
SyncState::Synced and pushing BlockHeaderSyncComplete, and keep the
finalize/event emission only after that request succeeds.
---
Nitpick comments:
In `@dash-spv/src/network/manager.rs`:
- Around line 1426-1434: Add in-module #[cfg(test)] coverage in manager.rs for
the reconnect lifecycle around connect() and shutdown(). Create a focused unit
test that exercises shutdown() → connect() → shutdown() again and verifies the
receiver handoff, peer-count reset, and CancellationToken re-arm behavior
through the NetworkManager methods involved (especially connect(), shutdown(),
and start()) so regressions in the reconnect flow are caught.
- Around line 1426-1434: Guard the token re-arm in connect so it does not
blindly replace self.shutdown_token on a second call without a prior
disconnect(). Update NetworkManager::connect to either reuse the existing token
when already connected or return an error/early no-op if a live token is still
in use, and keep the start() call tied to the same CancellationToken that
spawned the existing peer readers, maintenance loop, and request processor.
🪄 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: a2e3988e-21bf-49f5-a155-9ca1ce14d60e
📒 Files selected for processing (12)
dash-spv/src/client/config.rsdash-spv/src/client/core.rsdash-spv/src/client/event_handler.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mod.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/network/manager.rsdash-spv/src/network/mod.rsdash-spv/src/sync/block_headers/manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/test_utils/node.rsdash-spv/tests/dashd_sync/tests_multi_wallet.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #856 +/- ##
==========================================
+ Coverage 73.58% 73.68% +0.10%
==========================================
Files 324 324
Lines 73140 73397 +257
==========================================
+ Hits 53820 54085 +265
+ Misses 19320 19312 -8
|
…down If `storage.clear()` or `build_managers()` fails after `force_resync` has already torn down the coordinator and disconnected the network, the client was left with `running` set but a drained coordinator, so `run`'s `tick` reported no error and spun forever without surfacing the failure. Factor the body into `force_resync_body` and, on failure while `was_running`, drain the coordinator (also cleaning up any tasks a failed `start`/`connect` restart spawned) and flip `running` to false so `run` exits and the error surfaces. A later `force_resync` retries the rebuild cleanly. Addresses CodeRabbit review comment on PR #856 #856 (comment)
github-actions
Bot
added
ready-for-review
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
1 participant
force_resync: wipes chain storage and re-syncs from the checkpoint at or below the current minimum wallet birth height, so adding a wallet with older history gets its range scanned. Wallet state (per-wallet synced heights and discovered outputs) lives outside chain storage and is preserved, and the client keeps running across the resync.resync_neededreports when a wallet sits below the anchored start,resync_if_neededchecks and resyncs in one call, andEventHandler::on_resync_neededfires once on the rising edge so callers can react without polling.test-utils-only checkpoint override).Summary by CodeRabbit
New Features
Bug Fixes