feat(rpc): expose upcoming DKG participation#7428
Conversation
|
✅ Review complete (commit 2629a3b) |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
![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: 5a9b3d4605
ℹ️ 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".
| auto quorumMembers = ComputeQuorumMembersByQuarterRotation(llmq_params, util_params, pWorkBlockIndex, | ||
| cycleBaseHeight, modifier, /*storeSnapshot=*/false); | ||
| if (quorumMembers.empty()) { | ||
| return std::nullopt; | ||
| } | ||
| return quorumMembers[quorumIndex]; |
There was a problem hiding this comment.
When a rotated quorum is predicted and any of the three prior cycle snapshots is unavailable (GetSnapshotForBlock breaks out above), ComputeQuorumMembersByQuarterRotation still returns a non-empty outer vector containing only the quarters it could reconstruct plus the new quarter. This wrapper treats that as known and returns quorumMembers[quorumIndex], so quorum dkginfo can report a reduced memberCount and isMember=false for masternodes that are actually in one of the missing previous quarters (for example while the snapshot DB is incomplete or the required warm-up snapshots are not available). Please return std::nullopt unless the rotated prediction contains a full llmq_params.size members for the requested index.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
~~I found one correctness issue in the replacement branch. The simplified rotated prediction path can return known=true using incomplete previous-quarter data when the required rotated quorum snapshots are unavailable, which is unsafe for the Dashmate use case this RPC is meant to support.
I did not run the full functional tests locally; I did run git diff --check and Python compile checks for the touched functional-test files, both clean. CI is still partially pending on GitHub.
~~
Codex posted for me :(
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR refactors LLMQ quarter-rotation member selection to use explicit work-block, modifier, and cycle-base-height inputs, adds Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant quorum_dkginfo_RPC
participant ComputeQuorumMembersFromWorkBlock
participant ComputeQuorumMembersByQuarterRotation
participant BuildNewQuorumQuarterMembers
Client->>quorum_dkginfo_RPC: quorum dkginfo(proTxHash)
quorum_dkginfo_RPC->>quorum_dkginfo_RPC: compute cycle base height and work block
quorum_dkginfo_RPC->>ComputeQuorumMembersFromWorkBlock: predict members for quorumHeight
ComputeQuorumMembersFromWorkBlock->>ComputeQuorumMembersByQuarterRotation: quarter-rotation path
ComputeQuorumMembersByQuarterRotation->>BuildNewQuorumQuarterMembers: build with modifier and cycleBaseHeight
BuildNewQuorumQuarterMembers-->>ComputeQuorumMembersByQuarterRotation: quarter members
ComputeQuorumMembersByQuarterRotation-->>ComputeQuorumMembersFromWorkBlock: quorum list
ComputeQuorumMembersFromWorkBlock-->>quorum_dkginfo_RPC: optional member list
quorum_dkginfo_RPC-->>Client: upcoming_dkgs results
Possibly related PRs
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)
457-462: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGuard the implicit
m_base_index == cycleBaseHeightcoupling.The function was refactored to take
cycleBaseHeightexplicitly, but the snapshot path still relies onutil_params.m_base_indexfor bothBuildQuorumSnapshotandStoreSnapshotForBlock. This silently assumesm_base_index->nHeight == cycleBaseHeightwheneverstoreSnapshotis true. It holds today becauseGetAllQuorumMemberscallsreplace_index(pCycleQuorumBaseBlockIndex), but a future caller passing a divergentcycleBaseHeightwithstoreSnapshot=truewould persist a snapshot keyed to the wrong block. Consider asserting the invariant.🛡️ Suggested guard
if (storeSnapshot) { + ASSERT_IF_DEBUG(util_params.m_base_index->nHeight == cycleBaseHeight); llmq::CQuorumSnapshot quorumSnapshot{}; BuildQuorumSnapshot(llmqParams, util_params.m_chainman.GetConsensus(), allMns, MnsUsedAtH, sortedCombinedMnsList, quorumSnapshot, skipList, util_params.m_base_index); util_params.m_qsnapman.StoreSnapshotForBlock(llmqParams.type, util_params.m_base_index, quorumSnapshot); }🤖 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 457 - 462, The snapshot branch in the quorum member flow still assumes util_params.m_base_index matches cycleBaseHeight when storeSnapshot is true. In the BuildQuorumSnapshot and StoreSnapshotForBlock path, add an explicit guard/assert around that invariant in GetAllQuorumMembers so future callers cannot persist a snapshot under the wrong block index; keep the check close to the existing use of util_params.m_base_index and cycleBaseHeight.
🤖 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/test_framework/test_framework.py`:
- Around line 2146-2159: The quorum prediction validation in test_framework.py
only asserts that memberIndex is present or not -1, so an incorrect positive
index could still pass. Update the checks in the prediction loop around
quorum_info["members"] and predictions to compare entry["memberIndex"] against
the actual position of proTxHash within quorum_info["members"], while still
keeping the existing isMember and memberCount assertions.
---
Nitpick comments:
In `@src/llmq/utils.cpp`:
- Around line 457-462: The snapshot branch in the quorum member flow still
assumes util_params.m_base_index matches cycleBaseHeight when storeSnapshot is
true. In the BuildQuorumSnapshot and StoreSnapshotForBlock path, add an explicit
guard/assert around that invariant in GetAllQuorumMembers so future callers
cannot persist a snapshot under the wrong block index; keep the check close to
the existing use of util_params.m_base_index and cycleBaseHeight.
🪄 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: 0b14d51e-6d0b-49a4-9f20-9de5347ba22c
📒 Files selected for processing (5)
src/llmq/utils.cppsrc/llmq/utils.hsrc/rpc/quorums.cpptest/functional/feature_llmq_rotation.pytest/functional/test_framework/test_framework.py
There was a problem hiding this comment.
Code Review
Source: reviewers: codex/gpt-5.5 general, codex/gpt-5.5 dash-core-commit-history; failed lanes: claude/opus general, claude/opus dash-core-commit-history, claude/opus verifier (quota); verifier: codex/gpt-5.5.
The main correctness issue is confirmed: rotated quorum prediction can return known=true after silently dropping unavailable prior-quarter data, which contradicts the RPC's safety semantics for Dashmate. The added prediction tests also leave the new memberIndex field under-verified by checking only membership, not the returned position in the quorum member order.
1 blocking, 1 suggestion
Blocking
src/llmq/utils.cpp:490-500- Missing rotated snapshots still produce known predictions
In prediction mode, this loop stops as soon as a previous cycle ancestor or snapshot is unavailable, but it still proceeds with the remainingPreviousQuorumQuartersentries left empty.BuildNewQuorumQuarterMembersthen computes a new quarter from that incomplete history,ComputeQuorumMembersFromWorkBlockreturns a value, andquorum dkginforeportsknown=true. For the Dashmate restart-safety use case, that can falsely report a masternode as not selected when it would have appeared in one of the missing historical quarters; unavailable rotated snapshot data must make the prediction unknown instead of partial.
Suggestion
test/functional/test_framework/test_framework.py:2146-2159- Prediction tests do not verify the returned member index
The new RPC exposesmemberIndex, but the test only checks whether it is-1consistently withisMember. A regression that returns the wrong positive index for a real member would still pass, even thoughquorum infoexposes members in the same order used for quorum member indexes and consumers may rely on that position.
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`:
- [BLOCKING] src/llmq/utils.cpp:490-500: Missing rotated snapshots still produce known predictions
In prediction mode, this loop stops as soon as a previous cycle ancestor or snapshot is unavailable, but it still proceeds with the remaining `PreviousQuorumQuarters` entries left empty. `BuildNewQuorumQuarterMembers` then computes a new quarter from that incomplete history, `ComputeQuorumMembersFromWorkBlock` returns a value, and `quorum dkginfo` reports `known=true`. For the Dashmate restart-safety use case, that can falsely report a masternode as not selected when it would have appeared in one of the missing historical quarters; unavailable rotated snapshot data must make the prediction unknown instead of partial.
In `test/functional/test_framework/test_framework.py`:
- [SUGGESTION] test/functional/test_framework/test_framework.py:2146-2159: Prediction tests do not verify the returned member index
The new RPC exposes `memberIndex`, but the test only checks whether it is `-1` consistently with `isMember`. A regression that returns the wrong positive index for a real member would still pass, even though `quorum info` exposes members in the same order used for quorum member indexes and consumers may rely on that position.
| members = set(m["proTxHash"] for m in quorum_info["members"]) | ||
| work_height = quorum_info["height"] - quorum_info["quorumIndex"] - 8 | ||
| for proTxHash, entries in predictions.items(): | ||
| [entry] = [d for d in entries if d["quorumHeight"] == quorum_info["height"]] | ||
| assert_equal(entry["proTxHash"], proTxHash) | ||
| if not entry["known"]: | ||
| # no prediction was possible (e.g. pre-v20); the RPC must tell why | ||
| assert "reason" in entry | ||
| continue | ||
| assert_equal(entry["workBlockHeight"], work_height) | ||
| assert_equal(entry["workBlockHash"], self.nodes[0].getblockhash(work_height)) | ||
| assert_equal(entry["memberCount"], len(members)) | ||
| assert_equal(entry["isMember"], entry["memberIndex"] != -1) | ||
| assert_equal(entry["isMember"], proTxHash in members) |
There was a problem hiding this comment.
🟡 Suggestion: Prediction tests do not verify the returned member index
The new RPC exposes memberIndex, but the test only checks whether it is -1 consistently with isMember. A regression that returns the wrong positive index for a real member would still pass, even though quorum info exposes members in the same order used for quorum member indexes and consumers may rely on that position.
| members = set(m["proTxHash"] for m in quorum_info["members"]) | |
| work_height = quorum_info["height"] - quorum_info["quorumIndex"] - 8 | |
| for proTxHash, entries in predictions.items(): | |
| [entry] = [d for d in entries if d["quorumHeight"] == quorum_info["height"]] | |
| assert_equal(entry["proTxHash"], proTxHash) | |
| if not entry["known"]: | |
| # no prediction was possible (e.g. pre-v20); the RPC must tell why | |
| assert "reason" in entry | |
| continue | |
| assert_equal(entry["workBlockHeight"], work_height) | |
| assert_equal(entry["workBlockHash"], self.nodes[0].getblockhash(work_height)) | |
| assert_equal(entry["memberCount"], len(members)) | |
| assert_equal(entry["isMember"], entry["memberIndex"] != -1) | |
| assert_equal(entry["isMember"], proTxHash in members) | |
| member_list = [m["proTxHash"] for m in quorum_info["members"]] | |
| members = set(member_list) | |
| work_height = quorum_info["height"] - quorum_info["quorumIndex"] - 8 | |
| for proTxHash, entries in predictions.items(): | |
| [entry] = [d for d in entries if d["quorumHeight"] == quorum_info["height"]] | |
| assert_equal(entry["proTxHash"], proTxHash) | |
| if not entry["known"]: | |
| # no prediction was possible (e.g. pre-v20); the RPC must tell why | |
| assert "reason" in entry | |
| continue | |
| assert_equal(entry["workBlockHeight"], work_height) | |
| assert_equal(entry["workBlockHash"], self.nodes[0].getblockhash(work_height)) | |
| assert_equal(entry["memberCount"], len(members)) | |
| expected_member_index = member_list.index(proTxHash) if proTxHash in members else -1 | |
| assert_equal(entry["memberIndex"], expected_member_index) | |
| assert_equal(entry["isMember"], proTxHash in members) |
source: ['codex']
There was a problem hiding this comment.
Resolved in e0e1e8f — Prediction tests do not verify the returned member index no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Code Review
Source: reviewers: codex/gpt-5.5 general, codex/gpt-5.5 dash-core-commit-history; failed lanes: claude/opus general, claude/opus dash-core-commit-history; verifier: codex/gpt-5.5
prior-1 is STILL VALID: the current rotated prediction path can still return a known result after missing historical quarter data. prior-2 is FIXED by e0e1e8f, which now checks the exact ordered member index from quorum info. The latest push also introduced a small review-fix commit that should be folded into the feature commit before merge.
🔴 1 blocking | 🟡 1 suggestion(s)
2 finding(s) are included in the review body only; the current diff does not expose stable inline anchors for the carried-forward production issue or the commit-history note.
🤖 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`:
- [BLOCKING] src/llmq/utils.cpp:490-500: Missing rotated snapshots still produce known predictions
In prediction mode, this loop stops as soon as a previous cycle ancestor or snapshot is unavailable, but it still proceeds with the remaining PreviousQuorumQuarters entries left empty. BuildNewQuorumQuarterMembers then computes from incomplete history, ComputeQuorumMembersFromWorkBlock returns a value, and quorum dkginfo reports known=true. The feature commit explicitly promises structured unknown results for unavailable rotated snapshots, so this can falsely report a masternode as not selected when the missing historical quarter would have included it.
In `<commit:e0e1e8f>`:
- [SUGGESTION] <commit:e0e1e8f>:1: Squash memberIndex test fix into the feature commit
Commit e0e1e8f6d49 only adjusts the functional test helper added by the preceding feature commit so it verifies memberIndex correctly. This is review-fix history rather than a separate logical change, and Dash PRs are kept cleaner when these small corrections are folded into the commit that introduced the behavior.
That's not real life scenarios and have nothing to do with PR description. there's no combination of "historical data" and "alive valid masternode" |
Dashmate currently uses quorum dkginfo/quorum dkgstatus to avoid unsafe restart/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 dkginfo so tooling can ask whether a specific proTxHash is selected for an upcoming, already-determinable DKG. What was done? Added an optional quorum dkginfo proTxHash parameter, defaulting to the local active masternode when available. Added an upcoming_dkgs result array with quorum type, planned quorum height, blocks until start, known/unknown status, and per-proTxHash membership fields when prediction is possible. The RPC reports only upcoming sessions whose work block is already mined. Added utility helpers that compute future v20 quorum members from already-known DKG work blocks for both non-rotated and rotated quorum types. For rotated quorums, prediction uses the cycle base work block and historical quarter-rotation snapshots, without storing a snapshot for the future cycle base block. Shared the post-v20 work-block hash modifier helper with normal quorum selection, so prediction does not duplicate modifier logic. Snapshot the chain tip/work-block lookup consistently for the prediction path. Return structured unknown reasons instead of guessing for cases that cannot be predicted safely, including unavailable work blocks, unavailable rotated snapshots, and pre-v20 quorum selection. Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com>
…after missing historical quarter data
It never happens in real-life scenarios, only some corner cases such as:
- first few rotated cycles after DIP0024 activation / before 3 prior rotated snapshots exist
- devnet/regtest with DIP0024 active from height 1
- node with incomplete/corrupt/rebuilt EvoDB snapshot data
Though, review agents are concerned about that issue very much, so it should be fixed.
knst
force-pushed
the
rpc-dkg-scheduled-participation-2
branch
from
6f3f841 to
2629a3b
Compare
There was a problem hiding this comment.
Code Review
Source: reviewers: codex/gpt-5.5 general, codex/gpt-5.5 dash-core-commit-history; failed lanes: claude/opus general, claude/opus dash-core-commit-history, claude/opus verifier (quota); verifier: codex/gpt-5.5.
The prior rotated-snapshot correctness finding is fixed at the current head: prediction mode now returns unknown when required historical rotated quorum data is unavailable. No new code-correctness issue was verified in the latest delta. The remaining verified issue is commit hygiene: both follow-up fix commits correct behavior introduced by the main feature commit and should be folded before merge.
🟡 1 suggestion
Prior Reconciliation
- prior-1 FIXED:
2629a3b3570makes prediction mode return no result when a required previous rotated cycle ancestor or snapshot is missing, soquorum dkginforeportsknown=falseinstead of computing from incomplete historical quarters. - prior-2 STILL VALID: the memberIndex test correction is still present as a separate follow-up commit, now rewritten as
1f745e1f8db, rather than being folded into the feature commit that introduced the helper.
Carried-Forward Prior Findings
<commit:1f745e1>- Squash the memberIndex test fix intoa1e59f1b8b0so the feature lands with the corrected test helper from the start.
New Findings In Latest Delta
<commit:2629a3b>- The rotated prediction repair fixes the prior production issue, but it is also a direct review-fix commit for behavior introduced bya1e59f1b8b0; fold it into the feature commit before merge.
🤖 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:1f745e1..2629a3b>`:
- [SUGGESTION] <commit:1f745e1..2629a3b>:1: Squash review-fix commits into the feature commit
The current stack still has one feature commit followed by two small corrective commits: 1f745e1f8db only fixes the functional test helper added by a1e59f1b8b0, and 2629a3b3570 only fixes the rotated prediction behavior introduced by that same feature. These are not independent logical changes; they are review fixes for the new RPC/LLMQ prediction behavior. Since Dash preserves PR commit history, fold both fix commits into a1e59f1b8b0 so the merged history does not retain a known-bad intermediate implementation.
3 participants
Issue being fixed or feature implemented
Dashmate currently uses quorum dkginfo/quorum dkgstatus to avoid unsafe restart/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.
What was done?
This PR extends quorum dkginfo so tooling can ask whether a specific proTxHash is selected for an upcoming, already-determinable DKG. What was done?
Added an optional quorum dkginfo proTxHash parameter, defaulting to the local active masternode when available.
Added an upcoming_dkgs result array with quorum type, planned quorum height, blocks until start, known/unknown status, and per-proTxHash membership fields when prediction is possible. The RPC reports only upcoming sessions whose work block is already mined.
Added utility helpers that compute future v20 quorum members from already-known DKG work blocks for both non-rotated and rotated quorum types. For rotated quorums, prediction uses the cycle base work block and historical quarter-rotation snapshots, without storing a snapshot for the future cycle base block.
Shared the post-v20 work-block hash modifier helper with normal quorum selection, so prediction does not duplicate modifier logic. Snapshot the chain tip/work-block lookup consistently for the prediction path. Return structured unknown reasons instead of guessing for cases that cannot be predicted safely, including unavailable work blocks, unavailable rotated snapshots, and pre-v20 quorum selection.
How Has This Been Tested?
See updates for functional tests
Breaking Changes
N/A
Checklist: