backport: bitcoin#24408, #24959, #25161, #25170, #25237 #6887
Conversation
fa870e3 Remove not needed clang-format off comments (MarcoFalke) Pull request description: It seems odd to disable clang-format and force manual formatting when there is no need for it. So remove the clang-format comments and other unneeded comments. Can be reviewed with `--word-diff-regex=. --ignore-all-space` Looks like this was initially added in commit d9d7957 to accommodate a linter that has since been removed and replaced by a functional test. ACKs for top commit: laanwj: Code review ACK fa870e3 fanquake: ACK fa870e3 Tree-SHA512: 0f8f97c12f5dbe517dd96c10b10ce1b8772d8daed33e6b41f73ea1040e89888cf3b8c0ad7b20319e366fe30c71e8b181c89098ae7f6a3deb8647e1b4731db815
… 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
…c prevouts 4185570 Add RPC to get mempool txs spending outputs (t-bast) Pull request description: We add an RPC to fetch mempool transactions spending any of the given outpoints. Without this RPC, application developers need to first call `getrawmempool` which returns a long list of `txid`, then fetch each of these transactions individually (`getrawtransaction`) to check whether they spend the given outpoints, which wastes a lot of bandwidth (in the worst case we need to transfer the whole mempool). For example in lightning, when we discover that one of our channel funding transactions has been spent, we need to find the spending transaction to claim our outputs from it. We are currently forced to fetch the whole mempool to do the analysis ourselves, which is quite costly. I believe that this RPC is also generally useful when doing some introspection on your mempool after one of your transactions failed to broadcast, for example when you implement RBF at the application level. Fetching and analyzing the conflicting transaction gives you more information to successfully replace it. ACKs for top commit: darosior: re-utACK 4185570 vincenzopalazzo: re-ACK bitcoin@4185570 danielabrozzoni: re-tACK 4185570 w0xlt: reACK bitcoin@4185570 Tree-SHA512: 206687efb720308b7e0b6cf16dd0a994006c0b5a290c8eb386917a80130973a6356d0d5cae1c63a01bb29e066dd721594969db106cba7249214fcac90d2c3dbc
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
- masternode (composite command) - masternode connect - masternode count - masternode winners
Fixed: - `setcoinjoinrounds` - `setcoinjoinamount` - `coinjoinsalt` - composite command `coinjoin`
- gobject (composite command) - gobject check - gobject count - gobject deserialize - gobject get - gobject getcurrentvotes - gobject list - gobject listprepared - gobject prepare - gobject submit - gobject vote-alias - gobject vote-many - gobject voteraw
Fixed RPC: - bls (composite command) - protx (composite command) - protx list - protx listdiff
- quorum (composite command) - quorum dkgstats - quorum info - quorum sign - quorum platformsign - quorum verify - quorum hasrecsig - quorum isconflicting - quorum selectquorum - quorum dkgsimerror - quorum getdata - quorum verifychainlock - quorum verifyislock
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Walkthrough
Estimated code review effortMixed changes: one new RPC with argument handling and mempool scanning logic; a new runtime validation path gated by a flag; widespread but mostly mechanical help/schema updates; multiple table refactors; new functional tests and config tweak. Heterogeneous but most edits are repetitive documentation/registration changes; logic density concentrated in the new RPC and validation. 🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 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.
| RPCResult { | ||
| RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::NUM, "baseHeight", "height of base (starting) block"}, | ||
| {RPCResult::Type::NUM, "blockHeight", "height of target (ending) block"}, | ||
| {RPCResult::Type::ARR, "addedMNs", "added masternodes", | ||
| { | ||
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| // TODO: list fields of output for RPC help instead ELISION | ||
| {RPCResult::Type::ELISION, "", ""} | ||
| }}, | ||
| }, | ||
| }, | ||
| {RPCResult::Type::ARR, "removedMns", "added masternodes", | ||
| { | ||
| {RPCResult::Type::STR_HEX, "protx", "ProTx of removed masternode"}, | ||
| }, | ||
| }, | ||
| {RPCResult::Type::ARR, "addedMNs", "added masternodes", | ||
| { | ||
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::OBJ, "protx", "ProTx of updated masternode", | ||
| { | ||
| // TODO: list fields of output for RPC help instead ELISION | ||
| {RPCResult::Type::ELISION, "", ""} | ||
| }}, | ||
| }}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
protx listdiff result doc: “updatedMNs” mislabeled as “addedMNs”
The result actually returns “updatedMNs” (see Lines 1737–1749), but help documents “addedMNs” twice. Fix the second occurrence to “updatedMNs” and adjust the description.
- {RPCResult::Type::ARR, "addedMNs", "added masternodes",
+ {RPCResult::Type::ARR, "updatedMNs", "updated masternodes",
{
{RPCResult::Type::OBJ, "", "",
{
- {RPCResult::Type::OBJ, "protx", "ProTx of updated masternode",
+ {RPCResult::Type::OBJ, "protx", "ProTx of updated masternode",
{
// 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, "baseHeight", "height of base (starting) block"}, | |
| {RPCResult::Type::NUM, "blockHeight", "height of target (ending) block"}, | |
| {RPCResult::Type::ARR, "addedMNs", "added masternodes", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {RPCResult::Type::ELISION, "", ""} | |
| }}, | |
| }, | |
| }, | |
| {RPCResult::Type::ARR, "removedMns", "added masternodes", | |
| { | |
| {RPCResult::Type::STR_HEX, "protx", "ProTx of removed masternode"}, | |
| }, | |
| }, | |
| {RPCResult::Type::ARR, "addedMNs", "added masternodes", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::OBJ, "protx", "ProTx of updated masternode", | |
| { | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {RPCResult::Type::ELISION, "", ""} | |
| }}, | |
| }}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| RPCResult { | |
| RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::NUM, "baseHeight", "height of base (starting) block"}, | |
| {RPCResult::Type::NUM, "blockHeight", "height of target (ending) block"}, | |
| {RPCResult::Type::ARR, "addedMNs", "added masternodes", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {RPCResult::Type::ELISION, "", ""} | |
| }}, | |
| }, | |
| }, | |
| {RPCResult::Type::ARR, "removedMns", "added masternodes", | |
| { | |
| {RPCResult::Type::STR_HEX, "protx", "ProTx of removed masternode"}, | |
| }, | |
| }, | |
| {RPCResult::Type::ARR, "updatedMNs", "updated masternodes", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::OBJ, "protx", "ProTx of updated masternode", | |
| { | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {RPCResult::Type::ELISION, "", ""} | |
| }}, | |
| }}, | |
| }, | |
| }, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In src/rpc/evo.cpp around lines 1657 to 1689 the second array entry for addedMNs
is incorrectly labeled the same as the first; change the second
{RPCResult::Type::ARR, "addedMNs", "added masternodes", ...} entry to use the
key "updatedMNs" and update its description to "updated masternodes" (and ensure
any accompanying help text reflects that these entries are updated masternodes
rather than added ones).
| { | ||
| RPCResult{"for mode = json", | ||
| RPCResult::Type::OBJ, "", "", | ||
| { | ||
| // 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.
Clarify “mode=all” result description
The command returns a human-readable summary string (not a tx hash). Update the description.
Apply this diff:
- RPCResult{"for mode = all", RPCResult::Type::STR, "", "The transaction hash in hex"},
+ RPCResult{"for mode = all", RPCResult::Type::STR, "", "A human-readable summary string"},📝 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{"for mode = json", | |
| RPCResult::Type::OBJ, "", "", | |
| { | |
| // 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{"for mode = json", | |
| RPCResult::Type::OBJ, "", "", | |
| { | |
| // TODO: list fields of output for RPC help instead ELISION | |
| {RPCResult::Type::ELISION, "", ""} | |
| } | |
| }, | |
| RPCResult{"for mode = all", RPCResult::Type::STR, "", "A human-readable summary string"}, | |
| }, |
🤖 Prompt for AI Agents
In src/rpc/governance.cpp around lines 46 to 55, the RPCResult entry for "for
mode = all" incorrectly describes the return as "The transaction hash in hex";
update that description to state it returns a human-readable summary string
(e.g., "A human-readable summary of the governance information") so the help
text matches actual output. Replace the existing description text for the
RPCResult::Type::STR entry accordingly.
| { | ||
| 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{"if request is valid", | ||
| RPCResult::Type::OBJ, "hash", "Object details", | ||
| { | ||
| // TODO: list fields of output for RPC help instead ELISION | ||
| {RPCResult::Type::ELISION, "", ""} | ||
| }, | ||
| }, | ||
| RPCResult{"if request is invalid", | ||
| RPCResult::Type::STR_HEX, "", "Error string" | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Wrong type for invalid-request branch
When invalid, this path returns a plain error string. Use STR, not STR_HEX.
Apply this diff:
- RPCResult{"if request is invalid",
- RPCResult::Type::STR_HEX, "", "Error string"
- },
+ RPCResult{"if request is invalid",
+ RPCResult::Type::STR, "", "Error string"
+ },🤖 Prompt for AI Agents
In src/rpc/governance.cpp around lines 725 to 736, the RPCResult entry for the
"if request is invalid" branch uses RPCResult::Type::STR_HEX but the code
returns a plain error string; change the type to RPCResult::Type::STR to match
the actual returned value and update the RPCResult constructor call accordingly
so the invalid-response entry uses STR instead of STR_HEX.
| RPCResult{ | ||
| RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::NUM, "total", "Total amount Masternodes"}, | ||
| {RPCResult::Type::NUM, "enabled", "Amount of enabled Masternodes"}, | ||
| {RPCResult::Type::OBJ, "details", "Amount of enabled Masternodes", | ||
| {{RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::OBJ, "regular", "Details for regular masternodes", | ||
| { | ||
| {RPCResult::Type::NUM, "total", "Total amount regular Masternodes"}, | ||
| {RPCResult::Type::NUM, "enabled", "Amount of enabled regular Masternodes"} | ||
| }}, | ||
| {RPCResult::Type::OBJ, "evo", "Details for Evo nodes", | ||
| { | ||
| {RPCResult::Type::NUM, "total", "Total amount Evo nodes"}, | ||
| {RPCResult::Type::NUM, "enabled", "Amount of enabled Evo nodes"} | ||
| }}, | ||
| }}, | ||
| }} | ||
| } | ||
| }, |
There was a problem hiding this comment.
Doc/data mismatch: “details” vs “detailed” key breaks rpcdoccheck
The result doc uses "details" but the implementation pushes "detailed" (Line 139). Align the field name.
Apply this diff to the implementation:
- obj.pushKV("detailed", detailedObj);
+ obj.pushKV("details", detailedObj);📝 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, "total", "Total amount Masternodes"}, | |
| {RPCResult::Type::NUM, "enabled", "Amount of enabled Masternodes"}, | |
| {RPCResult::Type::OBJ, "details", "Amount of enabled Masternodes", | |
| {{RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::OBJ, "regular", "Details for regular masternodes", | |
| { | |
| {RPCResult::Type::NUM, "total", "Total amount regular Masternodes"}, | |
| {RPCResult::Type::NUM, "enabled", "Amount of enabled regular Masternodes"} | |
| }}, | |
| {RPCResult::Type::OBJ, "evo", "Details for Evo nodes", | |
| { | |
| {RPCResult::Type::NUM, "total", "Total amount Evo nodes"}, | |
| {RPCResult::Type::NUM, "enabled", "Amount of enabled Evo nodes"} | |
| }}, | |
| }}, | |
| }} | |
| } | |
| }, | |
| obj.pushKV("details", detailedObj); |
🤖 Prompt for AI Agents
In src/rpc/masternode.cpp around lines 90 to 111 (and the implementation push at
~139), the RPC result doc declares the key "details" but the code pushes
"detailed", causing rpcdoccheck failures; change the implementation to push the
object under the key "details" (replace any "detailed" occurrences with
"details"), and verify the nested regular/evo sub-objects remain unchanged so
the RPCResult documentation and the returned JSON keys match.
| RPCResult{ | ||
| RPCResult::Type::OBJ, "", "Details about a specific deterministic masternode", | ||
| { | ||
| {RPCResult::Type::STR, "height", "payment string"} | ||
| } | ||
| }, |
There was a problem hiding this comment.
Result schema should use dynamic keys keyed by height
The RPC returns an object mapping heights to payment strings. Use OBJ_DYN and document the dynamic key.
Apply this diff:
- RPCResult{
- RPCResult::Type::OBJ, "", "Details about a specific deterministic masternode",
- {
- {RPCResult::Type::STR, "height", "payment string"}
- }
- },
+ RPCResult{
+ RPCResult::Type::OBJ_DYN, "height", "Map of block height to payment string",
+ {
+ {RPCResult::Type::STR, "", "payment string"},
+ }
+ },📝 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, "", "Details about a specific deterministic masternode", | |
| { | |
| {RPCResult::Type::STR, "height", "payment string"} | |
| } | |
| }, | |
| RPCResult{ | |
| RPCResult::Type::OBJ_DYN, "height", "Map of block height to payment string", | |
| { | |
| {RPCResult::Type::STR, "", "payment string"}, | |
| } | |
| }, |
🤖 Prompt for AI Agents
In src/rpc/masternode.cpp around lines 264 to 269, the RPCResult currently
declares a fixed OBJ with a scalar "height" field but the RPC actually returns
an object keyed by height; change the schema to use RPCResult::Type::OBJ_DYN to
represent dynamic keys and document the dynamic key by replacing the inner field
with an entry describing the dynamic key (key name = "height", key type =
RPCResult::Type::STR for the payment string) and update the description to
indicate the object maps heights to payment strings; ensure the structure
follows the existing RPCResult OBJ_DYN pattern used elsewhere.
| 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, "", ""}}, | |
| }, | |
| }, | |
| }, |
3 participants
DRAFT, DEPENDS ON changes from #6886
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: