Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko 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
Copy link
Member

How does this compare/clash with #14502?

@maflcko
Copy link
Member Author

maflcko commented Oct 20, 2018

@fanquake See #14502 (comment)

@maflcko maflcko 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
Copy link
Contributor

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

@maflcko maflcko force-pushed the Mf1810-rpcHelpMan branch 6 times, most recently from 7017d03 to 034921c Compare October 21, 2018 05:32
@maflcko maflcko force-pushed the Mf1810-rpcHelpMan branch 2 times, most recently from e00e2b7 to 4116397 Compare October 22, 2018 14:57
@maflcko maflcko changed the title scripted-diff: Use RPCHelpMan to generate RPC doc strings Use RPCHelpMan to generate RPC doc strings Oct 22, 2018
@karelbilek
Copy link
Contributor

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

@karelbilek
Copy link
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
Copy link
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

@maflcko maflcko force-pushed the Mf1810-rpcHelpMan branch 3 times, most recently from d0e3b06 to e8573f6 Compare October 23, 2018 19:15
@maflcko maflcko force-pushed the Mf1810-rpcHelpMan branch 2 times, most recently from fa73e0f to fa75f1f Compare November 6, 2018 15:34
@maflcko
Copy link
Member Author

maflcko commented Nov 6, 2018

Rebased

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.
@maflcko
Copy link
Member Author

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

@laanwj
Copy link
Member

laanwj 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

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove RPCArg throughout?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maflcko
Copy link
Member Author

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

@karelbilek
Copy link
Contributor

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

@maflcko maflcko merged commit fa483e1 into bitcoin:master Nov 13, 2018
maflcko pushed a commit that referenced this pull request Nov 13, 2018
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
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 11, 2021
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 bitcoin#14378 and I will create pull requests for the next steps after this one is merged.

Tree-SHA512: 86f68322443ff01cd964aaf0ebe186be63fbebe4c47676cf7a622cc2b5305fd176bd57badfd1bbf788a036812253eb0dead74ecc3b30664c3e0d9392b2248054
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 12, 2021
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 bitcoin#14378 and I will create pull requests for the next steps after this one is merged.

Tree-SHA512: 86f68322443ff01cd964aaf0ebe186be63fbebe4c47676cf7a622cc2b5305fd176bd57badfd1bbf788a036812253eb0dead74ecc3b30664c3e0d9392b2248054
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants