doc: Fix RPC result documentation

MarcoFalke commented at 5:58 pm on August 25, 2021: member

Fix:

Incorrectly named fields
Add missing ones
Add missing optional flag

jonatack commented at 6:01 pm on August 25, 2021: member

Wow. Concept ACK.

ghost commented at 6:33 pm on August 25, 2021: none

If RPC result documentation is fixed in bulk, maybe this can be added: https://github.com/bitcoin/bitcoin/pull/22430/

DrahtBot added the label Mining on Aug 25, 2021

DrahtBot added the label RPC/REST/ZMQ on Aug 25, 2021

DrahtBot added the label Wallet on Aug 25, 2021

theStack commented at 7:21 pm on August 25, 2021: member

Concept ACK

MarcoFalke removed the label Wallet on Aug 25, 2021

MarcoFalke removed the label Mining on Aug 25, 2021

MarcoFalke added the label Docs on Aug 25, 2021

MarcoFalke commented at 7:22 pm on August 25, 2021: member

@prayank23 My plan is to auto-generate the RPC examples, so that such issues are impossible. See #22799 for the meta issue.

MarcoFalke commented at 7:44 pm on August 25, 2021: member

For reference, if someone wants to export the RPC docs to produce a rendered diff, the following patch might be useful:

(dump_dir is a directory created by git init)

 0diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
 1index cf80b08b96..6c6a71c6e4 100644
 2--- a/src/rpc/server.cpp
 3+++ b/src/rpc/server.cpp
 4@@ -95,7 +95,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
 5     {
 6         const CRPCCommand *pcmd = command.second;
 7         std::string strMethod = pcmd->name;
 8-        if ((strCommand != "" || pcmd->category == "hidden") && strMethod != strCommand)
 9+        if ((strCommand != "") && strMethod != strCommand)
10             continue;
11         jreq.strMethod = strMethod;
12         try
13diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py
14index de21f43747..753ee771ad 100755
15--- a/test/functional/rpc_help.py
16+++ b/test/functional/rpc_help.py
17@@ -100,7 +100,7 @@ class HelpRpcTest(BitcoinTestFramework):
18         # command titles
19         titles = [line[3:-3] for line in node.help().splitlines() if line.startswith('==')]
20 
21-        components = ['Blockchain', 'Control', 'Generating', 'Mining', 'Network', 'Rawtransactions', 'Util']
22+        components = ['Blockchain', 'Control', 'Generating', 'Hidden', 'Mining', 'Network', 'Rawtransactions', 'Util']
23 
24         if self.is_wallet_compiled():
25             components.append('Wallet')
26@@ -116,7 +116,8 @@ class HelpRpcTest(BitcoinTestFramework):
27     def dump_help(self):
28         dump_dir = os.path.join(self.options.tmpdir, 'rpc_help_dump')
29         os.mkdir(dump_dir)
30-        calls = [line.split(' ', 1)[0] for line in self.nodes[0].help().splitlines() if line and not line.startswith('==')]
31+        dump_dir = '/tmp/temp_git/' ##HACK
32+        calls = [line.split(' ', 1)[0].split('|', 1)[0] for line in self.nodes[0].help().splitlines() if line and not line.startswith('==')]
33         for call in calls:
34             with open(os.path.join(dump_dir, call), 'w', encoding='utf-8') as f:
35                 # Make sure the node can generate the help at runtime without crashing

DrahtBot commented at 11:40 pm on August 25, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22650 (Remove -deprecatedrpc=addresses flag and corresponding code/logic by mjdietzx)
#22558 (psbt: Taproot fields for PSBT by achow101)
#22525 (docs: Update the explainer text for the listunspent RPC by Fonta1n3)
#22016 (rpc: add period_start to version bits statistics by Sjors)
#21283 (Implement BIP 370 PSBTv2 by achow101)
#19888 (rpc, test: Improve getblockstats for unspendables by fjahr)
#19002 (docs: Document that ‘fee’ is unavailable when there are non-wallet inputs by shesek)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Herrytheeagle approved

Herrytheeagle commented at 3:26 am on August 26, 2021: none

Properly fixed with the comment integration for clear and understandable codebase

lsilva01 commented at 4:52 am on August 26, 2021: contributor

Concept ACK

practicalswift commented at 1:53 pm on September 4, 2021: contributor

Concept ACK

in src/rpc/rawtransaction.cpp:1083 in fa1d3ddaa3 outdated

1082@@ -1078,22 +1083,23 @@ static RPCHelpMan decodepsbt()
1083                                 }},

fanquake commented at 7:07 am on September 10, 2021:

In decodepsbt, looks like scriptPubKey -> address should also be marked optional.

MarcoFalke commented at 9:12 am on September 21, 2021:

Thanks, fixed.

in src/wallet/rpcwallet.cpp:3448 in fa1d3ddaa3 outdated

3428@@ -3426,6 +3429,10 @@ RPCHelpMan signrawtransactionwithwallet()
3429                             {
3430                                 {RPCResult::Type::STR_HEX, "txid", "The hash of the referenced, previous transaction"},
3431                                 {RPCResult::Type::NUM, "vout", "The index of the output to spent and used as input"},
3432+                                {RPCResult::Type::ARR, "witness", "",

fanquake commented at 7:31 am on September 10, 2021:

Should be added to signrawtransactionwithkey() as well?

MarcoFalke commented at 9:12 am on September 21, 2021:

It already is?

in src/rpc/net.cpp:143 in fa1d3ddaa3 outdated

141+                            {RPCResult::Type::NUM, "pingtime", /* optional */ true, "ping time (if available)"},
142+                            {RPCResult::Type::NUM, "minping", /* optional */ true, "minimum observed ping time (if any at all)"},
143+                            {RPCResult::Type::NUM, "pingwait", /* optional */ true, "ping wait (if non-zero)"},
144                             {RPCResult::Type::NUM, "version", "The peer version, such as 70001"},
145                             {RPCResult::Type::STR, "subver", "The string version"},
146                             {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"},

fanquake commented at 8:00 am on September 10, 2021:

Should startingheight, synced_headers, synced_blocks …. addr_rate_limited be optional as well? They are dependant on fStateStats same as pingwait.

MarcoFalke commented at 9:08 am on September 21, 2021:

Strictly they are conditional. Though I think the RPC should be changed to not be racy, unless there is a strong reason for it to prefer to be racy.

fanquake commented at 8:03 am on September 10, 2021: member

ACK fa1d3ddaa35424ce92a1d3c31aa7a94787811281 - Clearly this is a big improvement, some of the docs here are flat out wrong.

DrahtBot added the label Needs rebase on Sep 20, 2021

doc: Fix RPC result documentation fa10fbc665

MarcoFalke force-pushed on Sep 21, 2021

MarcoFalke commented at 9:14 am on September 21, 2021: member

Force pushed to reply to feedback

DrahtBot removed the label Needs rebase on Sep 21, 2021

MarcoFalke closed this on Sep 21, 2021

MarcoFalke reopened this on Sep 21, 2021

MarcoFalke commented at 1:21 pm on September 21, 2021: member

Failing CI can be ignored

fanquake approved

fanquake commented at 7:46 am on September 23, 2021: member

ACK fa10fbc6658fb8d87118cd550e4de406dca45f23

fanquake merged this on Sep 23, 2021

fanquake closed this on Sep 23, 2021

MarcoFalke deleted the branch on Sep 23, 2021

sidhujag referenced this in commit f28bac3d37 on Sep 24, 2021

luke-jr referenced this in commit 6f0ccf0dd3 on Dec 14, 2021

DrahtBot locked this on Oct 30, 2022

doc: Fix RPC result documentation #22798

Conflicts