docs(rpc): introduce more RPC help text and deduplicate governance RPC logic to resolve pending definitions #7061
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR centralizes JSON help/serialization and reshapes several RPC schemas. It adds governance/core_write.cpp to produce JSON help and ToJson/GetStateJson/GetVotesJson implementations; removes several legacy ToJson() methods; adds RPCResult-based GetJsonHelp() helpers across governance, evo, llmq, and related headers; introduces an optional Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/coinjoin.cpp (1)
411-448: Fix CI: clang-format-diff failure in this file must be resolved.Please re-run the repo’s clang-format-diff step (same as CI) and apply the suggested patch.
🧹 Nitpick comments (2)
src/llmq/debug.h (1)
81-97: Add an explicit<string>include to avoid relying on transitive headers.This header now exposes
std::stringin public declarations (GetJsonHelp).--- a/src/llmq/debug.h +++ b/src/llmq/debug.h @@ #include <univalue.h> #include <functional> #include <set> + #include <string>src/governance/core_write.cpp (1)
1-155: Address CI formatting failure.The Clang format check is failing for this file. Please run the clang-format-diff script to fix formatting before merging.
#!/bin/bash # Run from repository root to check formatting differences git diff -U0 HEAD^ -- src/governance/core_write.cpp | clang-format-diff -p1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between bac6bfa and 09b65a4ec9d12bcd08a7e5acfa0efbc6522a25c5.
📒 Files selected for processing (18)
src/Makefile.am(1 hunks)src/core_io.h(1 hunks)src/evo/core_write.cpp(5 hunks)src/governance/common.cpp(1 hunks)src/governance/common.h(1 hunks)src/governance/core_write.cpp(1 hunks)src/governance/governance.cpp(0 hunks)src/governance/governance.h(2 hunks)src/governance/object.cpp(1 hunks)src/governance/object.h(2 hunks)src/llmq/core_write.cpp(2 hunks)src/llmq/debug.h(3 hunks)src/qt/governancelist.cpp(1 hunks)src/rpc/coinjoin.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/governance.cpp(6 hunks)src/rpc/masternode.cpp(2 hunks)src/rpc/quorums.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- src/governance/governance.cpp
🧰 Additional context used
📓 Path-based instructions (9)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/governance/governance.hsrc/rpc/coinjoin.cppsrc/rpc/masternode.cppsrc/qt/governancelist.cppsrc/llmq/core_write.cppsrc/rpc/quorums.cppsrc/governance/common.cppsrc/governance/core_write.cppsrc/llmq/debug.hsrc/governance/object.cppsrc/evo/core_write.cppsrc/rpc/governance.cppsrc/core_io.hsrc/governance/common.hsrc/rpc/evo.cppsrc/governance/object.h
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/governance/governance.hsrc/llmq/core_write.cppsrc/governance/common.cppsrc/governance/core_write.cppsrc/llmq/debug.hsrc/governance/object.cppsrc/evo/core_write.cppsrc/governance/common.hsrc/governance/object.h
src/governance/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Files:
src/governance/governance.hsrc/governance/common.cppsrc/governance/core_write.cppsrc/governance/object.cppsrc/governance/common.hsrc/governance/object.h
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/governance/governance.hsrc/llmq/core_write.cppsrc/governance/common.cppsrc/governance/core_write.cppsrc/llmq/debug.hsrc/governance/object.cppsrc/evo/core_write.cppsrc/governance/common.hsrc/governance/object.h
src/qt/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
GUI implementation in src/qt/ must use Qt 5
Files:
src/qt/governancelist.cpp
src/{masternode,llmq}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
BLS integration must be used for cryptographic foundation of advanced masternode features
Files:
src/llmq/core_write.cppsrc/llmq/debug.h
src/llmq/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/llmq/**/*.{cpp,h}: LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Files:
src/llmq/core_write.cppsrc/llmq/debug.h
src/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/core_write.cpp
src/evo/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Files:
src/evo/core_write.cpp
🧠 Learnings (19)
📓 Common learnings
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:0-0
Timestamp: 2025-08-08T04:30:37.971Z
Learning: In the Dash codebase, direct RPC calling has been removed in recent commits, making suggestions about RPC command validation and error handling for direct RPC calls obsolete. The ProposalWizard and related components no longer use direct RPC calling patterns.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/governance/governance.hsrc/rpc/coinjoin.cppsrc/rpc/masternode.cppsrc/llmq/debug.hsrc/evo/core_write.cppsrc/rpc/governance.cppsrc/core_io.hsrc/governance/common.hsrc/rpc/evo.cppsrc/governance/object.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
src/governance/governance.hsrc/Makefile.amsrc/governance/common.cppsrc/governance/core_write.cppsrc/governance/object.cppsrc/rpc/governance.cppsrc/governance/common.hsrc/governance/object.h
📚 Learning: 2025-11-13T20:02:55.480Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6969
File: src/evo/deterministicmns.h:441-479
Timestamp: 2025-11-13T20:02:55.480Z
Learning: In `src/evo/deterministicmns.h`, the `internalId` field in `CDeterministicMN` and the `mnInternalIdMap` in `CDeterministicMNList` are non-deterministic and used only for internal bookkeeping and efficient lookups. Different nodes can assign different internalIds to the same masternode depending on their sync history. Methods like `IsEqual()` intentionally ignore internalId mappings and only compare consensus-critical deterministic fields (proTxHash, collateral, state, etc.).
Applied to files:
src/governance/governance.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/Makefile.am
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/Makefile.amsrc/rpc/masternode.cppsrc/evo/core_write.cppsrc/rpc/governance.cppsrc/rpc/evo.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/Makefile.amsrc/rpc/masternode.cppsrc/llmq/core_write.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/Makefile.am
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/Makefile.amsrc/rpc/masternode.cppsrc/evo/core_write.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/Makefile.amsrc/llmq/core_write.cppsrc/llmq/debug.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/rpc/coinjoin.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/rpc/masternode.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/rpc/masternode.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/rpc/masternode.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Applied to files:
src/llmq/core_write.cppsrc/llmq/debug.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/llmq/core_write.cppsrc/llmq/debug.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/debug.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/debug.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/core_io.hsrc/rpc/evo.cpp
🧬 Code graph analysis (12)
src/governance/governance.h (2)
src/governance/common.h (1)
nodiscard(41-52)src/governance/core_write.cpp (8)
GetJsonHelp(13-24)GetJsonHelp(13-13)GetJsonHelp(111-135)GetJsonHelp(111-111)ToJson(26-56)ToJson(26-26)ToJson(137-154)ToJson(137-137)
src/rpc/coinjoin.cpp (1)
src/evo/core_write.cpp (2)
GetRpcResult(78-86)GetRpcResult(78-78)
src/rpc/masternode.cpp (1)
src/evo/core_write.cpp (18)
GetRpcResult(78-86)GetRpcResult(78-78)GetJsonHelp(88-103)GetJsonHelp(88-88)GetJsonHelp(124-135)GetJsonHelp(124-124)GetJsonHelp(149-161)GetJsonHelp(149-149)GetJsonHelp(181-193)GetJsonHelp(181-181)GetJsonHelp(195-218)GetJsonHelp(195-195)GetJsonHelp(253-276)GetJsonHelp(253-253)GetJsonHelp(278-298)GetJsonHelp(278-278)GetJsonHelp(325-336)GetJsonHelp(325-325)
src/qt/governancelist.cpp (1)
src/net_types.cpp (1)
json(23-23)
src/llmq/core_write.cpp (1)
src/evo/core_write.cpp (18)
GetJsonHelp(88-103)GetJsonHelp(88-88)GetJsonHelp(124-135)GetJsonHelp(124-124)GetJsonHelp(149-161)GetJsonHelp(149-149)GetJsonHelp(181-193)GetJsonHelp(181-181)GetJsonHelp(195-218)GetJsonHelp(195-195)GetJsonHelp(253-276)GetJsonHelp(253-253)GetJsonHelp(278-298)GetJsonHelp(278-278)GetJsonHelp(325-336)GetJsonHelp(325-325)GetRpcResult(78-86)GetRpcResult(78-78)
src/llmq/debug.h (2)
src/evo/deterministicmns.h (8)
nodiscard(87-115)nodiscard(262-270)nodiscard(272-285)nodiscard(427-430)nodiscard(432-439)nodiscard(507-525)nodiscard(527-545)nodiscard(547-562)src/evo/core_write.cpp (32)
GetJsonHelp(88-103)GetJsonHelp(88-88)GetJsonHelp(124-135)GetJsonHelp(124-124)GetJsonHelp(149-161)GetJsonHelp(149-149)GetJsonHelp(181-193)GetJsonHelp(181-181)GetJsonHelp(195-218)GetJsonHelp(195-195)GetJsonHelp(253-276)GetJsonHelp(253-253)GetJsonHelp(278-298)GetJsonHelp(278-278)GetJsonHelp(325-336)GetJsonHelp(325-325)ToJson(105-122)ToJson(105-105)ToJson(137-147)ToJson(137-137)ToJson(163-178)ToJson(163-163)ToJson(220-250)ToJson(220-220)ToJson(300-323)ToJson(300-300)ToJson(338-350)ToJson(338-338)ToJson(363-371)ToJson(363-363)ToJson(390-408)ToJson(390-390)
src/governance/object.cpp (1)
src/governance/common.cpp (6)
GetDataAsHexString(41-44)GetDataAsHexString(41-41)GetDataAsPlainString(46-49)GetDataAsPlainString(46-46)GetHash(23-39)GetHash(23-23)
src/rpc/governance.cpp (1)
src/governance/core_write.cpp (8)
GetJsonHelp(13-24)GetJsonHelp(13-13)GetJsonHelp(111-135)GetJsonHelp(111-111)GetStateJsonHelp(69-87)GetStateJsonHelp(69-69)GetVotesJsonHelp(89-98)GetVotesJsonHelp(89-89)
src/core_io.h (1)
src/evo/core_write.cpp (2)
GetRpcResult(78-86)GetRpcResult(78-78)
src/governance/common.h (2)
src/governance/governance.h (1)
nodiscard(294-304)src/governance/core_write.cpp (8)
GetJsonHelp(13-24)GetJsonHelp(13-13)GetJsonHelp(111-135)GetJsonHelp(111-111)ToJson(26-56)ToJson(26-26)ToJson(137-154)ToJson(137-137)
src/rpc/evo.cpp (1)
src/evo/core_write.cpp (16)
GetJsonHelp(88-103)GetJsonHelp(88-88)GetJsonHelp(124-135)GetJsonHelp(124-124)GetJsonHelp(149-161)GetJsonHelp(149-149)GetJsonHelp(181-193)GetJsonHelp(181-181)GetJsonHelp(195-218)GetJsonHelp(195-195)GetJsonHelp(253-276)GetJsonHelp(253-253)GetJsonHelp(278-298)GetJsonHelp(278-278)GetJsonHelp(325-336)GetJsonHelp(325-325)
src/governance/object.h (3)
src/governance/governance.h (1)
nodiscard(294-304)src/governance/core_write.cpp (10)
GetInnerJsonHelp(58-61)GetInnerJsonHelp(58-58)GetInnerJson(63-66)GetInnerJson(63-63)GetStateJsonHelp(69-87)GetStateJsonHelp(69-69)GetVotesJsonHelp(89-98)GetVotesJsonHelp(89-89)GetVotesJson(100-108)GetVotesJson(100-100)src/governance/object.cpp (2)
GetStateJson(281-301)GetStateJson(281-281)
🪛 Cppcheck (2.18.0)
src/llmq/core_write.cpp
[error] 80-80: Uninitialized variable
(legacyUninitvar)
src/governance/core_write.cpp
[error] 80-80: Uninitialized variable
(legacyUninitvar)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/coinjoin.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff script (as in the CI step) to fix formatting in this file.
src/rpc/masternode.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff script (as in the CI step) to fix formatting in this file.
src/rpc/quorums.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff script (as in the CI step) to fix formatting in this file.
src/governance/core_write.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff script (as in the CI step) to fix formatting in this file.
src/evo/core_write.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff script (as in the CI step) to fix formatting in this file.
src/rpc/governance.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff script (as in the CI step) to fix formatting in this file.
src/rpc/evo.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff script (as in the CI step) to fix formatting in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (36)
src/qt/governancelist.cpp (1)
111-115: Verify Qt “Proposal Info” dialog still shows the intended fields after switching toGetInnerJson().This likely changes the displayed JSON structure (inner vs full object). If the dialog is meant to show full object metadata (hashes, timestamps, data hex), consider using the full JSON surface, or adjust the dialog label to clarify it’s “inner data”.
src/Makefile.am (1)
908-915: LGTM: new source wired intolibbitcoin_common_a_SOURCES.src/rpc/coinjoin.cpp (1)
411-448: EnsureRPCRESULT_MAPcontains"outpoint"(and that the type/description match the previousSTR_HEXintent).Switching to
GetRpcResult("outpoint")can throw during help/schema construction if the key is absent, and it may subtly change docs text/type if the mapped definition differs.src/llmq/debug.h (1)
15-21: Good: forward-declaringRPCResultkeeps the header lighter.src/governance/common.h (2)
16-19: LGTM: forward declarations reduce header coupling and compile cost.
32-42: LGTM:GetJsonHelp()+[[nodiscard]]onToJson()aligns with the new JSON/RPC help pattern.src/core_io.h (1)
60-62: Good, but please verify all call sites still compile cleanly after the signature change.src/governance/governance.h (1)
37-37: LGTM - Forward declaration and new JSON help infrastructure.The forward declaration of
RPCResultavoids including heavy RPC headers in this governance header. The new staticGetJsonHelp()method and[[nodiscard]]attribute onToJson()align with the broader JSON help infrastructure pattern established in other governance components (e.g.,Object::GetJsonHelpinsrc/governance/core_write.cpp).Also applies to: 291-295
src/governance/object.cpp (1)
281-301: LGTM - New GetStateJson method for governance object state serialization.The implementation correctly:
- Conditionally emits
SigningMasternodeonly for non-default outpoints- Acquires
cs_mainlock viaWITH_LOCKfor theIsValidLocallycall- Emits all expected fields consistent with the JSON help schema
Minor observation: Line 295 uses
strError.c_str()wherestrError(asstd::string) would work directly withpushKV, but this is functionally correct.src/governance/common.cpp (1)
11-50: LGTM - Removal of ToJson() aligns with centralized JSON help infrastructure.The removal of
Object::ToJson()from this file is intentional, as JSON serialization for governance objects is now handled through dedicated helpers insrc/governance/core_write.cpp. The remaining utility methods (GetHash,GetDataAsHexString,GetDataAsPlainString) are preserved and unchanged.src/llmq/core_write.cpp (3)
6-6: Include added for DKG debug types.The include of
<llmq/debug.h>is necessary for the newGetJsonHelpmethod implementations onCDKGDebugSessionStatusandCDKGDebugStatus.
19-78: Well-structured JSON help for DKG session status with detail levels.The implementation correctly documents the three detail levels:
- Level 0: Count fields (badMembers, weComplain, etc. as NUM)
- Level 1: Index arrays (ARR of NUM)
- Level 2: Object arrays with memberIndex and proTxHash
The conditional RPCResult entries using the
"for detail_level = N"prefix pattern is consistent with how Bitcoin Core documents optional/conditional fields.
80-95: CDKGDebugStatus::GetJsonHelp correctly nests session status help.The implementation properly composes the nested structure by calling
CDKGDebugSessionStatus::GetJsonHelp()for the inner session status object.Regarding the static analysis hint (Cppcheck "uninitialized variable" at line 80): this is a false positive. The method is a static function definition returning an
RPCResultobject constructed inline. All parameters are properly initialized.src/rpc/evo.cpp (2)
1660-1665: Improved RPC help documentation using GetJsonHelp helpers.The changes correctly leverage the new
GetJsonHelpinfrastructure:
addedMNs: UsesCDeterministicMN::GetJsonHelpfor full masternode detailsremovedMns: Documents the protx hash string formatupdatedMNs: Correctly documents the nested structure where each element is an object with the proTxHash as key and state diff as valueThis matches the actual JSON output structure in lines 1714-1722.
1-1: [rewritten comment]
[classification tag]src/evo/core_write.cpp (4)
40-53: New RPCRESULT_MAP entries look good.The additions of
collateralAddress,memberIndex,outpoint, andtype_strexpand the centralized RPC result definitions appropriately. These entries follow the established pattern usingRESULT_MAP_ENTRY.
78-86: Nicely implemented override mechanism for field naming.The
override_nameparameter enables backward-compatible field naming while reusing centralized definitions. This is a clean approach for cases where the internal key differs from the desired JSON field name.
503-521: Override name usage in CSimplifiedMNListEntry::GetJsonHelp is correct.Using
GetRpcResult("type", false, "nType")correctly maps the internal "type" key to the output field name "nType", maintaining consistency with the existingToJson()which emitsnType(line 527).
181-193: Consistency between GetJsonHelp and ToJson output is correct.The help structure accurately reflects the actual implementation:
- The
typefield is correctly emitted as a string (viaGetMnType(nType).description)- The
collateralAddressis properly marked optional and conditionally emitted based on transaction lookup and destination extraction availabilitysrc/rpc/masternode.cpp (2)
185-194: Enhanced masternode_status RPC result schema is comprehensive.The expanded result schema using
GetRpcResultandCDeterministicMNState::GetJsonHelpprovides better documentation. The optional markers align with the conditional emission in the implementation (lines 210-216 only emit dmn-related fields whendmnis non-null).
534-564: Comprehensive masternodelist RPC result documentation.The mode-specific result schemas using
GetRpcResultwith override names maintain backward compatibility while providing much-improved RPC help text. The field mappings (address→service,payee→payoutAddress,pospenaltyscore→PoSePenalty, etc.) correctly reflect the actual JSON output produced in the implementation.src/governance/object.h (2)
25-25: Forward declaration avoids header dependency.Forward declaring
RPCResulthere instead of including the full header is the right approach to minimize header dependencies.
264-274: Well-designed JSON emission API with proper annotations.The new JSON helper methods follow good patterns:
[[nodiscard]]prevents accidentally ignoring return values- Separation of
*Help(schema) and actual data emission methodsEXCLUSIVE_LOCKS_REQUIRED(!cs)annotations are consistent with existing methods in this class that useWITH_LOCK(cs, ...)- The
local_valid_keyparameter allows callers to customize the validity field name for different contextssrc/rpc/governance.cpp (6)
41-48: Effective reuse of GetJsonHelp for RPC result schema.Using
CGovernanceManager::GetJsonHelp()and extracting its fields for the RPCResult ensures the help text stays in sync with the actual implementation.
269-269: Consistent use of GetJsonHelp for gobject_list_prepared.Using
Governance::Object::GetJsonHelpfor the array element schema maintains consistency with the actualToJson()output.
634-651: ListObjects refactoring correctly combines state and vote data.The implementation properly:
- Gets the state JSON via
GetStateJson- Gets vote data via
GetVotesJson- Extracts individual vote counts and adds them to the entry
This maintains backward compatibility with the existing output format while using the new helper infrastructure.
656-664: ListObjectsHelp dynamically builds combined schema.This helper correctly constructs a combined schema from
GetStateJsonHelpandGetVotesJsonHelp, reflecting the merged output produced byListObjects.
723-732: gobject_get_help correctly describes nested vote result blocks.The helper appropriately wraps each
GetVotesJsonHelpin a named object (FundingResult,ValidResult,DeleteResult,EndorsedResult) matching the actual output structure.
752-764: gobject_get implementation cleanly uses new JSON helpers.The refactored implementation:
- Uses
GetStateJsonwith"fLocalValidity"as the validity key- Appends all four vote result blocks using
GetVotesJsonThis is cleaner than the previous manual field assembly while producing identical output.
src/governance/core_write.cpp (5)
13-24: CGovernanceManager::GetJsonHelp provides clear schema.The schema correctly documents all fields emitted by
ToJson():objects_total,proposals,triggers,other,erased, andvotes.
26-56: CGovernanceManager::ToJson implementation is correct.The implementation:
- Properly acquires
cs_storelock- Uses
Assert(govobj)for null check (consistent with codebase patterns)- Iterates objects and categorizes by type
- Returns counts matching the schema in
GetJsonHelp
68-87: GetStateJsonHelp schema is comprehensive.The static analysis warning about an uninitialized variable at line 80 is a false positive—
local_valid_keyis aconst std::string¶meter that is always initialized by the caller.The schema correctly documents all fields including the dynamic validity key name, which enables context-specific naming (
fLocalValidityvsfBlockchainValidity).
100-108: GetVotesJson correctly retrieves vote counts.The implementation properly calls the existing vote counting methods (
GetAbsoluteYesCount,GetYesCount,GetNoCount,GetAbstainCount) with the provided signal type.
137-154: Governance::Object::ToJson handles parse failures gracefully.The fallback logic at lines 146-150 properly handles cases where
GetDataAsPlainString()returns non-JSON data by:
- Clearing the failed parse result
- Creating a new object
- Adding the raw string under
"plain"keyThe
"hex"field is always emitted, providing a guaranteed machine-readable representation.src/rpc/quorums.cpp (2)
331-331: LGTM!Clean refactoring that delegates the RPCResult construction to the new helper function. This improves maintainability by centralizing the schema definition.
297-319: Fix the unmatched opening braces in thequorum_dkgstatus_help()function.The function has 2 unmatched opening braces (19 opening vs 17 closing). The code will not compile in this state. Review the nested structure at lines 303-319 and add the missing closing braces to properly close the
mod_inner.push_back()calls.⛔ Skipped due to learnings
Learnt from: CR Repo: dashpay/dash PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-24T16:41:22.457Z Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
|
This pull request has conflicts, please rebase. |
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Also gives us the help text for `gobject list-prepared`
Also gives us the help text for `gobject {diff,list}`
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 09b65a4ec9d12bcd08a7e5acfa0efbc6522a25c5 and 562aeb7.
📒 Files selected for processing (18)
src/Makefile.am(1 hunks)src/core_io.h(1 hunks)src/evo/core_write.cpp(4 hunks)src/governance/common.cpp(1 hunks)src/governance/common.h(1 hunks)src/governance/core_write.cpp(1 hunks)src/governance/governance.cpp(0 hunks)src/governance/governance.h(2 hunks)src/governance/object.cpp(1 hunks)src/governance/object.h(2 hunks)src/llmq/core_write.cpp(2 hunks)src/llmq/debug.h(3 hunks)src/qt/governancelist.cpp(1 hunks)src/rpc/coinjoin.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/governance.cpp(6 hunks)src/rpc/masternode.cpp(2 hunks)src/rpc/quorums.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- src/governance/governance.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/core_io.h
- src/governance/common.h
- src/governance/object.h
- src/llmq/debug.h
- src/qt/governancelist.cpp
- src/rpc/evo.cpp
- src/governance/core_write.cpp
- src/Makefile.am
- src/governance/governance.h
🧰 Additional context used
📓 Path-based instructions (8)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/governance/common.cppsrc/governance/object.cppsrc/llmq/core_write.cppsrc/rpc/quorums.cppsrc/rpc/governance.cppsrc/rpc/masternode.cppsrc/rpc/coinjoin.cppsrc/evo/core_write.cpp
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/governance/common.cppsrc/governance/object.cppsrc/llmq/core_write.cppsrc/evo/core_write.cpp
src/governance/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Files:
src/governance/common.cppsrc/governance/object.cpp
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/governance/common.cppsrc/governance/object.cppsrc/llmq/core_write.cppsrc/evo/core_write.cpp
src/{masternode,llmq}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
BLS integration must be used for cryptographic foundation of advanced masternode features
Files:
src/llmq/core_write.cpp
src/llmq/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/llmq/**/*.{cpp,h}: LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Files:
src/llmq/core_write.cpp
src/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/core_write.cpp
src/evo/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Files:
src/evo/core_write.cpp
🧠 Learnings (13)
📓 Common learnings
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:0-0
Timestamp: 2025-08-08T04:30:37.971Z
Learning: In the Dash codebase, direct RPC calling has been removed in recent commits, making suggestions about RPC command validation and error handling for direct RPC calls obsolete. The ProposalWizard and related components no longer use direct RPC calling patterns.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Applied to files:
src/governance/common.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
src/governance/common.cppsrc/governance/object.cppsrc/rpc/governance.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/governance/object.cppsrc/rpc/masternode.cppsrc/evo/core_write.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/llmq/core_write.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Applied to files:
src/llmq/core_write.cppsrc/rpc/quorums.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/llmq/core_write.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/llmq/core_write.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/governance.cppsrc/rpc/masternode.cppsrc/rpc/coinjoin.cppsrc/evo/core_write.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/rpc/masternode.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/rpc/masternode.cppsrc/evo/core_write.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/rpc/masternode.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/rpc/coinjoin.cpp
🔇 Additional comments (19)
src/governance/common.cpp (2)
11-11: LGTM: Cosmetic namespace formatting.The single-line namespace declaration is standard practice and improves readability.
11-50: Verify that allObject::ToJson()call sites have been migrated.The removal of the
Object::ToJson()method is a public API change. Ensure that all code previously calling this method has been updated to use the newGetJsonHelp()pattern introduced in this refactoring.src/rpc/coinjoin.cpp (1)
434-434: Verify schema consistency for GetRpcResult("outpoint") refactoring.This change centralizes RPC result descriptors by replacing an explicit RPCResult entry with
GetRpcResult("outpoint"). Please confirm that the helper function returns a schema equivalent to the previous definition and that backward compatibility is maintained for the RPC API.src/evo/core_write.cpp (3)
78-86: LGTM! Clean override mechanism for key name aliasing.The
override_nameparameter provides a clean way to emit different JSON key names while reusing the centralizedRPCRESULT_MAPdefinitions. This supports backward compatibility and more descriptive human-readable labels without duplicating entries.
40-53: LGTM! New RPC result entries are well-defined.The new entries for
collateralAddress,memberIndex,outpoint, andtype_strfollow the established pattern and provide appropriate types and descriptions.
185-192: LGTM! Consistent use of override_name for field aliasing.The aliasing of
type_strtotypemaintains the human-readable output format while leveraging the centralized definition. ThecollateralAddressbeing optional is appropriate as the address may not always be resolvable.src/rpc/masternode.cpp (2)
185-194: LGTM! Comprehensive RPC result schema for masternode_status.The expanded schema accurately documents the actual output fields, including the optional fields when a DMN is found. The use of
CDeterministicMNState::GetJsonHelpfor the nesteddmnStateobject is a clean approach.
534-564: LGTM! Accurate mode-based RPC result schema.The schema correctly documents the different output formats for each mode. The field name overrides (e.g.,
service→address,payoutAddress→payee,PoSePenalty→pospenaltyscore) accurately match the actual JSON keys emitted in the implementation.src/llmq/core_write.cpp (1)
19-78: LGTM! Comprehensive DKG session status schema with detail level variants.The schema correctly documents all three detail levels (0=counts, 1=indexes, 2=objects) for the DKG status fields. The use of conditional
RPCResultentries clearly indicates which fields appear at each detail level.src/rpc/quorums.cpp (2)
297-319: LGTM! Clean helper for extending DKG status schema.The
quorum_dkgstatus_help()function elegantly extends the baseCDKGDebugStatus::GetJsonHelpschema by appending thequorumConnectionsandminableCommitmentsarrays. This approach avoids duplication while maintaining the ability to augment RPC-specific output fields.
58-61: LGTM! Helpful additional examples.The added examples demonstrate the optional
countparameter usage, improving documentation for users.src/governance/object.cpp (2)
281-301: LGTM! Clean state JSON generation with configurable validity key.The
GetStateJsonmethod properly consolidates governance object state serialization. Thelocal_valid_keyparameter elegantly addresses the differing key names (fBlockchainValidityvsfLocalValidity) mentioned in the PR description, enabling the caller to specify the appropriate key for their context. Thecs_mainlock acquisition forIsValidLocallyis correctly handled.
290-292: LGTM! Correct conditional for SigningMasternode.The conditional inclusion of
SigningMasternodeonly when the outpoint is valid is correct—proposals don't have signing masternodes, while triggers do.src/rpc/governance.cpp (6)
41-48: LGTM: Clean refactoring to helper-based help generation.The use of
CGovernanceManager::GetJsonHelp()to generate help text is a good improvement over manual field-by-field documentation and aligns with the PR's goal of deduplication.
269-269: LGTM: Consistent use of helper-based help generation.
656-664: LGTM: Well-structured help text composition.The
ListObjectsHelp()function cleanly combines state and vote help text by unpacking the inner elements. This should produce help text matching the flattened structure returned byListObjects().Also applies to: 680-680
723-732: LGTM: Correct help structure for gobject_get.The helper correctly constructs help text with:
- State fields using "fLocalValidity" key (as intended per PR description to distinguish from gobject list/diff)
- Four nested vote result objects
This matches the implementation pattern in the RPC handler.
741-764: LGTM: Implementation matches help text structure.The refactored implementation correctly:
- Uses
GetStateJsonwith "fLocalValidity" key for consistency with help text- Adds four complete vote result objects (FundingResult, ValidResult, DeleteResult, EndorsedResult)
- Matches the structure defined in
gobject_get_help()The pattern here (adding full nested objects) differs intentionally from
gobject_list/gobject_diff(which flatten specific vote counts), providing more detailed information for the single-object query.
644-650: Verify that vote keys are always present and validate help text accuracy.The code extracts specific keys from
votes_fundingwithout checking if they exist. IfGetVotesJsondoesn't return these keys,UniValuewill create VNULL entries, which could break client expectations.Additionally, the implementation only extracts 4 specific vote count fields, while
ListObjectsHelp()(line 660) appends all inner fields fromGetVotesJsonHelp(). Verify that:
GetVotesJson(VOTE_SIGNAL_FUNDING)always returns these keys: AbsoluteYesCount, YesCount, NoCount, AbstainCountGetVotesJsonHelp()describes only these fields (not additional fields that aren't extracted here)Run this script to verify the consistency between GetVotesJson implementation and its usage:
#!/bin/bash # Verify GetVotesJson returns the expected keys and GetVotesJsonHelp matches # Find GetVotesJson implementation echo "=== GetVotesJson implementation ===" ast-grep --pattern 'GetVotesJson($$$) { $$$ }' # Find GetVotesJsonHelp implementation echo -e "\n=== GetVotesJsonHelp implementation ===" ast-grep --pattern 'GetVotesJsonHelp($$$) { $$$ }' # Search for the keys being accessed to understand the contract echo -e "\n=== Keys accessed from GetVotesJson ===" rg -n 'AbsoluteYesCount|YesCount|NoCount|AbstainCount' --type cpp -C 2
| {RPCResult::Type::ARR, "session", "", { | ||
| {RPCResult::Type::OBJ, "", "", { | ||
| {RPCResult::Type::NUM, "llmqType", "Name of quorum"}, | ||
| GetRpcResult("quorumIndex"), | ||
| CDKGDebugSessionStatus::GetJsonHelp(/*key=*/"status", /*optional=*/false) | ||
| }}, | ||
| }} |
There was a problem hiding this comment.
Minor: Misleading description for llmqType field.
Line 89 defines llmqType as RPCResult::Type::NUM with description "Name of quorum", but a numeric type should describe the quorum type identifier, not the name. Consider changing the description to "Quorum type" for consistency with the GetRpcResult("llmqType") definition used elsewhere.
- {RPCResult::Type::NUM, "llmqType", "Name of quorum"},
+ {RPCResult::Type::NUM, "llmqType", "Quorum type"},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {RPCResult::Type::ARR, "session", "", { | |
| {RPCResult::Type::OBJ, "", "", { | |
| {RPCResult::Type::NUM, "llmqType", "Name of quorum"}, | |
| GetRpcResult("quorumIndex"), | |
| CDKGDebugSessionStatus::GetJsonHelp(/*key=*/"status", /*optional=*/false) | |
| }}, | |
| }} | |
| {RPCResult::Type::ARR, "session", "", { | |
| {RPCResult::Type::OBJ, "", "", { | |
| {RPCResult::Type::NUM, "llmqType", "Quorum type"}, | |
| GetRpcResult("quorumIndex"), | |
| CDKGDebugSessionStatus::GetJsonHelp(/*key=*/"status", /*optional=*/false) | |
| }}, | |
| }} |
🤖 Prompt for AI Agents
In src/llmq/core_write.cpp around lines 87 to 93, the RPC field llmqType is
defined as RPCResult::Type::NUM but described as "Name of quorum" which is
misleading; update the description string to "Quorum type" (or "Quorum type
identifier") so it accurately reflects the numeric identifier and matches the
GetRpcResult("llmqType") usage elsewhere.
…ns for `quorum dkg{info,status}`, move `CDKGDebugManager` to `{Active,Observer}Context`
e98fe4f doc: add release notes for breaking changes (Kittywhiskers Van Gogh)
f5a1617 refactor: move `CDKGDebugManager` to `{Active,Observer}Context` (Kittywhiskers Van Gogh)
9d09f91 rpc: add watch-only/masternode mode limits for `quorum dkg{info,status}` (Kittywhiskers Van Gogh)
d9fe923 test: only call `quorum dkg{info,status}` on nodes that can see DKG (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on #7056
* Depends on #7061
* Dependency for #7063
## Breaking Changes
See release notes.
## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK e98fe4f
Tree-SHA512: c1430f8b9714ccd000552bd4116d2d339d483d6bfa206c6a01973acaf36da86639afc88342f6d1f80f738929c3f5b6068eee38d36cb5964765dcc6175a998d4d
4 participants
Motivation
dash#7062 introduces breaking changes. To convey the change in return object parameter's optional status, we need to be able to emit complete documentation to begin with. This pull request aims to fill up the remaining gaps in Dash-specific RPC documentation (which were thankfully annotated with TODOs courtesy of dash#6886, thanks knst!).
This is a continuation of the work started in dash#6872.
Additional Information
Dependency for refactor(rpc): add watch-only/masternode mode restrictions for
quorum dkg{info,status}, moveCDKGDebugManagerto{Active,Observer}Context#7062quorum dkgstatusrequires additional work to generate its help text as the RPC does something unusual, it takes the object returned by CDKGDebugStatus (source) and modifies it (source) instead of treating it as a distinct value paired to a key.This necessitated defining a separate function to generate the help text for
quorum dkgstatus,quorum_dkgstatus_help(), that has to mirror this mutating behavior due to theconst-only structure ofRPCResult(source).gobject getthough it is a product of deduplication done in this PR and not a decision made when the RPC was first introduced.Two governance RPCs,
gobject {diff,list}andgobject get, refer to the same value, local validity status, with different keys,fBlockchainValidityandfLocalValidityrespectively. This required a workaround to maintain deduplication by allowing to overwrite the key name after fetching the description using the key inGetRpcResultby specifyingoverride_name.Breaking Changes
None expected.
Checklist