doc: Fix RPC result documentation #22798

maflcko · 2021-08-25T17:58:47Z

Fix:

Incorrectly named fields
Add missing ones
Add missing optional flag

jonatack · 2021-08-25T18:01:45Z

Wow. Concept ACK.

ghost · 2021-08-25T18:33:25Z

If RPC result documentation is fixed in bulk, maybe this can be added: #22430

theStack · 2021-08-25T19:21:34Z

Concept ACK

maflcko · 2021-08-25T19:22:39Z

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

maflcko · 2021-08-25T19:44:21Z

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)

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

DrahtBot · 2021-08-25T23:40:39Z

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

Properly fixed with the comment integration for clear and understandable codebase

lsilva01 · 2021-08-26T04:52:16Z

Concept ACK

practicalswift · 2021-09-04T13:53:33Z

Concept ACK

fanquake

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

fanquake · 2021-09-10T07:07:37Z

src/rpc/rawtransaction.cpp

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


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

Thanks, fixed.

fanquake · 2021-09-10T07:31:20Z

src/wallet/rpcwallet.cpp

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


Should be added to signrawtransactionwithkey() as well?

It already is?

fanquake · 2021-09-10T08:00:43Z

src/rpc/net.cpp

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


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

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.

maflcko

Force pushed to reply to feedback

maflcko · 2021-09-21T09:08:01Z

src/rpc/net.cpp

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


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.

maflcko · 2021-09-21T09:12:19Z

src/wallet/rpcwallet.cpp

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


It already is?

maflcko · 2021-09-21T09:12:30Z

src/rpc/rawtransaction.cpp

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


Thanks, fixed.

maflcko · 2021-09-21T13:21:49Z

Failing CI can be ignored

fanquake

ACK fa10fbc

Github-Pull: bitcoin#22798 Rebased-From: fa10fbc

DrahtBot added Mining RPC/REST/ZMQ Wallet labels Aug 25, 2021

maflcko mentioned this pull request Aug 25, 2021

RPC documentation cleanup ideas #22799

Open

maflcko added Docs and removed Wallet Mining labels Aug 25, 2021

This was referenced Aug 26, 2021

policy/rbf: don't return "incorrect" replaceability status #22665

Closed

Remove -deprecatedrpc=addresses flag and corresponding code/logic #22650

Merged

Herrytheeagle approved these changes Aug 26, 2021

View reviewed changes

DrahtBot mentioned this pull request Aug 26, 2021

docs: Update the explainer text for the listunspent RPC #22525

Closed

fanquake reviewed Sep 10, 2021

View reviewed changes

DrahtBot added the Needs rebase label Sep 20, 2021

doc: Fix RPC result documentation

fa10fbc

maflcko force-pushed the 2108-docRpc branch from fa1d3dd to fa10fbc Compare September 21, 2021 09:13

maflcko commented Sep 21, 2021

View reviewed changes

DrahtBot removed the Needs rebase label Sep 21, 2021

maflcko closed this Sep 21, 2021

maflcko reopened this Sep 21, 2021

DrahtBot mentioned this pull request Sep 22, 2021

psbt: Taproot fields for PSBT #22558

Merged

fanquake approved these changes Sep 23, 2021

View reviewed changes

fanquake merged commit faecb2e into bitcoin:master Sep 23, 2021

maflcko deleted the 2108-docRpc branch September 23, 2021 14:25

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 24, 2021

Merge bitcoin#22798: doc: Fix RPC result documentation

f28bac3

maflcko mentioned this pull request Sep 24, 2021

rpc: Fail to return undocumented or misdocumented JSON #23083

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021

doc: Fix RPC result documentation

6f0ccf0

Github-Pull: bitcoin#22798 Rebased-From: fa10fbc

jnewbery mentioned this pull request Aug 25, 2022

p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates #25923

Closed

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Fix RPC result documentation #22798

doc: Fix RPC result documentation #22798

maflcko commented Aug 25, 2021

jonatack commented Aug 25, 2021

ghost commented Aug 25, 2021

theStack commented Aug 25, 2021

maflcko commented Aug 25, 2021

maflcko commented Aug 25, 2021

DrahtBot commented Aug 25, 2021 •

edited

Loading

Herrytheeagle left a comment

lsilva01 commented Aug 26, 2021

practicalswift commented Sep 4, 2021

fanquake left a comment •

edited

Loading

fanquake Sep 10, 2021

maflcko Sep 21, 2021

fanquake Sep 10, 2021

maflcko Sep 21, 2021

fanquake Sep 10, 2021

maflcko Sep 21, 2021

maflcko left a comment

maflcko Sep 21, 2021

maflcko Sep 21, 2021

maflcko Sep 21, 2021

maflcko commented Sep 21, 2021

fanquake left a comment

doc: Fix RPC result documentation #22798

doc: Fix RPC result documentation #22798

Conversation

maflcko commented Aug 25, 2021

jonatack commented Aug 25, 2021

ghost commented Aug 25, 2021

theStack commented Aug 25, 2021

maflcko commented Aug 25, 2021

maflcko commented Aug 25, 2021

DrahtBot commented Aug 25, 2021 • edited Loading

Conflicts

Herrytheeagle left a comment

Choose a reason for hiding this comment

lsilva01 commented Aug 26, 2021

practicalswift commented Sep 4, 2021

fanquake left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Sep 21, 2021

fanquake left a comment

Choose a reason for hiding this comment

DrahtBot commented Aug 25, 2021 •

edited

Loading

fanquake left a comment •

edited

Loading