feat(dash-spv): drive masternode sync via PipelineMode state machine#738
Open
xdustinface wants to merge 2 commits intov0.42-devfrom
Open
feat(dash-spv): drive masternode sync via PipelineMode state machine#738xdustinface wants to merge 2 commits intov0.42-devfrom
PipelineMode state machine#738xdustinface wants to merge 2 commits intov0.42-devfrom
Conversation
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactors masternode sync into PipelineMode (QuorumValidation vs Incremental), extends SyncEvent with optional QRInfo results, adds per-tip QRInfo gating and escalating QRInfo timeouts, introduces incremental completion and progress counters, updates header routing, tests, and the FFI callback match. ChangesMasternode Sync Pipeline Refactoring
Sequence DiagramsequenceDiagram
actor Sync as SyncManager
participant Header as BlockHeaderStorage
participant Manager as MasternodesManager
participant Engine as MasternodeListEngine
participant Requests as RequestSender
rect rgba(100, 150, 200, 0.5)
note over Sync,Requests: Pipeline Mode Selection
Sync->>Manager: next_pipeline_mode(tip_height)
Manager->>Manager: Check dkg window, cycles, attempts
alt Mining window + fresh cycle
Manager-->>Sync: PipelineMode::QuorumValidation
else Incremental or out-of-window
Manager-->>Sync: PipelineMode::Incremental
end
end
rect rgba(150, 100, 200, 0.5)
note over Sync,Requests: QuorumValidation Path
Sync->>Requests: send_qrinfo_request()
Sync->>Manager: record_qrinfo_attempt(tip_height)
Sync->>Sync: waiting_for_qrinfo ← true
Sync->>Engine: process qrinfo_feed()
Engine-->>Sync: qr_info_result
Sync->>Manager: mark_cycle_validated() if all verified
Sync->>Manager: verify_and_complete(qr_info_result)
Manager-->>Sync: SyncEvent[...MasternodeStateUpdated(qr_info_result)]
end
rect rgba(200, 150, 100, 0.5)
note over Sync,Requests: Incremental Path
Sync->>Manager: send_tip_mnlistdiff_update()
Manager->>Requests: send_getmnlistdiff(last_synced_block_hash..tip)
Manager-->>Sync: queued requests
Sync->>Engine: apply_diff() for each response
Sync->>Manager: complete_incremental_pipeline()
Manager->>Engine: verify_non_rotating_quorums(latest_height)
alt Verification succeeds
Manager-->>Sync: SyncEvent[...MasternodeStateUpdated]
else Verification fails
Manager-->>Sync: Ok(vec![]) no event, logged warn
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Collaborator
Author
|
@CodeRabbit review! |
Contributor
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #738 +/- ##
=============================================
+ Coverage 71.49% 71.54% +0.04%
=============================================
Files 320 320
Lines 69023 69327 +304
=============================================
+ Hits 49350 49600 +250
- Misses 19673 19727 +54
|
xdustinface
force-pushed
the
feat/mn-pipeline-mode-state-machine
branch
3 times, most recently
from
7d1a816 to
c1a1015
Compare
xdustinface
force-pushed
the
fix/qr-info-rotation-quorums
branch
from
c9d5141 to
25c3347
Compare
xdustinface
force-pushed
the
feat/mn-pipeline-mode-state-machine
branch
from
c1a1015 to
60bf031
Compare
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv-ffi/src/callbacks.rs (1)
182-185:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExpose the new masternode update payload through the FFI API.
The Rust event now carries
qr_info_result, but this callback still exports onlyheight, anddispatchdrops the new field with... That leaves C/Swift consumers unable to observe the qualification / verification / stored-cycle data this PR is adding toMasternodeStateUpdated. Please add a C-compatible payload struct or a companion callback so the FFI surface preserves the new event semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv-ffi/src/callbacks.rs` around lines 182 - 185, The current OnMasternodeStateUpdatedCallback only exposes height and drops the new qr_info_result field from SyncEvent::MasternodeStateUpdated in dispatch; update the FFI to preserve that payload by defining a C-compatible struct (e.g., MasternodeQrInfoPayload or MasternodeStateUpdatedPayload) that includes height and the qr_info_result fields (or minimal primitives representing qualification/verification/stored-cycle data), and change OnMasternodeStateUpdatedCallback to accept a pointer to that struct (or add a companion extern "C" callback type like OnMasternodeQrInfoCallback) and marshal qr_info_result into the C struct in the dispatch path so the FFI consumers (C/Swift) can access the new data rather than it being dropped.dash-spv/src/sync/masternodes/sync_manager.rs (1)
583-603:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe QRInfo retry budget is off by one.
retry_countis indexed so0is the initial send, but this condition still resends when the last schedule slot (retry_count == 2) times out. That produces a fourth request and another 60-second wait, so the actual worst-case is 160s instead of the documented 100s.Suggested fix
- if self.sync_state.qrinfo_retry_count < MAX_RETRY_ATTEMPTS { + if self.sync_state.qrinfo_retry_count + < MAX_RETRY_ATTEMPTS.saturating_sub(1) + { tracing::warn!( timeout_secs = timeout.as_secs(), retry_count = self.sync_state.qrinfo_retry_count, "Timeout waiting for QRInfo response, retrying..." ); self.sync_state.qrinfo_retry_count += 1; self.sync_state.clear_pending(); return self.send_qrinfo_for_tip(requests).await; } else { tracing::warn!( "QRInfo timeout after {} retries, skipping masternode sync", - MAX_RETRY_ATTEMPTS + MAX_RETRY_ATTEMPTS.saturating_sub(1) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/sync/masternodes/sync_manager.rs` around lines 583 - 603, The QRInfo retry condition allows one extra send because retry_count is zero-indexed; change the resend guard from self.sync_state.qrinfo_retry_count < MAX_RETRY_ATTEMPTS to self.sync_state.qrinfo_retry_count < (MAX_RETRY_ATTEMPTS - 1) so we don't schedule a fourth request when qrinfo_retry_count == 2. Update the branch that calls send_qrinfo_for_tip (in sync_manager.rs, around send_qrinfo_for_tip, complete_pipeline, qrinfo_timeout_for and self.sync_state.qrinfo_retry_count) to use that adjusted comparison and ensure the warning message still reflects the actual remaining retries if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/sync/masternodes/sync_manager.rs`:
- Around line 427-465: The branch that launches a new pipeline (via
next_pipeline_mode -> send_qrinfo_for_tip or send_tip_mnlistdiff_update) must
not start if a previous pipeline still has pending work; add a guard at the top
of this Synced branch that returns early when the sync_state indicates
outstanding pipeline work (e.g. implement SyncState::has_pending_work() or
similar to check waiting_for_qrinfo, an in-flight qr_info_result, or a non-empty
pending queue) and use it: if self.sync_state.has_pending_work() {
tracing::debug!("pipeline pending, skipping new tip pipeline"); return
Ok(vec![]); } Apply the same guard in the analogous BlockHeaderSyncComplete
branch so a new Incremental/QuorumValidation mode is not started while earlier
MnListDiffs are still draining.
---
Outside diff comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 182-185: The current OnMasternodeStateUpdatedCallback only exposes
height and drops the new qr_info_result field from
SyncEvent::MasternodeStateUpdated in dispatch; update the FFI to preserve that
payload by defining a C-compatible struct (e.g., MasternodeQrInfoPayload or
MasternodeStateUpdatedPayload) that includes height and the qr_info_result
fields (or minimal primitives representing
qualification/verification/stored-cycle data), and change
OnMasternodeStateUpdatedCallback to accept a pointer to that struct (or add a
companion extern "C" callback type like OnMasternodeQrInfoCallback) and marshal
qr_info_result into the C struct in the dispatch path so the FFI consumers
(C/Swift) can access the new data rather than it being dropped.
In `@dash-spv/src/sync/masternodes/sync_manager.rs`:
- Around line 583-603: The QRInfo retry condition allows one extra send because
retry_count is zero-indexed; change the resend guard from
self.sync_state.qrinfo_retry_count < MAX_RETRY_ATTEMPTS to
self.sync_state.qrinfo_retry_count < (MAX_RETRY_ATTEMPTS - 1) so we don't
schedule a fourth request when qrinfo_retry_count == 2. Update the branch that
calls send_qrinfo_for_tip (in sync_manager.rs, around send_qrinfo_for_tip,
complete_pipeline, qrinfo_timeout_for and self.sync_state.qrinfo_retry_count) to
use that adjusted comparison and ensure the warning message still reflects the
actual remaining retries if desired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 084ccea3-ad53-47c3-90b7-27037c6822ce
📒 Files selected for processing (5)
dash-spv-ffi/src/callbacks.rsdash-spv/src/sync/events.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/masternodes/progress.rsdash-spv/src/sync/masternodes/sync_manager.rs
xdustinface
added a commit
that referenced
this pull request
May 7, 2026
When a `QRInfo` response is processed the `waiting_for_qrinfo` flag clears immediately while the historical `MnListDiff`s still drain through `mnlistdiff_pipeline`. Until then `pipeline_mode` holds `QuorumValidation { qr_info_result }`. A subsequent `BlockHeadersStored` (or `BlockHeaderSyncComplete`) firing in `Synced` could see `next_pipeline_mode` return `Incremental` (the cycle was just marked validated), call `send_tip_mnlistdiff_update`, overwrite `pipeline_mode = Incremental`, and append a tip request onto the still-draining queue. When the shared pipeline finally completes, `complete_pipeline` dispatched to `complete_incremental_pipeline` and the original `qr_info_result` (with rotated quorum data) was discarded.
Add the `has_pending_requests()` early-return at the top of both `Synced` arms, mirroring the guard the `tick` handler already uses. The `tick` handler picks up any tip gap once the pipeline drains, so no incremental update is lost.
Addresses CodeRabbit review comment on PR [#738](#738)
[review comment](#738 (comment))
xdustinface
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
Introduce a `PipelineMode` enum (`QuorumValidation` / `Incremental`) on `MasternodeSyncState` so the sync manager decides per tip whether to fire a full `getqrinfo` or a targeted `GetMnListDiff`. `next_pipeline_mode` is mining-window-aware: inside a cycle's DKG window it picks `QuorumValidation` until the cycle is fully verified, otherwise it picks `Incremental` to keep the tip list fresh. Drop the `CHAINLOCK_RETRY_DELAY` retry (now redundant since `feed_qr_info` is resilient to missing rotation CL sigs) and replace the single `QRINFO_TIMEOUT_SECS` with a `[10, 30, 60]` per-attempt schedule. Add `should_process_qrinfo` to drop duplicate and unsolicited responses, and recover `last_synced_block_hash` from the engine on startup so restarts can resume with a targeted diff. Plumb `QRInfoFeedResult` through `SyncEvent::MasternodeStateUpdated` so consumers can see what was qualified, fully verified, and stored on the same event. Add `qr_infos_requested`, `validated_cycles`, and `rotation_cycles` counters on `MasternodesProgress` so the masternode sync exposes per-cycle progress alongside the existing `diffs_processed`.
When a `QRInfo` response is processed the `waiting_for_qrinfo` flag clears immediately while the historical `MnListDiff`s still drain through `mnlistdiff_pipeline`. Until then `pipeline_mode` holds `QuorumValidation { qr_info_result }`. A subsequent `BlockHeadersStored` (or `BlockHeaderSyncComplete`) firing in `Synced` could see `next_pipeline_mode` return `Incremental` (the cycle was just marked validated), call `send_tip_mnlistdiff_update`, overwrite `pipeline_mode = Incremental`, and append a tip request onto the still-draining queue. When the shared pipeline finally completes, `complete_pipeline` dispatched to `complete_incremental_pipeline` and the original `qr_info_result` (with rotated quorum data) was discarded.
Add the `has_pending_requests()` early-return at the top of both `Synced` arms, mirroring the guard the `tick` handler already uses. The `tick` handler picks up any tip gap once the pipeline drains, so no incremental update is lost.
Addresses CodeRabbit review comment on PR [#738](#738)
[review comment](#738 (comment))
xdustinface
force-pushed
the
feat/mn-pipeline-mode-state-machine
branch
from
4d3421b to
5bd2320
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/sync/masternodes/sync_manager.rs`:
- Around line 18-39: The retry loop currently lets qrinfo_retry_count reach a
value equal to QRINFO_TIMEOUT_SCHEDULE_SECS.len(), causing a 4th attempt that is
clamped by qrinfo_timeout_for; to fix (Option B) keep MAX_RETRY_ATTEMPTS =
QRINFO_TIMEOUT_SCHEDULE_SECS.len() as u8 and change the retry logic in the tick
loop that increments/compares qrinfo_retry_count so it prevents entering an
attempt whose retry_count == QRINFO_TIMEOUT_SCHEDULE_SECS.len() (i.e., check the
limit before doing the post-increment or use < MAX_RETRY_ATTEMPTS against the
value that will be used), ensuring qrinfo_timeout_for remains purely defensive
and total attempts equal the schedule length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f776e982-f511-45ec-afbd-5441b180959e
📒 Files selected for processing (1)
dash-spv/src/sync/masternodes/sync_manager.rs
QuantumExplorer
approved these changes
May 7, 2026
Member
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
AI likes it, I like it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce a
PipelineModeenum (QuorumValidation/Incremental) onMasternodeSyncStateso the sync manager decides per tip whether to fire a fullgetqrinfoor a targetedGetMnListDiff.next_pipeline_modeis mining-window-aware: inside a cycle's DKG window it picksQuorumValidationuntil the cycle is fully verified, otherwise it picksIncrementalto keep the tip list fresh.Drop the
CHAINLOCK_RETRY_DELAYretry (now redundant sincefeed_qr_infois resilient to missing rotation CL sigs) and replace the singleQRINFO_TIMEOUT_SECSwith a[10, 30, 60]per-attempt schedule. Addshould_process_qrinfoto drop duplicate and unsolicited responses, and recoverlast_synced_block_hashfrom the engine on startup so restarts can resume with a targeted diff.Plumb
QRInfoFeedResultthroughSyncEvent::MasternodeStateUpdatedso consumers can see what was qualified, fully verified, and stored on the same event. Addqr_infos_requested,validated_cycles, androtation_cyclescounters onMasternodesProgressso the masternode sync exposes per-cycle progress alongside the existingdiffs_processed.Summary by CodeRabbit
New Features
Bug Fixes