Use RPCHelpMan to generate RPC doc strings #14530

MarcoFalke commented at 1:14 PM on October 20, 2018: member

This introduces a manager for the RPC help generation and demonstrates its use of it in some RPCs.

It is the first non-exhaustive step toward #14378 and I will create pull requests for the next steps after this one is merged.

MarcoFalke added the label Refactoring on Oct 20, 2018

fanquake commented at 1:18 PM on October 20, 2018: member

How does this compare/clash with #14502?

MarcoFalke force-pushed on Oct 20, 2018

MarcoFalke commented at 1:36 PM on October 20, 2018: member

@fanquake See #14502 (comment)

MarcoFalke added the label RPC/REST/ZMQ on Oct 20, 2018

MarcoFalke force-pushed on Oct 20, 2018

MarcoFalke renamed this:
~~scripted-diff: Use RPCHelpMan on RPCs with no args~~
scripted-diff: Use RPCHelpMan to generate RPC doc strings
on Oct 20, 2018

DrahtBot commented at 2:19 PM on October 20, 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:

#14491 (Allow descriptor imports with importmulti by MeshCollider)
#14459 (More RPC help description fixes by ch4ot1c)
#14319 (doc: Fix PSBT howto and example parameters by priscoan)
#14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
#14021 (Import key origin data through descriptors in importmulti by achow101)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
#10973 (Refactor: separate wallet from node by ryanofsky)

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.

MarcoFalke force-pushed on Oct 21, 2018

MarcoFalke force-pushed on Oct 22, 2018

MarcoFalke renamed this:
~~scripted-diff: Use RPCHelpMan to generate RPC doc strings~~
Use RPCHelpMan to generate RPC doc strings
on Oct 22, 2018

karelbilek commented at 12:51 PM on October 23, 2018: contributor

Is the plan to write down all objects in the first argument line like this?

For example, fundrawtransaction has a large object; is the plan to use that instead of "options" at the first line?

https://bitcoincore.org/en/doc/0.17.0/rpc/rawtransactions/fundrawtransaction/

If not, I would suggest to use an option to RPCArg, for example "m_writeFirstAsObject"; it could still then be printed as an object in the longer argument writedown.

(The idea is great though.)

karelbilek commented at 1:04 PM on October 23, 2018: contributor

Also the PR changes the RPC from

"scriptPubKey":"hex" or "txid":"id" or "amount": value

to

"scriptPubKey":"str" or "txid":"str" or "amount": n

Is that intentional? I think it decreases readability... "scriptPubKey":"hex" tells me not just "this needs to be a string" but also "this needs to be a hex string".

(On some places in RPC, bitcoin core uses notation "amount" : x.xxx for amount, which is even better - it makes it clearer this is numeral that is in BTC and can contain decimal point.)

I think this should be kept - so there should be more subtypes than UniValue::VType - for example, extra type for amount, type for hex. (Txid can be hex.)

karelbilek commented at 1:18 PM on October 23, 2018: contributor

There will also be support for "option object" - that is, something can be two different things - for example as in walletcreatefundedpsbt object in outputs - can be either {"address":amount} or {"data":"hex"}

...that all leads me to think this shouldn't use "univalue" types but its own types

MarcoFalke force-pushed on Oct 23, 2018

MarcoFalke commented at 12:00 AM on October 24, 2018: member

Thanks, addressed your feedback by adding an enum class with all the (sub)types I could find.

karelbilek commented at 9:10 AM on October 24, 2018: contributor

I am thinking, how to address this pattern (from sendmany)

2. "amounts"             (string, required) A json object with addresses and amounts
    {
      "address":amount   (numeric or string) The bitcoin address is the key, the numeric amount (can be string) in BTC is the value
      ,...
    }

It looks like "address" is the string key in json object, but actually it isn't; the string key in actual usage will be the actual address, not string "address".

So in this object, adding ,... makes sense. But in other objects, adding ,... doesn't make sense (if it is an "options" object, for example).

Maybe there should be 2 kinds of object types, one where the list of keys is exhaustive and one where it isn't (and the user should actually change the key to his own strings)?

MarcoFalke force-pushed on Oct 24, 2018

MarcoFalke commented at 10:35 AM on October 24, 2018: member

Added a type (OBJ_USER_KEYS) where the user can set multiple keys (e.g. addresses).

karelbilek commented at 12:27 PM on October 24, 2018: contributor

I have played with your code and added functions for printing the whole argument table and got it working.

See this branch (I have just added new commits, no rebase)

https://github.com/karel-3d/bitcoin/commits/Mf1810-rpcHelpMan

I have moved the same functions as you did to this format. It works! But - I am not sure about the readability of the code and about the resulting RPC help. It needs some cleanup.

What I did not yet done is adding the "default" option, and adding "Result" (and "Examples").

It needs to be rebased on top of your new branch with OBJ_USER_KEYS.

MarcoFalke commented at 12:42 PM on October 24, 2018: member

Thanks for hacking on this! Transforming all of the rpc help texts can be done at a later point to avoid merge conflicts with this pull request for now. As soon as this one is merged I (or anyone else) can create these refactoring pull requests at low rate to not burden reviewers.

karelbilek commented at 1:40 PM on October 24, 2018: contributor

I'm not sure if it's a better idea to first merge this PR as-is and keep it only for generating the first arg line, or make it usable for the whole argument section even in this PR before merging (as in my repo) to better prove "usefulness".

MarcoFalke force-pushed on Oct 26, 2018

MarcoFalke added this to the milestone 0.18.0 on Oct 29, 2018

DrahtBot added the label Needs rebase on Nov 5, 2018

MarcoFalke force-pushed on Nov 5, 2018

DrahtBot removed the label Needs rebase on Nov 5, 2018

MarcoFalke force-pushed on Nov 6, 2018

MarcoFalke commented at 4:24 PM on November 6, 2018: member

Rebased

DrahtBot added the label Needs rebase on Nov 9, 2018

rpc: Include rpc/util.h where needed for RPCHelpMan

Just a preparatory commit to add the header to the includes and run
clang-format to sort the include lists.

Splitting this up into a separate commit makes future scripted-diffs
easier.

fa0d36f712

MarcoFalke force-pushed on Nov 9, 2018

MarcoFalke commented at 6:03 PM on November 9, 2018: member

Note that the generated doc can be easily compared against the previous one by running this weird hack:

<details><summary>Patch</summary>

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index f619857da1..188b2795c2 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -1782,7 +1782,6 @@ static const CRPCCommand commands[] =
     { "rawtransactions",    "decodescript",                 &decodescript,              {"hexstring"} },
     { "rawtransactions",    "sendrawtransaction",           &sendrawtransaction,        {"hexstring","allowhighfees"} },
     { "rawtransactions",    "combinerawtransaction",        &combinerawtransaction,     {"txs"} },
-    { "hidden",             "signrawtransaction",           &signrawtransaction,        {"hexstring","prevtxs","privkeys","sighashtype"} },
     { "rawtransactions",    "signrawtransactionwithkey",    &signrawtransactionwithkey, {"hexstring","privkeys","prevtxs","sighashtype"} },
     { "rawtransactions",    "testmempoolaccept",            &testmempoolaccept,         {"rawtxs","allowhighfees"} },
     { "rawtransactions",    "decodepsbt",                   &decodepsbt,                {"psbt"} },
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index dfd98bfc5f..4c08b6881b 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -165,7 +165,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 78d6e78aed..c6e01d9cfc 100755
--- a/test/functional/rpc_help.py
+++ b/test/functional/rpc_help.py
@@ -33,7 +33,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')
@@ -46,6 +46,7 @@ class HelpRpcTest(BitcoinTestFramework):
     def dump_help(self):
         dump_dir = os.path.join(self.options.tmpdir, 'rpc_help_dump')
         os.mkdir(dump_dir)
+        dump_dir = '/tmp/temp_git/' ##HACK
         calls = [line.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:

</details>

Which then gives the resulting diff for me:

diff --git a/createpsbt b/createpsbt
index 4b75e7f..6e2ec5c 100644
--- a/createpsbt
+++ b/createpsbt
@@ -1,4 +1,4 @@
-createpsbt [{"txid":"id","vout":n},...] [{"address":amount},{"data":"hex"},...] ( locktime ) ( replaceable )
+createpsbt [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount},{"data":"hex"},...] ( locktime replacable )
 
 Creates a transaction in the Partially Signed Transaction format.
 Implements the Creator role.
diff --git a/gettxoutproof b/gettxoutproof
index 5ed7748..45ba17a 100644
--- a/gettxoutproof
+++ b/gettxoutproof
@@ -1,4 +1,4 @@
-gettxoutproof ["txid",...] ( blockhash )
+gettxoutproof ["txid",...] ( "blockhash" )
 
 Returns a hex-encoded proof that "txid" was included in a block.
 
diff --git a/listunspent b/listunspent
index 39772c9..bc3baf8 100644
--- a/listunspent
+++ b/listunspent
@@ -1,4 +1,4 @@
-listunspent ( minconf maxconf  ["addresses",...] [include_unsafe] [query_options])
+listunspent ( minconf maxconf ["address",...] include_unsafe {"minimumAmount":amount,"maximumAmount":amount,"maximumCount":n,"minimumSumAmount":amount} )
 
 Returns array of unspent transaction outputs
 with between minconf and maxconf (inclusive) confirmations.
diff --git a/lockunspent b/lockunspent
index e21df7a..9b26c69 100644
--- a/lockunspent
+++ b/lockunspent
@@ -1,4 +1,4 @@
-lockunspent unlock ([{"txid":"txid","vout":n},...])
+lockunspent unlock ( [{"txid":"hex","vout":n},...] )
 
 Updates list of temporarily unspendable outputs.
 Temporarily lock (unlock=false) or unlock (unlock=true) specified transaction outputs.
diff --git a/signrawtransactionwithkey b/signrawtransactionwithkey
index 583b3d9..6113c66 100644
--- a/signrawtransactionwithkey
+++ b/signrawtransactionwithkey
@@ -1,4 +1,4 @@
-signrawtransactionwithkey "hexstring" ["privatekey1",...] ( [{"txid":"id","vout":n,"scriptPubKey":"hex","redeemScript":"hex"},...] sighashtype )
+signrawtransactionwithkey "hexstring" ["privatekey",...] ( [{"txid":"hex","vout":n,"scriptPubKey":"hex","redeemScript":"hex","amount":amount},...] "sighashtype" )
 
 Sign inputs for raw transaction (serialized, hex-encoded).
 The second argument is an array of base58-encoded private
diff --git a/signrawtransactionwithwallet b/signrawtransactionwithwallet
index 5a70fa4..ad9ce09 100644
--- a/signrawtransactionwithwallet
+++ b/signrawtransactionwithwallet
@@ -1,4 +1,4 @@
-signrawtransactionwithwallet "hexstring" ( [{"txid":"id","vout":n,"scriptPubKey":"hex","redeemScript":"hex"},...] sighashtype )
+signrawtransactionwithwallet "hexstring" ( [{"txid":"hex","vout":n,"scriptPubKey":"hex","redeemScript":"hex","amount":amount},...] "sighashtype" )
 
 Sign inputs for raw transaction (serialized, hex-encoded).
 The second optional argument (may be null) is an array of previous transaction outputs that
diff --git a/walletcreatefundedpsbt b/walletcreatefundedpsbt
index 81ba09b..25e77bd 100644
--- a/walletcreatefundedpsbt
+++ b/walletcreatefundedpsbt
@@ -1,4 +1,4 @@
-walletcreatefundedpsbt [{"txid":"id","vout":n},...] [{"address":amount},{"data":"hex"},...] ( locktime ) ( replaceable ) ( options bip32derivs )
+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 )
 
 Creates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough
 Implements the Creator and Updater roles.

</details>

MarcoFalke added this to the "Blockers" column in a project

DrahtBot removed the label Needs rebase on Nov 9, 2018

laanwj commented at 12:54 PM on November 12, 2018: member

Generated help looks fine to me; this also keeps the documentation with the RPC calls they document instead of moving it somewhere else, which is good. utACK

in src/rpc/rawtransaction.cpp:211 in fac524e4fd outdated

 205 | @@ -206,7 +206,16 @@ static UniValue gettxoutproof(const JSONRPCRequest& request)
 206 |  {
 207 |      if (request.fHelp || (request.params.size() != 1 && request.params.size() != 2))
 208 |          throw std::runtime_error(
 209 | -            "gettxoutproof [\"txid\",...] ( blockhash )\n"
 210 | +            RPCHelpMan{"gettxoutproof",
 211 | +                {
 212 | +                    RPCArg{"txids", RPCArg::Type::ARR,

promag commented at 1:45 PM on November 12, 2018:

Can remove RPCArg throughout?

MarcoFalke commented at 3:59 PM on November 12, 2018:

@promag Done and thx! That makes it look less verbose and probably easier to read the documentation straight from the cpp file.

promag commented at 1:49 PM on November 12, 2018: member

Concept ACK.

rpc: Add RPCHelpMan for machine-generated help fa483e13b3

MarcoFalke force-pushed on Nov 12, 2018

ryanofsky approved

ryanofsky commented at 10:21 PM on November 12, 2018: member

utACK fa483e13b387f244c1c72d4dbd709e669335618e. Changes all look good but I'm a surprised to see how incomplete this is. The other PR #14502 which was closed was much more ambitious and completely overhauled everything, while this is only printing the initial lines of a few RPC methods.

I will create pull requests for the next steps after this one is merged.

Curious how many PRs were you thinking of, and how would they be divided up.

MarcoFalke commented at 10:32 PM on November 12, 2018: member

Curious how many PRs were you thinking of, and how would they be divided up.

This is mostly refactoring, so I wouldn't want to create a pull request that takes more than, say, 30 minutes to review.

Logically, I'd like to split them into auto-generating the summary line (1), description (2) and extended documentation about input and output variables (3).

This is the first part of (1). In the next step I will add a linter to make sure all the new documentation is autogenerated and maybe add a scripted diff to convert all remaining ones. Then create one or two pull requests for everything else. So 2-3 in total.

karelbilek commented at 4:36 PM on November 13, 2018: contributor

I implemented (2) in the branch I linked above; but it still needs some work and cleanup

MarcoFalke merged this on Nov 13, 2018

MarcoFalke closed this on Nov 13, 2018

MarcoFalke referenced this in commit c651265c93 on Nov 13, 2018

fanquake removed this from the "Blockers" column in a project

pravblockc referenced this in commit 04b9a0f23e on Aug 10, 2021

pravblockc referenced this in commit ae8fa4c837 on Aug 10, 2021

pravblockc referenced this in commit 01792247fe on Aug 11, 2021

pravblockc referenced this in commit ac9f466e55 on Aug 11, 2021

pravblockc referenced this in commit 976d54f284 on Aug 12, 2021

pravblockc referenced this in commit 1a1378b96f on Aug 12, 2021

DrahtBot locked this on Sep 8, 2021