feat(rpc): expose upcoming DKG participation#7428
Conversation
|
✅ Review complete (commit 3035e51) |
![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 :(
|
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 refactors LLMQ quarter-rotation member selection around explicit work-block, modifier, and cycle-base inputs, adds future quorum prediction, extends 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: select quarter-rotation members
ComputeQuorumMembersByQuarterRotation->>BuildNewQuorumQuarterMembers: build with modifier and cycleBaseHeight
BuildNewQuorumQuarterMembers-->>ComputeQuorumMembersByQuarterRotation: return quarter members
ComputeQuorumMembersByQuarterRotation-->>ComputeQuorumMembersFromWorkBlock: return quorum list
ComputeQuorumMembersFromWorkBlock-->>quorum_dkginfo_RPC: return optional member list
quorum_dkginfo_RPC-->>Client: return upcoming_dkgs results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
kwvg
left a comment
kwvg
left a comment
There was a problem hiding this comment.
Preferable to use standardised docs for Dash-specific result fields
…utput Co-authored-by: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com>
knst
force-pushed
the
rpc-dkg-scheduled-participation-2
branch
from
d557a14 to
4534eb8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53be3436a0
ℹ️ 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.
Actionable comments posted: 1
🤖 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 `@src/rpc/quorums.cpp`:
- Around line 1072-1074: Replace the out-of-scope obj.pushKV call in the quorum
result-building logic with ret.pushKV so proTxHash is added to the top-level
response; keep the existing proTxHash null check and subsequent upcoming
construction unchanged.
🪄 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: a3f6ea75-9935-49d4-93c5-a64fe33d4c26
📒 Files selected for processing (2)
src/rpc/quorums.cpptest/functional/test_framework/test_framework.py
💤 Files with no reviewable changes (1)
- test/functional/test_framework/test_framework.py
knst
force-pushed
the
rpc-dkg-scheduled-participation-2
branch
from
53be343 to
f571843
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5718437fb
ℹ️ 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
Carried-forward prior findings: prior-2 (blocking) STILL VALID — verify_upcoming_dkg_predictions still reads entry["proTxHash"] at test_framework.py:2151 after the field moved to the dkginfo top level, so mine_quorum/rotation verification raises KeyError; prior-3, prior-4, prior-5 (suggestions) STILL VALID. New findings in the latest delta: none — the one-line 53be343..f571843 change (obj.pushKV -> ret.pushKV at quorums.cpp:1073) fixes prior-1 (compile break). Additional cumulative current-head findings: none. review_action REQUEST_CHANGES because the KeyError blocker remains.
Source: reviewers — general: opus (ok), gpt-5.6-sol (failed: revoked refresh token); dash-core-commit-history: opus (ok), gpt-5.6-sol (failed: revoked refresh token). Verifier: opus.
🔴 1 blocking | 🟡 3 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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 `test/functional/test_framework/test_framework.py`:
- [BLOCKING] test/functional/test_framework/test_framework.py:2151: verify_upcoming_dkg_predictions reads per-entry proTxHash that only exists at the response top level
The dkginfo RPC emits proTxHash once at the response top level (src/rpc/quorums.cpp:1073) and never inside each upcoming_dkgs entry. get_upcoming_dkg_predictions (line 2141) keeps only the upcoming_dkgs list and discards the top-level field, but verify_upcoming_dkg_predictions still reads entry["proTxHash"] at line 2151. Now that the compile error is fixed, this raises KeyError when a matching entry is verified. verify_upcoming_dkg_predictions runs inside the shared mine_quorum helper and the rotation flow, so functional tests break. The predictions dict is already keyed by the queried proTxHash, so drop this per-entry assertion or capture and assert against the new top-level field instead.
In `src/rpc/quorums.cpp`:
- [SUGGESTION] src/rpc/quorums.cpp:1006: llmqTypeName is emitted but not documented in the upcoming_dkgs help schema
Line 1103 emits llmqTypeName for every upcoming entry, but the upcoming_dkgs help block omits it. RPCResult object checking is shallow, so this response/help mismatch is not caught automatically. Re-add llmqTypeName to the help schema or stop emitting it.
- [SUGGESTION] src/rpc/quorums.cpp:1001: Top-level proTxHash documented as required but omitted when null
GetRpcResult defaults optional to false, so the help advertises top-level proTxHash as always present. But it is emitted only inside if (!proTxHash.IsNull()) at line 1073. In observer/watch-only mode with no explicit argument, node.active_ctx is null and proTxHash stays null, so the field is absent. Mark it optional to match the response shape.
In `<commit:a1e59f1..f571843>`:
- [SUGGESTION] <commit:a1e59f1..f571843>:1: Squash the within-PR fix/refactor/cleanup commits into the feature commit
Feature commit a1e59f1 is followed by four commits that only correct or polish this unmerged feature: 1f745e1, 2629a3b, 4534eb8, and f5718437. memberIndex/memberCount are introduced, tested, help-refactored, then removed across the stack, and f5718437 also repairs the earlier obj-vs-ret compile break. Fold the four follow-ups into a1e59f1 so permanent Dash develop history contains one coherent feature commit without artificial intermediate states.
| { | ||
| {RPCResult::Type::NUM, "active_dkgs", "Total number of active DKG sessions this node is participating in right now"}, | ||
| {RPCResult::Type::NUM, "next_dkg", "The number of blocks until the next potential DKG session"}, | ||
| GetRpcResult("proTxHash"), |
There was a problem hiding this comment.
🟡 Suggestion: Top-level proTxHash documented as required but omitted when null
GetRpcResult defaults optional to false, so the help advertises top-level proTxHash as always present. But it is emitted only inside if (!proTxHash.IsNull()) at line 1073. In observer/watch-only mode with no explicit argument, node.active_ctx is null and proTxHash stays null, so the field is absent. Mark it optional to match the response shape.
| GetRpcResult("proTxHash"), | |
| GetRpcResult("proTxHash", /*optional=*/true), |
source: ['claude']
knst
force-pushed
the
rpc-dkg-scheduled-participation-2
branch
from
f571843 to
618a62d
Compare
knst
force-pushed
the
rpc-dkg-scheduled-participation-2
branch
from
618a62d to
d913a48
Compare
There was a problem hiding this comment.
Code Review
Source: reviewers — general: opus, gpt-5.6-sol; dash-core-commit-history: opus, gpt-5.6-sol. Verifier: opus.
Prior Finding Reconciliation: prior-1 (per-entry proTxHash KeyError, blocking) is FIXED — the latest delta deletes the assert_equal(entry["proTxHash"], proTxHash) line at test_framework.py, and proTxHash is now sourced solely from the predictions dict keys. prior-2 (llmqTypeName undocumented) is FIXED — the response emits only llmqType (quorums.cpp:1102) and the help schema documents only llmqType (line 1006); no llmqTypeName remains. prior-3 (top-level proTxHash documented required) is STILL VALID. prior-4 (squash the within-PR commit stack) is STILL VALID. Carried-forward prior findings: prior-3 and prior-4, both re-anchored to current head d913a48. New findings in latest delta: none — the one-line deletion only resolves prior-1. Cumulative current-head review adds one performance suggestion in the rotated-quorum prediction loop. No blocking issues remain; three suggestions total.
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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`:
- [SUGGESTION] src/rpc/quorums.cpp:1125-1129: Compute each rotated cycle only once per LLMQ type
ComputeQuorumMembersFromWorkBlock is called inside the per-quorumIndex loop, but for rotated types it delegates to ComputeQuorumMembersByQuarterRotation (utils.cpp:624), which reconstructs every quorum in the cycle before returning just quorumMembers[quorumIndex] (utils.cpp:629). For mainnet LLMQ_60_75, signingActiveQuorumCount is 32, so a single dkginfo request rebuilds the same 32-quorum cycle 32 times — repeatedly loading historical snapshots and sorting masternode lists. Since all indices in a cycle share the same work block and cycle base, compute the rotated cycle once per LLMQ type and index into its cached results.
In `<commit:a1e59f1..d913a48>`:
- [SUGGESTION] <commit:a1e59f1..d913a48>:1: Squash the within-PR fix/refactor/cleanup commits into the feature commit
Feature commit a1e59f1 ("feat(rpc): expose upcoming DKG participation") is followed by four commits that only correct or polish this still-unmerged feature: 1f745e1 (adds memberIndex/memberCount functional assertions), 2629a3b (fixes rotated-prediction handling, framed as review feedback), 4534eb8 (refactors the RPC help), and d913a48 (removes memberIndex/memberCount/per-entry proTxHash/llmqTypeName that earlier commits introduced and drops the corresponding test assertions). Because Dash merges without squashing, permanent develop history would land with artificial intermediate states where fields appear then vanish and tests assert on fields later deleted — noise for git blame/bisect. d913a48 also carries a `feat:` prefix even though its diff is a cleanup/removal. Fold the follow-ups into a1e59f1 so the branch lands as one coherent feature commit; keep any change with durable standalone value as a separate correctly-prefixed commit. (Carried forward from prior review, STILL VALID at current head.)
| const LLMQContext& llmq_ctx = EnsureLLMQContext(node); | ||
| const auto predicted_members = llmq::utils::ComputeQuorumMembersFromWorkBlock( | ||
| llmq_params.type, | ||
| {*CHECK_NONFATAL(node.dmnman), *CHECK_NONFATAL(llmq_ctx.qsnapman), chainman, pindexTip}, | ||
| pWorkBlockIndex, quorumHeight); |
There was a problem hiding this comment.
🟡 Suggestion: Compute each rotated cycle only once per LLMQ type
ComputeQuorumMembersFromWorkBlock is called inside the per-quorumIndex loop, but for rotated types it delegates to ComputeQuorumMembersByQuarterRotation (utils.cpp:624), which reconstructs every quorum in the cycle before returning just quorumMembers[quorumIndex] (utils.cpp:629). For mainnet LLMQ_60_75, signingActiveQuorumCount is 32, so a single dkginfo request rebuilds the same 32-quorum cycle 32 times — repeatedly loading historical snapshots and sorting masternode lists. Since all indices in a cycle share the same work block and cycle base, compute the rotated cycle once per LLMQ type and index into its cached results.
source: ['codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior Finding Reconciliation: prior-1 (top-level proTxHash documented required at quorums.cpp:1001) is FIXED — the entire delta since d913a48 is exactly GetRpcResult("proTxHash", /*optional=*/true), verified in the current checkout. prior-2 (recompute rotated cycle once per LLMQ type instead of per quorumIndex, quorums.cpp:1125-1129) is STILL VALID — confirmed unchanged: ComputeQuorumMembersFromWorkBlock is still invoked inside the per-quorumIndex loop and calls ComputeQuorumMembersByQuarterRotation (llmq/utils.cpp:470-489, 624-629), which rebuilds the full rotated cycle before returning a single index; the existing non-predicting cache path (GetAllQuorumMembers, utils.cpp:699-708) is not reused for prediction. prior-3 (squash within-PR fix/refactor/cleanup commits) is STILL VALID and now spans a1e59f1..3035e51 (6 commits, verified via git log); commit 2629a3b's own message confirms it is a corner-case fix requested by review agents on the still-unmerged feature. Carried-forward prior findings: prior-2 and prior-3, both re-anchored to current head. New findings in latest delta: none — the one-line change is a correct, targeted fix with no side effects. No blocking issues; two suggestions total.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); opus/general=claude-opus-4-8(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); sonnet5/dash-core-commit-history=claude-sonnet-5(completed); opus/dash-core-commit-history=claude-opus-4-8(completed); verifier=verifier-sonnet5-7428-1783846201=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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:a1e59f1..3035e51>`:
- [SUGGESTION] <commit:a1e59f1..3035e51>:1: Squash the within-PR fix/refactor/cleanup commits into the feature commit
Feature commit a1e59f1 ('feat(rpc): expose upcoming DKG participation') is followed by five commits that only correct or polish this still-unmerged feature: 1f745e1 (adds memberIndex/memberCount functional assertions), 2629a3b (fixes rotated-prediction handling — its own message says the corner case 'never happens in real-life scenarios' but was fixed because 'review agents are concerned about that issue very much'), 4534eb8 (refactors the RPC help just added), d913a48 (removes memberIndex/memberCount/per-entry proTxHash/llmqTypeName that earlier commits introduced and drops the corresponding test assertions, under a `feat:` prefix despite being a cleanup), and 3035e51 ('fix: add optional for proTxHash') — a one-line correction to the exact same GetRpcResult("proTxHash") entry a1e59f1 introduced. Because Dash merges without squashing, permanent develop history would show fields appearing then vanishing and a help schema being wrong for several commits before correction — pure noise for git blame/bisect. Fold these five commits into a1e59f1 (git rebase -i / --autosquash) so the branch lands as one coherent feature commit; keep any change with durable standalone value as a separate correctly-prefixed commit.
4 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: