Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
| Layer / File(s) | Summary |
|---|---|
Peer-avoidance routing dash-spv/src/network/*, dash-spv/src/sync/filters/manager.rs |
Requests can include peers to avoid. Routing prefers other eligible peers and falls back to the full candidate set when needed. |
Fallible header lookups and replay dash-spv/src/sml_engine.rs, dash-spv/src/sml_engine/*, dash-spv/src/storage/masternode.rs, dash-spv/src/test_utils/mod.rs, dash-spv/tests/dashd_masternode/tests_sync.rs |
Header lookup and work-list APIs report storage failures. Callers use strict or best-effort lookups, and replay reports message application errors. |
Feed error propagation and rollback dash-spv/src/sml_engine.rs |
QRInfo and diff processing distinguish storage errors from refusals. QRInfo processing restores engine state when processing fails and refuses diffs without required snapshots. |
Bounded MnListDiff retries dash-spv/src/sync/download_coordinator.rs, dash-spv/src/sync/masternodes/pipeline.rs |
The pipeline tracks attempts, refusing peers, and request lifetime. It retries eligible requests and returns exhausted or expired targets. |
Peer-aware sync recovery dash-spv/src/sync/masternodes/manager.rs, dash-spv/src/sync/masternodes/sync_manager.rs |
Synchronization tracks QRInfo refusals and abandoned work, retries requests, distinguishes local gaps from peer refusals, and handles disconnects and stalled sync. |
Priority:
Estimated code review effort: 5 (Critical) | ~120 minutes
Change: Bug fix
Merge Risk: 
Minimal · up to 65348
-
The change makes masternode QRInfo and diff processing recover cleanly from storage failures and refused responses. It rolls back partial state and distinguishes storage errors from peer refusals. No actionable merge-blocking risk was identified in the supplied changes.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed -
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## shumkov/network-request-avoiding-peers #1205 +/- ##
==========================================================================
+ Coverage 78.88% 79.13% +0.24%
==========================================================================
Files 304 304
Lines 81419 82424 +1005
==========================================================================
+ Hits 64228 65225 +997
- Misses 17191 17199 +8
|
|
Bots are done — your move: address ZocoLini requested changes, then post |
ZocoLini
left a comment
There was a problem hiding this comment.
can you split the PR?? I can take care of it if you want
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
|
Bots are done — your move: address ZocoLini requested changes, then post |
36cc5f8 to
65348b0
Compare
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
|
PR Hygiene is not checking this pull request: it targets |
|
@ZocoLini thanks, fair point. It grew. I'll split it myself into a stack of three:
#1206 then stacks on 3. I'll link the new PRs here when they're up. 🤖 Generated with Claude Code |
65348b0 to
638d5a3
Compare
|
@ZocoLini split as promised:
#1206 stacks on this one. The commits are the same ones regrouped; four that spanned layers are split, and the stack's final tree is unchanged. 🤖 Generated with Claude Code |
6e3cb3e to
669d401
Compare
638d5a3 to
5f297ac
Compare
After the last of its three QRInfo attempts was refused or timed out, the masternode manager completed the pipeline, and on an engine without a list it set `SyncState::Error`. Nothing leaves that state: `tick` skips it, `BlockHeaderSyncComplete` does not restart it, and it wins the overall sync state, so a client whose first QRInfos failed never became synced for the life of the process. It now stays `Syncing` with nothing in flight and logs the failure. The stall watchdog re-dispatches a QRInfo with a fresh budget once the last dispatch is a minute old, and a later `BlockHeaderSyncComplete` restarts the sync as before. The masternode manager no longer reports `Error`. `test_rejected_qrinfo_retries_against_another_peer_then_gives_up` asserted that giving up does not leave the manager `Syncing`, to rule out a silent dead state; `Syncing` with the watchdog armed is neither, so it now asserts the state the watchdog restarts from. Test would have caught this in CI: `qrinfo_attempts_exhausted_on_an_empty_engine_are_retried_by_the_watchdog`. ✖ before the fix (the state was `Error`), ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Requests in the masternode diff pipeline had no bound: - a response matched its request by target hash alone, so any peer could answer any target from any base; - a diff whose block the header chain could not place was requeued on every response, and `requeue` reset the coordinator's retry count; - a request no peer answered timed out and went back in the queue forever. Core answers nothing for a base outside its active chain, so after a reorg one request kept the pipeline, and every tip update and QRInfo behind it, busy for the life of the process; - a diff the engine refused was dropped on first sight, so one peer could kill a target with a single malformed answer. Each target now gets three attempts. An attempt is spent by a timeout, by a block the header chain cannot place, or by a response the engine refuses from a peer that had not sent a refused one for that target. Retries avoid the refusing peers, a later response from one of them is ignored without spending anything, and a response from another base is not a match. Our own gaps (an unknown block, a missing base list, a header storage failure) do not blame the peer. After its third attempt the target is given up, and the pipeline completes as it does after an applied diff; a given-up tip update still publishes nothing. A full disconnect gives every target fresh attempts. The tests that pinned a refused tip update and a refused work-block list as given up on the first refusal now pin the three refusing peers it takes. Test would have caught this in CI, ✖ before the fix, ✔ after: - `an_mnlistdiff_whose_block_stays_unknown_is_given_up_after_three_attempts` (it was asked for a fourth time), - `test_unanswered_request_is_given_up_after_three_attempts` (it stayed queued), - `an_mnlistdiff_from_another_base_is_ignored` (it was applied), - `a_refused_tip_update_is_retried_on_other_peers_then_given_up` and `a_tip_update_completes_when_its_work_block_list_is_given_up` (the first refusal gave the target up). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A rejected QRInfo is retried on the same three-attempt budget, but nothing stopped the retry from going back to the peer that sent the rejected one, or that peer from answering again. A single peer could send bad QRInfos for the requested tip faster than an honest node answers and spend the whole budget, after which the stall watchdog started a new budget it spent again. The manager now records the peers whose QRInfo for the requested tip was rejected. A QRInfo names the blocks its diffs are built from, so a rejection for a block our header chain does not hold counts against the peer too. Retries avoid them, and another QRInfo from one of them is ignored without spending an attempt. The record outlives a budget at the same tip, so several bad peers cannot exhaust budget after budget, and is dropped when the requested tip changes, on success and on a full disconnect, so a deterministic failure cannot leave every peer ignored for longer than a block. The two tests that exhausted the budget with responses from one peer now use a peer per attempt. Test would have caught this in CI: `a_peer_whose_qrinfo_was_rejected_is_avoided_and_ignored`. ✖ before the fix (the retry did not avoid the peer), ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the engine refused a tip update, the synced manager's `tick` saw the masternode height behind the header tip and sent the same `GetMnListD` again, every 100 ms until the next block arrived. Now that a refused tip update is retried on other peers and then given up, that loop would restart after every give-up. The manager records the target of the last tip update it gave up. `send_tip_mnlistdiff_update` does not ask for it again, and `tick` skips the pipeline-mode selector, which logs past a mining window, while that block is the header tip. The next header tip is asked for as usual. Test would have caught this in CI: `a_given_up_tip_update_is_not_asked_again_until_the_tip_moves`. ✖ before the fix (each tick asked again), ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A peer disconnect moves every in-flight `getmnlistd` back to pending without spending an attempt, and the resend restarts its timeout. With one peer churning faster than the 15 s timeout while another stayed connected, a request no peer answers was resent forever: it never timed out, never ran out of attempts, and kept every later tip update and QRInfo waiting behind it. In `Syncing` it also kept the stall watchdog from firing. Each target now has a lifetime measured from its first send, as long as its three attempts take when each one times out (45 s). A target past it is given up like one out of attempts, whatever attempts it has left. A partial disconnect still spends no attempt, so a healthy request whose peer drops goes to another at once with its full timeout; only the time it already used counts. A full disconnect gives every target a new lifetime with its fresh attempts. The give-up log no longer claims three unanswered requests, since earlier attempts may have been refused. Test would have caught this in CI: `a_tip_update_whose_peers_keep_dropping_is_given_up_within_its_lifetime`. ✖ before the fix (run against the parent's pipeline with a no-op `set_lifetime`: sent 340 times and not given up after 3 s), ✔ after. `a_target_gets_a_new_lifetime_from_the_peers_after_a_full_disconnect` ✖ with `forget_attempts` keeping the first-send times, ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s peer A rejected QRInfo is recorded against the peer that sent it, and the peer's later QRInfos for that tip are ignored. That included a QRInfo the engine could not judge because our header storage did not read, which the engine reports as `FeedError::HeaderStorage`: an honest QRInfo that met an uncached header segment that could not be read made its peer a refuser. Once the storage read again, its QRInfos for that tip were still ignored, for good on a single-peer chain whose tip does not move. Only `FeedError::Refused` now records the peer. A QRInfo the storage could not judge is retried without recording its peer, while one naming a block the chain does not hold still counts against the peer, since those blocks are its choice. Test would have caught this in CI: `a_qr_info_our_header_storage_could_not_read_is_not_held_against_its_peer`. ✖ before the fix (peer 1, whose honest QRInfo met the unreadable storage, was recorded as a refuser; peer 2's QRInfo naming an unknown block was refused, as it should be), ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… tip update A tip update asks for the work-block lists of the quorums its own diff adds, and only a QRInfo asks for every list a quorum still needs. Once a work-block list was given up, after three timeouts or refusals, later tip updates did not bring the quorum again and so never asked for it: the quorum stayed `Skipped(MissedList)` until the next QRInfo, up to a rotation cycle later (about 12 hours on mainnet), and Platform proofs over it failed meanwhile. Before the bounded retries, a timed-out request stayed queued and a later answer could still recover it. The manager remembers the targets of the work-block lists it gives up. When the next tip update is applied, it asks again, with fresh attempts, for those a quorum of the newest list still needs, from the nearest list the engine still holds, and forgets the others. A list given up again waits for the tip after, so this is once per tip and never from `tick`. When the header chain cannot be read to tell which lists are still needed, the remembered targets are forgotten; a later commit in this series keeps them. Test would have caught this in CI: `a_given_up_work_block_list_is_asked_again_with_the_next_tip_update`. ✖ before the fix (the next tip update completed at once without asking for the list), ✔ after. `a_given_up_work_block_list_no_quorum_needs_is_forgotten` ✖ with the remembered targets kept after the tip update, ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t is given up The tests of a given-up work-block list covered tip updates only. An initial sync whose QRInfo was taken, and whose work-block list was then given up, could have stayed `Syncing` without any test noticing: making the give-up paths skip completion outside `Incremental` left every library test green. Such a sync would never publish its lists, and the stall watchdog would start the QRInfo over every minute. The new test takes a QRInfo whose tip list brings a non-rotating quorum, gives its work-block list up after three timeouts through `tick`, and after three refusing peers through the handler, and asserts the manager is `Synced` and published the tip list with the QRInfo's result. No behavior changes, so the red phase is that mutation: `an_initial_sync_completes_when_a_work_block_list_is_given_up` (both cases) ✖ with completion skipped outside `Incremental` after a give-up, ✔ on this commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…etries Several rules of the bounded masternode retries had no test that fails when they break: - a peer disconnect keeps the attempts a target spent, while a full disconnect forgets them; - a diff that fails on a gap of ours, a base block our header chain does not hold or a base list the engine lacks, is asked again without avoiding its peer, and `is_local_gap` counts a header storage failure as such a gap; - the full-disconnect test checked the pipeline's state but not that the retry actually goes out; - the peers whose QRInfo was refused are forgotten once a QRInfo for the tip is taken, and when the tip asked for moves on. No behavior changes, so each red phase is a mutation, ✖ with it and ✔ on this commit: - `a_peer_disconnect_keeps_the_attempts_a_target_spent`: a peer disconnect calling `forget_attempts`; - `an_mnlistdiff_failing_on_our_own_gap_is_not_held_against_its_peer` (both cases) and `a_header_storage_failure_is_a_gap_of_ours`: `is_local_gap` always false; - `a_full_disconnect_forgets_the_refusals_of_the_old_peers`: a full disconnect clearing the pipeline, which its old assertions passed; - `the_peers_that_refused_a_qr_info_are_forgotten_on_success_and_on_a_new_tip`: `qrinfo_received` keeping the refusers, and the avoid list kept for another tip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…igible peers The docs of the mnlistdiff retry said a retry goes to another connected peer. It goes to one of the peers eligible for the request and falls back to an avoided one when every eligible peer is avoided, so they now say so. The log of an unmatched `MnListDiff` printed its base hash where it said "from"; it now names the base and the peer. No behavior changes, so no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`retry_or_give_up` appended the refusing peer to the target's refusers without checking it was already there. The handler ignores a repeat refusal before it gets that far, so nothing duplicated a peer today, but the pipeline relied on its caller for it: a second record of one peer would put it twice in every retry's avoid list. The pipeline now records a peer once. Test would have caught this in CI: `a_peer_refusing_a_target_twice_is_avoided_once`. ✖ before the fix (the retry avoided the peer twice), ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… often A given-up work-block list is asked for again with the next tip update, so one no peer serves was asked for with every tip update for as long as a quorum needed it, up to about four days for `llmq_400_85`. A tip update completes only once its pipeline drains, so each one published `MasternodeStateUpdated` up to the list's 45 s lifetime late, and the tips behind it waited. A list given up again now waits twice as many tip updates as the last time before it is asked for again, up to every 32nd: it goes out with the first tip update after its first give-up, then after 2, 4, 8 and so on. An applied list is forgotten, so a later give-up starts over. When the scan for the lists still needed cannot read the header chain, the lists due are kept for the next tip update rather than forgotten. The test asking for a list again now runs for one three peers refused as well as one no peer answered, and checks the request goes from the nearest list below its work block the engine holds by then. Test would have caught this in CI, ✖ before the fix, ✔ after: - `a_work_block_list_given_up_again_is_asked_for_less_and_less_often` (it was asked for with every tip update), - `a_given_up_work_block_list_is_kept_while_the_headers_do_not_read` (it was forgotten). `a_given_up_work_block_list_is_asked_again_with_the_next_tip_update` after three refusing peers ✖ with the refusal path not remembering the list, which no test caught before, ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A target's lifetime was exactly its three 15 s timeouts, but a retry goes out on the tick after a timeout. So for a target that only ever timed out, the lifetime ran out a tick or two before its third attempt's own timeout, and an answer arriving then was dropped. The lifetime is now one timeout longer, 60 s, so it only cuts short sends that a peer disconnect restarted. The lifetime tests no longer race the clock: the full-disconnect test ages the first sends instead of sleeping, and also checks that a peer drop alone still gives the target up; the churn test runs a 1 s lifetime and expects the give-up within 3 s. Test would have caught this in CI: `a_target_s_lifetime_outlasts_its_attempts`. ✖ before the fix (45 s is not three timeouts with a second each to spare), ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The comment on a refused QRInfo says only a refusal is held against its peer. - The QRInfo retry no longer says the engine rejected a response it could not judge. - Two test docs read more plainly. No behavior changes beyond a log message, so no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A full disconnect gives every target fresh attempts and forgets what the old peers refused, but a work-block list given up again kept its wait: when the old peers were why it went unserved, the new ones were asked for it only after up to 32 tip updates, where before the backoff they were asked with the next one. A full disconnect now has every given-up list asked for with the next tip update and its backoff start over. The test asking for a list again now also checks that the list is forgotten once applied, which no test pinned: dropping that removal left every test green. Test would have caught this in CI: `a_full_disconnect_has_a_given_up_work_block_list_asked_with_the_next_tip_update`. ✖ before the fix (the list waited out its backoff), ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
669d401 to
1c67e9a
Compare
5f297ac to
a0ee545
Compare
Stacked on #1212, which is stacked on #1211; followed by #1206. At review this PR was split in three: the engine changes (atomic QRInfo feed, typed header storage errors) are in #1211, requests that avoid given peers are in #1212, and this PR keeps the masternode sync policy that uses both.
Basic explanation
The masternode sync in dash-spv had several ways to stall for good or to spin once a peer's response is refused: a sync whose QRInfos all failed ended in
SyncState::Error, which nothing leaves, anmnlistdiffthat no peer answered or whose block left the chain was re-requested forever, retries could go straight back to the peer that sent the refused response, and a refused tip update was re-requested on every 100 ms tick. All of them exist ondevtoday. They become more likely once diffs are refused for not matching their block's coinbase (#1206), so they are fixed first.What changes
Syncingwith nothing in flight, and the existing stall watchdog starts a fresh budget. The masternode manager no longer reportsSyncState::Error. The test that asserted giving up must not leaveSyncingnow asserts the state the watchdog restarts from.mnlistdiffretries. A response must carry the requested base. Each target gets three attempts, spent by a timeout, by a block the header chain cannot place, or by a refusal from a peer that had not refused it before; a repeat refusal from the same peer spends nothing. A peer disconnect moves a target back to pending without spending an attempt, so a target is also given up 60 s after its first send, whatever attempts it has left: four 15 s timeouts, so it only cuts short sends that a disconnect restarted. Before, peers dropping one after another kept a request no peer answered in flight forever. A given-up target completes the pipeline as an applied diff does. A full disconnect gives every target fresh attempts and a new lifetime. Ondev, timeouts and unknown heights were retried forever (after a reorg one unanswerable request blocked every later tip update and QRInfo) and an engine refusal dropped the target on first sight.Skipped(MissedList)until the next QRInfo, up to a rotation cycle later, and Platform proofs over it failed meanwhile.mnlistdiffretries avoid the peers that refused the target. QRInfo retries avoid the peers whose QRInfo for the requested tip was rejected, and another QRInfo from one of them is ignored without spending budget; the record outlives budgets at the same tip and is dropped when the tip changes.FeedError::HeaderStorage, from fix(dash-spv)!: leave the masternode engine untouched by a refused or interrupted QRInfo #1211) is retried without recording its peer as a refuser, while one naming a block the chain does not hold still counts against the peer, since those blocks are its choice. On themnlistdiffpath a storage failure is not charged to the peer either. A given-up work-block list is kept while the headers that say whether a quorum still needs it do not read.Breaking
No API changes. Behavior: the masternode manager no longer enters
SyncState::Error; anmnlistdiffwhose base is not the requested one is ignored; a refused or unansweredmnlistdiffrequest is sent at most three times, preferring other peers, and given up 60 s after its first send.Known limits
mnlistdiffwaits out two 15 s timeouts. Bounded, never a dead state.dev, one slot was enough). Charging only the asked peer needs the network layer to report which peer a request went to.MasternodeStateUpdatedof each tip update it is asked for again with by up to its 60 s lifetime. The backoff makes that one tip update in 32 once the list has been given up six times.Tests
Each fix has a test that fails without it. The commit messages name it and what failed. The commits that only add tests name the mutation their tests catch.
Red to green:
qrinfo_attempts_exhausted_on_an_empty_engine_are_retried_by_the_watchdog,an_mnlistdiff_whose_block_stays_unknown_is_given_up_after_three_attempts,test_unanswered_request_is_given_up_after_three_attempts,an_mnlistdiff_from_another_base_is_ignored,a_refused_tip_update_is_retried_on_other_peers_then_given_up,a_tip_update_completes_when_its_work_block_list_is_given_up,a_peer_whose_qrinfo_was_rejected_is_avoided_and_ignored,a_given_up_tip_update_is_not_asked_again_until_the_tip_moves,a_tip_update_whose_peers_keep_dropping_is_given_up_within_its_lifetime,a_target_gets_a_new_lifetime_from_the_peers_after_a_full_disconnect,a_target_s_lifetime_outlasts_its_attempts,a_qr_info_our_header_storage_could_not_read_is_not_held_against_its_peer,a_given_up_work_block_list_is_asked_again_with_the_next_tip_update,a_work_block_list_given_up_again_is_asked_for_less_and_less_often,a_given_up_work_block_list_is_kept_while_the_headers_do_not_read,a_full_disconnect_has_a_given_up_work_block_list_asked_with_the_next_tip_update,a_peer_refusing_a_target_twice_is_avoided_once.Pinned by mutation:
an_initial_sync_completes_when_a_work_block_list_is_given_up, and the disconnect, local-gap and refuser lifecycle tests of 30fdc27.Verified at this branch's tip:
cargo fmt --all -- --check,typos1.40.0,cargo +1.89 check --workspace --all-features,cargo clippy --workspace --all-targets --all-features -- -D warnings,cargo doc --workspace --all-features --no-depswith-D warnings,cargo test --workspace --all-featureswith dashd 23.1 (2883 passed, none failed:dashd_masternode11/11,dashd_sync35/35,dash-spv-ffidashd_sync7/7; run at 638d5a3, which differs from the tip only in one test's doc comment), and every commit on its own builds, lints and passes thedash-spvunit tests.🤖 Generated with Claude Code