Repository navigation
fix(dashmate): use complete DKG membership for safe stops - #5231
PastaPastaPasta wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSafe-stop checks now use ChangesMasternode safe-stop flow
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant waitForDKGWindowPass
participant checkMasternodeSafeToStop
participant rpcClient
participant isMasternodeSafeToStopDuringDkg
waitForDKGWindowPass->>checkMasternodeSafeToStop: request safe-stop check
checkMasternodeSafeToStop->>rpcClient: query dkginfo
checkMasternodeSafeToStop->>isMasternodeSafeToStopDuringDkg: evaluate DKG information
opt DKG status inspection required
checkMasternodeSafeToStop->>rpcClient: query dkgstatus and block height
checkMasternodeSafeToStop->>isMasternodeSafeToStopDuringDkg: evaluate status and block height
end
opt confirmation required
checkMasternodeSafeToStop->>rpcClient: query DKG information again after 5,000 ms
end
checkMasternodeSafeToStop-->>waitForDKGWindowPass: return safety result
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change permits stops for imminent DKGs when this masternode is not a member, while preserving the legacy fallback. No merge-blocking defect was established; the five-second confirmation assumption remains unverified against live Core v24 behavior. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes remain within the existing operator-controlled stop workflow and preserve conservative handling of unknown membership and malformed responses. A remaining uncertainty is whether the five-second confirmation delay reliably covers Core’s DKG-session publication delay; otherwise, a supposedly safe stop could still interrupt participation. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
the
waiting-bots
|
⛔ Final review complete — 1 blocking finding(s) (commit 3b540cc) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The membership-aware filtering and shared RPC helper are otherwise coherent, and the targeted quorum suite passes. However, the five-second confirmation does not establish that Core has initialized a start-at-tip DKG session; if tracking remains delayed, both snapshots can report no active session and the helper authorizes stopping a participating masternode. This is a newly exposed shutdown-safety regression affecting both ordinary stops and --safe polling.
🔴 1 blocking
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a substantial but well-contained dashmate behavioral change involving quorum stop-safety checks, polling, race handling, and extensive tests, but it does not modify consensus, funds, cryptography, key handling, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/src/core/quorum/checkMasternodeSafeToStop.js`:
- [BLOCKING] packages/dashmate/src/core/quorum/checkMasternodeSafeToStop.js:52-54: Fail closed until Core confirms start-at-tip session membership
The confirmation delay is only a timing assumption and provides no evidence that Core has completed asynchronous DKG session initialization. If a rotated member session starts at the current tip and Core still has not published it after five seconds, both evaluations can return `{ active_dkgs: 0, next_dkg: 1, upcoming_dkgs: [] }`. `isMasternodeSafeToStopDuringDkg` treats the empty upcoming list and zero active sessions as safe, so this helper returns true without querying `dkgstatus` or obtaining any readiness evidence. The previous `next_dkg <= 6` rule rejected this state, so the PR newly permits stopping a participating masternode during the session-start race; the same helper is used by both ordinary stops and `--safe`. Gate the relaxed decision on a Core response that covers the current-tip session or otherwise exposes a processed-tip/readiness guarantee. Until that contract exists, retain a fail-closed guard for the ambiguous snapshot rather than relying on a fixed timeout.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
github-actions
Bot
added
waiting-self-review
The safe-stop check refused to stop, or with --safe waited, whenever `quorum dkginfo` reported next_dkg <= 6. next_dkg is the minimum across every LLMQ type and stays at 1 for the whole rotation signing window, so on mainnet about a third of block heights blocked a stop, for up to 39 blocks, even on nodes that were not selected for any DKG. Missing a DKG you are not a member of carries no PoSe penalty. Dash Core v24 (dashpay/dash#7428) adds `upcoming_dkgs` to `quorum dkginfo`, with per-DKG membership for the local proTxHash. When it is present, an imminent DKG blocks the stop only if this node is a member or Core cannot determine membership. Older Core keeps the next_dkg rule. Malformed data fails safe. Core lists DKGs strictly above the tip, so a rotated session starting at the tip only shows up in active_dkgs once Core initializes it. When membership data is what cleared an imminent DKG, the safe verdict is re-checked after 5 seconds. The Core RPC sequence is moved into checkMasternodeSafeToStop so `stop` and `--safe` share it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
600bfd4 to
b5d0721
Compare
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the combined findings against head b5d0721, the complete seven-file diff, and Dash Core v24.0.0-rc.2's relevant RPC and initialization code. One blocking shutdown-safety regression remains: the confirmation delay does not establish that current-tip DKG membership has been published. This was a static review; the supplied CI snapshot shows Dashmate lint passing, with Dashmate tests and the helper-image build still in progress.
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change adds nontrivial membership-aware shutdown logic and a delayed confirmation check, but does not itself modify consensus rules or another qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/src/core/quorum/checkMasternodeSafeToStop.js`:
- [BLOCKING] packages/dashmate/src/core/quorum/checkMasternodeSafeToStop.js:52-54: Fail closed until Core confirms start-at-tip session membership
(existing thread: https://github.com/dashpay/platform/pull/5231#discussion_r4150904825)
Both evaluations can receive `{ active_dkgs: 0, next_dkg: 1, upcoming_dkgs: [] }` after a rotated member session's start block connects but before its initialization completes. The predicate accepts that snapshot, skips `dkgstatus`, and this return authorizes shutdown through both ordinary stops and `--safe`. The previous implementation rejected it because `next_dkg <= 6`, so this is a newly exposed rotated-session gap, not merely the pre-existing non-rotated race.
Core v24.0.0-rc.2 confirms the missing synchronization: `quorum_dkginfo` reads the debug-session counter separately from the chain tip and advances every `quorumHeight <= nTipHeight` into the next cycle. Meanwhile, `NetDKG::HandleDKGRound` waits for the initialized phase, acquires `cs_main`, and calls `InitNewQuorum`; member status is published later in `CDKGSession::Init`. The handler's 100 ms polling sleep bounds neither scheduling nor completion of those operations. A second response after five seconds therefore need not contain any new readiness evidence.
The documented limitation and default v23 image do not prevent this path for operators configured to use v24. Gate the relaxation on a Core capability that covers current-tip membership or provides synchronized readiness evidence, and retain conservative handling for ambiguous responses without that capability. Add coverage for initialization remaining unpublished through the confirmation read; the existing race test only covers publication before that read.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
Verified the complete diff at acc0f3a and found no remaining in-scope defects. The relaxed stop decision requires complete active and upcoming membership, blocks participating or unknown active sessions, and preserves the legacy guards when the new Core contract is unavailable. This was a static review: the supplied exact-head CI snapshot shows Dashmate tests, lint, all three Dashmate E2E jobs, and the JS build passing; two broader test-suite jobs and PR Hygiene remain pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change introduces nontrivial RPC-dependent stop-safety logic and legacy fallbacks, but does not itself modify consensus, cryptography, funds handling, peer-facing deserialization, or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 18 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Core no longer reports active_dkg_sessions (dashpay/dash#7812 was slimmed). When upcoming_dkgs is present, stopping is safe only if active_dkgs is 0 and no member, unknown-membership or malformed upcoming DKG starts within MIN_BLOCKS_BEFORE_DKG. The legacy next_dkg guard and dkgstatus refinement are kept for responses without upcoming_dkgs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the shared safety checker, both stop paths, the base-to-head regression, Core v24.0.0-rc.2, and the proposed Core correction. One blocking issue remains: the relaxed decision accepts existing release candidates whose response shape does not guarantee complete active-session membership. This was a static review; the supplied exact-head CI snapshot still had JS and Docker builds queued or running, so it does not establish completed validation for this head.
🔴 1 blocking
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff changes operational stop-safety decisions, RPC compatibility handling, and polling behavior, but does not itself modify consensus rules or another qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/src/core/quorum/isMasternodeSafeToStopDuringDkg.js`:
- [BLOCKING] packages/dashmate/src/core/quorum/isMasternodeSafeToStopDuringDkg.js:98-101: Fail closed until Core confirms start-at-tip session membership
The presence of `upcoming_dkgs` does not identify a Core build with complete active-session membership. Core v24.0.0-rc.2 already emits that field, but its `quorum_dkginfo` obtains `active_dkgs` from `dkgdbgman.GetSessionCount()` and advances every `quorumHeight <= nTipHeight` into the next cycle. Local session status is initialized separately in `CDKGSession::Init`. A participating session starting at the tip can therefore be absent from both counters while the remaining upcoming entries are empty or non-blocking. These lines immediately authorize stopping in that state, and `checkMasternodeSafeToStop` passes the verdict to both ordinary stops and `--safe` polling without a capability or fixed-build check. The base implementation rejected this rotated-window state through `next_dkg <= MIN_BLOCKS_BEFORE_DKG`, so this is an in-scope safety regression that can interrupt required participation and risk PoSe penalties.
The membership-derived count in dashpay/dash#7812 addresses the ambiguity at the correct layer, including sessions not yet initialized locally. However, the same-release plan and default Core 23 image do not exclude the affected release candidates: `core.docker.image` is configurable, and their existing response shape activates this branch. Gate the relaxed policy on a verifiably corrected Core build or explicit capability, retaining the legacy or fail-closed decision for uncorrected builds. This preserves immediate non-member stops on corrected Core without imposing a blanket rotation-window restriction there. Add adapter coverage distinguishing pre-fix release candidates from builds implementing the corrected contract.
Accepted risk: only manually configured Core v24.0.0 release candidates are affected; see reply on the thread. (Dismissed by Claude on behalf of pasta.)
|
@coderabbitai review No review for |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
1ed0310 docs: add release note for current DKG membership RPC (pasta) 7283b7d fix(rpc): derive active DKG count from membership (pasta) Pull request description: ## Issue being fixed or feature implemented `quorum dkginfo` is meant to answer a simple question: is this masternode participating in a DKG right now, or can it be restarted safely? `active_dkgs` answers it wrongly at both ends of a session. At a DKG start block it stays zero until the asynchronous DKG worker publishes its local debug record, and afterwards it keeps counting expired debug records. ## What was done? `active_dkgs` is now computed from membership at the captured chain tip instead of debug-session tracking: - Sessions the given or local masternode is a member of count from their start block, even before the DKG worker initializes them. - Each session counts through its Commit phase and stops at `quorumHeight + 5 * dkgPhaseBlocks`. Rotated indices expire individually, not at the shared cycle mining window. - If membership can't be determined (for example before v20), the session is counted, so zero never hides possible participation. - Calls without a supplied or local proTxHash report zero. No RPC fields are added. `upcoming_dkgs` is unchanged. The membership lookup is shared between `active_dkgs` and `upcoming_dkgs`, and `CDKGDebugManager::GetSessionCount()`, which lost its only caller, is removed. Together with `upcoming_dkgs`, this lets dashpay/platform#5231 decide whether a masternode can stop safely from a single `quorum dkginfo` call. ## How Has This Been Tested? - `test/functional/rpc_quorum.py`: with DKG publication disabled, checks the count for a member and a non-member at the start block, inside the window, and after expiry, against the `upcoming_dkgs` predictions taken one block before the start. - `test/functional/feature_llmq_rotation.py`: checks conservative counting of unknown membership before v20, zero counts despite expired debug sessions, and per-index expiry of rotated sessions at cycle offsets 0, 1, 9, 10, 11, 12 and 13. ## Breaking Changes `active_dkgs` stays numeric, but its meaning is corrected: it includes sessions not yet initialized locally, excludes expired sessions, counts unknown membership, and follows the supplied proTxHash. Clients that need the local DKG worker state should use `quorum dkgstatus`. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 3c12676a7c8d386521a2bed1fafc71979c8837b678651fda2f1944836651d02223a4e198ba5d31a4c9c1998bce728c2a743fa27d9b1225ce2fc2c063715d9e43
This branch has not been deployed
2 participants
Issue being fixed or feature implemented
Dashmate blocks masternode stops and restarts whenever any DKG is imminent, even if the node is not a member. Core pins
next_dkgto 1 throughout the rotated signing window, making harmless fleet maintenance wait unnecessarily.Core follow-up dashpay/dash#7812 corrects
active_dkgsto count the masternode's membership in every active DKG session, independently of asynchronous local session initialization. Together with the existingupcoming_dkgs, this lets Dashmate distinguish participating nodes from non-members without any new RPC fields.What was done?
upcoming_dkgs, a positiveactive_dkgscount prevents stopping, and upcoming member or unknown-membership sessions block within the existing six-block restart margin. Malformed membership data fails closed.next_dkgand per-sessiondkgstatus/height checks for responses withoutupcoming_dkgs(older Core, or no proTxHash).needsSafeStopConfirmation. Complete membership responses need only onedkginfocall, even if local session records have not appeared or remain stale.--safepolling loop.The
upcoming_dkgspath relies on the correctedactive_dkgsfrom dashpay/dash#7812, so that change needs to ship in the same Core release asupcoming_dkgs(v24.0.0). Core v24.0.0 release candidates without it reportupcoming_dkgswith the oldactive_dkgscount.How Has This Been Tested?
Validated locally with Node v22.23.2 and Yarn 4.12.0.
yarn workspace dashmate mocha test/unit/core/quorum): 58 passing tests, covering positiveactive_dkgs, members and unknown membership inside and outside the six-block restart margin, malformedupcoming_dkgs, legacy responses, stale records, and the safe-stop polling loop.yarn workspace dashmate lint: no errors.CI on this head passed the JS build, Dashmate unit tests and lint, and all three Dashmate E2E jobs. The broader Platform test suite failed in the unchanged SDK proof-verification path with
Quorum not found in cache, followed by exhausted DAPI addresses (job). The same error occurs in earlierv5.1-devCI (baseline job) and failed again on retry; the affected proof-verifier and quorum-cache code is identical.Self-review found no correctness blockers in the changed safety logic.
Breaking Changes
None. Older Core keeps its existing behavior. Fixed Core allows known non-members to stop immediately without a confirmation delay.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
This pull request was updated by Codex.
PR Hygiene ·
3b540cc/self-revieweddashmate(packages/dashmate/src/constants.js,packages/dashmate/src/core/quorum/checkMasternodeSafeToStop.js,packages/dashmate/src/core/quorum/isMasternodeSafeToStopDuringDkg.jsand 5 more) — ktechmidas or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.