backport: bitcoin/bitcoin#23083: rpc: Fail to return undocumented or misdocumented JSON - #7502
Conversation
|
✅ Final review complete — no blockers (commit 9bf0816) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR updates RPC help schemas across core, governance, quorum, masternode, transaction, networking, and wallet commands. It adds dynamic-object descriptions, marks conditional fields optional, corrects field names and types, and documents additional response fields. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RPCCommand
participant RPCResult
participant UniValue
RPCCommand->>RPCResult: provide documented result schema
RPCResult->>UniValue: inspect returned response
UniValue-->>RPCResult: return keys and values
RPCResult-->>RPCCommand: validate documented response shape
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/evo.cpp (1)
1-1: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winKeep production RPCResult schemas renderable in
ToSections.
RPCResult::Type::ANYis only for test schemas, but adding it to real help paths such asprotx listinserts an unreachable branch into the schema traversal used to emit-helpdocumentation. Replace these production usages with renderable alternatives or makeToSectionssafely handleskip_type_check/dynamic shapes without relying onANY.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/evo.cpp` at line 1, Replace production RPCResult::Type::ANY usages in the evo RPC help schemas, including protx list, with concrete renderable types or supported dynamic-shape definitions. Ensure the affected schemas remain traversable by ToSections and do not introduce unreachable branches; reserve ANY for test-only schemas.
🧹 Nitpick comments (1)
src/rpc/quorums.cpp (1)
117-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a descriptive placeholder instead of
"xxxx"for the dynamic quorum-hash key.This key name is rendered verbatim in generated help text (
"xxxx" : {...}) sinceOBJ_DYN's inner entry is emitted with itsm_key_namewhen nested under anOBJcontext. Elsewhere in this same PR, dynamic-key placeholders use a descriptive form (<outpoint>,<sig_hex>,<protx_hash>); consider<quorumHash>here for consistency and clearer generated docs.✏️ Proposed rename
- {RPCResult::Type::OBJ, "xxxx", "Quorum details", + {RPCResult::Type::OBJ, "<quorumHash>", "Quorum details",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/quorums.cpp` around lines 117 - 127, Replace the dynamic quorum-hash key placeholder "xxxx" in the RPC result schema with the descriptive placeholder "<quorumHash>", preserving the existing OBJ_DYN structure and quorum detail fields so generated help text clearly identifies the key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/governance/core_write.cpp`:
- Around line 122-138: Replace the seven RPCResult::Type::ANY entries in the
data schema with appropriate concrete types so RPCResult::ToSections can render
help without reaching NONFATAL_UNREACHABLE(). Preserve skip_type_check=true on
the enclosing data object, which continues to allow mixed and extra proposal
fields during validation; do not modify unrelated fields such as plain or hex.
In `@src/llmq/core_write.cpp`:
- Around line 22-53: Replace the RPCResult::Type::ANY usage in the member_tally
helper with a schema-specific representation that accurately documents the
detail_level-dependent scalar and array variants without relying on ANY. Update
the six member tally fields in the surrounding DKG result schema to use this
representation while preserving their existing descriptions and runtime help
validation.
In `@src/rpc/evo.cpp`:
- Around line 1507-1515: Replace RPCResult::Type::ANY in the protx list result
declaration with typed result definitions: document the non-detailed array
elements as ProTx-hash strings and add a separate typed array shape for detailed
masternode objects, using the existing RPCResult field definitions where
available. Ensure ToSections() can recurse through both array shapes without
encountering ANY.
In `@src/rpc/rawtransaction.cpp`:
- Around line 576-578: Update the getislocks RPC summary text to state that it
returns the string "None" when no InstantSend lock is known, replacing the
inaccurate reference to Null. Keep the existing implementation and detailed
result documentation unchanged.
In `@src/wallet/rpc/transactions.cpp`:
- Around line 424-445: Update WalletTxToJSON() to filter out the internal
Dash-specific “DS” mapValue entry before serializing mapValue into the
transaction JSON. Preserve serialization of all documented entries and ensure
the RPC output conforms to its declared schema.
In `@src/wallet/rpc/wallet.cpp`:
- Line 126: Update the getwalletinfo RPC result definition for
keypoolsize_hd_internal to match its unconditional emission: remove the optional
flag and clarify that a value of zero indicates no internal keypool, or
alternatively update the producer to omit the field when the feature is inactive
while preserving the documented behavior.
---
Outside diff comments:
In `@src/rpc/evo.cpp`:
- Line 1: Replace production RPCResult::Type::ANY usages in the evo RPC help
schemas, including protx list, with concrete renderable types or supported
dynamic-shape definitions. Ensure the affected schemas remain traversable by
ToSections and do not introduce unreachable branches; reserve ANY for test-only
schemas.
---
Nitpick comments:
In `@src/rpc/quorums.cpp`:
- Around line 117-127: Replace the dynamic quorum-hash key placeholder "xxxx" in
the RPC result schema with the descriptive placeholder "<quorumHash>",
preserving the existing OBJ_DYN structure and quorum detail fields so generated
help text clearly identifies the key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15abbeec-1c9c-4130-a5ac-badb191f7d73
📒 Files selected for processing (19)
src/evo/core_write.cppsrc/governance/core_write.cppsrc/llmq/core_write.cppsrc/rpc/blockchain.cppsrc/rpc/coinjoin.cppsrc/rpc/evo.cppsrc/rpc/governance.cppsrc/rpc/masternode.cppsrc/rpc/mempool.cppsrc/rpc/mining.cppsrc/rpc/net.cppsrc/rpc/node.cppsrc/rpc/quorums.cppsrc/rpc/rawtransaction.cppsrc/rpc/util.cppsrc/rpc/util.hsrc/wallet/rpc/backup.cppsrc/wallet/rpc/transactions.cppsrc/wallet/rpc/wallet.cpp
| // An element is the plain string "None" when no InstantSend Lock is known for | ||
| // that txid, so the element type cannot be checked against this object. | ||
| /*skip_type_check=*/true}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the getislocks summary to document the string "None".
The implementation returns "None" at Line 619, and this change now explicitly documents that string. However, the RPC summary still says it returns Null, which describes the wrong JSON value type. Update the summary text accordingly.
Proposed documentation fix
- "\nReturns the raw InstantSend lock data for each txids. Returns Null if there is no known IS yet.",
+ "\nReturns the raw InstantSend lock data for each txids. Returns the string \"None\" if there is no known IS yet.",📝 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.
| // An element is the plain string "None" when no InstantSend Lock is known for | |
| // that txid, so the element type cannot be checked against this object. | |
| /*skip_type_check=*/true}, | |
| "\nReturns the raw InstantSend lock data for each txids. Returns the string \"None\" if there is no known IS yet.", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rpc/rawtransaction.cpp` around lines 576 - 578, Update the getislocks RPC
summary text to state that it returns the string "None" when no InstantSend lock
is known, replacing the inaccurate reference to Null. Keep the existing
implementation and detailed result documentation unchanged.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The upstream backport and most Dash-specific schema adaptations are coherent, but the newly introduced nested ANY entries violate the existing help-renderer contract and make several RPC help pages throw NonFatalCheckError. The revised mempool result documentation also omits the normal "false" value for instantlock, so changes are required before merge.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/llmq/core_write.cpp`:
- [BLOCKING] src/llmq/core_write.cpp:28-32: Nested ANY result types make RPC help throw
`RPCResult::Type::ANY` is only skipped when it is the top-level result. Nested entries are passed to `RPCResult::ToSections()`, whose `Type::ANY` branch calls `NONFATAL_UNREACHABLE()` and throws `NonFatalCheckError`. Therefore, rendering `help quorum dkgstatus` traverses this member-tally entry and fails instead of returning help. The same PR-introduced problem affects the nested `ANY` entries in `src/governance/core_write.cpp:127-133`, `src/rpc/rawtransaction.cpp:474`, `src/rpc/evo.cpp:1514`, and `src/wallet/rpc/transactions.cpp:868`. The new `m_skip_type_check` flag only affects `MatchesType()` and does not prevent help rendering. Add a displayable representation for nested `ANY`, or use displayable result types with type checking skipped.
| RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction", | ||
| {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}}, | ||
| RPCResult{RPCResult::Type::BOOL, "instantsend", "True if this transaction was locked via InstantSend"}, | ||
| RPCResult{RPCResult::Type::STR, "instantlock", "\"true\" if this transaction was locked via InstantSend, \"unknown\" if InstantSend is unavailable"}, |
There was a problem hiding this comment.
💬 Nitpick: Document the unlocked instantlock value
entryToJSON() serializes isman->IsLocked(...) through util::to_string(bool), which returns either "true" or "false"; it returns "unknown" only when the InstantSend manager is unavailable. The revised documentation omits the normal "false" response value, leaving this result incompletely documented in a PR specifically correcting RPC result schemas.
| RPCResult{RPCResult::Type::STR, "instantlock", "\"true\" if this transaction was locked via InstantSend, \"unknown\" if InstantSend is unavailable"}, | |
| RPCResult{RPCResult::Type::STR, "instantlock", "\"true\" if this transaction was locked via InstantSend, \"false\" if it was not locked, and \"unknown\" if InstantSend is unavailable"}, |
source: ['codex']
|
This pull request has conflicts, please rebase. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8647e75e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // cannot be listed as alternatives here - the first entry would be applied | ||
| // to element 0 only and the second to every element after it. | ||
| // TODO: document fields of the detailed entry | ||
| {RPCResult::Type::ANY, "", "The ProTx hash when detailed=false, otherwise an object describing the masternode"}, |
There was a problem hiding this comment.
When users request help for protx list, this nested RPCResult::Type::ANY is still rendered by RPCResult::ToSections(), whose ANY case calls NONFATAL_UNREACHABLE(); RPCResults::ToDescriptionString() only skips top-level ANY results. This pattern is repeated by the other new nested ANY entries, so those command help paths return an internal-bug error instead of help text. Teach ToSections() how to print ANY, or avoid using it in public nested result docs.
Useful? React with 👍 / 👎.
| if (doc_keys.find(result_entry.first) == doc_keys.end()) { | ||
| return false; // missing documentation |
There was a problem hiding this comment.
When -rpcdoccheck is enabled, this new unknown-key rejection makes listtransactions fail for CoinJoin sends: SendMoney() stores map_value["DS"] = "1" when coin_control.IsUsingCoinJoin(), and WalletTxToJSON() pushes every wtx.mapValue key into the result object, but the listtransactions result docs in this commit still do not include DS. In that scenario the returned object contains an otherwise-valid Dash-specific field and MatchesType() returns false, turning the RPC into an internal bug during functional tests; either document/whitelist the wallet metadata keys or avoid strict checking for that map.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f22c9ccc9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {RPCResult::Type::BOOL, "running", /*optional=*/true, "Whether mixing is currently running (not returned when no wallet is loaded)"}, | ||
| {RPCResult::Type::NUM, "pending_inputs", "The number of successfully mixed inputs kept locked until the transaction spending them is observed"}, |
There was a problem hiding this comment.
When getcoinjoininfo runs on a regular node without a selected/loaded wallet, the handler returns at src/rpc/coinjoin.cpp:485-487 before CCoinJoinClientManager::getJsonInfo() adds pending_inputs, but this result entry is still required while the neighboring wallet-only fields were made optional. With -rpcdoccheck, that no-wallet/reduced-functionality path still fails the new result matcher instead of returning the CoinJoin options object; make pending_inputs optional or document a separate no-wallet result shape.
Useful? React with 👍 / 👎.
thephez
left a comment
thephez
left a comment
There was a problem hiding this comment.
Language looks okay to me. Just one comment regarding this rename.
2.
|
…d JSON fc892c3 rpc: Fail to return undocumented or misdocumented JSON (MarcoFalke) f4bc4a7 rpc: Add m_skip_type_check to RPCResult (MarcoFalke) Pull request description: This avoids documentation shortcomings such as the ones fixed in commit e7b6272, 138d55e, 577bd51, f8c84e0, 0ee9a00, 13f4185, or faecb2e ACKs for top commit: fanquake: ACK fc892c3 - tested that this catches issue, i.e bitcoin#24691: Tree-SHA512: 9d0d7e6291bfc6f67541a4ff746d374ad8751fefcff6d103d8621c0298b190ab1d209ce96cfc3a0d4a6a5460a9f9bb790eb96027b16e5ff91f2512e40c92ca84 Co-authored-by: fanquake <[email protected]>
… JSON-RPC interface Includes 6a1a66c rpc: render Type::ANY in help text instead of aborting RPCResult::Type::ANY triggers NONFATAL_UNREACHABLE() in ToSections(), which crashes the help() RPC when a command uses Type::ANY in a nested result field. Previously this was never hit because Type::ANY was only used as a top-level alternate result type, filtered out before ToSections() is called. getopenrpcinfo() will use this result type, so render it like other types allowing it to be used in nested result definitions like schema. Co-authored-by: will <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b77fd4ac77
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!doc_entry.MatchesType(result_it->second)) { | ||
| return false; // wrong type |
There was a problem hiding this comment.
When -rpcdoccheck is enabled and getpeerinfo reports a peer with a non-zero AS mapping, this new recursive object check now validates the existing mapped_as result doc and rejects it: src/rpc/net.cpp documents mapped_as as RPCResult::Type::STR, but the handler pushes uint64_t(stats.m_mapped_as) at src/rpc/net.cpp:230-231. In that asmap-enabled context the RPC returns an internal doccheck bug instead of peer info; update the result doc to NUM (or otherwise match the emitted type) before enabling strict nested checks.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The recursive RPC result checker still rejects two supported responses: wallet-disabled getcoinjoininfo returns an empty object that matches neither documented variant, and getpeerinfo documents numeric mapped_as values as strings. The current head also contains one non-buildable intermediate commit and three smaller documentation inconsistencies.
Source: reviewers gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history), and gpt-5.6-sol (backport-reviewer); final verifier gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
2 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/coinjoin.cpp`:
- [BLOCKING] src/rpc/coinjoin.cpp:423-429: Mark pending_inputs optional for no-wallet CoinJoin info
Making pending_inputs optional fixes the wallet-enabled path where no wallet is selected, but builds without ENABLE_WALLET still fail result checking. getcoinjoininfo remains registered in that configuration, while the entire regular-node producer at lines 476-502 is compiled out, so the handler returns {}. The regular-node schema still requires enabled through denoms_hardcap, and the masternode schema also requires fields absent from {}, so -rpcdoccheck turns this supported reduced response into an internal documentation-check error. Emit the base options outside the wallet guard, make these base fields optional, or add a distinct empty-object result variant.
In `src/rpc/net.cpp`:
- [BLOCKING] src/rpc/net.cpp:129: Missing prerequisite: bitcoin#17812
The newly recursive checker validates each getpeerinfo entry, but mapped_as is still declared as STR even though the producer inserts uint64_t(stats.m_mapped_as). With AS mapping enabled and a nonzero mapping, the checker therefore replaces getpeerinfo's response with an internal documentation-check error. Upstream bitcoin#23083 already had mapped_as declared as NUM because bitcoin#17812 commit 5ba829e12e99f119df56cab422f827b9be03fe57 fixed it before this backport; carry that prerequisite fix into Dash.
In `src/governance/core_write.cpp`:
- [SUGGESTION] src/governance/core_write.cpp:138: Place the RPCResult API backport before its callers
Commit 3344adbae81 uses the new RPCResult constructor argument ending in /*skip_type_check=*/true here and in rawtransaction.cpp, but that commit's parent only has constructors accepting the old argument shapes. The required m_skip_type_check API is introduced by the following commit f2e13f0f46a, so the first PR commit cannot compile independently even though the final head can. Reorder the API backport before its callers or squash the dependent changes so the permanent history remains buildable and bisectable.
In `src/rpc/rawtransaction.cpp`:
- [NITPICK] src/rpc/rawtransaction.cpp:548-550: Update the getislocks summary to document the string "None"
The implementation appends the JSON string "None" when no InstantSend lock is known, and the detailed result declaration now accommodates that string. The command summary still says the RPC returns Null, which denotes a different JSON type and contradicts the implementation.
In `src/wallet/rpc/wallet.cpp`:
- [NITPICK] src/wallet/rpc/wallet.cpp:126: Do not mark `keypoolsize_hd_internal` optional while always emitting it
getwalletinfo unconditionally inserts keypoolsize_hd_internal at line 193, including when its value is zero. Marking the field optional and saying it only appears when the feature is used therefore misdocuments the actual response. Keep the field required and explain the zero value instead.
In `src/rpc/mempool.cpp`:
- [NITPICK] src/rpc/mempool.cpp:276: Document the unlocked instantlock value
(existing thread: https://github.com/dashpay/dash/pull/7502#discussion_r3685409612)
entryToJSON() serializes isman->IsLocked(...) through util::to_string(bool), which returns either "true" or "false"; "unknown" is emitted only when the InstantSend manager is unavailable. The result description omits the normal unlocked value "false", leaving this Dash-specific schema adaptation incomplete.
| {RPCResult::Type::NUM, "max_amount", "Target CoinJoin balance in " + CURRENCY_UNIT + ""}, | ||
| {RPCResult::Type::NUM, "denoms_goal", "How many inputs of each denominated amount to target"}, | ||
| {RPCResult::Type::NUM, "denoms_hardcap", "Maximum limit of how many inputs of each denominated amount to create"}, |
There was a problem hiding this comment.
🔴 Blocking: Mark pending_inputs optional for no-wallet CoinJoin info
Making pending_inputs optional fixes the wallet-enabled path where no wallet is selected, but builds without ENABLE_WALLET still fail result checking. getcoinjoininfo remains registered in that configuration, while the entire regular-node producer at lines 476-502 is compiled out, so the handler returns {}. The regular-node schema still requires enabled through denoms_hardcap, and the masternode schema also requires fields absent from {}, so -rpcdoccheck turns this supported reduced response into an internal documentation-check error. Emit the base options outside the wallet guard, make these base fields optional, or add a distinct empty-object result variant.
| {RPCResult::Type::NUM, "max_amount", "Target CoinJoin balance in " + CURRENCY_UNIT + ""}, | |
| {RPCResult::Type::NUM, "denoms_goal", "How many inputs of each denominated amount to target"}, | |
| {RPCResult::Type::NUM, "denoms_hardcap", "Maximum limit of how many inputs of each denominated amount to create"}, | |
| {RPCResult::Type::BOOL, "enabled", /*optional=*/true, "Whether mixing functionality is enabled"}, | |
| {RPCResult::Type::BOOL, "multisession", /*optional=*/true, "Whether CoinJoin Multisession option is enabled"}, | |
| {RPCResult::Type::NUM, "max_sessions", /*optional=*/true, "How many parallel mixing sessions can there be at once"}, | |
| {RPCResult::Type::NUM, "max_rounds", /*optional=*/true, "How many rounds to mix"}, | |
| {RPCResult::Type::NUM, "max_amount", /*optional=*/true, "Target CoinJoin balance in " + CURRENCY_UNIT + ""}, | |
| {RPCResult::Type::NUM, "denoms_goal", /*optional=*/true, "How many inputs of each denominated amount to target"}, | |
| {RPCResult::Type::NUM, "denoms_hardcap", /*optional=*/true, "Maximum limit of how many inputs of each denominated amount to create"}, |
source: ['codex']
There was a problem hiding this comment.
Indeed reproducing for non-wallet build:
src/dash-cli -regtest getcoinjoininfo
error code: -1
error message:
Internal bug detected: std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [&ret](const RPCResult& res) { return res.MatchesType(ret); })
rpc/util.cpp:533 (HandleRequest)
Dash Core v23.1.7-1031-gb77fd4ac7748-dirty
Please report this issue here: https://github.com/dashpay/dash/issues
Though, please note that bitcoin core originally firstly merged this linter, and after that fixed one-by-one all appearing issues, see:
- Fix getpeerinfo doc bitcoin/bitcoin#24691
- rpc: Exclude descriptor when address is excluded bitcoin/bitcoin#24636
- doc: Fix getblockchaininfo/getdeploymentinfo RPC docs bitcoin/bitcoin#24579
- doc: Add missing optional to MempoolEntryDescription bitcoin/bitcoin#23694
So, if there's any more special corner cases will be found, they no need to be included in this PR and any more blocked.
There was a problem hiding this comment.
Resolved in ab89fbe — Mark pending_inputs optional for no-wallet CoinJoin info no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // Always emitted by ToJson() | ||
| {RPCResult::Type::STR_HEX, "hex", "Governance object data as hex"}, | ||
| }}, | ||
| }, /*skip_type_check=*/true}, |
There was a problem hiding this comment.
🟡 Suggestion: Place the RPCResult API backport before its callers
Commit 3344adb uses the new RPCResult constructor argument ending in /skip_type_check=/true here and in rawtransaction.cpp, but that commit's parent only has constructors accepting the old argument shapes. The required m_skip_type_check API is introduced by the following commit f2e13f0, so the first PR commit cannot compile independently even though the final head can. Reorder the API backport before its callers or squash the dependent changes so the permanent history remains buildable and bisectable.
source: ['codex']
| {RPCResult::Type::NUM_TIME, "timefirstkey", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " of the oldest known key in the wallet. Legacy wallets only"}, | ||
| {RPCResult::Type::NUM_TIME, "keypoololdest", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " of the oldest pre-generated key in the key pool. Legacy wallets only"}, | ||
| {RPCResult::Type::NUM, "keypoolsize", "how many new keys are pre-generated (only counts external keys)"}, | ||
| {RPCResult::Type::NUM, "keypoolsize_hd_internal", /*optional=*/ true, "how many new keys are pre-generated for internal use (used for change outputs and mobile coinjoin, only appears if the wallet is using this feature, otherwise external keys are used)"}, |
There was a problem hiding this comment.
💬 Nitpick: Do not mark keypoolsize_hd_internal optional while always emitting it
getwalletinfo unconditionally inserts keypoolsize_hd_internal at line 193, including when its value is zero. Marking the field optional and saying it only appears when the feature is used therefore misdocuments the actual response. Keep the field required and explain the zero value instead.
| {RPCResult::Type::NUM, "keypoolsize_hd_internal", /*optional=*/ true, "how many new keys are pre-generated for internal use (used for change outputs and mobile coinjoin, only appears if the wallet is using this feature, otherwise external keys are used)"}, | |
| {RPCResult::Type::NUM, "keypoolsize_hd_internal", "how many new keys are pre-generated for internal use (used for change outputs and mobile coinjoin; zero if no internal keypool is in use)"}, |
source: ['coderabbit']
There was a problem hiding this comment.
Resolved in ab89fbe — Do not mark keypoolsize_hd_internal optional while always emitting it no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The recursive RPC checker still rejects a supported getpeerinfo response because mapped_as is documented as a string but emitted as a number, so this backport is not ready to merge. The two latest schema corrections are effective, while one commit-ordering issue and two minor documentation mismatches remain.
Source: reviewers gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history), and gpt-5.6-sol (backport-reviewer); final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/net.cpp`:
- [BLOCKING] src/rpc/net.cpp:129: Missing prerequisite: bitcoin#17812
The backported recursive checker now validates each `getpeerinfo` array element. This schema declares `mapped_as` as `STR`, but the producer at lines 230-231 inserts `uint64_t(stats.m_mapped_as)`, so an asmap-enabled peer with a nonzero mapping makes `-rpcdoccheck` reject a valid response and return an internal documentation-check error. Upstream bitcoin#17812 commit `5ba829e12e99f119df56cab422f827b9be03fe57` changed this declaration to `NUM` before bitcoin#23083; carry that prerequisite correction into Dash.
In `src/rpc/rawtransaction.cpp`:
- [NITPICK] src/rpc/rawtransaction.cpp:549: Update the getislocks summary to document the string "None"
The implementation appends the JSON string `"None"` when no InstantSend lock is known, and the detailed result declaration now permits that value by skipping the element type check. The command summary still says the RPC returns Null, which denotes a different JSON type and contradicts the actual response.
In `src/governance/core_write.cpp`:
- [SUGGESTION] src/governance/core_write.cpp:138: Place the RPCResult API backport before its callers
(existing thread: https://github.com/dashpay/dash/pull/7502#discussion_r3761606796)
Commit `3344adbae81` passes the trailing `/*skip_type_check=*/true` constructor argument here and in `src/rpc/rawtransaction.cpp`, but its parent and that commit's own `src/rpc/util.h` only provide the old constructor signatures. The required `m_skip_type_check` member and constructor parameter arrive in the following commit `f2e13f0f46a`, so this intermediate commit cannot compile even though the final tree can. Reorder the API backport before its callers or squash the dependent changes to keep the series buildable and bisectable.
In `src/rpc/mempool.cpp`:
- [NITPICK] src/rpc/mempool.cpp:276: Document the unlocked instantlock value
(existing thread: https://github.com/dashpay/dash/pull/7502#discussion_r3685409612)
`entryToJSON()` serializes `isman->IsLocked(...)` through `util::to_string(bool)`, which emits either the string `"true"` or `"false"`; `"unknown"` is used only when the InstantSend manager is unavailable. The result description omits the normal unlocked value `"false"`, leaving the documented value set incomplete.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The recursive RPC checker still rejects a supported getpeerinfo response because mapped_as is documented as a string but emitted as a number; the commit-ordering issue and two documentation inconsistencies also remain.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol.
Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/net.cpp`:
- [BLOCKING] src/rpc/net.cpp:129: Missing prerequisite: bitcoin#17812
The backported recursive checker now validates each object in the `getpeerinfo` result array. This schema declares `mapped_as` as `STR`, while the producer at lines 230-231 inserts `uint64_t(stats.m_mapped_as)`. With AS mapping enabled and a peer having a nonzero mapping, result validation rejects the valid numeric response and returns an internal documentation-check error. Upstream bitcoin#17812 commit `5ba829e12e99f119df56cab422f827b9be03fe57` corrected this field to `NUM` before bitcoin#23083, so that prerequisite correction must be carried into Dash.
In `src/rpc/rawtransaction.cpp`:
- [NITPICK] src/rpc/rawtransaction.cpp:549: Update the getislocks summary to document the string "None"
The implementation appends the JSON string `"None"` when no InstantSend lock is known, and the detailed result declaration permits that alternate element type by skipping its type check. The command summary still says the RPC returns Null, which denotes a different JSON type and contradicts the actual response.
In `src/governance/core_write.cpp`:
- [SUGGESTION] src/governance/core_write.cpp:138: Place the RPCResult API backport before its callers
(existing thread: https://github.com/dashpay/dash/pull/7502#discussion_r3761606796)
Commit `3344adbae81` passes the trailing `/*skip_type_check=*/true` constructor argument here and in `src/rpc/rawtransaction.cpp`, but that commit's `src/rpc/util.h` still exposes only the old constructor signatures. The `m_skip_type_check` member and constructor parameter arrive in the following commit `f2e13f0f46a`, so the intermediate commit cannot compile even though the final tree can. Reorder the API commit before its callers or squash the dependent commits to preserve a buildable, bisectable series.
In `src/rpc/mempool.cpp`:
- [NITPICK] src/rpc/mempool.cpp:276: Document the unlocked instantlock value
(existing thread: https://github.com/dashpay/dash/pull/7502#discussion_r3685409612)
`entryToJSON()` serializes `isman->IsLocked(...)` through `util::to_string(bool)`, which emits the string `"true"` or `"false"`; `"unknown"` is emitted only when the InstantSend manager is unavailable. The description omits the normal unlocked value `"false"`, leaving the documented value set incomplete.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The mapped_as type mismatch that previously blocked recursive getpeerinfo result validation is fixed at the current head. Three non-blocking issues remain: one non-buildable intermediate commit and two inaccurate RPC help descriptions.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/rawtransaction.cpp`:
- [NITPICK] src/rpc/rawtransaction.cpp:549: Update the getislocks summary to document the string "None"
The implementation appends the JSON string `"None"` when no InstantSend lock is known, and the detailed result declaration permits that alternate element shape by skipping its type check. The command summary still says the RPC returns Null, which denotes a different JSON type and contradicts the actual response.
In `src/governance/core_write.cpp`:
- [SUGGESTION] src/governance/core_write.cpp:138: Place the RPCResult API backport before its callers
(existing thread: https://github.com/dashpay/dash/pull/7502#discussion_r3761606796)
Commit `3344adbae81` passes the trailing `/*skip_type_check=*/true` constructor argument here and in `src/rpc/rawtransaction.cpp`, but that commit's `src/rpc/util.h` still provides only the old constructor signatures. The `m_skip_type_check` member and corresponding constructor parameter are introduced by the following commit, `f2e13f0f46a`. Checking out or bisecting through `3344adbae81` therefore produces a compilation failure even though the final tree compiles; reorder the API commit before its callers or squash the dependent commits.
In `src/rpc/mempool.cpp`:
- [NITPICK] src/rpc/mempool.cpp:276: Document the unlocked instantlock value
(existing thread: https://github.com/dashpay/dash/pull/7502#discussion_r3685409612)
`entryToJSON()` serializes `isman->IsLocked(...)` through `util::to_string(bool)`, which emits the string `"true"` or `"false"`; `"unknown"` is emitted only when the InstantSend manager is unavailable. The result description omits the normal unlocked value `"false"`, leaving the documented value set incomplete.
4 participants
Issue being fixed or feature implemented
Backport bitcoin#23083 discovered multiple issues in our help for RPC
What was done?
AI crafted fixes for RPC help + backport bitcoin#23083 + fixes from bitcoin#34683
How Has This Been Tested?
Run unit / functional tests, fix multiple failures such as:
Breaking Changes
Debug builds going to crash every time when RPC help is not matching with real RPC response.
Checklist: