feat(rpc): expose upcoming DKG participation#7404
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds work-block-based quorum member prediction helpers in Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant A
participant B
A->>B: observable interaction
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 (1)
src/llmq/utils.cpp (1)
582-585: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtract the shared V20 modifier logic. The V20 branch here duplicates
GetHashModifier; sharing it would keep the prediction path from drifting if the selection rules change.🤖 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 `@src/llmq/utils.cpp` around lines 582 - 585, The V20 prediction path in GetV20HashModifier duplicates the same modifier-selection logic used by GetHashModifier, so extract that shared logic into a common helper and have both paths call it. Keep the helper responsible for selecting the correct quorum base block hash / modifier source based on the deployment state, then reuse it from the V20 branch and the existing modifier calculation to prevent drift if the selection rules change.
🤖 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 `@test/functional/feature_llmq_rotation.py`:
- Around line 265-266: The `assert_raises_rpc_error` call in
`feature_llmq_rotation.py` has a continuation line that is over-indented,
triggering flake8 E127. Update the indentation in the
`assert_raises_rpc_error(...)` invocation so the wrapped arguments align
consistently with the opening parenthesis, keeping the formatting around
`active_mn.get_node(self).quorum` and its following arguments compliant.
---
Nitpick comments:
In `@src/llmq/utils.cpp`:
- Around line 582-585: The V20 prediction path in GetV20HashModifier duplicates
the same modifier-selection logic used by GetHashModifier, so extract that
shared logic into a common helper and have both paths call it. Keep the helper
responsible for selecting the correct quorum base block hash / modifier source
based on the deployment state, then reuse it from the V20 branch and the
existing modifier calculation to prevent drift if the selection rules change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc1011c1-247c-4fe7-9019-f3ac675e4b51
📒 Files selected for processing (4)
src/llmq/utils.cppsrc/llmq/utils.hsrc/rpc/quorums.cpptest/functional/feature_llmq_rotation.py
thepastaclaw
force-pushed
the
rpc-dkg-scheduled-participation
branch
from
a49287a to
0b83dc4
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/functional/feature_llmq_evo.py (1)
89-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded DKG interval/lookahead values couple test to RPC internals.
dkg_interval = 24(Line 89) and the lookahead of8passed toquorum("dkginfo", ...)(Line 100) mirrorllmq_params.dkgIntervalandllmq::WORK_DIFF_DEPTHinsrc/rpc/quorums.cpp. If either constant changes forllmq_test_platform, this test will silently miscomputeexpected_quorum_height/blocks_to_mineand become flaky rather than failing clearly. Consider omitting thelookahead_blocksparam to rely on the RPC's own default (WORK_DIFF_DEPTH), and/or sourcingdkg_intervalfromself.nodes[0].quorum("list", ...)params or an existing test helper if one exists.🤖 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 `@test/functional/feature_llmq_evo.py` around lines 89 - 100, The test currently hardcodes the DKG interval and lookahead, which ties feature_llmq_evo to rpc/quorums.cpp internals and can silently drift if llmq_test_platform settings change. Update the setup around getblockcount/expected_quorum_height to derive dkg_interval from the quorum config (for example via self.nodes[0].quorum("list", ... ) or an existing helper) instead of using 24. Also remove the explicit 8 passed to quorum("dkginfo", ...) so the RPC default lookahead is used, or centralize that value through the same shared source.
🤖 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.
Nitpick comments:
In `@test/functional/feature_llmq_evo.py`:
- Around line 89-100: The test currently hardcodes the DKG interval and
lookahead, which ties feature_llmq_evo to rpc/quorums.cpp internals and can
silently drift if llmq_test_platform settings change. Update the setup around
getblockcount/expected_quorum_height to derive dkg_interval from the quorum
config (for example via self.nodes[0].quorum("list", ... ) or an existing
helper) instead of using 24. Also remove the explicit 8 passed to
quorum("dkginfo", ...) so the RPC default lookahead is used, or centralize that
value through the same shared source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d067e03f-d086-4f92-998a-6619ea22f00f
📒 Files selected for processing (5)
src/llmq/utils.cppsrc/llmq/utils.hsrc/rpc/quorums.cpptest/functional/feature_llmq_evo.pytest/functional/feature_llmq_rotation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/llmq/utils.h
- src/rpc/quorums.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
rpc-dkg-scheduled-participation
branch
from
b3e4105 to
aa15b30
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit f26c8be) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/quorums.cpp (1)
1058-1117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
rotation_enabledconsistently here.GetEnabledQuorumTypes()only says the quorum type is active; the current-cycle rotation state can still differ, soworkHeightand the member-selection branch should followrotation_enabledinstead ofllmq_params.useRotation.🤖 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 `@src/rpc/quorums.cpp` around lines 1058 - 1117, Use rotation_enabled consistently in this quorum-upcoming calculation path. In the logic inside the loop over GetEnabledQuorumTypes, compute workHeight and choose the member-selection branch based on rotation_enabled rather than llmq_params.useRotation, so the current-cycle rotation state matches the same condition already used to set quorums_num. Update the related workHeight calculation and the if/else that calls ComputeRotatedQuorumMembersFromWorkBlock versus ComputeNonRotatedQuorumMembersFromWorkBlock accordingly.
🧹 Nitpick comments (2)
src/rpc/quorums.cpp (1)
1035-1036: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRaw dereference of
pindexTip->nHeight.
pindexTipis taken fromchainman.ActiveChain().Tip()and immediately dereferenced. If the active chain tip is ever null (e.g., extremely early startup edge case), this is a null-pointer dereference. The prior implementation reportedly derived height via a chain accessor rather than dereferencing the tip directly. Low real-world likelihood since the RPC framework guards on active/observer context being available, but a defensive guard is cheap.🤖 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 `@src/rpc/quorums.cpp` around lines 1035 - 1036, The `GetQuorumInfo` path is dereferencing `pindexTip->nHeight` immediately after fetching `chainman.ActiveChain().Tip()`, so add a defensive null check before using the tip. Update the logic around `pindexTip`/`nTipHeight` to handle a null active-chain tip safely, preferably by using an accessor-based height lookup or by returning an appropriate early error when `Tip()` is absent.test/functional/feature_llmq_rotation.py (1)
275-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate hardcoded
8(work-diff depth) across multiple test sections.The value mirroring
llmq::WORK_DIFF_DEPTHis hardcoded independently at Lines 111, 284, 310-311, and 328-329. Consider hoisting it into a single local/class constant (e.g.,self.work_diff_depth = 8set once inrun_test) to avoid drift if the depth changes.🤖 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 `@test/functional/feature_llmq_rotation.py` around lines 275 - 334, The test is duplicating the work-diff depth value in multiple assertions and calculations, which risks drift if the depth changes. Hoist the shared `8` used for the `llmq::WORK_DIFF_DEPTH`-based timing into a single constant or `run_test`-level field, then reuse it throughout `feature_llmq_rotation.py` in the `dkginfo` checks. Update the calculations for `blocks_to_mine`, `workBlockHeight`, and related expected heights to reference that shared symbol instead of repeating the literal. Use a clear identifier like `work_diff_depth` so the values in the `dkginfo` test stay consistent.
🤖 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.
Outside diff comments:
In `@src/rpc/quorums.cpp`:
- Around line 1058-1117: Use rotation_enabled consistently in this
quorum-upcoming calculation path. In the logic inside the loop over
GetEnabledQuorumTypes, compute workHeight and choose the member-selection branch
based on rotation_enabled rather than llmq_params.useRotation, so the
current-cycle rotation state matches the same condition already used to set
quorums_num. Update the related workHeight calculation and the if/else that
calls ComputeRotatedQuorumMembersFromWorkBlock versus
ComputeNonRotatedQuorumMembersFromWorkBlock accordingly.
---
Nitpick comments:
In `@src/rpc/quorums.cpp`:
- Around line 1035-1036: The `GetQuorumInfo` path is dereferencing
`pindexTip->nHeight` immediately after fetching `chainman.ActiveChain().Tip()`,
so add a defensive null check before using the tip. Update the logic around
`pindexTip`/`nTipHeight` to handle a null active-chain tip safely, preferably by
using an accessor-based height lookup or by returning an appropriate early error
when `Tip()` is absent.
In `@test/functional/feature_llmq_rotation.py`:
- Around line 275-334: The test is duplicating the work-diff depth value in
multiple assertions and calculations, which risks drift if the depth changes.
Hoist the shared `8` used for the `llmq::WORK_DIFF_DEPTH`-based timing into a
single constant or `run_test`-level field, then reuse it throughout
`feature_llmq_rotation.py` in the `dkginfo` checks. Update the calculations for
`blocks_to_mine`, `workBlockHeight`, and related expected heights to reference
that shared symbol instead of repeating the literal. Use a clear identifier like
`work_diff_depth` so the values in the `dkginfo` test stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 07160cc2-1cd5-4d7b-b27f-a64e7308fd91
📒 Files selected for processing (3)
src/rpc/quorums.cpptest/functional/feature_llmq_evo.pytest/functional/feature_llmq_rotation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/feature_llmq_evo.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR is generally sound and its refactor preserves the real quorum-formation path, but there is one in-scope correctness bug in the RPC filter: rotated cycles share one work block, yet the filter uses per-quorumHeight distance, dropping every quorumIndex>=1 entry the moment the tip enters the work-diff window — silently defeating the RPC's stated purpose for later rotated slots. Two lower-severity notes: the prediction path evaluates DEPLOYMENT_V19/EvoOnly against the work block rather than the future quorum base block (dormant today), and the commit stack contains a same-PR add/undo of the lookahead_blocks RPC parameter that should be folded before merge.
🔴 1 blocking | 🟡 2 suggestion(s)
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history.
🤖 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 `src/rpc/quorums.cpp`:
- [BLOCKING] src/rpc/quorums.cpp:1070-1072: Rotated quorumIndex >= 1 entries are dropped even when their work block is mined
The eligibility filter `quorumHeight - nTipHeight > llmq::WORK_DIFF_DEPTH` gates on the individual quorum's start height, but for rotated quorums every quorumIndex in a cycle shares the same work block at `cycleBaseHeight - WORK_DIFF_DEPTH`. As soon as that work block is mined, `ComputeRotatedQuorumMembersFromWorkBlock` can predict all indices 0..signingActiveQuorumCount-1 at once, yet this filter rejects indices 1..N-1 because their `blocksUntilStart` is `WORK_DIFF_DEPTH + quorumIndex`, exceeding the threshold. Concrete case: with `dkgInterval=24`, `WORK_DIFF_DEPTH=8`, `signingActiveQuorumCount=4` and tip at `cycleBase-8`, only quorumIndex=0 is reported; quorumIndex=1,2,3 (blocksUntilStart=9,10,11) are silently dropped even though they would all succeed the subsequent `workHeight > nTipHeight` check. This defeats the RPC's stated purpose (dashmate restart-window planning): a masternode whose only upcoming slot is a later quorumIndex will see no entry and may restart into an active DKG. The RPC docstring — 'sessions starting within the work-diff depth' — also conflates the two conditions, which are only equivalent for non-rotated quorums. Gate the pre-filter on the work block rather than on quorumHeight for rotated types; the existing `workHeight`/tip check already handles the truly-unavailable case, so the redundant guard can be tightened or dropped. Extend `feature_llmq_rotation.py` to compare prediction against at least one non-zero quorumIndex against the mined rotated quorum.
In `src/llmq/utils.cpp`:
- [SUGGESTION] src/llmq/utils.cpp:607-617: Deployment gates evaluated against the work block, not the future quorum base
In the real path, `ComputeQuorumMembers` and `ComputeQuorumMembersByQuarterRotation` evaluate `DEPLOYMENT_V19` (for `EvoOnly` and `skipRemovedMNs`) against the quorum's own base block. The prediction path evaluates the same gates against `pWorkBlockIndex`, which is `WORK_DIFF_DEPTH` blocks earlier: `ComputeNonRotatedQuorumMembersFromWorkBlock` uses `pWorkBlockIndex` directly for `EvoOnly`, and `ComputeRotatedQuorumMembersFromWorkBlock` passes `pDeploymentIndex=pWorkBlockIndex` into `BuildNewQuorumQuarterMembers`, where `skipRemovedMNs` is derived. If a future deployment gating member selection activates within that 8-block window, prediction will diverge from the actual mined quorum's membership. In practice this is dormant — V19 is long since active on all real networks before V20 (which itself gates prediction). Either document the constraint explicitly or bail out of prediction when the relevant deployment isn't yet active at the projected quorum base height.
In `<commit:f455571aea>`:
- [SUGGESTION] <commit:f455571aea>:1: Squash the transient `lookahead_blocks` add-then-remove into the introducing commit
Commit `0b83dc4895` introduces a `lookahead_blocks` optional RPC arg (RPCArg entry, `lookaheadBlocks` local, negative-value validation, and a functional-test `assert_raises_rpc_error` for the negative case). Commit `f455571aea` deletes every trace of it and replaces the threshold with `llmq::WORK_DIFF_DEPTH`. Because Dash Core merges the stack as-is (no squash on merge), this leaves permanent `git log`/`git blame` churn for an RPC parameter that never shipped, and adds a needless bisect step where the RPC surface changes shape mid-stack. Interactive-rebase the `f455571aea` hunks into `0b83dc4895` so the final history introduces the RPC in its final, parameter-less form. Two feature commits (`expose upcoming DKG participation`, `predict rotated DKG participation`) are the clean landing shape.
| if (quorumHeight <= nTipHeight || quorumHeight - nTipHeight > llmq::WORK_DIFF_DEPTH) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Rotated quorumIndex >= 1 entries are dropped even when their work block is mined
The eligibility filter quorumHeight - nTipHeight > llmq::WORK_DIFF_DEPTH gates on the individual quorum's start height, but for rotated quorums every quorumIndex in a cycle shares the same work block at cycleBaseHeight - WORK_DIFF_DEPTH. As soon as that work block is mined, ComputeRotatedQuorumMembersFromWorkBlock can predict all indices 0..signingActiveQuorumCount-1 at once, yet this filter rejects indices 1..N-1 because their blocksUntilStart is WORK_DIFF_DEPTH + quorumIndex, exceeding the threshold. Concrete case: with dkgInterval=24, WORK_DIFF_DEPTH=8, signingActiveQuorumCount=4 and tip at cycleBase-8, only quorumIndex=0 is reported; quorumIndex=1,2,3 (blocksUntilStart=9,10,11) are silently dropped even though they would all succeed the subsequent workHeight > nTipHeight check. This defeats the RPC's stated purpose (dashmate restart-window planning): a masternode whose only upcoming slot is a later quorumIndex will see no entry and may restart into an active DKG. The RPC docstring — 'sessions starting within the work-diff depth' — also conflates the two conditions, which are only equivalent for non-rotated quorums. Gate the pre-filter on the work block rather than on quorumHeight for rotated types; the existing workHeight/tip check already handles the truly-unavailable case, so the redundant guard can be tightened or dropped. Extend feature_llmq_rotation.py to compare prediction against at least one non-zero quorumIndex against the mined rotated quorum.
| if (quorumHeight <= nTipHeight || quorumHeight - nTipHeight > llmq::WORK_DIFF_DEPTH) { | |
| continue; | |
| } | |
| const int cycleBaseHeight{quorumHeight - quorumIndex}; | |
| const int workHeight{(llmq_params.useRotation ? cycleBaseHeight : quorumHeight) - llmq::WORK_DIFF_DEPTH}; | |
| if (quorumHeight <= nTipHeight || workHeight > nTipHeight) { | |
| continue; | |
| } |
source: ['claude', 'codex']
| if (!DeploymentActiveAfter(pWorkBlockIndex.get(), chainparams.GetConsensus(), Consensus::DEPLOYMENT_V20)) { | ||
| // pre-v20 modifier calculation needs the future quorum base block hash, which isn't known yet. | ||
| return {}; | ||
| } | ||
|
|
||
| bool EvoOnly = (chainparams.GetConsensus().llmqTypePlatform == llmqType) && | ||
| DeploymentActiveAfter(pWorkBlockIndex.get(), chainparams.GetConsensus(), Consensus::DEPLOYMENT_V19); | ||
|
|
||
| const auto modifier = GetHashModifierFromWorkBlock(llmq_params, pWorkBlockIndex.get()); | ||
|
|
||
| return CalculateQuorum(mn_list, modifier, llmq_params.size, EvoOnly); |
There was a problem hiding this comment.
🟡 Suggestion: Deployment gates evaluated against the work block, not the future quorum base
In the real path, ComputeQuorumMembers and ComputeQuorumMembersByQuarterRotation evaluate DEPLOYMENT_V19 (for EvoOnly and skipRemovedMNs) against the quorum's own base block. The prediction path evaluates the same gates against pWorkBlockIndex, which is WORK_DIFF_DEPTH blocks earlier: ComputeNonRotatedQuorumMembersFromWorkBlock uses pWorkBlockIndex directly for EvoOnly, and ComputeRotatedQuorumMembersFromWorkBlock passes pDeploymentIndex=pWorkBlockIndex into BuildNewQuorumQuarterMembers, where skipRemovedMNs is derived. If a future deployment gating member selection activates within that 8-block window, prediction will diverge from the actual mined quorum's membership. In practice this is dormant — V19 is long since active on all real networks before V20 (which itself gates prediction). Either document the constraint explicitly or bail out of prediction when the relevant deployment isn't yet active at the projected quorum base height.
source: ['claude']
There was a problem hiding this comment.
Correction: this prior finding is still valid at 9b71d560, not resolved. The current top-level review carries it forward under "Carried-forward prior findings"; leaving this thread open.
Repair note: the automated reconciliation step incorrectly emitted a resolved reply for this thread even though the verifier classified prior-1 as STILL VALID.
There was a problem hiding this comment.
Still valid at f26c8bea; the new review carries this forward as the remaining suggestion. The stack rewrite fixed the commit-history findings, but this deployment-gate prediction/production divergence is unchanged.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94eb4ad9f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (quorumHeight <= nTipHeight) { | ||
| quorumHeight += llmq_params.dkgInterval; | ||
| } | ||
| if (quorumHeight <= nTipHeight || quorumHeight - nTipHeight > llmq::WORK_DIFF_DEPTH) { |
There was a problem hiding this comment.
For rotated quorums this filters by quorumHeight - nTipHeight, but their work block is cycleBaseHeight - WORK_DIFF_DEPTH, independent of quorumIndex. On mainnet llmq_60_75 has 32 rotated indexes while WORK_DIFF_DEPTH is 8, so at cycleBaseHeight - 8 the work block for all 32 upcoming DKGs is already mined, yet this RPC only reports index 0 and later only indexes within 8 blocks of starting. That makes upcoming_dkgs incomplete for determinable rotated DKG participation; the availability check should be based on the computed workHeight <= nTipHeight instead.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| bool EvoOnly = (chainparams.GetConsensus().llmqTypePlatform == llmqType) && | ||
| DeploymentActiveAfter(pWorkBlockIndex.get(), chainparams.GetConsensus(), Consensus::DEPLOYMENT_V19); |
There was a problem hiding this comment.
When predicting platform quorums in the V19 activation window, this checks Evo-only selection against the work block, but the normal path in ComputeQuorumMembers checks DeploymentActiveAfter(pQuorumBaseBlockIndex, ..., V19). If V19 activates during the 8-block gap between the work block and the upcoming quorum height, dkginfo will rank regular masternodes too and can report a membership set that differs from the quorum that is actually mined.
Useful? React with 👍 / 👎.
| auto newQuarterMembers = BuildNewQuorumQuarterMembers(llmq_params, util_params, allMns, previousQuarters, | ||
| modifier, pWorkBlockIndex, cycleBaseHeight, |
There was a problem hiding this comment.
For rotated predictions this passes the work block as the deployment index used by BuildNewQuorumQuarterMembers to decide skipRemovedMNs, while the production path passes the cycle base block. During the first cycle where V19 is active at the future cycle base but not at cycleBase - WORK_DIFF_DEPTH, removed previous-quarter MNs are retained in the prediction even though the mined quorum skips them, so upcoming_dkgs can report the wrong rotated membership.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
The latest delta (94eb4ad) correctly aligns the rotated/non-rotated dispatch with DIP-0024 activation state by using tip-evaluated rotation_enabled in place of the static useRotation flag and adds a CHECK_NONFATAL guard on the active tip. However, prior-1's blocking bug in the eligibility pre-filter at src/rpc/quorums.cpp:1070 was not addressed and continues to silently drop every rotated quorumIndex >= 1 slot for quorumIndex blocks after the shared cycle work block is mined — defeating the RPC's stated purpose for later rotated slots. Two lower-severity carried-forward items (deployment gates evaluated against the work block instead of the future quorum base, and pre-merge commit-stack hygiene) also remain.
Blocking: 1 | Suggestions: 3 | Nitpicks: 0
Carried-forward prior findings still valid at 94eb4ad9: prior-1 blocker in src/rpc/quorums.cpp, prior-2 deployment-gate suggestion in src/llmq/utils.cpp, and prior-3 commit-history suggestion for f455571a.
New finding in the latest delta: commit-history suggestion to fold 94eb4ad9f8 into the commits it patches.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking finding.
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 `src/rpc/quorums.cpp`:
- [BLOCKING] src/rpc/quorums.cpp:1066: Rotated quorumIndex >= 1 entries are dropped even when the shared cycle work block is mined
STILL VALID — carried forward from the prior review at f455571a; the 94eb4ad9 delta only renamed `useRotation` to `rotation_enabled` on lines 1083/1100 and did not touch this pre-filter.
For rotated quorums the whole cycle shares a single work block at `cycleBaseHeight - WORK_DIFF_DEPTH`, so as soon as that shared work block is mined `ComputeRotatedQuorumMembersFromWorkBlock` can predict every `quorumIndex` in `0..signingActiveQuorumCount - 1`. But this filter gates on per-quorum distance `quorumHeight - nTipHeight > WORK_DIFF_DEPTH` where `quorumHeight = cycleBaseHeight + quorumIndex`, so later indexes are rejected for `quorumIndex` additional blocks.
Concrete case (`dkgInterval=24`, `WORK_DIFF_DEPTH=8`, `signingActiveQuorumCount=4`, tip=16 — the cycle=24 work block at height 16 has just been mined):
- `quorumIndex=0`: `blocksUntilStart=8` → passes → reported.
- `quorumIndex=1`: `blocksUntilStart=9` → dropped (even though the shared work block is mined).
- `quorumIndex=2,3`: dropped for even more blocks.
This silently defeats the RPC's stated purpose (dashmate restart-window planning): a masternode whose only upcoming slot is a later `quorumIndex` sees no entry at all in `upcoming_dkgs` for that cycle and can be restarted straight into an already-determinable DKG window. `feature_llmq_rotation.py`'s new assertions only check `quorumIndex == 0` and therefore do not exercise the miss — extending the test to cover `quorumIndex >= 1` immediately after the cycle work block is mined would reproduce it.
Fix: filter by the height that actually governs work-block availability (`cycleBaseHeight` for rotated, `quorumHeight` otherwise). See suggestion.
Suggested patch:
const int cycleBaseHeight{quorumHeight - quorumIndex};
const int workHeight{(rotation_enabled ? cycleBaseHeight : quorumHeight) - llmq::WORK_DIFF_DEPTH};
if (quorumHeight <= nTipHeight || workHeight > nTipHeight) {
continue;
}
In `src/llmq/utils.cpp`:
- [SUGGESTION] src/llmq/utils.cpp:607: Deployment gates evaluated against the work block, not the future quorum base
STILL VALID but currently dormant — carried forward from the prior review. In the real (non-predictive) path, `ComputeQuorumMembers` derives `EvoOnly` from `DeploymentActiveAfter` against the quorum's own base block. Here, `ComputeNonRotatedQuorumMembersFromWorkBlock` evaluates the V20 gate and the DEPLOYMENT_V19 `EvoOnly` flag against `pWorkBlockIndex`, and `ComputeRotatedQuorumMembersFromWorkBlock` passes `pDeploymentIndex=pWorkBlockIndex` into `BuildNewQuorumQuarterMembers` where `skipRemovedMNs` is derived — both `WORK_DIFF_DEPTH` blocks earlier than the true future quorum/cycle base height.
Currently dormant because (1) V19 activates before V20 in Dash's deployment order, so any block past V20 is also past V19, and (2) the RPC bails out with `known=false` when V20 is not active at the work block. But if a future deployment gating member selection activates inside the WORK_DIFF_DEPTH window, the prediction will diverge from the actual mined quorum. Not required for this PR; worth a comment or aligning the check with the real path for correctness clarity.
In `<commit:f455571aea>`:
- [SUGGESTION] <commit:f455571aea>:1: Squash the transient `lookahead_blocks` add-then-remove into the introducing commit
STILL VALID — carried forward. Commit 0b83dc4895 introduces the `lookahead_blocks` RPC parameter (RPCArg, validation for negative values, RPC help, functional-test coverage), and commit f455571aea two commits later removes every trace of it and replaces the threshold with `llmq::WORK_DIFF_DEPTH`. Because dashpay/dash preserves PR commit history at merge time, this leaves permanent `git log`/`git blame` churn for a public RPC parameter that never shipped. Fold f455571aea's reversion back into 0b83dc4895 via interactive rebase so the introducing commit reflects the final parameter-less shape directly.
In `<commit:94eb4ad9f8>`:
- [SUGGESTION] <commit:94eb4ad9f8>:1: Fold the `rotation_enabled` / `CHECK_NONFATAL` fixup into the commits it patches
Commit 94eb4ad9f8 is a 3-line corrective follow-up to code introduced earlier in the same unmerged PR: it swaps `llmq_params.useRotation` for the tip-evaluated `rotation_enabled` on lines 1083/1100 (rotation-selection branches introduced by aa15b305f3 and 0b83dc4895) and hardens the tip lookup added by 0b83dc4895 with `CHECK_NONFATAL`. As permanent Dash Core history, this is the canonical 'commit A introduces logic, commit B in the same PR fixes a bug in it before it ever ships' pattern the project's no-squash-merge convention is most sensitive to: `git bisect` between aa15b305f3 and 94eb4ad9f8 lands on a state where rotated prediction ignores DIP-0024 activation state. Squash the `rotation_enabled` change into aa15b305f3 (and 0b83dc4895 for the non-rotated site) and the `CHECK_NONFATAL` change into 0b83dc4895 so each commit in the merged stack is individually correct.
There was a problem hiding this comment.
Code Review
The blocking bug from the prior review (prior-1: rotated quorumIndex>=1 dropped by the availability filter) is fixed at head 783ca1d by computing workHeight from cycleBaseHeight when rotation is enabled, and functional coverage was extended to assert both rotated indices per cycle. Remaining items are all carried forward suggestions: one dormant code-logic concern in src/llmq/utils.cpp (deployment gates evaluated against the work block, not the future quorum base) and three commit-history hygiene items — the transient lookahead_blocks add/remove, the rotation_enabled/CHECK_NONFATAL fixup, and now this new same-PR fix commit itself patching aa15b30.
🟡 4 suggestion(s)
Prior finding reconciliation:
- prior-1: FIXED — Verified in src/rpc/quorums.cpp at lines 1066-1076: workHeight is derived from cycleBaseHeight when rotation_enabled, and the pre-filter now uses that shared value, so rotated indices >= 1 are reported once the shared cycle work block is mined. Functional coverage extended for both rotated indices.
- prior-2: STILL VALID — src/llmq/utils.cpp lines 607-613 and 647-681 still evaluate DEPLOYMENT_V19/V20 and derive skipRemovedMNs from pWorkBlockIndex rather than the future quorum/cycle base block. Untouched by the latest delta. Currently dormant but retained as a low-confidence suggestion.
- prior-3: STILL VALID — Commit stack still contains 0b83dc4 introducing the lookahead_blocks RPCArg and f455571 two commits later removing every trace of it. Add-then-remove pair for a public RPC parameter that never ships.
- prior-4: STILL VALID — 94eb4ad remains a standalone corrective commit patching aa15b30/0b83dc4895 (rotation_enabled dispatch + CHECK_NONFATAL). The latest push added a further same-PR fixup (783ca1d) on the same code path, compounding the hygiene concern; captured as an additional finding.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history.
🤖 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 `src/llmq/utils.cpp`:
- [SUGGESTION] src/llmq/utils.cpp:607: Deployment gates evaluated against the work block, not the future quorum base
Carried forward from the prior review — dormant but unresolved. ComputeNonRotatedQuorumMembersFromWorkBlock evaluates both the DEPLOYMENT_V20 gate and the EvoOnly (DEPLOYMENT_V19) flag against pWorkBlockIndex; ComputeRotatedQuorumMembersFromWorkBlock similarly threads pWorkBlockIndex through BuildNewQuorumQuarterMembers where skipRemovedMNs is derived. The canonical production selection path evaluates DEPLOYMENT_V19 against the future quorum/cycle base block instead, which is WORK_DIFF_DEPTH blocks later. Currently harmless because V19 activates before V20 in Dash's deployment order and the RPC already bails with known=false when V20 is not active at the work block, but this leaves the prediction algorithm subtly divergent from the real member-selection algorithm if a future deployment gates member selection and activates inside the WORK_DIFF_DEPTH window. Worth aligning with the canonical path or documenting the deliberate divergence.
In `<commit:f455571aea>`:
- [SUGGESTION] <commit:f455571aea>:1: Squash the transient lookahead_blocks add-then-remove into the introducing commit
Commit 0b83dc4895 introduces the lookahead_blocks RPCArg, its help text, negative-value validation, and functional-test coverage; commit f455571aea removes every trace of that parameter two commits later, replacing the threshold with llmq::WORK_DIFF_DEPTH. Since dashpay/dash preserves PR commit history at merge time, the add-then-remove pair for a public RPC surface that never shipped becomes permanent noise in git log and git blame. Fold f455571aea back into 0b83dc4895 via interactive rebase so the introducing commit lands with the final parameter-less shape.
In `<commit:94eb4ad9f8>`:
- [SUGGESTION] <commit:94eb4ad9f8>:1: Fold the rotation_enabled / CHECK_NONFATAL fixup into the commits it patches
94eb4ad9f8 is a 3-line corrective follow-up to logic introduced earlier in the same unmerged PR: it replaces static useRotation checks with tip-evaluated rotation_enabled and adds CHECK_NONFATAL around the active tip lookup. Landed as a standalone commit, this leaves the merged stack with an intermediate state where rotated dkginfo prediction ignores DIP-0024 activation state — a bisect hazard on LLMQ/DKG behavior. Squash the rotation_enabled hunk into aa15b305f3 and the CHECK_NONFATAL hunk into 0b83dc4895 so each commit in the final stack is individually correct.
In `<commit:783ca1d1ef>`:
- [SUGGESTION] <commit:783ca1d1ef>:1: Fold the rotated-index work-block fix into the commit that introduced the bug
This new top commit fixes the rotated pre-filter by gating on the shared cycle-base work block and extends feature_llmq_rotation.py to cover both rotated indices — the right final behavior. However, the bug it fixes was introduced earlier in the same unmerged PR by aa15b305f3 ('rpc: predict rotated DKG participation'). Since the PR merges without squashing, a git bisect landing between aa15b305f3 and 783ca1d1ef would find rotated quorumIndex>=1 prediction silently broken. This is the same pattern already flagged for 94eb4ad9f8 and f455571aea — the fixup chain on this file now compounds. Squash 783ca1d1ef into aa15b305f3 (and fold its test extension into whichever commit first added rotated coverage) so each commit is individually correct.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
Latest delta (9b71d56) is a correct, small refactor that separates CHECK_NONFATAL from the WITH_LOCK(cs_main, ...) lambda for the active-chain-tip lookup — no new runtime defects. However, it is now the fourth same-PR corrective commit patching earlier commits in this unmerged stack (f455571, 94eb4ad, 783ca1d, 9b71d56), all touching src/rpc/quorums.cpp. Under Dash's no-squash merge policy each fixup becomes permanent bisect noise on a subsystem (LLMQ/DKG) where reviewers rely on individually-correct commits. Recommend an autosquash rebase before merge.
5 suggestion(s)
Prior finding reconciliation:
- prior-1: STILL VALID - src/llmq/utils.cpp unchanged by the latest delta. Deployment gates still evaluate DEPLOYMENT_V20 / EvoOnly against pWorkBlockIndex rather than the future quorum/cycle base block. Dormant, low-severity. Carried forward as a finding.
- prior-2: STILL VALID - git log develop..HEAD confirms 0b83dc4 (introducing lookahead_blocks) and f455571 (removing it) both remain unsquashed in the stack. Carried forward as a finding.
- prior-3: STILL VALID (and compounded) - 94eb4ad remains a standalone corrective commit patching aa15b30/0b83dc4895 (rotation_enabled dispatch + CHECK_NONFATAL). The latest push (9b71d56) further patches 94eb4ad's CHECK_NONFATAL placement, extending the fixup chain. Carried forward as a finding; new fixup captured as a separate new finding.
- prior-4: STILL VALID - 783ca1d remains a standalone fixup commit on top of aa15b30. Confirmed fix at src/rpc/quorums.cpp:1071-1077 (cycleBaseHeight/workHeight gating). Carried forward as a finding.
Carried-forward prior findings:
- [SUGGESTION] commit:f455571aea:1: Carried forward: squash the transient lookahead_blocks add-then-remove into the introducing commit
STILL VALID from prior-2. Commit 0b83dc4 introduces thelookahead_blocksRPCArg forquorum dkginfowith help text, negative-value validation, and functional-test coverage; commit f455571 two commits later removes every trace of that public parameter, replacing the threshold withllmq::WORK_DIFF_DEPTH. Since dashpay/dash preserves PR commit history at merge time, the add-then-remove pair for an RPC surface that never shipped becomes permanent noise ingit log/git blame, and any consumer bisecting between the two commits sees an RPC parameter that never existed on develop. Fold f455571 back into 0b83dc4 via interactive rebase so the introducing commit lands with the final parameter-less shape. - [SUGGESTION] commit:94eb4ad9f8:1: Carried forward: fold the rotation_enabled / CHECK_NONFATAL fixup into the commits it patches
STILL VALID from prior-3. 94eb4ad is a small corrective follow-up to logic introduced earlier in this same unmerged PR: it (a) replaces a staticuseRotationcheck with tip-evaluatedIsQuorumRotationEnabledin the rotated-prediction dispatch (fixing logic from aa15b30) and (b) hardens the active tip lookup withCHECK_NONFATAL(fixing 0b83dc4 — itself further patched by 9b71d56, see finding above). Landed standalone, this leaves the merged stack with an intermediate state where rotated dkginfo prediction ignores DIP-0024 activation state — a bisect hazard on LLMQ/DKG behavior. Squash the rotation_enabled hunk into aa15b30 and the CHECK_NONFATAL hunk into 0b83dc4 so each commit in the final stack is individually correct. - [SUGGESTION] commit:783ca1d1ef:1: Carried forward: fold the rotated-index work-block fix into aa15b30
STILL VALID from prior-4. 783ca1d fixes the rotated pre-filter to gate on the shared cycle-base work block (verified fixed at src/rpc/quorums.cpp:1071-1077) and extendsfeature_llmq_rotation.pyto cover both rotated indices — the right final behavior. But the bug it fixes was introduced earlier in this same unmerged PR by aa15b30 ('rpc: predict rotated DKG participation'). Because the PR merges without squashing, agit bisectbetween aa15b30 and 783ca1d would find rotatedquorumIndex >= 1prediction silently broken. Squash 783ca1d into aa15b30 (and fold its test extension into whichever commit first added rotated coverage) so the rotated-prediction feature commit is individually correct on develop. - [SUGGESTION] src/llmq/utils.cpp:607: Carried forward (dormant): deployment gates evaluated against the work block, not the future quorum base
STILL VALID from prior-1. Unchanged by the latest delta (9b71d56 touches only src/rpc/quorums.cpp).ComputeNonRotatedQuorumMembersFromWorkBlockevaluates theDEPLOYMENT_V20gate and theEvoOnly(DEPLOYMENT_V19) flag againstpWorkBlockIndex;ComputeRotatedQuorumMembersFromWorkBlockthreadspWorkBlockIndexintoBuildNewQuorumQuarterMemberswhereskipRemovedMNs-style gating is derived. The canonical non-predictive member-selection path evaluates these gates against the quorum's own future base block, which isWORK_DIFF_DEPTHblocks later than the work block used here. Currently dormant because V19 activates before V20 in Dash's deployment order and the RPC bails withknown=falsewhen V20 is not active at the work block — but the prediction algorithm remains subtly divergent from the real selection algorithm if a future deployment gate activates inside theWORK_DIFF_DEPTHwindow. Worth aligning or documenting; not required for this PR to merge.
New findings in the latest delta:
- [SUGGESTION] commit:9b71d5609d:1: New: fold the WITH_LOCK/CHECK_NONFATAL fixup into the commit that introduced the pattern
NEW in this delta. 9b71d56 changesWITH_LOCK(cs_main, return CHECK_NONFATAL(chainman.ActiveChain().Tip()))to fetch the tip inside the lock and callCHECK_NONFATAL(pindexTip)afterward (verified at src/rpc/quorums.cpp:1035-1036). The fix itself is worth keeping: WITH_LOCK is adecltype(auto)-returning lambda and CHECK_NONFATAL returns a forwarding reference (T&&), so the previous form deduced an rvalue-reference return type bound to a temporary pointer value — at best a warning, at worst a lifetime hazard. But the exact pattern being fixed was itself added two commits earlier by 94eb4ad in this same unmerged PR, extending the same-PR fixup chain on src/rpc/quorums.cpp to four commits. Because dashpay/dash merges PR stacks without squashing, this leaves permanent history where a bisect landing on 94eb4ad finds a construct the same PR explicitly considered worth changing. Squash 9b71d56's hunk into 94eb4ad (or, once 94eb4ad is folded per the finding below, into 0b83dc4 where pindexTip was originally introduced).
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history.
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 `<commit:9b71d5609d>`:
- [SUGGESTION] <commit:9b71d5609d>:1: New: fold the WITH_LOCK/CHECK_NONFATAL fixup into the commit that introduced the pattern
NEW in this delta. 9b71d5609d changes `WITH_LOCK(cs_main, return CHECK_NONFATAL(chainman.ActiveChain().Tip()))` to fetch the tip inside the lock and call `CHECK_NONFATAL(pindexTip)` afterward (verified at src/rpc/quorums.cpp:1035-1036). The fix itself is worth keeping: WITH_LOCK is a `decltype(auto)`-returning lambda and CHECK_NONFATAL returns a forwarding reference (`T&&`), so the previous form deduced an rvalue-reference return type bound to a temporary pointer value — at best a warning, at worst a lifetime hazard. But the exact pattern being fixed was itself added two commits earlier by 94eb4ad9f8 in this same unmerged PR, extending the same-PR fixup chain on src/rpc/quorums.cpp to four commits. Because dashpay/dash merges PR stacks without squashing, this leaves permanent history where a bisect landing on 94eb4ad9f8 finds a construct the same PR explicitly considered worth changing. Squash 9b71d5609d's hunk into 94eb4ad9f8 (or, once 94eb4ad9f8 is folded per the finding below, into 0b83dc4895 where pindexTip was originally introduced).
In `<commit:f455571aea>`:
- [SUGGESTION] <commit:f455571aea>:1: Carried forward: squash the transient lookahead_blocks add-then-remove into the introducing commit
STILL VALID from prior-2. Commit 0b83dc4895 introduces the `lookahead_blocks` RPCArg for `quorum dkginfo` with help text, negative-value validation, and functional-test coverage; commit f455571aea two commits later removes every trace of that public parameter, replacing the threshold with `llmq::WORK_DIFF_DEPTH`. Since dashpay/dash preserves PR commit history at merge time, the add-then-remove pair for an RPC surface that never shipped becomes permanent noise in `git log`/`git blame`, and any consumer bisecting between the two commits sees an RPC parameter that never existed on develop. Fold f455571aea back into 0b83dc4895 via interactive rebase so the introducing commit lands with the final parameter-less shape.
In `<commit:94eb4ad9f8>`:
- [SUGGESTION] <commit:94eb4ad9f8>:1: Carried forward: fold the rotation_enabled / CHECK_NONFATAL fixup into the commits it patches
STILL VALID from prior-3. 94eb4ad9f8 is a small corrective follow-up to logic introduced earlier in this same unmerged PR: it (a) replaces a static `useRotation` check with tip-evaluated `IsQuorumRotationEnabled` in the rotated-prediction dispatch (fixing logic from aa15b305f3) and (b) hardens the active tip lookup with `CHECK_NONFATAL` (fixing 0b83dc4895 — itself further patched by 9b71d5609d, see finding above). Landed standalone, this leaves the merged stack with an intermediate state where rotated dkginfo prediction ignores DIP-0024 activation state — a bisect hazard on LLMQ/DKG behavior. Squash the rotation_enabled hunk into aa15b305f3 and the CHECK_NONFATAL hunk into 0b83dc4895 so each commit in the final stack is individually correct.
In `<commit:783ca1d1ef>`:
- [SUGGESTION] <commit:783ca1d1ef>:1: Carried forward: fold the rotated-index work-block fix into aa15b305f3
STILL VALID from prior-4. 783ca1d1ef fixes the rotated pre-filter to gate on the shared cycle-base work block (verified fixed at src/rpc/quorums.cpp:1071-1077) and extends `feature_llmq_rotation.py` to cover both rotated indices — the right final behavior. But the bug it fixes was introduced earlier in this same unmerged PR by aa15b305f3 ('rpc: predict rotated DKG participation'). Because the PR merges without squashing, a `git bisect` between aa15b305f3 and 783ca1d1ef would find rotated `quorumIndex >= 1` prediction silently broken. Squash 783ca1d1ef into aa15b305f3 (and fold its test extension into whichever commit first added rotated coverage) so the rotated-prediction feature commit is individually correct on develop.
In `src/llmq/utils.cpp`:
- [SUGGESTION] src/llmq/utils.cpp:607: Carried forward (dormant): deployment gates evaluated against the work block, not the future quorum base
STILL VALID from prior-1. Unchanged by the latest delta (9b71d5609d touches only src/rpc/quorums.cpp). `ComputeNonRotatedQuorumMembersFromWorkBlock` evaluates the `DEPLOYMENT_V20` gate and the `EvoOnly` (`DEPLOYMENT_V19`) flag against `pWorkBlockIndex`; `ComputeRotatedQuorumMembersFromWorkBlock` threads `pWorkBlockIndex` into `BuildNewQuorumQuarterMembers` where `skipRemovedMNs`-style gating is derived. The canonical non-predictive member-selection path evaluates these gates against the quorum's own future base block, which is `WORK_DIFF_DEPTH` blocks later than the work block used here. Currently dormant because V19 activates before V20 in Dash's deployment order and the RPC bails with `known=false` when V20 is not active at the work block — but the prediction algorithm remains subtly divergent from the real selection algorithm if a future deployment gate activates inside the `WORK_DIFF_DEPTH` window. Worth aligning or documenting; not required for this PR to merge.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
rpc-dkg-scheduled-participation
branch
from
9b71d56 to
f26c8be
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f26c8bead8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| bool rotation_enabled = llmq::IsQuorumRotationEnabled(llmq_params, pindexTip); | ||
| int quorums_num = rotation_enabled ? llmq_params.signingActiveQuorumCount : 1; |
There was a problem hiding this comment.
When a useRotation quorum type is enabled by DIP0024 but the tip is still in the previous non-rotated cycle, GetEnabledQuorumTypes() can include that type while IsQuorumRotationEnabled(..., pindexTip) is still false because it checks the tip's current cycle base. In that transition window this sets quorums_num to 1 and later takes the non-rotated helper path, which immediately returns an empty member list for llmq_params.useRotation; the RPC then reports known: true with memberCount: 0 instead of predicting/marking unknown for the upcoming rotated DKG.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
Source: reviewers = claude general opus; codex general gpt-5.5; claude dash-core-commit-history opus; codex dash-core-commit-history gpt-5.5; verifier = claude opus; specialists = dash-core-commit-history.
Cumulative review at f26c8be. The 6-commit fixup stack was squashed into a clean two-commit stack (rpc: expose upcoming DKG participation and rpc: predict rotated DKG participation), resolving all four commit-hygiene priors. The RPC logic itself is sound: non-rotated and rotated V20 prediction paths use the work-block-derived modifier consistently with production, the availability filter correctly gates rotated indices on the shared cycle-base work block, and tests cross-check predicted rotated members. The only carried-forward item is prior-5: the prediction helpers still evaluate deployment gates (DEPLOYMENT_V20, DEPLOYMENT_V19) at the work block rather than the future quorum/cycle base block. This remains dormant under current activation ordering but structurally diverges from the canonical member-selection algorithm.
1 suggestion
Prior finding reconciliation:
- prior-1: FIXED - Squashed into 9ce1c4d; no standalone fixup commit remains.
- prior-2: FIXED - No
lookahead_blockssymbol exists in the current tree; add-then-remove sequence has been squashed away. - prior-3: FIXED - 94eb4ad is no longer in the ancestry; effects folded into the two-commit stack.
- prior-4: FIXED - 783ca1d fixup is no longer standalone; rotated work-block gating lands in f26c8be directly.
- prior-5: STILL VALID - Verified at current head: src/llmq/utils.cpp:607, 612-613, 647, 679-681 still evaluate deployment gates against
pWorkBlockIndexand thread it intoBuildNewQuorumQuarterMembers. Carried forward as the sole remaining finding.
Carried-forward prior findings:
-
[SUGGESTION] src/llmq/utils.cpp:607-681: Prediction helpers gate deployments on the work block, not the future quorum/cycle base
Verified at the assigned head.ComputeNonRotatedQuorumMembersFromWorkBlockevaluates theDEPLOYMENT_V20gate at line 607 and theEvoOnly(DEPLOYMENT_V19) flag at lines 612-613 againstpWorkBlockIndex.get().ComputeRotatedQuorumMembersFromWorkBlockdoes the same for its V20 gate at line 647 and threadspWorkBlockIndexintoBuildNewQuorumQuarterMembersat lines 679-681, whereskipRemovedMNsis derived from that same index viaDEPLOYMENT_V19.The canonical, non-predictive member-selection path (
ComputeQuorumMembersByQuarterRotation,GetHashModifier) evaluates these deployment gates against the future quorum's own base block, which isWORK_DIFF_DEPTH(=8) blocks later than the work block used here. The two paths are guaranteed to agree only when no deployment activation falls inside theWORK_DIFF_DEPTHwindow between the work block and the future quorum base.This is currently dormant because (a) V19 activated well before V20 in Dash's deployment order, (b) the RPC bails with
known=falsewhen V20 is not active at the work block (src/rpc/quorums.cpp:1095-1100), so V19 must also be active whenever prediction proceeds, and (c) no future deployment currently gates member selection. But it leaves the prediction algorithm subtly divergent from the real selection algorithm, and would silently mispredict membership if a future deployment gate activates inside theWORK_DIFF_DEPTHwindow. Consider either aligning with the canonical path (evaluate gates against the future base block, derivable by walking forward from the work block via the tip's ancestor lookup atpWorkBlockIndex->nHeight + WORK_DIFF_DEPTH) or documenting the divergence in the helper comments.
New findings in the latest delta:
- None. The latest push rewrote the stack without changing the final tree; prior-1 through prior-4 are fixed by the rewrite.
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 `src/llmq/utils.cpp`:
- [SUGGESTION] src/llmq/utils.cpp:607-681: Prediction helpers gate deployments on the work block, not the future quorum/cycle base
Verified at the assigned head. `ComputeNonRotatedQuorumMembersFromWorkBlock` evaluates the `DEPLOYMENT_V20` gate at line 607 and the `EvoOnly` (`DEPLOYMENT_V19`) flag at lines 612-613 against `pWorkBlockIndex.get()`. `ComputeRotatedQuorumMembersFromWorkBlock` does the same for its V20 gate at line 647 and threads `pWorkBlockIndex` into `BuildNewQuorumQuarterMembers` at lines 679-681, where `skipRemovedMNs` is derived from that same index via `DEPLOYMENT_V19`.
The canonical, non-predictive member-selection path (`ComputeQuorumMembersByQuarterRotation`, `GetHashModifier`) evaluates these deployment gates against the future quorum's own base block, which is `WORK_DIFF_DEPTH` (=8) blocks later than the work block used here. The two paths are guaranteed to agree only when no deployment activation falls inside the `WORK_DIFF_DEPTH` window between the work block and the future quorum base.
This is currently dormant because (a) V19 activated well before V20 in Dash's deployment order, (b) the RPC bails with `known=false` when V20 is not active at the work block (src/rpc/quorums.cpp:1095-1100), so V19 must also be active whenever prediction proceeds, and (c) no future deployment currently gates member selection. But it leaves the prediction algorithm subtly divergent from the real selection algorithm, and would silently mispredict membership if a future deployment gate activates inside the `WORK_DIFF_DEPTH` window. Consider either aligning with the canonical path (evaluate gates against the future base block, derivable by walking forward from the work block via the tip's ancestor lookup at `pWorkBlockIndex->nHeight + WORK_DIFF_DEPTH`) or documenting the divergence in the helper comments.
| if (!DeploymentActiveAfter(pWorkBlockIndex.get(), chainparams.GetConsensus(), Consensus::DEPLOYMENT_V20)) { | ||
| // pre-v20 modifier calculation needs the future quorum base block hash, which isn't known yet. | ||
| return {}; | ||
| } | ||
|
|
||
| bool EvoOnly = (chainparams.GetConsensus().llmqTypePlatform == llmqType) && | ||
| DeploymentActiveAfter(pWorkBlockIndex.get(), chainparams.GetConsensus(), Consensus::DEPLOYMENT_V19); | ||
|
|
||
| const auto modifier = GetHashModifierFromWorkBlock(llmq_params, pWorkBlockIndex.get()); | ||
|
|
||
| return CalculateQuorum(mn_list, modifier, llmq_params.size, EvoOnly); | ||
| } | ||
|
|
||
| std::optional<std::vector<CDeterministicMNCPtr>> ComputeRotatedQuorumMembersFromWorkBlock( | ||
| Consensus::LLMQType llmqType, const UtilParameters& util_params, | ||
| gsl::not_null<const CBlockIndex*> pWorkBlockIndex, int quorumHeight) | ||
| { | ||
| const auto& llmq_params_opt = util_params.m_chainman.GetParams().GetLLMQ(llmqType); | ||
| if (!llmq_params_opt.has_value()) { | ||
| ASSERT_IF_DEBUG(false); | ||
| return std::nullopt; | ||
| } | ||
| const auto& llmq_params = llmq_params_opt.value(); | ||
| if (!llmq_params.useRotation) { | ||
| ASSERT_IF_DEBUG(false); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| const int quorumIndex{quorumHeight % llmq_params.dkgInterval}; | ||
| if (quorumIndex < 0 || quorumIndex >= llmq_params.signingActiveQuorumCount) { | ||
| ASSERT_IF_DEBUG(false); | ||
| return std::nullopt; | ||
| } | ||
| const int cycleBaseHeight{quorumHeight - quorumIndex}; | ||
| if (cycleBaseHeight % llmq_params.dkgInterval != 0 || | ||
| pWorkBlockIndex->nHeight != cycleBaseHeight - llmq::WORK_DIFF_DEPTH) { | ||
| ASSERT_IF_DEBUG(false); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| if (!DeploymentActiveAfter(pWorkBlockIndex.get(), util_params.m_chainman.GetConsensus(), Consensus::DEPLOYMENT_V20)) { | ||
| // pre-v20 modifier calculation needs the future cycle base block context, which isn't known yet. | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| const auto nQuorums{static_cast<size_t>(llmq_params.signingActiveQuorumCount)}; | ||
| PreviousQuorumQuarters previousQuarters(nQuorums); | ||
| auto prev_cycles{previousQuarters.GetCycles()}; | ||
| for (size_t idx{0}; idx < prev_cycles.size(); idx++) { | ||
| const int previousCycleHeight{cycleBaseHeight - llmq_params.dkgInterval * static_cast<int>(idx + 1)}; | ||
| if (previousCycleHeight < 0) { | ||
| return std::nullopt; | ||
| } | ||
| prev_cycles[idx]->m_cycle_index = pWorkBlockIndex->GetAncestor(previousCycleHeight); | ||
| if (prev_cycles[idx]->m_cycle_index == nullptr) { | ||
| return std::nullopt; | ||
| } | ||
| if (auto opt_snap = util_params.m_qsnapman.GetSnapshotForBlock(llmq_params.type, prev_cycles[idx]->m_cycle_index); | ||
| opt_snap.has_value()) { | ||
| prev_cycles[idx]->m_snap = opt_snap.value(); | ||
| } else { | ||
| return std::nullopt; | ||
| } | ||
| prev_cycles[idx]->m_members = GetQuorumQuarterMembersBySnapshot(llmq_params, util_params.m_dmnman, | ||
| util_params.m_chainman.GetConsensus(), | ||
| prev_cycles[idx]->m_cycle_index, | ||
| prev_cycles[idx]->m_snap, | ||
| cycleBaseHeight); | ||
| } | ||
|
|
||
| CDeterministicMNList allMns = util_params.m_dmnman.GetListForBlock(pWorkBlockIndex); | ||
| const auto modifier = GetHashModifierFromWorkBlock(llmq_params, pWorkBlockIndex.get()); | ||
| auto newQuarterMembers = BuildNewQuorumQuarterMembers(llmq_params, util_params, allMns, previousQuarters, | ||
| modifier, pWorkBlockIndex, cycleBaseHeight, | ||
| /*storeSnapshot=*/false); |
There was a problem hiding this comment.
🟡 Suggestion: Prediction helpers gate deployments on the work block, not the future quorum/cycle base
Verified at the assigned head. ComputeNonRotatedQuorumMembersFromWorkBlock evaluates the DEPLOYMENT_V20 gate at line 607 and the EvoOnly (DEPLOYMENT_V19) flag at lines 612-613 against pWorkBlockIndex.get(). ComputeRotatedQuorumMembersFromWorkBlock does the same for its V20 gate at line 647 and threads pWorkBlockIndex into BuildNewQuorumQuarterMembers at lines 679-681, where skipRemovedMNs is derived from that same index via DEPLOYMENT_V19.
The canonical, non-predictive member-selection path (ComputeQuorumMembersByQuarterRotation, GetHashModifier) evaluates these deployment gates against the future quorum's own base block, which is WORK_DIFF_DEPTH (=8) blocks later than the work block used here. The two paths are guaranteed to agree only when no deployment activation falls inside the WORK_DIFF_DEPTH window between the work block and the future quorum base.
This is currently dormant because (a) V19 activated well before V20 in Dash's deployment order, (b) the RPC bails with known=false when V20 is not active at the work block (src/rpc/quorums.cpp:1095-1100), so V19 must also be active whenever prediction proceeds, and (c) no future deployment currently gates member selection. But it leaves the prediction algorithm subtly divergent from the real selection algorithm, and would silently mispredict membership if a future deployment gate activates inside the WORK_DIFF_DEPTH window. Consider either aligning with the canonical path (evaluate gates against the future base block, derivable by walking forward from the work block via the tip's ancestor lookup at pWorkBlockIndex->nHeight + WORK_DIFF_DEPTH) or documenting the divergence in the helper comments.
source: ['claude', 'codex']
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
feat(rpc): expose upcoming DKG participation
Issue being fixed or feature implemented
Dashmate currently uses
quorum dkginfo/quorum dkgstatusto avoid unsaferestart/reindex operations around DKG windows, but Core only exposes global DKG
timing. That forces Dashmate to treat "a DKG is soon" as "this node might be
needed", which blocks harmless recovery operations for masternodes that are not
actually selected for the upcoming DKG.
This PR extends
quorum dkginfoso tooling can ask whether a specificproTxHashis selected for an upcoming, already-determinable DKG.What was done?
quorum dkginfoproTxHashparameter, defaulting to thelocal active masternode when available.
upcoming_dkgsresult array with quorum type, planned quorum height,blocks until start, known/unknown status, and per-
proTxHashmembershipfields when prediction is possible. The RPC reports only upcoming sessions
whose work block is already mined.
DKG work blocks for both non-rotated and rotated quorum types.
quarter-rotation snapshots, without storing a snapshot for the future cycle
base block.
selection, so prediction does not duplicate modifier logic.
predicted safely, including unavailable work blocks, unavailable rotated
snapshots, and pre-v20 quorum selection.
feature_llmq_rotation.pyto verify:proTxHashdefaults to the local masternode;llmq_test_dip0024members match the quorum that is latermined.
feature_llmq_evo.pyto verify platform quorum prediction uses onlyEvoNodes and matches the later mined
llmq_test_platformquorum members.How Has This Been Tested?
git diff --checkpython3 -m py_compile test/functional/feature_llmq_rotation.py test/functional/feature_llmq_evo.pypython3 -m flake8 test/functional/feature_llmq_rotation.py test/functional/feature_llmq_evo.py 2>&1 | rg 'E127' || true./autogen.sh./configure --with-gui=no --disable-bench --disable-fuzz-binary --disable-testsmake -j4 src/dashdtest/functional/feature_llmq_rotation.pytest/functional/feature_llmq_evo.pyship with fixeslookahead_blocksoverflow edge case before later removingthe custom lookahead parameter
feature_llmq_rotation.pyafter the fixBreaking Changes
None expected. Existing
active_dkgsandnext_dkgfields are preserved; thenew
upcoming_dkgsfield is additive.Checklist
(for repository code-owners and collaborators only)