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 RPC as quorum dkginfo
participant Utils as llmq::utils
participant Chain as CBlockIndex
participant MNList as CDeterministicMNList
RPC->>Utils: request upcoming quorum prediction
Utils->>Chain: inspect tip, work block, and deployment state
Utils->>MNList: compute predicted members
Utils-->>RPC: return upcoming_dkgs metadata
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 12b3ddb) |
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']
|
@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.
|
|
Manual follow-up on current head I found one additional issue that is separate from the already-posted exact-SHA review:
The steady-state tests pass because they cover pre-v20/no-prediction and post-activation rotation, but not the activation-boundary window where the type is enabled and the next cycle's rotation state differs from |
|
Pushed fixes for the review items in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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: 999b0ca34e
ℹ️ 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".
There was a problem hiding this comment.
Code Review
Source: reviewers = claude general opus; codex general gpt-5.5 (retry after unavailable gpt-5.5[high] alias); claude dash-core-commit-history opus; codex dash-core-commit-history gpt-5.5 (retry); verifier = claude opus (full retry); specialists = dash-core-commit-history.
Cumulative review at 999b0ca. The prior f26c8be deployment-gating finding is FIXED: the prediction helpers now take a deployment-tip context, refuse prediction when future V19 state is not knowable from the tip, and thread that context into BuildNewQuorumQuarterMembers; the RPC also predicts upcoming DIP0024 rotation state instead of using the tip's current cycle. No carried-forward prior findings remain, and the latest delta has no new runtime correctness issues. One new commit-history hygiene suggestion remains for the two review-fix commits added after f26c8be.
1 suggestion
Prior finding reconciliation:
- prior-1: FIXED - Fixed at 999b0ca.
ComputeNonRotatedQuorumMembersFromWorkBlock(src/llmq/utils.cpp:592-628) now acceptspDeploymentTipIndex, returnsstd::nulloptfor platform LLMQ types when V19 is not active at tip, and derivesEvoOnly = is_platform_llmq && v19_active_at_tip.ComputeRotatedQuorumMembersFromWorkBlock(src/llmq/utils.cpp:631-718) also acceptspDeploymentTipIndex, refuses to predict at lines 669-671 when!v19_active_at_tip && !TESTNET(matching the OR structure ofskipRemovedMNs), and threadspDeploymentTipIndexintoBuildNewQuorumQuarterMembersat lines 700-702.BuildNewQuorumQuarterMemberswas refactored to accept an explicitpDeploymentIndexandcycleBaseHeightsoskipRemovedMNsis evaluated against the caller-supplied context (tip for prediction, base index for canonical via the wrapper). The RPC dispatch at src/rpc/quorums.cpp:1079-1088 handles the analogous DIP0024 rotation prediction, reportingknown=falsewhen the future rotation state cannot be determined. The monotonic-deployment reasoning is sound: V19-active-at-tip implies V19-active-at-future-base; V19-inactive-at-tip means the future-base state is unknown. Commit 999b0ca correctly drops the redundant work-block V19 check because the work block is an ancestor of the tip, so V19-active-at-work is implied by V19-active-at-tip.
Carried-forward prior findings:
- None.
New findings in the latest delta:
- [SUGGESTION] commit:9c02c020:1: Fold the review-fix commits 9c02c02 and 999b0ca into the commits that introduced the affected code
The last two commits in this stack patch behavior introduced earlier in the same PR that has never left this branch. 9c02c02 fixes (a)quorum_dkginforouting the upcoming cycle through the wrong helper by callingIsQuorumRotationEnabled(pindexTip)(a call site added in 9ce1c4d and extended in f26c8be), and (b)ComputeNon/RotatedQuorumMembersFromWorkBlockgatingDEPLOYMENT_V19on the work block instead of the future quorum/cycle base (helpers introduced in 9ce1c4d/f26c8bea). 999b0ca then removes the!v19_active_at_work && !v19_active_at_tipredundancy and hoistsis_platform_llmq— both against code the same PR just introduced two commits earlier. The 9c02c02 commit message itself is structured as "First review item... Second review item", the classic address-review-feedback pattern. Dash merges PR commits without squashing, so leaving these as separate commits permanently records intermediate versions of the prediction helpers that return known-wrong membership predictions around DIP0024 / V19 activation — a bisect hazard for anyone later chasing a DKG-prediction bug, withgit blameon the helpers pointing at 999b0ca for a one-liner. Rebase the 9c02c02 hunks into 9ce1c4d (V19-gating signature/threading) and f26c8be (rotation-route fix), and fold 999b0ca into that cleanup, preserving any useful rationale in the final commit messages.
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:9c02c020>`:
- [SUGGESTION] <commit:9c02c020>:1: Fold the review-fix commits 9c02c020 and 999b0ca3 into the commits that introduced the affected code
The last two commits in this stack patch behavior introduced earlier in the same PR that has never left this branch. 9c02c020ed fixes (a) `quorum_dkginfo` routing the upcoming cycle through the wrong helper by calling `IsQuorumRotationEnabled(pindexTip)` (a call site added in 9ce1c4def6 and extended in f26c8bead8), and (b) `ComputeNon/RotatedQuorumMembersFromWorkBlock` gating `DEPLOYMENT_V19` on the work block instead of the future quorum/cycle base (helpers introduced in 9ce1c4de/f26c8bea). 999b0ca34e then removes the `!v19_active_at_work && !v19_active_at_tip` redundancy and hoists `is_platform_llmq` — both against code the same PR just introduced two commits earlier. The 9c02c020 commit message itself is structured as "First review item... Second review item", the classic address-review-feedback pattern. Dash merges PR commits without squashing, so leaving these as separate commits permanently records intermediate versions of the prediction helpers that return known-wrong membership predictions around DIP0024 / V19 activation — a bisect hazard for anyone later chasing a DKG-prediction bug, with `git blame` on the helpers pointing at 999b0ca3 for a one-liner. Rebase the 9c02c020 hunks into 9ce1c4def6 (V19-gating signature/threading) and f26c8bead8 (rotation-route fix), and fold 999b0ca34e into that cleanup, preserving any useful rationale in the final commit messages.
Add optional proTxHash support to quorum dkginfo and report upcoming DKG sessions whose work block is already available. Use the already-mined work block to predict non-rotated v20 quorum membership, while reporting unknown membership when the future quorum base's deployment state cannot be determined from the current tip. This keeps the RPC from returning known membership based on a V19 state that may change before the future base block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
rpc-dkg-scheduled-participation
branch
from
999b0ca to
9315b27
Compare
|
Rewrote the PR branch to address the commit-history feedback from the 999b0ca review.
Local validation: |
|
@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: 9315b2778b
ℹ️ 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".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental delta from 999b0ca3 to 9315b277 is empty, so this push rewrites history without changing the final tree. The carried-forward prior finding is FIXED: the stack is now two coherent commits on upstream/develop (62e8beeff4 rpc: expose upcoming DKG participation and 9315b2778b rpc: predict rotated DKG participation), with the previous review-fix commits folded away. No new in-scope findings were identified in the cumulative review.
Prior finding reconciliation:
- prior-1: FIXED — The prior commit-history suggestion asked to fold review-fix commits
9c02c020and999b0ca3into the commits that introduced the affected code. The current head no longer contains those commits as separate history, and the final tree is unchanged from999b0ca3.
Carried-forward prior findings: none.
New findings in latest delta: none.
Source: reviewers = claude general opus (failed: quota exhausted, resets Jul 10 8am America/Chicago); codex general gpt-5.5; claude dash-core-commit-history opus (failed: quota exhausted, resets Jul 10 8am America/Chicago); codex dash-core-commit-history gpt-5.5; verifier = codex gpt-5.5; specialists = dash-core-commit-history.
Note: GitHub does not allow me to submit an approval on my own PR, so this is posted as a COMMENT while preserving the clean verified result.
thepastaclaw
force-pushed
the
rpc-dkg-scheduled-participation
branch
from
9315b27 to
598a810
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Extend quorum dkginfo upcoming membership prediction to rotated LLMQs by reusing the known cycle work block, historical quarter snapshots, and an explicit deployment-tip context. Predict the upcoming cycle's DIP0024 state rather than the tip's current cycle state, and return known=false when future deployment state or rotated snapshots are not yet available. The same deployment-tip context is threaded through the rotated helper so V19-dependent skipRemovedMNs behavior matches the future cycle base without claiming certainty before the tip proves it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
rpc-dkg-scheduled-participation
branch
from
598a810 to
12b3ddb
Compare
|
Pushed
Resolved the two Codex review threads. Local checks: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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: 12b3ddb46b
ℹ️ 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".
| predicted_members = llmq::utils::ComputeRotatedQuorumMembersFromWorkBlock( | ||
| llmq_params.type, | ||
| {*CHECK_NONFATAL(node.dmnman), *CHECK_NONFATAL(llmq_ctx.qsnapman), chainman, pindexTip}, | ||
| pWorkBlockIndex, quorumHeight, pindexTip); |
There was a problem hiding this comment.
Fresh evidence is that this call still passes the current tip even for remaining indexes of an already-started rotated cycle. In regtest/custom activation windows where V20 is active and V19 activates after cycleBaseHeight but before a later quorum index starts, production built the cycle with BuildNewQuorumQuarterMembers(..., pCycleQuorumBaseBlockIndex) and skipRemovedMNs == false, while this prediction uses pindexTip and flips it to true, so upcoming_dkgs can report a different rotated membership set for the rest of that cycle.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental delta from 9315b277 to 598a8106 adds the disabled-quorum-type guard in src/rpc/quorums.cpp. The prior exact-SHA review at 9315b277 found no issues, and the cumulative recheck at 598a8106 remains clean: no carried-forward prior findings and no new in-scope findings in the latest delta.
Prior finding reconciliation:
- prior review at
9315b277: STILL CLEAN - it had no findings, and the current-head recheck did not uncover any issue that should have been carried forward.
Carried-forward prior findings: none.
New findings in latest delta: none.
Source: reviewers = claude general opus (failed: quota exhausted, resets Jul 10 8am America/Chicago); codex general gpt-5.5; claude dash-core-commit-history opus (failed: quota exhausted, resets Jul 10 8am America/Chicago); codex dash-core-commit-history gpt-5.5; verifier = codex gpt-5.5; specialists = dash-core-commit-history.
Note: GitHub does not allow me to submit an approval on my own PR, so this is posted as a COMMENT while preserving the clean verified result.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers = claude general opus (failed: quota exhausted, resets Jul 10 8am America/Chicago); codex general gpt-5.5; claude dash-core-commit-history opus (failed: quota exhausted, resets Jul 10 8am America/Chicago); codex dash-core-commit-history gpt-5.5; verifier = codex gpt-5.5 (recovery after opus quota exhaustion and an initial codex verifier run produced no final JSON); specialists = dash-core-commit-history.
Prior review at 9315b27 had no findings and remains reconciled as clean for carried-forward issues. The current cumulative review finds one new in-scope issue in src/rpc/quorums.cpp: upcoming_dkgs seeds candidates from quorum types enabled at the current tip, which can omit a determinable future DKG type before the later quorum-base enablement guard is reached.
Prior finding reconciliation:
- prior review at
9315b277: STILL CLEAN - it had no findings, and the current cumulative recheck found no carried-forward prior issues.
Carried-forward prior findings: none.
New findings in latest delta:
- blocking:
src/rpc/quorums.cpp:1084seedsupcoming_dkgsfrom quorum types enabled at the current tip, so a future quorum type whose quorum-base predecessor is already determinable can be omitted before the new future-base guard runs.
🔴 1 blocking
🤖 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:1084: Do not seed upcoming DKGs from tip-enabled quorum types only
`upcoming_dkgs` iterates `llmq::GetEnabledQuorumTypes(chainman, pindexTip)`, so quorum types that are disabled at the RPC tip are skipped before the later upcoming-quorum-base checks can run. Canonical DKG validation checks enablement against the quorum base predecessor, and buried deployments such as DIP0024/DIP0020 can be determinable by height before they are active at the current tip. In that window, the work block for the first post-activation DKG can already be known, but this RPC will omit the session entirely instead of returning an unknown or known upcoming entry. Iterate the configured LLMQ params as the initial universe, then filter with the upcoming quorum-base predecessor logic.
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.
|
|
||
| if (!proTxHash.IsNull()) { | ||
| UniValue upcoming(UniValue::VARR); | ||
| for (const auto& type : llmq::GetEnabledQuorumTypes(chainman, pindexTip)) { |
There was a problem hiding this comment.
🔴 Blocking: Do not seed upcoming DKGs from tip-enabled quorum types only
upcoming_dkgs iterates llmq::GetEnabledQuorumTypes(chainman, pindexTip), so quorum types that are disabled at the RPC tip are skipped before the later upcoming-quorum-base checks can run. Canonical DKG validation checks enablement against the quorum base predecessor, and buried deployments such as DIP0024/DIP0020 can be determinable by height before they are active at the current tip. In that window, the work block for the first post-activation DKG can already be known, but this RPC will omit the session entirely instead of returning an unknown or known upcoming entry. Iterate the configured LLMQ params as the initial universe, then filter with the upcoming quorum-base predecessor logic.
source: ['codex-general']
2 participants
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)