backport: bitcoin#24408, #24959, #25161, #25170, #25237 #6887
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a build-time preprocessor flag and runtime option to enable RPC documentation checks: configure.ac adds AX_CHECK_PREPROC_FLAG for -DRPC_DOC_CHECK; src/rpc/util.h defines Sequence Diagram(s)sequenceDiagram
participant Client
participant RPCHelper as RPCHelpMan::HandleRequest
participant Validator as RPCResult Validator
participant Result
Client->>RPCHelper: Invoke RPC method
RPCHelper->>RPCHelper: Compute ret (RPC result)
alt RPC_DOC_CHECK built-in AND -rpcdoccheck enabled at runtime
rect rgb(230,245,255)
RPCHelper->>Validator: Validate ret against RPCResult schema
Validator-->>RPCHelper: Validation outcome (logged, non-fatal)
end
end
RPCHelper-->>Result: Return ret
Result-->>Client: Respond
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/fuzz/rpc.cpp (1)
128-141: Remove the duplicate entry.The command "gettxspendingprevout" appears twice in
RPC_COMMANDS_SAFE_FOR_FUZZING(lines 128 and 141). This duplication will cause the validation check at lines 331-334 to fail with an error stating the command is found in both lists.Apply this diff to remove the duplicate:
"getmempoolentry", - "gettxspendingprevout", "getmempoolinfo",src/rpc/rawtransaction.cpp (1)
375-379: Fix RPC example: missing trailing quoteThe RPC example is missing the closing single-quote.
Apply this diff:
-+ HelpExampleRpc("getrawtransactionmulti", -+ R"('{"blockhash1":["txid1","txid2"], "0":["txid3"]})") ++ HelpExampleRpc("getrawtransactionmulti", ++ R"('{"blockhash1":["txid1","txid2"], "0":["txid3"]}')")
🧹 Nitpick comments (5)
src/rpc/util.cpp (1)
540-545: Good addition: non-fatal runtime doc/type check
- Properly gated by -rpcdoccheck; won’t affect normal flow.
- Note: if m_results is empty, CHECK_NONFATAL will trigger. Ensure all RPCs define at least one RPCResult when enabling the flag.
- Minor: avoid an extra UniValue copy.
- const UniValue ret = m_fun(*this, request); + const UniValue& ret = m_fun(*this, request);Please confirm intent regarding empty m_results causing a NonFatalCheckError when -rpcdoccheck=1.
src/rpc/mempool.cpp (1)
325-327: Consider emitting a boolean instead of string for instantlockCurrently writes "true"/"false"/"unknown" strings. Prefer a boolean (and omit the field if unknown) for type stability.
- info.pushKV("instantlock", isman ? (isman->IsLocked(tx.GetHash()) ? "true" : "false") : "unknown"); + if (isman) { + info.pushKV("instantlock", isman->IsLocked(tx.GetHash())); + }If adopting this, update the help to Type::BOOL and mark it optional.
src/rpc/quorums.cpp (2)
106-123: Prefer OBJ_DYN for dynamic quorum-hash keyed objectsThe listextended result nests an object keyed by dynamic quorum hashes but uses a placeholder field name "xxxx". Use RPCResult::Type::OBJ_DYN like getrawmempool’s verbose path to better reflect dynamic keys in doc checks.
780-788: Document recoveryMembers in quorum selectquorumThe RPC returns “recoveryMembers” (Lines 812–818) but the help omits it. Add it to the result schema for completeness.
RPCResult{ RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR_HEX, "quorumHash", "Hash of chosen quorum"}, + {RPCResult::Type::ARR, "recoveryMembers", "List of members selected for signature recovery", + {{RPCResult::Type::STR_HEX, "", "ProTx hash of member"}}}, } },src/rpc/rawtransaction.cpp (1)
361-373: Result schema matches implementation; consider OBJ_DYN for clarityDocs align with returned hex/object/"None". Optionally, use OBJ_DYN with a dynamic "txid" key to mirror map semantics more explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
configure.ac(1 hunks)doc/release-notes/release-notes-24408.md(1 hunks)src/init.cpp(1 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/coinjoin.cpp(3 hunks)src/rpc/evo.cpp(6 hunks)src/rpc/governance.cpp(15 hunks)src/rpc/masternode.cpp(7 hunks)src/rpc/mempool.cpp(2 hunks)src/rpc/mining.cpp(1 hunks)src/rpc/net.cpp(1 hunks)src/rpc/node.cpp(1 hunks)src/rpc/quorums.cpp(17 hunks)src/rpc/rawtransaction.cpp(2 hunks)src/rpc/server.cpp(1 hunks)src/rpc/txoutproof.cpp(0 hunks)src/rpc/util.cpp(2 hunks)src/rpc/util.h(1 hunks)src/test/fuzz/rpc.cpp(1 hunks)src/wallet/rpc/wallet.cpp(3 hunks)src/zmq/zmqrpc.cpp(1 hunks)test/functional/mempool_packages.py(1 hunks)test/functional/rpc_mempool_info.py(1 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/test_runner.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/rpc/txoutproof.cpp
🧰 Additional context used
📓 Path-based instructions (4)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/mempool_packages.pytest/functional/test_runner.pytest/functional/rpc_mempool_info.pytest/functional/test_framework/util.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/util.hsrc/rpc/node.cppsrc/rpc/util.cppsrc/rpc/coinjoin.cppsrc/rpc/mining.cppsrc/init.cppsrc/rpc/blockchain.cppsrc/rpc/client.cppsrc/rpc/server.cppsrc/wallet/rpc/wallet.cppsrc/rpc/net.cppsrc/rpc/evo.cppsrc/rpc/quorums.cppsrc/test/fuzz/rpc.cppsrc/rpc/masternode.cppsrc/rpc/mempool.cppsrc/rpc/governance.cppsrc/zmq/zmqrpc.cppsrc/rpc/rawtransaction.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes/release-notes-24408.md
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/fuzz/rpc.cpp
🔇 Additional comments (40)
doc/release-notes/release-notes-24408.md (1)
1-5: LGTM!The release notes clearly document the new
gettxspendingprevoutRPC and its purpose.src/zmq/zmqrpc.cpp (2)
34-34: LGTM!Adding the RPC example improves documentation completeness for the
getzmqnotificationsRPC.
54-56: LGTM!The compact initializer list format aligns with the broader refactoring across RPC command registrations in this PR.
test/functional/test_framework/util.py (1)
393-393: LGTM!Adding
rpcdoccheck=1to the test configuration enables runtime RPC documentation validation during functional tests, which aligns with the new-rpcdoccheckflag introduced in this PR.src/rpc/client.cpp (1)
202-202: LGTM!The conversion entry for
gettxspendingprevoutfollows the established pattern and correctly specifies that the first parameter ("outputs") needs JSON conversion.test/functional/mempool_packages.py (1)
102-106: LGTM!The consistency check validates that
gettxspendingprevoutcorrectly identifies spending transactions in the mempool, providing good test coverage for the new RPC.test/functional/test_runner.py (1)
395-395: LGTM!Adding
rpc_mempool_info.pyto the extended test suite expands coverage for mempool-related RPCs, including the newly introducedgettxspendingprevout.src/rpc/util.h (1)
26-32: LGTM!The
DEFAULT_RPC_DOC_CHECKconstant is correctly defined usingstatic constexprwith internal linkage, which is appropriate for a header file constant. The conditional compilation based onRPC_DOC_CHECKenables compile-time control of runtime RPC documentation validation.src/rpc/util.cpp (1)
17-17: Include looks correctNeeded for gArgs usage below. No issues.
src/rpc/blockchain.cpp (1)
2762-2795: RPC registration table LGTMConsistent categories, complete set of blockchain commands. No functional concerns.
src/rpc/mining.cpp (1)
1097-1108: RPC registration table LGTMCommand set unchanged and correctly categorized. Good cleanup.
src/rpc/node.cpp (1)
828-828: Explicit NONE result is correctMatches returned NullUniValue and aligns with help validation.
src/rpc/server.cpp (1)
270-276: Command list init LGTMStyle-only change; bindings preserved.
src/wallet/rpc/wallet.cpp (3)
99-99: Explicit NONE result is correctSwitching to RPCResult::Type::NONE aligns with commands returning NullUniValue. No behavior change.
130-130: Explicit NONE result is correctConsistent with returning NullUniValue; matches RPC doc checks.
1156-1226: Command table refactor LGTMStatic const initializer improves clarity and removes clang-format guards; command set/order preserved.
src/rpc/mempool.cpp (3)
596-677: New RPC gettxspendingprevout: solid implementationStrict arg checking, negative vout guard, mempool-locked lookup, and per-prevout result shaping look correct. Matches the functional test expectations.
774-775: Registration LGTMgettxspendingprevout properly registered under “blockchain”.
236-268: RPC result doc/type mismatches: instantsend vs instantlock; fee unitsTwo issues that will trip -rpcdoccheck:
- Field name/type: help documents “instantsend” as BOOL, but code emits “instantlock” as a string ("true"/"false"/"unknown") in entryToJSON.
- Deprecated fee fields “descendantfees” and “ancestorfees” are documented as STR_AMOUNT but code emits numeric satoshis (no ValueFromAmount), and the description says “CURRENCY_ATOMs”.
Fix the help to match actual output:
@@ - RPCResult{RPCResult::Type::STR_AMOUNT, "descendantfees", /*optional=*/true, - "transaction fees of in-mempool descendants (including this one) with fee deltas used for mining priority, denominated in " + - CURRENCY_ATOM + "s (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, + RPCResult{RPCResult::Type::NUM, "descendantfees", /*optional=*/true, + "transaction fees of in-mempool descendants (including this one) with fee deltas used for mining priority, denominated in " + + CURRENCY_ATOM + "s (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, @@ - RPCResult{RPCResult::Type::NUM, "ancestorcount", "number of in-mempool ancestor transactions (including this one)"}, + RPCResult{RPCResult::Type::NUM, "ancestorcount", "number of in-mempool ancestor transactions (including this one)"}, @@ - RPCResult{RPCResult::Type::STR_AMOUNT, "ancestorfees", /*optional=*/true, - "transaction fees of in-mempool ancestors (including this one) with fee deltas used for mining priority, denominated in " + - CURRENCY_ATOM + "s (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, + RPCResult{RPCResult::Type::NUM, "ancestorfees", /*optional=*/true, + "transaction fees of in-mempool ancestors (including this one) with fee deltas used for mining priority, denominated in " + + CURRENCY_ATOM + "s (DEPRECATED, returned only if config option -deprecatedrpc=fees is passed)"}, @@ - RPCResult{RPCResult::Type::BOOL, "instantsend", "True if this transaction was locked via InstantSend"}, + RPCResult{RPCResult::Type::STR, "instantlock", "InstantSend lock status: \"true\", \"false\", or \"unknown\""},Alternative (if you prefer strong typing): change entryToJSON to emit a boolean field named “instantsend” (and omit it when unknown), and keep the original help. Happy to provide that diff if preferred.
src/rpc/evo.cpp (2)
1889-1909: Wallet evo command table refactor LGTMInitializer list is clear; same commands preserved.
1915-1922: Non-wallet command registration refactor LGTMSeparation between always-available and wallet-dependent commands is correct and preserved.
src/rpc/quorums.cpp (1)
245-251: quorum info result doc uses ELISION: acceptableUsing ELISION is fine given the detailed, versioned structure.
test/functional/rpc_mempool_info.py (1)
16-102: Good coverage for gettxspendingprevoutBuilds a mempool DAG and validates spent/unspent cases and error paths. Matches the RPC behavior and strict arg checking.
src/rpc/rawtransaction.cpp (1)
1923-1944: LGTM: registration table modernizationInitializer list + append loop is consistent and correct.
src/rpc/masternode.cpp (4)
52-55: LGTM: return shape for connectReturns a simple status string as documented.
184-189: LGTM: ELISION placeholder for statusAcceptable interim schema until a strict schema is defined.
722-725: LGTM: wallet masternode commands registrationInitializer list is correct.
731-740: LGTM: masternode commands registrationRegistration table looks consistent.
src/rpc/governance.cpp (12)
119-124: LGTM: simple “OK” status schemaMatches implementation.
174-175: LGTM: prepare returns hex stringDoc matches returning the collateral txid.
285-293: LGTM: list-prepared returns array of objectsSchema aligns with code.
523-538: LGTM: shared vote result schemaReusable object matches VoteWithMasternodes output.
550-551: LGTM: use shared vote schema (vote-many)Consistent with helper output.
603-604: LGTM: use shared vote schema (vote-alias)Consistent with helper output.
783-790: LGTM: gobject_get returns an objectDoc aligns with implementation.
887-891: LGTM: current votes mapSchema matches “hash -> presentation string”.
957-958: LGTM: umbrella command returns NONEMatches throwing behavior.
979-979: LGTM: voteraw returns a stringAligned with implementation.
1132-1137: LGTM: wallet governance commands registrationInitializer list is fine.
1145-1157: LGTM: governance commands registrationLooks correct.
src/rpc/governance.cpp
Outdated
| { | ||
| RPCResult{ | ||
| RPCResult::Type::OBJ, "", "Deserialized governance object", | ||
| { | ||
| // TODO: list fields of output for RPC help instead ELISION | ||
| {RPCResult::Type::ELISION, "", ""} | ||
| } | ||
| }, | ||
| RPCResult{"for mode = all", | ||
| RPCResult::Type::STR, "", "The transaction hash in hex" | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Deserialize returns string, but doc says object; also stray “mode = all”
Either the code should return an object or the doc should say STR. Prefer returning the parsed object.
Apply these diffs:
Change the result doc and remove the irrelevant alternate:
- {
- RPCResult{
- RPCResult::Type::OBJ, "", "Deserialized governance object",
- {
- // TODO: list fields of output for RPC help instead ELISION
- {RPCResult::Type::ELISION, "", ""}
- }
- },
- RPCResult{"for mode = all",
- RPCResult::Type::STR, "", "The transaction hash in hex"
- },
- },
+ RPCResult{
+ RPCResult::Type::OBJ, "", "Deserialized governance object",
+ {
+ // TODO: list fields of output for RPC help instead ELISION
+ {RPCResult::Type::ELISION, "", ""}
+ }
+ },Return the parsed object:
- return u.write().c_str();
+ return u;🤖 Prompt for AI Agents
In src/rpc/governance.cpp around lines 83 to 94, the RPC result currently
documents an OBJ but actually returns a STR and contains a stray alternate entry
"for mode = all"; update the implementation to return the parsed governance
object and adjust the RPCResult documentation accordingly: change the RPCResult
to Type::OBJ and replace the ELISION placeholder with concrete field entries
that mirror the parsed governance object fields (e.g., governance id, time,
payload, signature, status — list actual struct members), remove the stray
alternate RPCResult for "for mode = all", and ensure the function returns the
constructed object representation rather than a hex/string value so the doc and
output match.
| RPCResult{ | ||
| RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::NUM, "time", "Adjusted time for the last update, timestamp"}, | ||
| {RPCResult::Type::STR, "timeStr", "Adjusted time for the last update, human friendly"}, | ||
| {RPCResult::Type::ARR, "session", "", | ||
| { | ||
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::NUM, "llmqType", "Name of quorum"}, | ||
| {RPCResult::Type::NUM, "quorumIndex", "Relevant for rotation quorums only, for non rotating quorums is 0"}, | ||
| {RPCResult::Type::OBJ, "status", "", | ||
| { | ||
| // TODO: list fields of output for RPC help instead ELISION | ||
| {RPCResult::Type::ELISION, "", ""}, | ||
| }}, | ||
| }}, | ||
| }, | ||
| }, | ||
| {RPCResult::Type::ARR, "quorumConnections", "", | ||
| // TODO: list fields of output for RPC help instead ELISION | ||
| {{RPCResult::Type::ELISION, "", ""}}, | ||
| }, | ||
| {RPCResult::Type::ARR, "mineableCommitments", "", | ||
| // TODO: list fields of output for RPC help instead ELISION | ||
| {{RPCResult::Type::ELISION, "", ""}}, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Typo in documented key: “mineableCommitments” vs code “minableCommitments”
Help says “mineableCommitments”, but code produces key “minableCommitments” (Lines 343, 407). Align the help to match the output or rename the output; recommend updating the help to avoid breaking clients.
- {RPCResult::Type::ARR, "mineableCommitments", "",
+ {RPCResult::Type::ARR, "minableCommitments", "",
// TODO: list fields of output for RPC help instead ELISION
{{RPCResult::Type::ELISION, "", ""}},
},📝 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{ | |
| RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::NUM, "time", "Adjusted time for the last update, timestamp"}, | |
| {RPCResult::Type::STR, "timeStr", "Adjusted time for the last update, human friendly"}, | |
| {RPCResult::Type::ARR, "session", "", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::NUM, "llmqType", "Name of quorum"}, | |
| {RPCResult::Type::NUM, "quorumIndex", "Relevant for rotation quorums only, for non rotating quorums is 0"}, | |
| {RPCResult::Type::OBJ, "status", "", | |
| { | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {RPCResult::Type::ELISION, "", ""}, | |
| }}, | |
| }}, | |
| }, | |
| }, | |
| {RPCResult::Type::ARR, "quorumConnections", "", | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {{RPCResult::Type::ELISION, "", ""}}, | |
| }, | |
| {RPCResult::Type::ARR, "mineableCommitments", "", | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {{RPCResult::Type::ELISION, "", ""}}, | |
| }, | |
| }, | |
| }, | |
| RPCResult{ | |
| RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::NUM, "time", "Adjusted time for the last update, timestamp"}, | |
| {RPCResult::Type::STR, "timeStr", "Adjusted time for the last update, human friendly"}, | |
| {RPCResult::Type::ARR, "session", "", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::NUM, "llmqType", "Name of quorum"}, | |
| {RPCResult::Type::NUM, "quorumIndex", "Relevant for rotation quorums only, for non rotating quorums is 0"}, | |
| {RPCResult::Type::OBJ, "status", "", | |
| { | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {RPCResult::Type::ELISION, "", ""}, | |
| }}, | |
| }}, | |
| }, | |
| }, | |
| {RPCResult::Type::ARR, "quorumConnections", "", | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {{RPCResult::Type::ELISION, "", ""}}, | |
| }, | |
| {RPCResult::Type::ARR, "minableCommitments", "", | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {{RPCResult::Type::ELISION, "", ""}}, | |
| }, | |
| }, | |
| }, |
9af4c55 fix: typos and capitalizations (Konstantin Akimov) 690ff3a fix: apply suggestions from code review (Konstantin Akimov) 6e89094 fix: multiple code-review comments (Konstantin Akimov) 661db51 fix: add missing RPC help result for quorum RPCs (Konstantin Akimov) 72284b5 fix: add missing RPC help result for RPC getrawtransactionmulti (Konstantin Akimov) 7d6b606 fix: add missing RPC help result for evo RPCs (Konstantin Akimov) 3b4095c fix: add proper RPC result for governance RPC (Konstantin Akimov) b216d13 fix: set proper RPCResult for coinjoin RPC (Konstantin Akimov) b174fe0 fix: add missing RPC help result for RPC mockscheduler (Konstantin Akimov) cc1a8fd fix: add missing RPC help result for masternode RPCs (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented For many dash specific RPC we use stub `RPCResults` instead real documentation for returned values. It was possible because some important changes from backport bitcoin#20495 has been skipped. ## What was done? Replaced all stub RPCResults{} in RPC code to real help, or map with ELISION inside. For some RPC replaced ELISION to real description. ## How Has This Been Tested? See PR #6887: done extra testing with backports bitcoin#20495, bitcoin#25161, bitcoin#25170, bitcoin#24408, bitcoin#25237 and fixed all failures in functional tests. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK 9af4c55 kwvg: utACK 9af4c55 Tree-SHA512: 7b8842255050a9154ea4e2d8e7245888dfe96543c856b0e299b95e861156a2fc22a20abb882153ede75f901cfe420df344bbfd1c82364a7b44eb357c38fab4bb
|
This pull request has conflicts, please rebase. |
1 similar comment
|
This pull request has conflicts, please rebase. |
… runtime flag b953ea6 rpc: Put undocumented JSON failure mode behind a runtime flag (Suhail Saqan) Pull request description: Fixes bitcoin#24695 (Put undocumented JSON failure mode behind a runtime flag) ACKs for top commit: luke-jr: utACK b953ea6 vincenzopalazzo: ACK bitcoin@b953ea6 Tree-SHA512: 2005ee1b1f3b637918390b2ecd4166f2fd8c86e3c59fba3da8a0cbd5b1dffd03190c92f6dca3c489ecce4276eaf3108b2edcf9cd6224b713adb52f5bb848163b
fafae67 build: Enable RPC_DOC_CHECK on --enable-debug (MacroFake) Pull request description: This probably makes no large difference, as the setting is already enabled by default in the functional tests. However, I think it is nice to also enable it in debug builds by default to catch issues while manually testing without the runtime flags specified. See also bitcoin#24709 ACKs for top commit: vincenzopalazzo: utACK bitcoin@fafae67 Tree-SHA512: cea3276fc9b5a3bc0f6d9819be9a50b19ecf762729d3e3975abdf00da06beaa3f664b18a826fbab1fedd9143bc0624a95a490bfe40c4b5b0a0f94dbc565ce738
20ff499 rpc: Capture potentially large UniValue by ref for rpcdoccheck (Martin Zumsande) Pull request description: Capturing it by reference instead of value should save us from making a copy of a potentially large object. Saw this while having a look at bitcoin#25229 although I couldn't reproduce an actual leak, so this is not a fix for that issue. ACKs for top commit: MarcoFalke: review ACK 20ff499 theStack: Code-review ACK 20ff499 furszy: Code ACK 20ff499 Tree-SHA512: faf7bb14e37f8324b93a39095b07693626329da47c4a1ac8929bf99385e2e0567008e959e7e8540bc7d454d08fa41cccd39f55253c9a839fa88362922058a93b
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR backports Bitcoin Core's RPC documentation validation system (bitcoin#24408, bitcoin#24959, bitcoin#25161, bitcoin#25170, bitcoin#25237), introducing runtime checks that verify RPC help documentation matches actual return values. The implementation adds a new -rpcdoccheck flag (enabled by default in debug builds via --enable-debug or explicitly via -Drpcdoccheck) that triggers non-fatal assertions when RPCResult specifications diverge from actual UniValue responses. The system works by capturing RPC function return values in RPCHelpMan::HandleRequest() and validating them against the documented RPCResult structure using MatchesType(). This backport required comprehensive documentation updates across the codebase, replacing numerous empty RPCResults{} placeholders with explicit field specifications. The functional test framework now enables this validation by default (rpcdoccheck=1) to catch documentation drift during testing. The change integrates with Dash's existing assertion infrastructure (CHECK_NONFATAL) to surface issues during development while avoiding production overhead.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| src/rpc/util.h | 5/5 | Added DEFAULT_RPC_DOC_CHECK compile-time constant controlled by RPC_DOC_CHECK preprocessor macro |
| configure.ac | 5/5 | Added -DRPC_DOC_CHECK to debug build flags via AX_CHECK_PREPROC_FLAG |
| src/init.cpp | 5/5 | Registered -rpcdoccheck command-line argument as DEBUG_ONLY flag |
| test/functional/test_framework/util.py | 5/5 | Enabled rpcdoccheck=1 in all functional test node configurations |
| src/rpc/util.cpp | 5/5 | Implemented runtime validation by capturing return values and checking against RPCResult specs |
| src/rpc/quorums.cpp | 3.5/5 | Added detailed RPCResult documentation for quorum commands; contains typo ("collaterial") and potential type mismatch (service field documented as NUM but likely returns string) |
Confidence score: 4/5
- This PR is safe to merge with minimal risk in production but requires attention to documentation accuracy issues flagged in previous reviews.
- Score reflects solid implementation of the validation infrastructure itself, but deducted one point due to outstanding documentation issues identified in previous reviews (typos like "collaterial", type mismatches in service fields, and incorrect field names in other RPC files that may still exist).
- Pay close attention to src/rpc/quorums.cpp and verify all previously flagged documentation issues in other RPC files (src/rpc/masternode.cpp, src/rpc/governance.cpp, src/rpc/evo.cpp mentioned in previous reviews) have been addressed before enabling this validation in production.
Context used:
- Context from
dashboard- CLAUDE.md (source)
6 files reviewed, 2 comments
src/rpc/quorums.cpp
Outdated
| { | ||
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::STR_HEX, "proTxHash", "ProTx of collaterial"}, |
There was a problem hiding this comment.
syntax: Typo: 'collaterial' should be 'collateral'
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/rpc/quorums.cpp
Line: 259:259
Comment:
**syntax:** Typo: 'collaterial' should be 'collateral'
How can I resolve this? If you propose a fix, please make it concise.
src/rpc/quorums.cpp
Outdated
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::STR_HEX, "proTxHash", "ProTx of collaterial"}, | ||
| {RPCResult::Type::NUM, "service", "Address of node. Legacy, will be deprecated in further versions. See `addresses` instead."}, |
There was a problem hiding this comment.
logic: Type mismatch: service field is documented as NUM but the value is a string (address). Should be STR or STR_ADDR.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/rpc/quorums.cpp
Line: 260:260
Comment:
**logic:** Type mismatch: `service` field is documented as NUM but the value is a string (address). Should be STR or STR_ADDR.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
configure.ac(1 hunks)src/init.cpp(1 hunks)src/rpc/quorums.cpp(3 hunks)src/rpc/util.cpp(2 hunks)src/rpc/util.h(1 hunks)test/functional/test_framework/util.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- configure.ac
- test/functional/test_framework/util.py
- src/init.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/util.cppsrc/rpc/quorums.cppsrc/rpc/util.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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.
🧬 Code graph analysis (1)
src/rpc/util.cpp (1)
src/rpc/util.h (1)
DEFAULT_RPC_DOC_CHECK(26-32)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/quorums.cpp
[error] 1-1: Clang format differences found. Run clang-format-diff.py (or clang-format) 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). (8)
- GitHub Check: linux64_nowallet-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
🔇 Additional comments (6)
src/rpc/util.h (1)
26-32: LGTM!The preprocessor-driven constant definition is clean and appropriate for controlling RPC documentation validation at compile time.
src/rpc/util.cpp (2)
17-17: LGTM!The include is necessary to access
gArgsfor the runtime flag check added below.
539-543: LGTM!The post-execution validation logic is sound. Using
std::any_ofcorrectly handles RPCResults with multiple alternatives, andCHECK_NONFATALprovides appropriate debug-time validation without crashing production builds.src/rpc/quorums.cpp (3)
332-332: LGTM!The documentation now correctly matches the implementation at line 428, which uses
minableCommitments(notmineableCommitments). This addresses the typo flagged in the past review comment.
450-450: LGTM!The updated help text is clearer and more grammatically correct than the original.
1-1: Verify formatting locally using clang-format.The sandbox environment does not have clang-format available. Please run clang-format locally on your machine to identify and fix any formatting issues in
src/rpc/quorums.cpp:clang-format -i src/rpc/quorums.cpp git diff src/rpc/quorums.cppAfter applying the formatting fixes, commit the changes and re-run the pipeline to confirm the clang-format-diff check passes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/rpc/quorums.cpp
Outdated
| { | ||
| {RPCResult::Type::STR_HEX, "proTxHash", "ProTx of collaterial"}, | ||
| {RPCResult::Type::STR, "service", "Address of node. Legacy, will be deprecated in further versions. See `addresses` instead."}, | ||
| {RPCResult::Type::ARR, "addresses", "Addresses of node", |
src/rpc/quorums.cpp
Outdated
| // TODO - expand it when extended addresses are fully implemented | ||
| {{RPCResult::Type::ELISION, "", ""}}, | ||
| }, | ||
| {RPCResult::Type::STR_HEX, "pubKeyOperator", "The operator BLS public key"}, |
There was a problem hiding this comment.
We've already defined pubKeyOperator (see below), please use GetRpcResult()
Line 60 in f650f65
src/rpc/quorums.cpp
Outdated
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::STR_HEX, "proTxHash", "ProTx of collaterial"}, | ||
| {RPCResult::Type::STR, "service", "Address of node. Legacy, will be deprecated in further versions. See `addresses` instead."}, |
src/rpc/quorums.cpp
Outdated
src/rpc/quorums.cpp
Outdated
| {RPCResult::Type::NUM, "height", "Quorum Height"}, | ||
| {RPCResult::Type::STR, "type", "Quorum type"}, | ||
| {RPCResult::Type::STR_HEX, "quorumHash", "Quorum hash"}, | ||
| {RPCResult::Type::NUM, "quorumIndex", "Quorum index (applicable only to rotated quorums)."}, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/rpc/quorums.cpp (1)
252-252: Consider usingGetRpcResult("quorumIndex")for consistency.Since
quorumIndexis now defined inRPCRESULT_MAP(src/evo/core_write.cpp line 61), consider usingGetRpcResult("quorumIndex")instead of the inline definition for consistency with other fields in this result. As per coding guidelines.Apply this diff if you'd like to align with the pattern:
- {RPCResult::Type::NUM, "quorumIndex", "Quorum index (applicable only to rotated quorums)."}, + GetRpcResult("quorumIndex"),Note: The description in
RPCRESULT_MAPis "Quorum index. Relevant for rotation quorums only, 0 for non-rotating quorums" which differs slightly from the inline "Quorum index (applicable only to rotated quorums)." You may want to align the descriptions as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
src/evo/core_write.cpp(1 hunks) -
src/rpc/quorums.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/core_write.cppsrc/rpc/quorums.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/core_write.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
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: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
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: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
📚 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/evo/core_write.cppsrc/rpc/quorums.cpp
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/rpc/quorums.cpp
🧬 Code graph analysis (1)
src/rpc/quorums.cpp (1)
src/evo/core_write.cpp (2)
GetRpcResult(74-82)GetRpcResult(74-74)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/quorums.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff tool to fix formatting. Command failed: git diff -U0 origin/develop --
🔇 Additional comments (3)
src/rpc/quorums.cpp (3)
316-316: Good use ofGetRpcResult("quorumIndex").This correctly references the centralized definition from
RPCRESULT_MAP, aligning with the pattern established in other RPC help definitions.
329-329: Correct fix for the documentation mismatch.This change aligns the RPC help documentation with the actual output key name
minableCommitmentsused at line 425, addressing the past review comment about the typo mismatch.
447-447: LGTM: Clear reference to detailed documentation.The help text update appropriately directs users to
help quorum infofor field details, avoiding duplication while providing clear guidance.
kwvg
left a comment
kwvg
left a comment
There was a problem hiding this comment.
One more pending quorumIndex use, otherwise LGTM
Co-authored-by: Kittywhiskers Van Gogh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/rpc/quorums.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/quorums.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
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: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
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: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused 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/rpc/quorums.cpp
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/rpc/quorums.cpp
🔇 Additional comments (4)
src/rpc/quorums.cpp (4)
329-329: LGTM! Documentation now matches implementation.The field name has been corrected to
minableCommitmentsto match the actual output key used in the implementation (line 425).
316-316: LGTM! Proper use of GetRpcResult for consistency.Using
GetRpcResult("quorumIndex")ensures consistent documentation across different RPC methods.
251-252: LGTM! Excellent use of GetRpcResult for shared fields.Using
GetRpcResult()forquorumHash,quorumIndex,proTxHash,service,addresses, andpubKeyOperatorensures consistent type definitions and descriptions across all RPC methods that return these fields.Also applies to: 259-262
447-447: LGTM! Appropriate use of cross-reference.Using
ELISIONwith a reference tohelp quorum infois a good approach to avoid duplicating detailed documentation.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
Looks good overall but pls check coderabbit's suggestions
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
see udjin / coderabbit
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
-
src/rpc/quorums.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/quorums.cpp
🧠 Learnings (17)
📓 Common learnings
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: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
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: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused 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/rpc/quorums.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
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.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/rpc/quorums.cpp
📚 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: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
src/rpc/quorums.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/quorums.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/rpc/quorums.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
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.
Applied to files:
src/rpc/quorums.cpp
🧬 Code graph analysis (1)
src/rpc/quorums.cpp (1)
src/evo/core_write.cpp (2)
GetRpcResult(74-82)GetRpcResult(74-74)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/quorums.cpp
[error] 1-1: Clang format differences found in src/rpc/quorums.cpp. Run the clang-format-diff.py tool (via the diff_output hook) to fix formatting issues. Command step: git diff -U0 origin/develop --
⏰ 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). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (4)
src/rpc/quorums.cpp (4)
249-267: LGTM: RPC schema documentation correctly implements reviewer feedback.The RPCResult schema properly documents all fields returned by BuildQuorumInfo, uses GetRpcResult() helpers for reusable fields, includes the conditional note for previousConsecutiveDKGFailures, and documents the optional pubKeyShare field with corrected grammar.
317-317: LGTM: Consistent use of GetRpcResult() helper.Using GetRpcResult("quorumIndex") maintains consistency across quorum RPC documentation.
330-330: LGTM: Documentation now matches code output.The field name "minableCommitments" correctly matches the key used in the actual output at line 426, addressing the previously flagged typo.
448-448: LGTM: Helpful cross-reference to detailed documentation.The updated help text appropriately directs users to
quorum infofor field details.
| {RPCResult::Type::NUM, "height", "Quorum Height"}, | ||
| {RPCResult::Type::STR, "type", "Quorum type"}, | ||
| GetRpcResult("quorumHash"), | ||
| GetRpcResult("quorumIndex"), | ||
| {RPCResult::Type::STR_HEX, "minedBlock", "Blockhash where the commitment was mined."}, | ||
| {RPCResult::Type::NUM, "previousConsecutiveDKGFailures", "Number of previous consecutive DKG failures. Only present for rotation-enabled quorums."}, | ||
| {RPCResult::Type::ARR, "members", "Members of quorum", | ||
| { | ||
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| GetRpcResult("proTxHash"), | ||
| GetRpcResult("service"), | ||
| GetRpcResult("addresses"), | ||
| GetRpcResult("pubKeyOperator"), | ||
| {RPCResult::Type::BOOL, "valid", "True if member is valid for this DKG"}, | ||
| {RPCResult::Type::STR_HEX, "pubKeyShare", /*optional=*/true, "Share of BLS public key of the member. Only present if member is valid."} | ||
| }}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Fix clang-format violations to pass CI checks.
The CI pipeline reports clang-format differences in this file. Run the formatting tool to resolve this before merge.
#!/bin/bash
# Apply clang-format to fix formatting issues
git diff -U0 origin/develop -- src/rpc/quorums.cpp | ./contrib/devtools/clang-format-diff.py -p1 -i🤖 Prompt for AI Agents
In src/rpc/quorums.cpp around lines 249 to 267 there are clang-format style
violations; run the repository's clang-format diff tool or clang-format directly
to reformat this file (for example run the provided script: git diff -U0
origin/develop -- src/rpc/quorums.cpp | ./contrib/devtools/clang-format-diff.py
-p1 -i or run clang-format with the project config), stage and commit the
resulting changes so CI no longer reports formatting differences.
5 participants
What was done?
Backports from bitcoin that validates RPC help.
Also missing changes from bitcoin#20459.
How Has This Been Tested?
Replaced all RPCResults{} to relevant doc.
Run functional tests and fix leftover failures one-by-one.
Breaking Changes
Only if running with
-rpcdoccheckor if configure&build with--enable_debug.Some type of failures when RPCResult could be caught now in runtime.
When calling RPC:
When app starts:
Checklist: