Skip to content
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

Use RPCHelpMan to generate RPC doc strings #14530

Merged
merged 2 commits into from Nov 13, 2018

Conversation

Projects
None yet
7 participants
@MarcoFalke
Copy link
Member

commented Oct 20, 2018

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.

@fanquake

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

How does this compare/clash with #14502?

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch Oct 20, 2018

@MarcoFalke MarcoFalke referenced this pull request Oct 20, 2018

Closed

Rpc help helper class #14502

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch Oct 20, 2018

@MarcoFalke MarcoFalke changed the title scripted-diff: Use RPCHelpMan on RPCs with no args scripted-diff: Use RPCHelpMan to generate RPC doc strings Oct 20, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

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 MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch 6 times, most recently Oct 21, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch Oct 21, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch 2 times, most recently Oct 21, 2018

@MarcoFalke MarcoFalke changed the title scripted-diff: Use RPCHelpMan to generate RPC doc strings Use RPCHelpMan to generate RPC doc strings Oct 22, 2018

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

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.)

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

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.)

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

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 MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch 3 times, most recently to e8573f6 Oct 23, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

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.

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

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 MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch Oct 26, 2018

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch Nov 5, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 5, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch 2 times, most recently Nov 6, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

Rebased

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.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch Nov 9, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Nov 9, 2018

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

Patch
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:

Which then gives the resulting diff for me:

Doc diff
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.

@MarcoFalke MarcoFalke added this to Blockers in High-priority for review Nov 9, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 9, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

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

@promag
Copy link
Member

left a comment

Concept ACK.

src/rpc/rawtransaction.cpp Outdated
"gettxoutproof [\"txid\",...] ( blockhash )\n"
RPCHelpMan{"gettxoutproof",
{
RPCArg{"txids", RPCArg::Type::ARR,

This comment has been minimized.

Copy link
@promag

promag Nov 12, 2018

Member

Can remove RPCArg throughout?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 12, 2018

Author Member

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch to fa483e1 Nov 12, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK fa483e1. 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

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2018

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.

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

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

@MarcoFalke MarcoFalke merged commit fa483e1 into bitcoin:master Nov 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Nov 13, 2018

Merge #14530: Use RPCHelpMan to generate RPC doc strings
fa483e1 rpc: Add RPCHelpMan for machine-generated help (MarcoFalke)
fa0d36f rpc: Include rpc/util.h where needed for RPCHelpMan (MarcoFalke)

Pull request description:

  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.

Tree-SHA512: 86f68322443ff01cd964aaf0ebe186be63fbebe4c47676cf7a622cc2b5305fd176bd57badfd1bbf788a036812253eb0dead74ecc3b30664c3e0d9392b2248054

@fanquake fanquake removed this from Blockers in High-priority for review Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.