Repository navigation
fix(dash-spv)!: keep masternode sync live on bad or missing diffs - #1258
Conversation
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Request release contract and callers dash-spv/src/network/broker.rs, dash-spv/src/network/{manager.rs,mod.rs}, dash-spv/src/sync/{block_headers/manager.rs,blocks/sync_manager.rs,filter_headers/sync_manager.rs,filters/sync_manager.rs}, dash-spv/src/test_utils/network.rs |
The request API now uses release_request and Broker::release. Releasing a queued key removes its message only when no other registered keys remain. Sync callers and the mock network manager use and record releases. |
Typed diff request pipeline dash-spv/src/sync/masternodes/pipeline.rs |
The pipeline records each request’s base, kind, and queue time. It matches on target and base, releases requests through the network manager, and identifies requests past the 60-second deadline. |
Diff settlement and retry state dash-spv/src/sync/masternodes/manager.rs |
The manager tracks given-up tip updates and work-list retry state. It settles applied and expired diffs and clears pending requests through the network manager. |
Response classification and sync orchestration dash-spv/src/sml_engine.rs, dash-spv/src/sync/masternodes/sync_manager.rs |
FeedError::blames_peer identifies the listed peer-fault errors. The sync manager uses that classification, settles other failures, processes expired requests, and retries eligible work-block lists. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Peer
participant MasternodesManager
participant MnListDiffPipeline
participant SmlEngine
participant NetworkManager
Peer->>MasternodesManager: deliver MnListDiff
MasternodesManager->>MnListDiffPipeline: match target and base
MasternodesManager->>SmlEngine: feed matched diff
SmlEngine-->>MasternodesManager: applied result or error
MasternodesManager->>NetworkManager: ban sender for peer fault
MasternodesManager->>MnListDiffPipeline: settle applied or given-up request
MnListDiffPipeline->>NetworkManager: release request key
Suggested reviewers: zocolini, pastapastapasta
Merge Risk: 
460bd
-
If diff requests wait in the broker for over a minute, masternode sync can be delayed until a later update. This is a bounded risk to track, not an established merge blocker.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 60.87% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the primary change: preventing masternode synchronization from stalling when diffs are bad or missing. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.87% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 23 functions across 12 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #1258 +/- ##
==========================================
+ Coverage 81.03% 81.14% +0.10%
==========================================
Files 291 291
Lines 77992 78478 +486
==========================================
+ Hits 63202 63682 +480
- Misses 14790 14796 +6
|
|
/self-reviewed |
|
Policy satisfied — this can merge. |
github-actions
Bot
removed
the
waiting-self-review
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
The stall fixes look right, but I think this PR adds more machinery than the bug needs. dev already marks every matching response as answered, even a refused one, so what's missing is small:
- release the wanted diffs in
clear() - match responses on the base as well
- give up a diff nobody answers (the 60s deadline)
- drop a still-queued copy on release
Could we keep it to that? Specifically:
given_up_work_listsandWorkListRetry: I see why the retry exists, since a list that stays missing leaves its quorum unverified until the next QRInfo, and that can block ChainLock validation. Could we move it to its own PR, so this one stays focused on the stall? If it has to stay here, that's fine.- Move
blames_peerand the ban to a separate PR. They don't cause the stall, and they're worth reviewing on their own. They also conflict with #1276, which makesFeedErrornon-generic. - Simplify
given_up_tipto just the hash. The next block retries it, so it doesn't need its own deadline. - Drop
tick_at. It only serves the tests, and they can callexpired(now)directly. - Replace
settle_diffandDiffOutcomewith the existing flow, if they aren't needed once the above is gone. - Skip the
request_answeredrename, or make it its own PR. It touches several files without changing behaviour.
The trade-off is that, until those follow-ups land, a given-up work-block list waits for the next QRInfo and a peer sending a contradictory diff isn't banned. If you've seen a case that needs one of these pieces here, let me know.
|
Bots are done — your move: address ZocoLini requested changes, then post |
github-actions
Bot
added
waiting-self-review
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
changed the title
mnlistdiff requests from stalling masternode sync
`tick` read the clock itself through `Instant::now()` and `elapsed()`, so a test could only reach one of its timeouts by rewinding the instants the manager stores. `tick` now passes `Instant::now()` to `tick_at`, which reads every clock it checks from that instant, as `InstantSendManager::tick_at` does. Behaviour is unchanged.
A sync manager also needs to stop the broker's tracking of a request it no longer wants, without any response arriving, as the masternode pipeline does when it gives up a diff. `request_answered` named only the response case. `release_request` and `Broker::release` cover both, and `MockNetworkManager` records released keys.
`Broker::release` removed the registry entry but left a request still queued in `MsgQueue`, which `route` later sent untracked. That happens when a response arrives after a timeout re-queued its request, and when a caller releases a request it no longer wants. When the same key was declared again, as an `mnlistdiff` re-asked from another base is, `route` registered the stale message under the new request and its retries resent the stale one. Releasing a queued key now also drops its queued message, unless another of its keys is still in play, as a block of a multi-block `getdata` can be. Both new broker tests fail without the removal.
|
Bots are done — your move: address ZocoLini requested changes, then post |
`clear_pending` dropped the wanted diffs without telling the broker. The broker keeps a request until it is answered, so it went on sending a dropped `GetMnListDiff` to new peers. Their answers were ignored as unexpected, so the request never settled, and each one that timed out kicked the peer that had answered it. A reconnect hits this: the diffs a disconnect kept are dropped when the sync restarts from a fresh QRInfo. The pipeline also stored only target to base. The handler worked out whether a response was a tip update by comparing its base with the engine's newest list, which holds only while a work-block list's base lies below the newest list, so a work-block list whose base equalled the newest list was applied as a tip update and moved the newest list. A response was matched by target alone, so a diff for a wanted target built from another base was applied as if it answered the request, although Core echoes the requested base. And a diff for a block the header chain could not place was left wanted, so the peer that answered it was kicked as stalled, again and again. The pipeline now keeps one `WantedDiff` per target with its base and `DiffKind`, set by the caller that queues it: `send_tip_mnlistdiff_update` queues a `TipUpdate`, the QRInfo and post-apply paths queue `WorkBlockList`s. `match_response` requires the requested base and returns the kind. `clear` releases every request it drops through `release_request`, and `queue_requests` releases a target queued again from another base, which also drops the old request if it is still queued, since the broker keys `RequestKey::MnListDiff` by target alone and would keep sending the old request, whose answers no longer match. `settle_diff` is now the one place a matched diff is applied or given up. It releases the diff through the pipeline's `release`, counts it as processed only when it was applied, and completes the pipeline once nothing is left. A tip update given up publishes nothing, a work-block list given up still lets its tip update complete. A diff the engine refuses for any reason, an unplaceable block included, is given up. `test_clear` and `test_on_disconnect_requeues_instead_of_clearing` check that dropped diffs are released, `test_queue_requests` checks the release on a base change, and `test_match_response` and `a_tip_update_completes_when_its_work_block_list_fails` check that a diff from another base answers nothing.
The broker retries an unanswered request forever, across peers, and the masternode manager waited on it just as long. A diff no peer can answer, such as one for a block a reorg orphaned, kept the pipeline busy for good, which held back every later tip update and QRInfo. Each wanted diff now records when it was queued, and `tick_at` gives up those wanted longer than `MNLISTDIFF_DEADLINE` (60 s) through `settle_diff`, before it re-declares the rest. A given-up diff is released from the broker. The broker hides its attempts, so this deadline is the only clock. A full disconnect never runs it, because `tick` is idle until the sync restarts and the restart drops the old diffs. A tip update given up, at its deadline or refused, left the pipeline empty with the header tip still ahead, so `tick` asked for it again on every 100 ms pass. `given_up_tip` now holds it back until the header tip moves or `MNLISTDIFF_DEADLINE` has passed since it was given up, so a chain without new blocks, such as regtest or a devnet, still asks for it again at most once a minute. A disconnect clears it. `an_unanswered_diff_is_given_up_at_its_deadline` drives `tick_at` past the deadline of a work-block list and of a tip update, and `a_tip_update_given_up_is_not_asked_again_until_the_tip_moves` covers the guard.
Every diff the engine refused was given up on first sight, whoever was at fault. A peer that sent a diff contradicting itself was never held to account, and one such peer could cancel a tip update or a work-block list that the broker could have asked another peer for. `FeedError::blames_peer` now tells the faults apart in one exhaustive match. Only `InvalidCoinbaseProof` and `InvalidIndexInSignatureSet` prove the peer wrong whatever our state, since the diff then contradicts itself or the header that commits to it. Such a peer is banned, the diff stays wanted so the broker sends it to another peer, and the handler returns `MasternodeSyncFailed`, as the block and header managers do after their bans. Every other refusal can be a gap or bug of ours, or depend on our own state, so it is still given up without a ban. `CoinbaseCommitmentMismatch` checks the list built from our stored base, `IncompleteSignatureSet` fires on a devnet before V20 because its activation height is 0, `IncompleteMnListDiff` is what Core sends for a list from genesis to a block before any masternode registered, and `BaseBlockNotGenesis` and `BaseBlockHashMismatch` judge the base we requested against what we hold. Banning for those would ban every honest peer in turn, and with a single configured peer the ban would close the connection and loop through a full disconnect and restart. `a_work_block_list_that_contradicts_itself_bans_its_peer` follows the ban through to another peer's answer completing the tip update, `a_refused_tip_update_is_given_up_without_a_ban` checks that a missing ChainLock signature bans nobody, and `a_refused_diff_is_blamed_only_when_our_state_cannot_explain_it` pins the verdict for every refusal. `MockNetworkManager` now records the peers it is asked to ban.
A tip update only asks for the work-block lists of the quorums it brings, so a list given up at its deadline or refused by the engine left its quorum `Skipped(MissedList)` until the next QRInfo, a rotation cycle later. Given-up work-block lists are now remembered in one `WorkListRetry` map, and the next tip update applied also retries those a quorum of the newest list still needs, from the nearest lower list, through `readable_missing_work_block_list_requests`. Each is retried at most once, since the next QRInfo asks for every missing list anyway. The map is cleared when a QRInfo queues its lists and on disconnect. `a_given_up_work_block_list_is_retried_once` fails without the retry and when a list is retried a second time.
db5d698 to
460bd07
Compare
github-actions
Bot
removed
the
merge-conflict
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
`queue_requests`, `settle_diff` and `send_tip_mnlistdiff_update` take `now` instead of reading the clock themselves. A diff queued or given up from `tick_at` is stamped with the tick's time, so both deadlines follow the same clock the tests inject. Message handlers pass `Instant::now()`.
A target queued again from the same base replaced its entry, which reset `queued_at` and pushed back its 60s deadline. It now keeps its entry, so only a request from a different base starts a new deadline.
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
|
Reworks #1205 on top of #902. It fixes the ways a masternode list diff (
mnlistdiff) request could stall masternode sync or get honest peers kicked.Problem
Fix
request_answeredis renamed torelease_requestbecause it now covers requests that are no longer wanted too, and releasing also drops a still-queued copy.settle_diffis the one place a diff is applied or given up.MNLISTDIFF_DEADLINE) is given up. A given-up tip update is not asked for again until the header tip moves or the deadline passes again.FeedError::blames_peerdecides who is at fault. OnlyInvalidCoinbaseProofandInvalidIndexInSignatureSetban the peer, because those diffs contradict themselves or their block header. The diff stays wanted and goes to another peer. Any other refusal can come from our side, such as a stale base list or wrong activation heights, so it is given up without a ban.tickpasses the current time totick_at, so tests can reach the deadline.PR Hygiene ·
839aa46/skip-botsproceeds without the ones not yet reported/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit