Skip to content

fix(dash-spv)!: keep masternode sync live after refused responses - #1205

Open
shumkov wants to merge 15 commits into
shumkov/network-request-avoiding-peersfrom
shumkov/masternode-sync-liveness
Open

shumkov wants to merge 15 commits into
shumkov/network-request-avoiding-peersfrom
shumkov/masternode-sync-liveness

Conversation

@shumkov

@shumkov shumkov commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

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, an mnlistdiff that 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 on dev today. They become more likely once diffs are refused for not matching their block's coinbase (#1206), so they are fixed first.

What changes

  • No dead end after failed QRInfos. When every attempt fails on an empty engine, the manager stays Syncing with nothing in flight, and the existing stall watchdog starts a fresh budget. The masternode manager no longer reports SyncState::Error. The test that asserted giving up must not leave Syncing now asserts the state the watchdog restarts from.
  • Bounded mnlistdiff retries. 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. On dev, 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.
  • Given-up work-block lists are asked for again. A tip update asks for the work-block lists of the quorums it brings. A list given up is remembered and asked for again with the next tip update, with fresh attempts and from the nearest lower list the engine holds, while a quorum of the newest list needs it. Each time it is given up again it waits twice as many tip updates, up to every 32nd, and a full disconnect resets the wait. Otherwise its quorum stayed Skipped(MissedList) until the next QRInfo, up to a rotation cycle later, and Platform proofs over it failed meanwhile.
  • Retries avoid refusing peers, through the requests of feat(dash-spv)!: let a network request avoid given peers #1212. mnlistdiff retries 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.
  • A header storage failure is not the peer's fault. A QRInfo the engine could not judge (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 the mnlistdiff path 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.
  • Tip guard. A tip update that was given up is not asked for again until the header tip moves.

Breaking

No API changes. Behavior: the masternode manager no longer enters SyncState::Error; an mnlistdiff whose base is not the requested one is ignored; a refused or unanswered mnlistdiff request is sent at most three times, preferring other peers, and given up 60 s after its first send.

Known limits

  • With a single connected peer, avoidance falls back to that peer and its repeated answers are ignored, so a refused mnlistdiff waits out two 15 s timeouts. Bounded, never a dead state.
  • A peer whose QRInfo was refused is ignored for that tip until the tip moves or every peer disconnects, even when it is the only peer connected: on a static regtest or devnet chain with one peer, one refusal stops QRInfo recovery until the next block. A bounded cooldown or an explicit fallback is a possible follow-up. Exempting QRInfo refusals for blocks the chain does not hold is not, since a QRInfo's blocks are the peer's choice.
  • A response is matched to its request by base and target, not by the peer that was asked, so peers that were never asked can spend a target's three attempts: an attacker with three peer slots can give a request up before the honest answer arrives (on dev, one slot was enough). Charging only the asked peer needs the network layer to report which peer a request went to.
  • A peer disconnect moves in-flight requests back to pending until the next 100 ms tick re-sends them, and a response arriving in between is dropped. With the lifetime, drops paced to an honest peer's latency can now give a target up.
  • A work-block list no peer serves delays the MasternodeStateUpdated of 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.
  • After a reorg that orphans the newest list, requests built on it are now dropped instead of hanging, and following the fork drops the lists above it (fix(dash-spv): drop masternode lists above a followed fork, refuse forks below the checkpoint #1201). The restart that follows a fork starts this PR's retry state afresh, and fork detection acts on headers before they are validated (dash-spv: protect fork recovery against malicious headers #1103, dash-spv: fork detection acts on headers before they are validated #1193), so any peer can trigger it, clearing the QRInfo refusers, the given-up lists and the retry budgets.
  • The stall watchdog does not back off: a deterministic QRInfo failure costs three feeds a minute.

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, typos 1.40.0, cargo +1.89 check --workspace --all-features, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo doc --workspace --all-features --no-deps with -D warnings, cargo test --workspace --all-features with dashd 23.1 (2883 passed, none failed: dashd_masternode 11/11, dashd_sync 35/35, dash-spv-ffi dashd_sync 7/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 the dash-spv unit tests.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: dashpay/rust-dashcore/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 19d5f4fb-09f7-476c-9052-8b10f0c1c621
📥 Commits

Reviewing files that changed from the base of the PR and between 36cc5f8 and 65348b0.

📒 Files selected for processing: 3
  • dash-spv/src/sml_engine.rs
  • dash-spv/src/sml_engine/helpers.rs
  • dash-spv/src/storage/masternode.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The changes add peer-avoidance routing, distinguish header-storage failures from message refusals, and update masternode synchronization to retry requests, track peer refusals, and handle abandoned work.

Changes: Masternode synchronization

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: Normal

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: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 73.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 201 functions across 15 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the primary change: keeping dash-spv masternode synchronization active after refused responses.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@codecov

codecov Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.31144% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.13%. Comparing base (1c67e9a) to head (a0ee545).

Files with missing lines Patch % Lines
dash-spv/src/sync/masternodes/sync_manager.rs 98.08% 15 Missing ⚠️
dash-spv/src/sync/masternodes/manager.rs 97.27% 3 Missing ⚠️
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     
Flag Coverage Δ
core 77.75% <ø> (ø)
ffi 50.55% <ø> (-0.06%) ⬇️
rpc 52.05% <ø> (ø)
spv 92.71% <98.31%> (+0.30%) ⬆️
wallet 82.54% <ø> (ø)
Files with missing lines Coverage Δ
dash-spv/src/sync/download_coordinator.rs 100.00% <100.00%> (ø)
dash-spv/src/sync/masternodes/pipeline.rs 98.63% <100.00%> (+0.74%) ⬆️
dash-spv/src/sync/masternodes/manager.rs 97.05% <97.27%> (+0.02%) ⬆️
dash-spv/src/sync/masternodes/sync_manager.rs 98.32% <98.08%> (-0.54%) ⬇️

... and 8 files with indirect coverage changes

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Bots are done — your move: address ZocoLini requested changes, then post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. and removed waiting-bots Waiting for the review bots to report on this head labels Oct 20, 2026

@ZocoLini ZocoLini left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you split the PR?? I can take care of it if you want

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. labels Oct 21, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes Oct 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Bots are done — your move: address ZocoLini requested changes, then post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 21, 2026
@shumkov
shumkov force-pushed the shumkov/masternode-sync-liveness branch from 36cc5f8 to 65348b0 Compare October 21, 2026 14:44
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 21, 2026
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

PR Hygiene is not checking this pull request: it targets shumkov/network-request-avoiding-peers, outside the policy. What was said about its diffs is kept for when it returns; it will queue for a review slot again.

@github-actions github-actions Bot removed the waiting-bots Waiting for the review bots to report on this head label Oct 21, 2026
@shumkov

shumkov commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@ZocoLini thanks, fair point. It grew. I'll split it myself into a stack of three:

  1. engine: the atomic QRInfo feed (rollback on any error, including an aborted feed), refusing a QRInfo with more diffs than snapshots, and typed header-storage errors (sml_engine)
  2. network: NetworkRequest::SendMessageAvoiding and the shared peer selection
  3. masternode sync policy (this PR, rebased onto 1+2): bounded retries and the per-target lifetime, refusing peers, the tip guard, staying Syncing on an empty engine, and retrying given-up work-block lists

#1206 then stacks on 3. I'll link the new PRs here when they're up.

🤖 Generated with Claude Code

@shumkov
shumkov force-pushed the shumkov/masternode-sync-liveness branch from 65348b0 to 638d5a3 Compare October 21, 2026 16:58
@shumkov
shumkov changed the base branch from dev to shumkov/network-request-avoiding-peers October 21, 2026 16:58
@shumkov

shumkov commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@ZocoLini split as promised:

  1. fix(dash-spv)!: leave the masternode engine untouched by a refused or interrupted QRInfo #1211, engine: the atomic QRInfo feed with its drop guard, refusing a QRInfo with more diffs than snapshots, and the typed header storage errors (sml_engine). Base dev.
  2. feat(dash-spv)!: let a network request avoid given peers #1212, network: NetworkRequest::SendMessageAvoiding and the shared peer selection. Base fix(dash-spv)!: leave the masternode engine untouched by a refused or interrupted QRInfo #1211.
  3. This PR, now based on feat(dash-spv)!: let a network request avoid given peers #1212, keeps only the masternode sync policy.

#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

@shumkov
shumkov force-pushed the shumkov/network-request-avoiding-peers branch from 6e3cb3e to 669d401 Compare October 21, 2026 17:13
@shumkov
shumkov force-pushed the shumkov/masternode-sync-liveness branch from 638d5a3 to 5f297ac Compare October 21, 2026 17:13
@github-actions github-actions Bot added ready-for-review CodeRabbit has approved this PR and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 21, 2026
@shumkov
shumkov requested a review from ZocoLini October 21, 2026 19:18
shumkov and others added 15 commits October 24, 2026 11:08
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>
@shumkov
shumkov force-pushed the shumkov/network-request-avoiding-peers branch from 669d401 to 1c67e9a Compare October 24, 2026 04:13
@shumkov
shumkov force-pushed the shumkov/masternode-sync-liveness branch from 5f297ac to a0ee545 Compare October 24, 2026 04:13
@github-actions github-actions Bot removed the ready-for-review CodeRabbit has approved this PR label Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants