fix(dashmate): use active_dkgs for safe DKG stop#3941
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new ChangesDKG Safe-Stop Safety Logic Rewrite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
✅ Review complete (commit a14c214) |
This comment was marked as duplicate.
This comment was marked as duplicate.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Centralizes the DKG safe-stop predicate into a shared isMasternodeSafeToStopDuringDkg helper that requires both active_dkgs === 0 and next_dkg > MIN_BLOCKS_BEFORE_DKG, fixing the prior bug where a wrapped next_dkg countdown was read as safe while the node was still mid-session. The --safe wait loop adopts the same predicate and gains both a block-height deadline and a no-progress wall-clock backstop. No in-scope defects identified; all agent findings are pre-existing fragilities or speculative tuning notes outside this PR's scope.
thepastaclaw
force-pushed
the
fix-dashmate-safe-dkg-restart
branch
from
e73077b to
a9cf279
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Current-head review for a9cf279c9c after scope reduction.
The PR now stays focused on the active-DKG regression: both the immediate stop check and the --safe wait loop use the same predicate, active_dkgs === 0 && next_dkg > MIN_BLOCKS_BEFORE_DKG. This preserves the imminent-DKG guard while preventing the countdown-wrap case (next_dkg back to a large value while active_dkgs > 0) from unblocking a safe stop.
The previous wall-clock/no-progress exit hatch has been removed from the diff and from the PR body.
Review result: no significant issues found.
Validation reviewed:
node --checkon the changed files- direct helper smoke checks for safe/unsafe combinations
git diff --check upstream/v3.1-dev..HEAD- formal dashmate unit command still blocked locally by Node 25 + Yarn PnP
EBADFbefore repo code executes
Recommendation: ship.
This comment was marked as duplicate.
This comment was marked as duplicate.
thepastaclaw
force-pushed
the
fix-dashmate-safe-dkg-restart
branch
from
a9cf279 to
81550f0
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
thepastaclaw
force-pushed
the
fix-dashmate-safe-dkg-restart
branch
from
81550f0 to
2ba3c83
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-review of head SHA 81550f0 (incremental + cumulative). Prior review at a9cf279 was clean and the delta has not introduced new issues. Only one in-scope nitpick from claude-general was validated: the offset < 0 clause at line 116 is mathematically redundant given the minimum window value of 10, but harmless and arguably documentary. No blocking or suggestion-level findings; codex-general, both security auditors, and CodeRabbit all reported clean.
💬 1 nitpick(s)
This comment was marked as duplicate.
This comment was marked as duplicate.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative PR is a sound, conservative safe-stop fix that composes the imminent-DKG guard with per-session active-window inspection and fails closed on malformed input. Latest delta is test-only Sinon formatting and introduces no behavioral change. One pre-existing nitpick about a redundant offset < 0 clause persists at head; not blocking.
💬 1 nitpick(s)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/dashmate/src/core/quorum/isMasternodeSafeToStopDuringDkg.js (1)
1-1: ⚡ Quick winRename this new package file to kebab-case.
Please rename
isMasternodeSafeToStopDuringDkg.jsto kebab-case (and update imports) to match package file naming rules.
As per coding guidelines,packages/**/!(node_modules)/**/*.{js,jsx,ts,tsx}should use kebab-case filenames within JS packages.🤖 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 `@packages/dashmate/src/core/quorum/isMasternodeSafeToStopDuringDkg.js` at line 1, The file isMasternodeSafeToStopDuringDkg.js is named in camelCase but should follow kebab-case naming convention according to package guidelines. Rename the file from isMasternodeSafeToStopDuringDkg.js to is-masternode-safe-to-stop-during-dkg.js and update all import statements throughout the codebase that reference this file to use the new kebab-case filename instead of the camelCase version.Source: Coding guidelines
packages/dashmate/test/unit/core/quorum/waitForDKGWindowPass.spec.js (1)
1-1: ⚡ Quick winRename this new package test file to kebab-case.
Please rename
waitForDKGWindowPass.spec.jsto kebab-case and update imports accordingly.
As per coding guidelines,packages/**/!(node_modules)/**/*.{js,jsx,ts,tsx}should use kebab-case filenames within JS packages.🤖 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 `@packages/dashmate/test/unit/core/quorum/waitForDKGWindowPass.spec.js` at line 1, The test file waitForDKGWindowPass.spec.js uses camelCase naming instead of the required kebab-case format per coding guidelines. Rename the file from waitForDKGWindowPass.spec.js to wait-for-dkg-window-pass.spec.js and update the import statement at the top of the file to reference the new kebab-case filename accordingly.Source: Coding guidelines
packages/dashmate/test/unit/core/quorum/isMasternodeSafeToStopDuringDkg.spec.js (2)
1-3: ⚡ Quick winRename this new package test file to kebab-case.
Please rename
isMasternodeSafeToStopDuringDkg.spec.jsto kebab-case and update imports accordingly.
As per coding guidelines,packages/**/!(node_modules)/**/*.{js,jsx,ts,tsx}should use kebab-case filenames within JS packages.🤖 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 `@packages/dashmate/test/unit/core/quorum/isMasternodeSafeToStopDuringDkg.spec.js` around lines 1 - 3, The test file isMasternodeSafeToStopDuringDkg.spec.js does not follow the kebab-case naming convention required for JavaScript package files. Rename the file from camelCase to kebab-case format and update the import statement at the top of the file that references isMasternodeSafeToStopDuringDkg to match the new filename.Source: Coding guidelines
127-189: ⚡ Quick winAdd fail-closed tests for malformed
dkgInfo.Current fail-safe coverage is strong for
dkgStatus/currentHeight, but it does not assert malformeddkgInfocases (e.g., missing/non-numericnext_dkgoractive_dkgs). Add explicitfalseexpectations for those payloads.🤖 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 `@packages/dashmate/test/unit/core/quorum/isMasternodeSafeToStopDuringDkg.spec.js` around lines 127 - 189, Add test cases within the "fail-safe on malformed inputs while active_dkgs > 0" describe block to verify that isMasternodeSafeToStopDuringDkg returns false when dkgInfo is malformed, specifically when next_dkg is missing or non-numeric, when active_dkgs is missing or non-numeric, and when dkgInfo itself is undefined or null. Each test should follow the existing pattern of setting up dkgInfo with a valid dkgStatus and currentHeight while varying only the dkgInfo payload to trigger the malformed input conditions.
🤖 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 `@packages/dashmate/src/core/quorum/isMasternodeSafeToStopDuringDkg.js`:
- Around line 84-92: The function isMasternodeSafeToStopDuringDkg fails to
validate that the destructured properties next_dkg and active_dkgs from dkgInfo
are properly defined and valid before using them. If these properties are
missing or malformed, the function can incorrectly return true, breaking the
fail-closed behavior. Add validation at the beginning after the destructuring to
check that both nextDkg and activeDkgs are valid numbers, and return false
immediately if either property is missing, undefined, or malformed, ensuring the
function fails closed when dkgInfo is malformed.
---
Nitpick comments:
In `@packages/dashmate/src/core/quorum/isMasternodeSafeToStopDuringDkg.js`:
- Line 1: The file isMasternodeSafeToStopDuringDkg.js is named in camelCase but
should follow kebab-case naming convention according to package guidelines.
Rename the file from isMasternodeSafeToStopDuringDkg.js to
is-masternode-safe-to-stop-during-dkg.js and update all import statements
throughout the codebase that reference this file to use the new kebab-case
filename instead of the camelCase version.
In
`@packages/dashmate/test/unit/core/quorum/isMasternodeSafeToStopDuringDkg.spec.js`:
- Around line 1-3: The test file isMasternodeSafeToStopDuringDkg.spec.js does
not follow the kebab-case naming convention required for JavaScript package
files. Rename the file from camelCase to kebab-case format and update the import
statement at the top of the file that references isMasternodeSafeToStopDuringDkg
to match the new filename.
- Around line 127-189: Add test cases within the "fail-safe on malformed inputs
while active_dkgs > 0" describe block to verify that
isMasternodeSafeToStopDuringDkg returns false when dkgInfo is malformed,
specifically when next_dkg is missing or non-numeric, when active_dkgs is
missing or non-numeric, and when dkgInfo itself is undefined or null. Each test
should follow the existing pattern of setting up dkgInfo with a valid dkgStatus
and currentHeight while varying only the dkgInfo payload to trigger the
malformed input conditions.
In `@packages/dashmate/test/unit/core/quorum/waitForDKGWindowPass.spec.js`:
- Line 1: The test file waitForDKGWindowPass.spec.js uses camelCase naming
instead of the required kebab-case format per coding guidelines. Rename the file
from waitForDKGWindowPass.spec.js to wait-for-dkg-window-pass.spec.js and update
the import statement at the top of the file to reference the new kebab-case
filename accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba636f47-6c04-4284-ab0f-8675f5e276dd
📒 Files selected for processing (5)
packages/dashmate/src/core/quorum/isMasternodeSafeToStopDuringDkg.jspackages/dashmate/src/core/quorum/waitForDKGWindowPass.jspackages/dashmate/src/listr/tasks/stopNodeTaskFactory.jspackages/dashmate/test/unit/core/quorum/isMasternodeSafeToStopDuringDkg.spec.jspackages/dashmate/test/unit/core/quorum/waitForDKGWindowPass.spec.js
`isMasternodeSafeToStopDuringDkg` previously destructured `dkgInfo` unconditionally, so a missing/null/malformed `quorum dkginfo` payload could throw (or worse, coerce its way to `true`) instead of being treated as unsafe. Validate that `dkgInfo` is an object with finite, non-negative `active_dkgs` and `next_dkg` before reasoning about them, and share that guard with `shouldInspectDkgStatusForSafeStop` so the poll path can't reach the destructure with a bad shape either. Add specs covering undefined/null `dkgInfo` and missing/non-numeric/NaN/ negative `active_dkgs` and `next_dkg`.
|
Addressed the malformed
I intentionally did not apply the kebab-case rename nitpicks: Formal yarn/mocha/eslint commands are still blocked locally by the known Node 25 + Yarn PnP |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push (a14c214) adds fail-closed handling for malformed dkginfo plus tests. Verified at head: two nitpicks remain — stale JSDoc on shouldInspectDkgStatusForSafeStop (Claude) and the redundant offset<0 clause carried over from the prior review (Codex). No blocking issues; the session-window math, llmqType mapping, and fail-closed paths are consistent between the immediate stop check and the --safe wait loop.
💬 2 nitpick(s)
| /** | ||
| * @param {{ active_dkgs: number, next_dkg: number }} dkgInfo | ||
| * @return {boolean} | ||
| */ | ||
| export function shouldInspectDkgStatusForSafeStop(dkgInfo) { | ||
| if (!hasValidDkgInfoShape(dkgInfo)) { | ||
| return false; | ||
| } | ||
|
|
||
| return dkgInfo.active_dkgs > 0 && dkgInfo.next_dkg > MIN_BLOCKS_BEFORE_DKG; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: JSDoc on shouldInspectDkgStatusForSafeStop omits malformed-input behavior
The function now returns false for null/undefined/non-object dkgInfo and for non-finite active_dkgs/next_dkg via hasValidDkgInfoShape, but the JSDoc still types dkgInfo as { active_dkgs: number, next_dkg: number } and only documents the happy-path return. Future callers (waitForDKGWindowPass, stopNodeTaskFactory) could read this as 'expects a well-formed dkgInfo' and add redundant upstream guards, defeating the centralization this PR just introduced. Tighten the JSDoc to state the predicate is also the malformed-dkgInfo guard.
source: ['claude']
| if (offset < 0 || offset < windowLength) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Redundant negative-offset check
Every value in DKG_MINING_WINDOW_START_BY_LLMQ_TYPE is positive (10/12/20/42), and unknown llmqTypes return false at the windowLength === undefined branch above. So any negative offset already satisfies offset < windowLength, making offset < 0 strictly redundant. The doc comment at lines 79-81 explicitly calls out the negative branch as fail-safe, so this is style-only — keep it if you want the intent to read explicitly at the call site, otherwise collapse to offset < windowLength.
| if (offset < 0 || offset < windowLength) { | |
| return false; | |
| } | |
| if (offset < windowLength) { | |
| return false; | |
| } |
source: ['codex']
1 participant
Issue being fixed or feature implemented
dashmate stop --safe/dashmate restart --safeusedquorum dkginfo.next_dkgas the only DKG safety signal.next_dkgis a countdown until the next potential DKG cycle; it is not proof that this node is not currently participating in DKG.When the countdown wraps at the start of a new cycle,
next_dkgcan become large again while the node is still in an active DKG session. In that case dashmate could treat the node as safe to stop during active DKG participation, risking a failed DKG and PoSe penalty.What was done?
isMasternodeSafeToStopDuringDkg(...)as the shared safety predicate.next_dkg <= MIN_BLOCKS_BEFORE_DKG/6blocks.active_dkgs > 0, inspectquorum dkgstatussession data plus the current block height instead of treating the aggregateactive_dkgscounter as permanently unsafe.llmqTypeandstatus.quorumHeightagainst Dash Core'sdkgMiningWindowStartvalues, so stale sessions no longer block once their active DKG phase has passed.--safecheck and the--safewait path to use the shared predicate.--safefail-closed: it only proceeds when Core reports a safe state.active_dkgs === 0;10even ifactive_dkgsis still non-zero;llmq_400_60still blocking at offset15;waitForDKGWindowPasswaiting through an active session and resolving once the session reaches the safe window.How Has This Been Tested?
node --input-type=moduleagainst the changed dashmate modules:llmq_test_platformsession offset5is unsafe;llmq_test_platformsession offset10is safe;llmq_400_60session offset15is unsafe and offset20is safe;node --checkon all changed source/test files.git diff --check upstream/v3.1-dev..HEADship.Formal Mocha/eslint commands were attempted but are currently blocked in this environment before repo code executes by Node 25 + Yarn PnP loader failure:
Blocked commands:
Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit