-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: bitcoin/bitcoin#23083: rpc: Fail to return undocumented or misdocumented JSON #7502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
3344adb
f2e13f0
b77fd4a
9d93f74
9bf0816
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,20 +119,23 @@ RPCResult Object::GetJsonHelp(const std::string& key, bool optional) | |
| GetRpcResult("collateralHash"), | ||
| {RPCResult::Type::NUM, "createdAt", "Proposal creation timestamp"}, | ||
| {RPCResult::Type::NUM, "revision", "Proposal revision number"}, | ||
| // Everything but "hex" is the submitter's own JSON echoed back verbatim, so neither the | ||
| // set of keys nor their types is ours to promise: a proposal may carry extra fields and | ||
| // may encode the numeric ones as strings. Hence the type check is skipped here. | ||
| {RPCResult::Type::OBJ, "data", "", { | ||
| // Fields emitted through GetDataAsPlainString(), read by CProposalValidator | ||
| {RPCResult::Type::STR, "end_epoch", /*optional=*/true, "Proposal end timestamp"}, | ||
| {RPCResult::Type::STR, "name", /*optional=*/true, "Proposal name"}, | ||
| {RPCResult::Type::STR, "payment_address", /*optional=*/true, "Proposal payment address"}, | ||
| {RPCResult::Type::STR, "payment_amount", /*optional=*/true, "Proposal payment amount"}, | ||
| {RPCResult::Type::STR, "start_epoch", /*optional=*/true, "Proposal start timestamp"}, | ||
| {RPCResult::Type::STR, "type", /*optional=*/true, "Object type"}, | ||
| {RPCResult::Type::STR, "url", /*optional=*/true, "Proposal URL"}, | ||
| {RPCResult::Type::ANY, "end_epoch", /*optional=*/true, "Proposal end timestamp"}, | ||
| {RPCResult::Type::ANY, "name", /*optional=*/true, "Proposal name"}, | ||
| {RPCResult::Type::ANY, "payment_address", /*optional=*/true, "Proposal payment address"}, | ||
| {RPCResult::Type::ANY, "payment_amount", /*optional=*/true, "Proposal payment amount"}, | ||
| {RPCResult::Type::ANY, "start_epoch", /*optional=*/true, "Proposal start timestamp"}, | ||
| {RPCResult::Type::ANY, "type", /*optional=*/true, "Object type"}, | ||
| {RPCResult::Type::ANY, "url", /*optional=*/true, "Proposal URL"}, | ||
| // Failure case for GetDataAsPlainString() | ||
| {RPCResult::Type::STR, "plain", /*optional=*/true, "Governance object data as string"}, | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 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'] |
||
| }}; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -427,17 +427,17 @@ static RPCHelpMan getcoinjoininfo() | |||||||||||||||||||||
| {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"}, | ||||||||||||||||||||||
|
Comment on lines
427
to
429
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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.
Suggested change
source: ['codex']
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed reproducing for non-wallet build: Though, please note that bitcoin core originally firstly merged this linter, and after that fixed one-by-one all appearing issues, see:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread. |
||||||||||||||||||||||
| {RPCResult::Type::NUM, "queue_size", "How many queues there are currently on the network"}, | ||||||||||||||||||||||
| {RPCResult::Type::BOOL, "running", "Whether mixing is currently running"}, | ||||||||||||||||||||||
| {RPCResult::Type::NUM, "pending_inputs", "The number of successfully mixed inputs kept locked until the transaction spending them is observed"}, | ||||||||||||||||||||||
| {RPCResult::Type::ARR, "sessions", "", | ||||||||||||||||||||||
| {RPCResult::Type::NUM, "queue_size", /*optional=*/true, "How many queues there are currently on the network"}, | ||||||||||||||||||||||
| {RPCResult::Type::BOOL, "running", /*optional=*/true, "Whether mixing is currently running (not returned when no wallet is loaded)"}, | ||||||||||||||||||||||
| {RPCResult::Type::NUM, "pending_inputs", /*optional=*/true, "The number of successfully mixed inputs kept locked until the transaction spending them is observed (not returned when no wallet is loaded)"}, | ||||||||||||||||||||||
| {RPCResult::Type::ARR, "sessions", /*optional=*/true, "Not returned when no wallet is loaded", | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| {RPCResult::Type::OBJ, "", "", | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| {RPCResult::Type::STR_HEX, "protxhash", "The ProTxHash of the masternode"}, | ||||||||||||||||||||||
| GetRpcResult("outpoint"), | ||||||||||||||||||||||
| {RPCResult::Type::STR, "service", "The IP address and port of the masternode (DEPRECATED, returned only if config option -deprecatedrpc=service is passed)"}, | ||||||||||||||||||||||
| {RPCResult::Type::ARR, "addrs_core_p2p", "Network addresses of the masternode used for protocol P2P", | ||||||||||||||||||||||
| {RPCResult::Type::STR_HEX, "protxhash", /*optional=*/true, "The ProTxHash of the masternode (only while connected to one)"}, | ||||||||||||||||||||||
| GetRpcResult("outpoint", /*optional=*/true), | ||||||||||||||||||||||
| {RPCResult::Type::STR, "service", /*optional=*/true, "The IP address and port of the masternode (DEPRECATED, returned only if config option -deprecatedrpc=service is passed)"}, | ||||||||||||||||||||||
| {RPCResult::Type::ARR, "addrs_core_p2p", /*optional=*/true, "Network addresses of the masternode used for protocol P2P", | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| {RPCResult::Type::STR, "address", ""}, | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -448,7 +448,7 @@ static RPCHelpMan getcoinjoininfo() | |||||||||||||||||||||
| }}, | ||||||||||||||||||||||
| }}, | ||||||||||||||||||||||
| {RPCResult::Type::NUM, "keys_left", /*optional=*/true, "How many new keys are left since last automatic backup (if applicable)"}, | ||||||||||||||||||||||
| {RPCResult::Type::STR, "warnings", "Warnings if any"}, | ||||||||||||||||||||||
| {RPCResult::Type::STR, "warnings", /*optional=*/true, "Warnings if any (not returned when no wallet is loaded)"}, | ||||||||||||||||||||||
| }}, | ||||||||||||||||||||||
| RPCResult{"for masternodes", | ||||||||||||||||||||||
| RPCResult::Type::OBJ, "", "", | ||||||||||||||||||||||
|
|
@@ -458,6 +458,8 @@ static RPCHelpMan getcoinjoininfo() | |||||||||||||||||||||
| {RPCResult::Type::STR_HEX, "state", "Current state of the mixing session"}, | ||||||||||||||||||||||
| {RPCResult::Type::NUM, "entries_count", "The number of entries in the mixing session"}, | ||||||||||||||||||||||
| }}, | ||||||||||||||||||||||
| RPCResult{"for non-masternodes without wallet support", | ||||||||||||||||||||||
| RPCResult::Type::OBJ, "", /*optional=*/false, "", {}}, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| RPCExamples{ | ||||||||||||||||||||||
| HelpExampleCli("getcoinjoininfo", "") | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1513,12 +1513,11 @@ static RPCHelpMan protx_list() | |
| RPCResult{ | ||
| RPCResult::Type::ARR, "", "List of masternodes", | ||
| { | ||
| RPCResult{"when detailed=false", RPCResult::Type::STR, "", "ProTx hash"}, | ||
| RPCResult{"when detailed=true", RPCResult::Type::OBJ, "", "", | ||
| { | ||
| // TODO: document fields of the detailed entry | ||
| {RPCResult::Type::ELISION, "", ""} | ||
| }}, | ||
| // Array elements are matched against this doc by index, so the two shapes | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When users request help for Useful? React with 👍 / 👎. |
||
| }}, | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| RPCExamples{""}, | ||
| [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue | ||
|
|
@@ -1772,11 +1771,11 @@ static RPCHelpMan protx_listdiff() | |
| {RPCResult::Type::NUM, "blockHeight", "Height of target (ending) block"}, | ||
| {RPCResult::Type::ARR, "addedMNs", "Added masternodes", | ||
| {CDeterministicMN::GetJsonHelp(/*key=*/"", /*optional=*/false)}}, | ||
| {RPCResult::Type::ARR, "removedMns", "Removed masternodes", | ||
| {RPCResult::Type::ARR, "removedMNs", "Removed masternodes", | ||
| {{RPCResult::Type::STR_HEX, "protx", "ProTx of removed masternode"}}}, | ||
| {RPCResult::Type::ARR, "updatedMNs", "Updated masternodes", | ||
| {{RPCResult::Type::OBJ, "<protx_hash>", "", | ||
| {CDeterministicMNStateDiff::GetJsonHelp(/*key=*/"", /*optional=*/false)}}}}, | ||
| {{RPCResult::Type::OBJ_DYN, "", "json object with ProTx hash as keys", | ||
| {CDeterministicMNStateDiff::GetJsonHelp(/*key=*/"<protx_hash>", /*optional=*/false)}}}}, | ||
| }, | ||
| }, | ||
| RPCExamples{""}, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.