Use RPCHelpMan for all RPCs

MarcoFalke commented at 2:09 am on November 14, 2018: member

The resulting documentation should not change unless the type in the oneline-summary was previously incorrect. (E.g. string vs bool)

MarcoFalke added the label Docs on Nov 14, 2018

MarcoFalke added the label RPC/REST/ZMQ on Nov 14, 2018

DrahtBot commented at 2:16 am on November 14, 2018: 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:

#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
#14670 (http: Fix HTTP server shutdown by promag)
#14602 (Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") by luke-jr)
#14459 (More RPC help description fixes by ch4ot1c)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)
#13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
#12096 ([rpc] [wallet] Allow specifying the output index when using bumpfee by kallewoof)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

laanwj commented at 10:23 am on November 14, 2018: member

Concept ACK!

in test/lint/lint-rpc-help.sh:6 in fa20188aa1 outdated

0@@ -0,0 +1,17 @@
1+#!/usr/bin/env bash
2+#
3+# Copyright (c) 2018 The Bitcoin Core developers
4+# Distributed under the MIT software license, see the accompanying
5+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
6+

practicalswift commented at 12:18 pm on November 14, 2018:

Add a description of what the linter does :-)

in test/lint/lint-rpc-help.sh:10 in fa20188aa1 outdated

 5+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
 6+
 7+export LC_ALL=C
 8+
 9+EXIT_CODE=0
10+non_autogenerated_help=$(grep --perl-regexp --null-data --only-matching 'runtime_error\(\n\s*".*\\n"\n' $(git ls-files -- "*.cpp"))

practicalswift commented at 12:18 pm on November 14, 2018:

I’m afraid BSD grep does not support --perl-regexp and --null-data :-\

ryanofsky commented at 3:58 pm on November 15, 2018:

This linter doesn’t seem like very reliable. Maybe we can just keep it temporarily (add a comment to remove in 6 months)?

If we’re are going to keep this, I would suggest adding a comment that describes what this regex is looking for. It seems like it just looking for std::runtime_error constructions with a specifically formatted type of multiline string, which could miss non-uses of RPCHelpMan, and also get in the way of providing detailed error messages in other cases.

practicalswift commented at 9:11 pm on November 15, 2018:

Perhaps the linter can just exit 0 with an information message if BSD grep is used?

practicalswift commented at 3:02 pm on November 14, 2018: contributor

Concept ACK

Extra credit for adding a linting which makes sure we’ll never have to think about this again :-)

promag commented at 3:50 pm on November 14, 2018: member

Concept ACK.

MarcoFalke added this to the milestone 0.18.0 on Nov 14, 2018

MarcoFalke commented at 4:43 pm on November 14, 2018: member

For reference the diff in the resulting rpc doc is:

  0diff --git a/bumpfee b/bumpfee
  1index b47ef5b..892a1e6 100644
  2--- a/bumpfee
  3+++ b/bumpfee
  4@@ -1,4 +1,4 @@
  5-bumpfee "txid" ( options ) 
  6+bumpfee "txid" ( options )
  7 
  8 Bumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.
  9 An opt-in RBF transaction with the given txid must be in the wallet.
 10diff --git a/estimaterawfee b/estimaterawfee
 11index 16f57ca..3b99c08 100644
 12--- a/estimaterawfee
 13+++ b/estimaterawfee
 14@@ -1,4 +1,4 @@
 15-estimaterawfee conf_target (threshold)
 16+estimaterawfee conf_target ( threshold )
 17 
 18 WARNING: This interface is unstable and may disappear or change!
 19 
 20diff --git a/estimatesmartfee b/estimatesmartfee
 21index fc53c26..af7dd35 100644
 22--- a/estimatesmartfee
 23+++ b/estimatesmartfee
 24@@ -1,4 +1,4 @@
 25-estimatesmartfee conf_target ("estimate_mode")
 26+estimatesmartfee conf_target ( "estimate_mode" )
 27 
 28 Estimates the approximate fee per kilobyte needed for a transaction to begin
 29 confirmation within conf_target blocks if possible and return the number of blocks
 30diff --git a/generatetoaddress b/generatetoaddress
 31index 4506040..7713819 100644
 32--- a/generatetoaddress
 33+++ b/generatetoaddress
 34@@ -1,4 +1,4 @@
 35-generatetoaddress nblocks address (maxtries)
 36+generatetoaddress nblocks "address" ( maxtries )
 37 
 38 Mine blocks immediately to a specified address (before the RPC call returns)
 39 
 40diff --git a/getblock b/getblock
 41index 5f94e32..a6e6654 100644
 42--- a/getblock
 43+++ b/getblock
 44@@ -1,4 +1,4 @@
 45-getblock "blockhash" ( verbosity ) 
 46+getblock "blockhash" ( verbosity )
 47 
 48 If verbosity is 0, returns a string that is serialized, hex-encoded data for block 'hash'.
 49 If verbosity is 1, returns an Object with information about block <hash>.
 50diff --git a/getmemoryinfo b/getmemoryinfo
 51index ab184e3..515a275 100644
 52--- a/getmemoryinfo
 53+++ b/getmemoryinfo
 54@@ -1,4 +1,4 @@
 55-getmemoryinfo ("mode")
 56+getmemoryinfo ( "mode" )
 57 Returns an object containing information about memory usage.
 58 Arguments:
 59 1. "mode" determines what kind of information is returned. This argument is optional, the default mode is "stats".
 60diff --git a/getmempoolancestors b/getmempoolancestors
 61index 406f659..9382b12 100644
 62--- a/getmempoolancestors
 63+++ b/getmempoolancestors
 64@@ -1,4 +1,4 @@
 65-getmempoolancestors txid ( verbose )
 66+getmempoolancestors "txid" ( verbose )
 67 
 68 If txid is in the mempool, returns all in-mempool ancestors.
 69 
 70diff --git a/getmempooldescendants b/getmempooldescendants
 71index 1198324..aacabbe 100644
 72--- a/getmempooldescendants
 73+++ b/getmempooldescendants
 74@@ -1,4 +1,4 @@
 75-getmempooldescendants txid ( verbose )
 76+getmempooldescendants "txid" ( verbose )
 77 
 78 If txid is in the mempool, returns all in-mempool descendants.
 79 
 80diff --git a/getmempoolentry b/getmempoolentry
 81index 468470a..cf95910 100644
 82--- a/getmempoolentry
 83+++ b/getmempoolentry
 84@@ -1,4 +1,4 @@
 85-getmempoolentry txid
 86+getmempoolentry "txid"
 87 
 88 Returns mempool data for given transaction
 89 
 90diff --git a/importprivkey b/importprivkey
 91index 6ee2531..cd361d5 100644
 92--- a/importprivkey
 93+++ b/importprivkey
 94@@ -1,4 +1,4 @@
 95-importprivkey "privkey" ( "label" ) ( rescan )
 96+importprivkey "privkey" ( "label" rescan )
 97 
 98 Adds a private key (as returned by dumpprivkey) to your wallet. Requires a new wallet backup.
 99 Hint: use importmulti to import more than one private key.
100diff --git a/listreceivedbyaddress b/listreceivedbyaddress
101index 95a4a26..baa068d 100644
102--- a/listreceivedbyaddress
103+++ b/listreceivedbyaddress
104@@ -1,4 +1,4 @@
105-listreceivedbyaddress ( minconf include_empty include_watchonly address_filter )
106+listreceivedbyaddress ( minconf include_empty include_watchonly "address_filter" )
107 
108 List balances by receiving address.
109 
110diff --git a/listreceivedbylabel b/listreceivedbylabel
111index 23961c3..40ecc52 100644
112--- a/listreceivedbylabel
113+++ b/listreceivedbylabel
114@@ -1,4 +1,4 @@
115-listreceivedbylabel ( minconf include_empty include_watchonly)
116+listreceivedbylabel ( minconf include_empty include_watchonly )
117 
118 List received transactions by label.
119 
120diff --git a/listunspent b/listunspent
121index bc3baf8..84235b4 100644
122--- a/listunspent
123+++ b/listunspent
124@@ -1,4 +1,4 @@
125-listunspent ( minconf maxconf ["address",...] include_unsafe {"minimumAmount":amount,"maximumAmount":amount,"maximumCount":n,"minimumSumAmount":amount} )
126+listunspent ( minconf maxconf ["address",...] include_unsafe query_options )
127 
128 Returns array of unspent transaction outputs
129 with between minconf and maxconf (inclusive) confirmations.
130diff --git a/logging b/logging
131index 43d6505..56bd6c5 100644
132--- a/logging
133+++ b/logging
134@@ -1,4 +1,4 @@
135-logging ( <include> <exclude> )
136+logging ( "include" "exclude" )
137 Gets and sets the logging configuration.
138 When called without an argument, returns the list of categories with status that are currently being debug logged or not.
139 When called with arguments, adds or removes categories from debug logging and return the lists above.
140diff --git a/rescanblockchain b/rescanblockchain
141index 8971533..fcd8024 100644
142--- a/rescanblockchain
143+++ b/rescanblockchain
144@@ -1,4 +1,4 @@
145-rescanblockchain ("start_height") ("stop_height")
146+rescanblockchain ( start_height stop_height )
147 
148 Rescan the local blockchain for wallet related transactions.
149 
150diff --git a/sendmany b/sendmany
151index ead3d41..79f54cb 100644
152--- a/sendmany
153+++ b/sendmany
154@@ -1,4 +1,4 @@
155-sendmany "" {"address":amount,...} ( minconf "comment" ["address",...] replaceable conf_target "estimate_mode")
156+sendmany "" {"address":amount} ( minconf "comment" ["address",...] replaceable conf_target "estimate_mode" )
157 
158 Send multiple times. Amounts are double-precision floating point numbers.
159 
160diff --git a/sendtoaddress b/sendtoaddress
161index 5b04077..3ca8683 100644
162--- a/sendtoaddress
163+++ b/sendtoaddress
164@@ -1,4 +1,4 @@
165-sendtoaddress "address" amount ( "comment" "comment_to" subtractfeefromamount replaceable conf_target "estimate_mode")
166+sendtoaddress "address" amount ( "comment" "comment_to" subtractfeefromamount replaceable conf_target "estimate_mode" )
167 
168 Send an amount to a given address.
169 
170diff --git a/sethdseed b/sethdseed
171index 6b4b16f..b2be7d2 100644
172--- a/sethdseed
173+++ b/sethdseed
174@@ -1,4 +1,4 @@
175-sethdseed ( "newkeypool" "seed" )
176+sethdseed ( newkeypool "seed" )
177 
178 Set or generate a new HD wallet seed. Non-HD wallets will not be upgraded to being a HD wallet. Wallets that are already
179 HD will have a new HD seed set so that new keys added to the keypool will be derived from this new seed.
180diff --git a/submitblock b/submitblock
181index d33307a..cdf8d0d 100644
182--- a/submitblock
183+++ b/submitblock
184@@ -1,4 +1,4 @@
185-submitblock "hexdata"  ( "dummy" )
186+submitblock "hexdata" ( "dummy" )
187 
188 Attempts to submit new block to network.
189 See https://en.bitcoin.it/wiki/BIP_0022 for full specification.
190diff --git a/waitforblock b/waitforblock
191index 20889b4..45de4eb 100644
192--- a/waitforblock
193+++ b/waitforblock
194@@ -1,4 +1,4 @@
195-waitforblock <blockhash> (timeout)
196+waitforblock "blockhash" ( timeout )
197 
198 Waits for a specific new block and returns useful info about it.
199 
200diff --git a/waitfornewblock b/waitfornewblock
201index ea184e3..f66d804 100644
202--- a/waitfornewblock
203+++ b/waitfornewblock
204@@ -1,4 +1,4 @@
205-waitfornewblock (timeout)
206+waitfornewblock ( timeout )
207 
208 Waits for a specific new block and returns useful info about it.
209 
210diff --git a/walletcreatefundedpsbt b/walletcreatefundedpsbt
211index 25e77bd..2260770 100644
212--- a/walletcreatefundedpsbt
213+++ b/walletcreatefundedpsbt
214@@ -1,4 +1,4 @@
215-walletcreatefundedpsbt [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount},{"data":"hex"},...] ( locktime {"changeAddress":"hex","changePosition":n,"change_type":"str","includeWatching":bool,"lockUnspents":bool,"feeRate":n,"subtractFeeFromOutputs":[int,...],"replaceable":bool,"conf_target":n,"estimate_mode":"str"} bip32derivs )
216+walletcreatefundedpsbt [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount},{"data":"hex"},...] ( locktime options bip32derivs )
217 
218 Creates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough
219 Implements the Creator and Updater roles.

lint: Must use RPCHelpMan to generate the RPC docs fa520e72f7

MarcoFalke force-pushed on Nov 14, 2018

DrahtBot added the label Needs rebase on Nov 14, 2018

DrahtBot removed the label Needs rebase on Nov 14, 2018

in src/rpc/mining.cpp:341 in 777758a79d outdated

329+                                },
330+                                true},
331+                        },
332+                        true},
333+                }}
334+                .ToString() +

ryanofsky commented at 5:24 pm on November 14, 2018:

I probably should have asked this in #14530. But is there a plan for:

 0{"template_request", RPCArg::Type::OBJ,
 1    {
 2        {"mode", RPCArg::Type::STR, true},
 3        {"capabilities", RPCArg::Type::ARR,
 4            {
 5                {"support", RPCArg::Type::STR, true},
 6            },
 7            true},
 8        {"rules", RPCArg::Type::ARR,
 9            {
10                {"support", RPCArg::Type::STR, true},
11            },
12            true},
13    },
14    true},

to not duplicate:

 0"1. template_request         (json object, optional) A json object in the following spec\n"
 1"     {\n"
 2"       \"mode\":\"template\"    (string, optional) This must be set to \"template\", \"proposal\" (see BIP 23), or omitted\n"
 3"       \"capabilities\":[     (array, optional) A list of strings\n"
 4"           \"support\"          (string) client side supported feature, 'longpoll', 'coinbasetxn', 'coinbasevalue', 'proposal', 'serverlist', 'workid'\n"
 5"           ,...\n"
 6"       ],\n"
 7"       \"rules\":[            (array, optional) A list of strings\n"
 8"           \"support\"          (string) client side supported softfork deployment\n"
 9"           ,...\n"
10"       ]\n"
11"     }\n"
12"\n"

below?

MarcoFalke commented at 5:33 pm on November 14, 2018:

@ryanofsky You simply pass through the description string for each arg. Though the resulting diff will be larger because in the verbose description the types are not named in the same way across all docs, so I’d like to do that as a follow up pull request.

promag commented at 5:50 pm on November 14, 2018:

I had the same doubt as @ryanofsky. Before merging it would be great to see a POC of that @MarcoFalke.

MarcoFalke commented at 4:51 pm on November 15, 2018:

@ryanofsky @promag See the code in 92377d37cc0e59713874d5af406abfd84a7d972a, which produces

 01. template_request    (json object, optional, default=m_default) m_description
 1     {                 (json object)
 2       "mode":"str"    (string, optional, default=m_default) m_description
 3       [               (json array)
 4         "support"     (string, optional, default=m_default) m_description
 5         ,...          
 6       ]               
 7       [               (json array)
 8         "support"     (string, optional, default=m_default) m_description
 9         ,...          
10       ]               
11       ,...            
12     }

Obviously needs someone to pass in the members (m_description and m_default), like I mentioned above.

ryanofsky commented at 5:52 pm on November 14, 2018: member

I think in some of these cases, the previous output might be more readable than the new output. Examples:

0-scantxoutset "action" [scanobjects,...]
1+scantxoutset "action" [{"desc":"str","range":n},...]

0-sendmany "" {"address":amount,...} ( minconf "comment" ["address",...] replaceable conf_target "estimate_mode")
1+sendmany "dummy" {"address":amount} ( minconf "comment" ["address",...] replaceable conf_target "estimate_mode" )

In the first case instead of showing name of the argument, this now shows the structure of the argument without any context that would help understand it. Losing the name also seems like an inconvenience if you want to pass named arguments.

In the second case, the change goes in the opposite direction. Instead of showing the right dummy value to pass ("") it now shows the string "dummy" which would get rejected.

I don’t know if these changes are desirable, but I think it would be desirable to add some basic flags or options to RPCArg to avoid making these changes in this PR. (I think they’d be fine for a much smaller followup PR.)

MarcoFalke force-pushed on Nov 14, 2018

Use RPCHelpMan for all RPCs fa91e8eda5

MarcoFalke force-pushed on Nov 14, 2018

MarcoFalke commented at 8:47 pm on November 14, 2018: member

Restored the inconsistencies. Resulting changes should be formatting-only.

fanquake commented at 8:32 am on November 15, 2018: member

Concept ACK

in src/rpc/misc.cpp:464 in fa91e8eda5 outdated

464-            "\nThe difference between echo and echojson is that echojson has argument conversion enabled in the client-side table in"
465-            "bitcoin-cli and the GUI. There is no server-side difference."
466-        );
467+            RPCHelpMan{"echo|echojson ...",
468+                "\nSimply echo back the input arguments. This command is for testing.\n"
469+                "\nThe difference between echo and echojson is that echojson has argument conversion enabled in the client-side table in"

ryanofsky commented at 4:15 pm on November 15, 2018:

Not directly related (space was missing previously) but maybe add space between “in” and “bitcoin-cli”

ryanofsky approved

ryanofsky commented at 5:04 pm on November 15, 2018: member

ACK fa91e8eda541acdb78ca481b74605639f319c108

I dumped and diffed help with: Code

 0diff --git a/src/init.cpp b/src/init.cpp
 1index 3ab97be329..bc119ab1ca 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -46,6 +46,7 @@
 5 #include <util/system.h>
 6 #include <util/moneystr.h>
 7 #include <validationinterface.h>
 8+#include <wallet/wallet.h>
 9 #include <warnings.h>
10 #include <walletinitinterface.h>
11 #include <stdint.h>
12@@ -1240,6 +1241,29 @@ bool AppInitMain(InitInterfaces& interfaces)
13     for (const auto& client : interfaces.chain_clients) {
14         client->registerRpcs();
15     }
16+    AddWallet(std::make_shared<CWallet>(*interfaces.chain, WalletLocation(), WalletDatabase::CreateDummy()));
17+    JSONRPCRequest jreq;
18+    jreq.fHelp = true;
19+    for (const auto& entry : tableRPC.mapCommands) {
20+        try {
21+            entry.second->actor(jreq);
22+        } catch (const std::exception& e) {
23+            printf("===\n%s\n", e.what());
24+            continue;
25+        } catch (const UniValue& e) {
26+            printf("===\n%s\n", e.write().c_str());
27+            continue;
28+        } catch (...) {
29+            printf("===\n%s\n", entry.second->name.c_str());
30+            fflush(stdout);
31+            assert(0);
32+        }
33+        printf("===\n%s\n", entry.second->name.c_str());
34+        fflush(stdout);
35+        assert(0);
36+    }
37+    fflush(stdout);
38+    abort();
39     g_rpc_interfaces = &interfaces;
40 #if ENABLE_ZMQ
41     RegisterZMQRPCCommands(tableRPC);
42diff --git a/src/rpc/server.h b/src/rpc/server.h
43index 2d62a76f3c..6fbb35ec57 100644
44--- a/src/rpc/server.h
45+++ b/src/rpc/server.h
46@@ -142,7 +142,7 @@ public:
47  */
48 class CRPCTable
49 {
50-private:
51+public:
52     std::map<std::string, const CRPCCommand*> mapCommands;
53 public:
54     CRPCTable();

Complete output ignoring whitespace was: Output

 0@@ -263 +263 @@
 1-createpsbt [{"txid":"id","vout":n},...] [{"address":amount},{"data":"hex"},...] ( locktime ) ( replaceable )
 2+createpsbt [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount},{"data":"hex"},...] ( locktime replaceable )
 3@@ -2022 +2022 @@
 4-gettxoutproof ["txid",...] ( blockhash )
 5+gettxoutproof ["txid",...] ( "blockhash" )
 6@@ -2537 +2537 @@
 7-listunspent ( minconf maxconf  ["addresses",...] [include_unsafe] [query_options])
 8+listunspent ( minconf maxconf ["address",...] include_unsafe {"minimumAmount":amount,"maximumAmount":amount,"maximumCount":n,"minimumSumAmount":amount} )
 9@@ -2640 +2640 @@
10-lockunspent unlock ([{"txid":"txid","vout":n},...])
11+lockunspent unlock ( [{"txid":"hex","vout":n},...] )
12@@ -3131 +3131 @@
13-signrawtransactionwithkey "hexstring" ["privatekey1",...] ( [{"txid":"id","vout":n,"scriptPubKey":"hex","redeemScript":"hex"},...] sighashtype )
14+signrawtransactionwithkey "hexstring" ["privatekey",...] ( [{"txid":"hex","vout":n,"scriptPubKey":"hex","redeemScript":"hex","amount":amount},...] "sighashtype" )
15@@ -3186 +3186 @@
16-signrawtransactionwithwallet "hexstring" ( [{"txid":"id","vout":n,"scriptPubKey":"hex","redeemScript":"hex"},...] sighashtype )
17+signrawtransactionwithwallet "hexstring" ( [{"txid":"hex","vout":n,"scriptPubKey":"hex","redeemScript":"hex","amount":amount},...] "sighashtype" )
18@@ -3484 +3484 @@
19-walletcreatefundedpsbt [{"txid":"id","vout":n},...] [{"address":amount},{"data":"hex"},...] ( locktime ) ( replaceable ) ( options bip32derivs )
20+walletcreatefundedpsbt [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount},{"data":"hex"},...] ( locktime {"changeAddress":"hex","changePosition":n,"change_type":"str","includeWatching":bool,"lockUnspents":bool,"feeRate":n,"subtractFeeFromOutputs":[int,...],"replaceable":bool,"conf_target":n,"estimate_mode":"str"} bip32derivs )

rpc: Documentation fixups fa5e0452e8

MarcoFalke commented at 5:19 pm on November 15, 2018: member

Added a separate commit for the documentation fixups requested by @practicalswift and @ryanofsky

ryanofsky approved

ryanofsky commented at 5:32 pm on November 15, 2018: member

utACK fa5e0452e875a7ca6bf6fe61fdd652d341eece40. Only change is new commit with whitespace/comments.

laanwj commented at 8:42 am on November 23, 2018: member

utACK fa5e0452e875a7ca6bf6fe61fdd652d341eece40

This is a nice step forward in the consistency of RPC help.

laanwj merged this on Nov 23, 2018

laanwj closed this on Nov 23, 2018

laanwj referenced this in commit a0d86815cd on Nov 23, 2018

MarcoFalke deleted the branch on Nov 23, 2018

Sjors commented at 3:16 pm on November 27, 2018: member

Nice! Does this allow us to get rid of CRPCConvertParam, which is quite easy to forget?

MarcoFalke commented at 7:47 pm on December 3, 2018: member

@Sjors Not so quick. It could potentially help with those, but I’d suggest to first focus on the formatting and getting all parameters in correctly, which is being done in #14796. I have some fun changes piled up locally, but I can only do them after #14796.

deadalnix referenced this in commit 3a84ae8b0c on Mar 25, 2020

ftrader referenced this in commit d21e79aa47 on Dec 1, 2020

MarcoFalke locked this on Sep 8, 2021

Use RPCHelpMan for all RPCs #14726

Conflicts