fix(dash-spv): requeue in-flight downloads when a single peer disconnects - #941
Merged
Conversation
…ects `DownloadCoordinator::requeue_in_flight` was only ever reached through `SyncManager::stop_sync`, which fires exclusively when the last peer goes away. Losing one peer out of several therefore stranded everything that peer owed us until the per-item timeout expired, 15s for `getmnlistd`, 20s for `getcfheaders`, 30s for headers and blocks. On mainnet that showed up as an evicted peer's batches trickling back in over 20 seconds while the filter header out-of-order buffer opened a six-figure height gap behind them. Add `SyncManager::on_peer_disconnect`, a no-op by default, dispatched from `NetworkEvent::PeerDisconnected`. That event already exists, already carries the dropped address, and already precedes `PeersUpdated` on every removal path, so a drop that leaves other peers connected becomes observable without touching the network layer. It stays separate from `on_disconnect` on purpose: several `on_disconnect` implementations are written for total peer loss and are destructive, `FilterHeadersManager` rebuilds its whole pipeline and `InstantSendManager` drops its unvalidated cache, so reusing that hook for routine peer churn would repeatedly discard real progress. In-flight items are not attributed to a peer, so a requeue reissues everything outstanding, including requests a surviving peer will still answer. That trade is acceptable rather than merely tolerable: `requeue_in_flight` preserves retry counts, so nothing escapes or prematurely exhausts its budget, and every receive path treats a response it no longer tracks as unrequested, `FiltersPipeline` even cancels the pending entry when a late reply completes a requeued batch. The cost is redundant traffic, bounded by the concurrency limit, in exchange for dropping the worst-case recovery from 30s to the next 100ms tick. Per-item peer attribution stays deferred. Per manager: - `BlocksManager` and `FiltersManager` requeue their coordinator, matching what they already do on total loss. - `FilterHeadersManager` requeues in-flight batches only, leaving `batch_starts`, `next_expected` and the out-of-order buffer intact rather than resetting the pipeline the way total loss does. - `MasternodesManager` requeues the `getmnlistd` pipeline. The QRInfo request is tracked outside the coordinator with its own escalating timeout and attempt budget, so it is left to that path. - `BlockHeadersManager` clears per-segment in-flight bookkeeping and reissues synchronously, but only while `Syncing`. A segment rejects headers whose request it no longer tracks, so deferring the resend to the next tick would open a window where a surviving peer's reply looks unsolicited. Outside `Syncing` nothing reissues header requests, so clearing there would strand the request instead of retrying it. - `ChainLockManager`, `InstantSendManager` and `MempoolManager` keep the default no-op. None of them drive a `DownloadCoordinator`, so nothing is stranded behind a timeout, and their disconnect state is either a dedup set that would cause redundant `getdata` if cleared or, for the mempool, already tracked per peer.
Pins the two properties the fix depends on: a drop that leaves peers connected reaches the requeue hook and not the destructive total-loss one, and a requeue reissues each request with the identity it was originally paired with, block hash for `getdata`, start height for `getcfheaders`, base hash for `getmnlistd`. Also pins that a requeue is not an attempt against an item's retry budget, since a peer disconnect is not the item's fault and repeated churn would otherwise exhaust the budget and drop work.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe synchronization framework now requeues in-flight requests when one peer disconnects. It preserves progress and retry metadata, resends pending requests immediately where required, and keeps total-peer-loss cleanup separate. ChangesPeer disconnect recovery
Sequence Diagram(s)sequenceDiagram
participant Peer
participant SyncManager
participant BlocksManager
participant DownloadCoordinator
Peer->>SyncManager: PeerDisconnected
SyncManager->>BlocksManager: on_peer_disconnect()
BlocksManager->>DownloadCoordinator: requeue in-flight requests
DownloadCoordinator-->>BlocksManager: pending requests
BlocksManager-->>Peer: resend GetData requests
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}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #941 +/- ##
==========================================
+ Coverage 75.17% 75.24% +0.06%
==========================================
Files 328 328
Lines 78194 78370 +176
==========================================
+ Hits 58783 58967 +184
+ Misses 19411 19403 -8
|
ZocoLini
approved these changes
Aug 11, 2026
ZocoLini
added a commit
that referenced
this pull request
Aug 11, 2026
The old network module gave each sync manager an `on_peer_disconnect` hook that requeued its own in-flight work, and three separate fixes landed against it (#941, #943, #953) — one for the block pipeline, one for progress being discarded along with the requeue, one for requeued work never being reissued. The broker owns a request from send to response, so it replaced all three hooks with a single central requeue, and their regression tests went with the hooks: the replacement path had no coverage at all, in the area with the worst track record. Both callers — the timeout monitor kicking a stalled peer and the pump seeing a socket close — did this inline and identically, buried in spawned tasks where nothing could reach them. Lift it into `requeue_requests_from` and pin the three properties the old tests guarded: - a departed peer's requests come back, a healthy peer's do not - the key stays registered as `Queued`, so a pipeline re-declaring the request cannot queue a duplicate on top of the retry - only the response retires the key, so a requeued request stays owned by someone Checked against injected regressions: dropping the key instead of requeuing it fails two of the three, and requeuing nothing fails all three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ToH2xGXqVcxiwMkNYaWkh7
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.
Problem
requeue_in_flightwas only reachable through the total-peer-loss path. When one of several peers disconnected, the items in flight to that peer stayed stranded until the 15-30s stall timeout picked them up, delaying sync on every ordinary peer drop.Fix
A new
on_peer_disconnecthook requeues the disconnected peer's in-flight items immediately while the remaining peers keep working. It is deliberately separate fromon_disconnect, which resets manager state and would be destructive for the filter header and InstantSend managers on a single-peer drop.Covered by a new test for the single-peer disconnect requeue path.
Summary by CodeRabbit