Uh oh!
There was an error while loading. Please reload this page.
fix(dash-spv): reissue requeued masternode requests from tick - #953
Merged
Conversation
`MasternodesManager::tick` only ran the MnListDiff timeout-and-resend branch while requests were in flight. A single-peer disconnect requeues every in-flight `getmnlistd` as pending via `on_peer_disconnect`, leaving zero in flight, so the pending work was never reissued: responses arriving after the requeue are dropped as untracked and no other path calls `send_pending`. Masternode sync then stalled permanently with no error logged. Observed on a mainnet fresh sync where a stalled-peer eviction hit mid-pipeline, freezing at `diffs_processed: 6` with 42 requests stranded until restart. Gate the branch on the pipeline having outstanding work instead, so requeued pending requests go out on the next tick. `MnListDiffPipeline::active_count` becomes test-only.
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 (3)
📝 WalkthroughWalkthroughThe MnListDiff timeout path now processes incomplete pipelines after peer disconnects. A Tokio integration test verifies that queued requests are resent and tracked as active. The active-count helper is limited to test builds. ChangesMnListDiff requeue handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## dev #953 +/- ##
==========================================
+ Coverage 75.52% 75.57% +0.04%
==========================================
Files 329 329 Lines 79136 79169 +33 ==========================================
+ Hits 59769 59829 +60 + Misses 19367 19340 -27
|
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
bfoss765 added a commit
that referenced
this pull request
Aug 21, 2026
The branch was cut at b056d07, before #953 landed on dev; the dev merge (08961af) surfaced two artifacts: - pipeline.rs carried a second MnListDiffPipeline::requeue_in_flight, identical in body to the one #953 added. The two sat in different parts of the file so git merged cleanly, but the result failed to compile with E0592. Drop ours, keep dev's; pipeline.rs is now byte-identical to dev. on_peer_disconnect now routes through the MasternodeSyncState::requeue_in_flight wrapper like on_disconnect, instead of reaching into the pipeline directly (same behaviour: the wrapper only delegates to the pipeline). - tick's MnListDiff section had been reshaped by both sides for the same reason, leaving two consecutive `if !is_complete()` blocks after the merge, the first holding only handle_timeouts(). Restore dev's single block; dev already provides the flush behaviour this PR's version described, and test_tick_reissues_requeued_mnlistdiffs passes against it. Also correct the watchdog's "known route in" example: a peerless send cannot strand the manager (request_qr_info pushes onto an unbounded channel and only errors once the channel is closed, so it fails later in the network task with the slot still armed). The real routes in are send_qrinfo_for_tip's early Ok returns - no stored tip, or a tip at genesis - after the caller has already cleared the slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
xdustinface added a commit
that referenced
this pull request
Aug 22, 2026
* fix(dash-spv): recover masternode sync from a rejected QRInfo One QRInfo response the engine rejects permanently strands masternode sync. `qrinfo_received()` cleared `qrinfo_in_flight` before the fallible `feed_qr_info`, so a validation failure returned `Err` leaving the manager in `Syncing` with nothing in flight and an empty diff pipeline. The [10, 30, 60] retry ladder is armed solely by `qrinfo_in_flight`, so `tick` fell through forever - a frozen `qr_infos_requested: 1` for the life of the process, with the error surfacing only as a `SyncEvent::ManagerError` that has no consumer. Masternode sync gates overall SYNCED, so while stalled every InstantLock fails verification, DAPI has no masternode list, and platform features are dead until a restart. Three changes: 1. Release the request slot only after the last fallible step, just before `queue_requests`. On the `feed_qr_info` error branch keep the slot armed and flag the attempt `rejected`, which `tick` treats as an elapsed timeout: the retry rotates to the next peer via `send_distributed`'s round-robin, on the existing budget, so a deterministically-bad response still terminates after MAX_RETRY_ATTEMPTS dispatches. `last_processed_qrinfo_tip` continues to be set only on success, so the retry is not dropped as a duplicate. 2. Add a stall watchdog to `tick`: `Syncing` with no QRInfo in flight and an empty diff pipeline for longer than 60s re-dispatches a QRInfo. This covers the routes the in-flight flag cannot, notably a `send_qrinfo_for_tip` that fails after its caller already cleared the slot. Timed off a new `last_qrinfo_dispatch` rather than `progress.last_activity()`, which unrelated block events keep bumping. 3. `on_disconnect` requeues in-flight `GetMnListDiff`s instead of clearing, mirroring `BlocksManager` and `FiltersManager`, and leaves the QRInfo slot armed. `tick` now flushes the pending queue whenever the pipeline is non-empty, which is what actually reissues requeued requests - every other `send_pending` call site hangs off a response handler that cannot run while nothing is in flight. * fix(dash-spv): record the processed QRInfo tip only after the last fallible step Recording last_processed_qrinfo_tip before build_mnlistdiff_request_pairs turned the dedup gate against the retry ladder: a failure in that step left the request slot armed, but every retried response for the same tip was rejected at the handler entry by should_process_qrinfo, so the retry budget burned down with no way to succeed - reintroducing the permanent stall this branch exists to fix, through a different fallible step. Defer the record to the success path, next to qrinfo_received(). A straggler can only arrive after the handler returns, so the dedup gate loses nothing. Re-feeding the engine on such a retry is already accepted by design - the on_disconnect path clears the recorded tip for the same reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(dash-spv): de-link a pub(super) item rustdoc cannot resolve Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(dash-spv): drop rebase artifacts duplicated by #953 landing on dev The branch was cut at b056d07, before #953 landed on dev; the dev merge (08961af) surfaced two artifacts: - pipeline.rs carried a second MnListDiffPipeline::requeue_in_flight, identical in body to the one #953 added. The two sat in different parts of the file so git merged cleanly, but the result failed to compile with E0592. Drop ours, keep dev's; pipeline.rs is now byte-identical to dev. on_peer_disconnect now routes through the MasternodeSyncState::requeue_in_flight wrapper like on_disconnect, instead of reaching into the pipeline directly (same behaviour: the wrapper only delegates to the pipeline). - tick's MnListDiff section had been reshaped by both sides for the same reason, leaving two consecutive `if !is_complete()` blocks after the merge, the first holding only handle_timeouts(). Restore dev's single block; dev already provides the flush behaviour this PR's version described, and test_tick_reissues_requeued_mnlistdiffs passes against it. Also correct the watchdog's "known route in" example: a peerless send cannot strand the manager (request_qr_info pushes onto an unbounded channel and only errors once the channel is closed, so it fails later in the network task with the slot still armed). The real routes in are send_qrinfo_for_tip's early Ok returns - no stored tip, or a tip at genesis - after the caller has already cleared the slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com>
romchornyi pushed a commit
that referenced
this pull request
Aug 25, 2026
* fix(dash-spv): recover masternode sync from a rejected QRInfo One QRInfo response the engine rejects permanently strands masternode sync. `qrinfo_received()` cleared `qrinfo_in_flight` before the fallible `feed_qr_info`, so a validation failure returned `Err` leaving the manager in `Syncing` with nothing in flight and an empty diff pipeline. The [10, 30, 60] retry ladder is armed solely by `qrinfo_in_flight`, so `tick` fell through forever - a frozen `qr_infos_requested: 1` for the life of the process, with the error surfacing only as a `SyncEvent::ManagerError` that has no consumer. Masternode sync gates overall SYNCED, so while stalled every InstantLock fails verification, DAPI has no masternode list, and platform features are dead until a restart. Three changes: 1. Release the request slot only after the last fallible step, just before `queue_requests`. On the `feed_qr_info` error branch keep the slot armed and flag the attempt `rejected`, which `tick` treats as an elapsed timeout: the retry rotates to the next peer via `send_distributed`'s round-robin, on the existing budget, so a deterministically-bad response still terminates after MAX_RETRY_ATTEMPTS dispatches. `last_processed_qrinfo_tip` continues to be set only on success, so the retry is not dropped as a duplicate. 2. Add a stall watchdog to `tick`: `Syncing` with no QRInfo in flight and an empty diff pipeline for longer than 60s re-dispatches a QRInfo. This covers the routes the in-flight flag cannot, notably a `send_qrinfo_for_tip` that fails after its caller already cleared the slot. Timed off a new `last_qrinfo_dispatch` rather than `progress.last_activity()`, which unrelated block events keep bumping. 3. `on_disconnect` requeues in-flight `GetMnListDiff`s instead of clearing, mirroring `BlocksManager` and `FiltersManager`, and leaves the QRInfo slot armed. `tick` now flushes the pending queue whenever the pipeline is non-empty, which is what actually reissues requeued requests - every other `send_pending` call site hangs off a response handler that cannot run while nothing is in flight. * fix(dash-spv): record the processed QRInfo tip only after the last fallible step Recording last_processed_qrinfo_tip before build_mnlistdiff_request_pairs turned the dedup gate against the retry ladder: a failure in that step left the request slot armed, but every retried response for the same tip was rejected at the handler entry by should_process_qrinfo, so the retry budget burned down with no way to succeed - reintroducing the permanent stall this branch exists to fix, through a different fallible step. Defer the record to the success path, next to qrinfo_received(). A straggler can only arrive after the handler returns, so the dedup gate loses nothing. Re-feeding the engine on such a retry is already accepted by design - the on_disconnect path clears the recorded tip for the same reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(dash-spv): de-link a pub(super) item rustdoc cannot resolve Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(dash-spv): drop rebase artifacts duplicated by #953 landing on dev The branch was cut at b056d07, before #953 landed on dev; the dev merge (08961af) surfaced two artifacts: - pipeline.rs carried a second MnListDiffPipeline::requeue_in_flight, identical in body to the one #953 added. The two sat in different parts of the file so git merged cleanly, but the result failed to compile with E0592. Drop ours, keep dev's; pipeline.rs is now byte-identical to dev. on_peer_disconnect now routes through the MasternodeSyncState::requeue_in_flight wrapper like on_disconnect, instead of reaching into the pipeline directly (same behaviour: the wrapper only delegates to the pipeline). - tick's MnListDiff section had been reshaped by both sides for the same reason, leaving two consecutive `if !is_complete()` blocks after the merge, the first holding only handle_timeouts(). Restore dev's single block; dev already provides the flush behaviour this PR's version described, and test_tick_reissues_requeued_mnlistdiffs passes against it. Also correct the watchdog's "known route in" example: a peerless send cannot strand the manager (request_qr_info pushes onto an unbounded channel and only errors once the channel is closed, so it fails later in the network task with the slot still armed). The real routes in are send_qrinfo_for_tip's early Ok returns - no stored tip, or a tip at genesis - after the caller has already cleared the slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
MasternodesManager::tickonly ran the MnListDiff timeout-and-resend branch while requests were in flight. A single-peer disconnect requeues every in-flightgetmnlistdas pending viaon_peer_disconnect, leaving zero in flight, so the pending work was never reissued: responses arriving after the requeue are dropped as untracked and no other path callssend_pending. Masternode sync then stalled permanently with no error logged. Observed on a mainnet fresh sync where a stalled-peer eviction hit mid-pipeline, freezing atdiffs_processed: 6with 42 requests stranded until restart.Gate the branch on the pipeline having outstanding work instead, so requeued pending requests go out on the next tick.
MnListDiffPipeline::active_countbecomes test-only.Summary by CodeRabbit