Rpc help helper class

karelbilek commented at 11:44 am on October 17, 2018: contributor

Inspired by some discussions about RPC documentations ( #14399, #14378 ) I have decided to create a RPC help helper class, that automatically formats the RPC help.

This should be helpful in few respects:

it makes the code cleaner
it formats the help automatically
in the future (I am leaving this for a follow-up PR), it can allow to export JSON, which will allow to make nicer HTML documentation :)

The new classes are in rpc/doc.cpp and they are added in the first commit

The second commit is the actual doc rewrite. The process of rewriting current docs to what is in the PR was semi-automatic - unfortunately, not automatic enough to make it a scripted diff. What I did:

wrote a parser of the current RPC documentation
made some changes to the RPC help to be parseable
generated the code in the PR from that
manually cleaned up the code on a lot of places

It’s still a huge chunk of code, even when it is just RPC help.

(Third commit is then removing the unused helper function.)

I think it does the same what @achow101 has been trying (by adding what I have in RPCDoc to UniValue instead) here https://github.com/achow101/bitcoin/tree/rpc-help-univ ; I did not use that because I did not fully understand how it was supposed to be working

fanquake added the label RPC/REST/ZMQ on Oct 17, 2018

karelbilek renamed this:
~~Rpc helper class~~
Rpc help helper class
on Oct 17, 2018

ryanofsky commented at 3:04 pm on October 17, 2018: member

This is a nice change, and I think making documentation more structured will make it easier to maintain.

I do think it’d be good to replace Table/Row methods with explicit Params / Result methods that make it possible to extract names, types, and structure of parameters. This could be used to generate richer documentation, and support other applications like autocomplete.

karelbilek commented at 3:56 pm on October 17, 2018: contributor

That’s a good point.

I used “Table” and “Row”, since there are few more different table types, and also “Arguments” and “Results” tables are basically the same thing (Arguments have the numbers next to some rows, but only some of them)

The possible table types, by just grepping the source code:

Arguments
Examples of output descriptors
Modes
Result
Result with verbosity classifier
- Result (for verbose = false)
- Result (for verbose = true)
- Result (for verbose=false)
- Result (for verbosity = 0)
- Result (for verbosity = 1)
- Result (for verbosity = 2)
- Result (if verbose is not set or set to false)
- Result (if verbose is set to true)
- Result: (for verbose = false)
- Result: (for verbose = true)
Result with mode clasifier
- Result (mode "mallocinfo")
- Result (mode "stats")

All the Result subtypes are…. yeah basically the same thing, could be treated in some way.

karelbilek commented at 4:04 pm on October 17, 2018: contributor

Of course another level would be to somehow match the actual RPC arguments to the arguments in the table, and to the arguments in the short example in the first line, but that is … just too complex. :)

Good first step would be to replace Table("Arguments") with just Arguments(), the rows with numbered arguments replace with Argument(), add Result("classifier") as a thing, and … yeah the rest of the tables could stay as Table

karelbilek commented at 4:11 pm on October 17, 2018: contributor

~~The fuctional tests seem to be failing for some reason. I will investigate if I get more general concept ACKs :)~~

Finally fixed

ryanofsky commented at 5:07 pm on October 17, 2018: member

Concept ACK. The functional tests do seem to be passing for me when I try them locally.

meshcollider commented at 5:11 am on October 18, 2018: contributor

Concept ACK

in src/rpc/mining.cpp:189 in e60061c524 outdated

203-            "\nExamples:\n"
204-            + HelpExampleCli("getmininginfo", "")
205-            + HelpExampleRpc("getmininginfo", "")
206-        );
207-
208+        throw RPCDoc("getmininginfo", "")

kallewoof commented at 7:44 am on October 18, 2018:

Why do you need the , "" part? It looks like it does not require it:

https://github.com/bitcoin/bitcoin/blob/e60061c524f14df8b9e856ec0b89eb6805baf824/src/rpc/doc.h#L70-L71

(this applies to multiple files/places)

karelbilek commented at 8:15 am on October 18, 2018:

I just forgot I made the constructor with just one argument. :)

kallewoof commented at 7:46 am on October 18, 2018: member

Concept ACK

in src/rpc/doc.h:80 in e60061c524 outdated

76+    RPCDoc& Row(const std::string& code, const std::initializer_list<std::string>& types);
77+    RPCDoc& Row(const std::string& code, const std::string& description);
78+    RPCDoc& Row(const std::string& code, const std::initializer_list<std::string>& types, const std::string& description);
79+    RPCDoc& Rows(const std::vector<RPCDocTableRow>& rows);
80+    RPCDoc& Example(const std::string& code);
81+    RPCDoc& Example(const std::string& description, const std::string& code);

practicalswift commented at 8:30 pm on October 18, 2018:

Make sure this matches the parameter naming in:

0RPCDoc& RPCDoc::Example(const std::string& code, const std::string& example)

Now it is (description, code) vs (code, example) :-)

karelbilek commented at 8:33 am on October 19, 2018:

fixed

in src/rpc/doc.cpp:90 in e60061c524 outdated

85+    m_rows.push_back(row);
86+}
87+
88+std::string RPCDocTable::AsText() const
89+{
90+    std::string res = "";

practicalswift commented at 8:31 pm on October 18, 2018:

The initialisation here is redundant.

in src/rpc/doc.cpp:96 in e60061c524 outdated

91+    res += m_name;
92+    res += ":\n";
93+
94+    size_t prefixLen = PrefixLength();
95+    for (auto const& row : m_rows) {
96+        std::string code = row.Code();

practicalswift commented at 8:31 pm on October 18, 2018:

Could be const reference?

in src/rpc/doc.cpp:150 in e60061c524 outdated

145+    return *this;
146+}
147+
148+RPCDoc& RPCDoc::Table(const std::string& name)
149+{
150+    m_tables.push_back(RPCDocTable(name));

practicalswift commented at 8:32 pm on October 18, 2018:

emplace_back instead?

karelbilek commented at 7:48 am on October 19, 2018:

I have to admit, I don’t really understand the difference between emplace_back and push_back

What good does it bring here?

karelbilek commented at 7:51 am on October 19, 2018:

…but online wisdom seems to be that emplace_back is faster, because it doesn’t create temporary objects, so… ok :)

in src/rpc/doc.cpp:200 in e60061c524 outdated

195+    return *this;
196+}
197+
198+RPCDoc& RPCDoc::Example(const std::string& code)
199+{
200+    m_examples.push_back(RPCDocExample(code));

practicalswift commented at 8:32 pm on October 18, 2018:

emplace_back instead?

in src/rpc/doc.cpp:206 in e60061c524 outdated

201+    return *this;
202+}
203+
204+RPCDoc& RPCDoc::Example(const std::string& code, const std::string& example)
205+{
206+    m_examples.push_back(RPCDocExample(code, example));

practicalswift commented at 8:32 pm on October 18, 2018:

emplace_back instead?

in src/rpc/doc.cpp:212 in e60061c524 outdated

207+    return *this;
208+}
209+
210+RPCDoc& RPCDoc::ExampleCli(const std::string& description, const std::string& methodName, const std::string& args)
211+{
212+    m_examples.push_back(

practicalswift commented at 8:32 pm on October 18, 2018:

emplace_back instead?

in src/rpc/doc.cpp:219 in e60061c524 outdated

214+    return *this;
215+}
216+
217+RPCDoc& RPCDoc::ExampleRpc(const std::string& description, const std::string& methodName, const std::string& args)
218+{
219+    m_examples.push_back(RPCDocExample(

practicalswift commented at 8:32 pm on October 18, 2018:

emplace_back instead?

in src/rpc/mining.cpp:753 in e60061c524 outdated

759-            "\nResult:\n"
760-            "None"
761-            "\nExamples:\n" +
762-            HelpExampleCli("submitheader", "\"aabbcc\"") +
763-            HelpExampleRpc("submitheader", "\"aabbcc\""));
764+        throw RPCDoc("submitheader", "\"hexdata\"")

practicalswift commented at 8:35 pm on October 18, 2018:

RPCDoc should be derived from std::exception?

karelbilek commented at 7:35 am on October 19, 2018:

Hm, not sure if I like it… I like how explicit it is now, so you can either have a RPCDoc object (which could later be used somewhere else too) and then explicitly make exception out of it

Also I think that right now when you throw different exception than runtime_error, bitcoind crashes instead of bitcoin-cli writing out the help. I will look once more to be sure

in src/rpc/doc.h:25 in e60061c524 outdated

20+    const std::string m_description;
21+    const std::string m_code;
22+
23+public:
24+    RPCDocExample(const std::string& description, const std::string& code);
25+    RPCDocExample(const std::string& code);

practicalswift commented at 8:42 pm on October 18, 2018:

Should be explicit?

in src/rpc/doc.h:37 in e60061c524 outdated

32+    const std::string m_code;
33+    std::vector<std::string> m_types;
34+    const std::string m_description;
35+
36+public:
37+    RPCDocTableRow(const std::string& code);

practicalswift commented at 8:42 pm on October 18, 2018:

Should be explicit?

in src/rpc/doc.h:54 in e60061c524 outdated

49+    std::vector<RPCDocTableRow> m_rows;
50+
51+    size_t PrefixLength() const;
52+
53+public:
54+    RPCDocTable(const std::string& name);

practicalswift commented at 8:42 pm on October 18, 2018:

Should be explicit?

in src/rpc/doc.h:70 in e60061c524 outdated

65+    std::string m_description;
66+    std::vector<RPCDocTable> m_tables;
67+    std::vector<RPCDocExample> m_examples;
68+
69+public:
70+    RPCDoc(std::string methodName);

practicalswift commented at 8:42 pm on October 18, 2018:

Should be explicit?

in src/rpc/doc.cpp:132 in e60061c524 outdated

127+        }
128+    }
129+    return res;
130+}
131+
132+RPCDoc::RPCDoc(std::string methodName, std::string firstArguments)

practicalswift commented at 8:45 pm on October 18, 2018:

Should be const references?

in src/rpc/doc.cpp:137 in e60061c524 outdated

132+RPCDoc::RPCDoc(std::string methodName, std::string firstArguments)
133+    : m_methodName(methodName), m_firstArguments(firstArguments)
134+{
135+}
136+
137+RPCDoc::RPCDoc(std::string methodName)

practicalswift commented at 8:45 pm on October 18, 2018:

Should be const reference?

[rpc] Helper class for RPC help format 635fa2f266

[rpc] Rewrite all RPC commands to use new helper a9b9eedc20

[rpc] Removing unused helper functions c78baa144a

in src/rpc/doc.cpp:198 in e60061c524 outdated

193+        m_tables.back().AddRow(row);
194+    }
195+    return *this;
196+}
197+
198+RPCDoc& RPCDoc::Example(const std::string& code)

practicalswift commented at 8:46 pm on October 18, 2018:

This function is never used?

DrahtBot commented at 9:53 am on October 20, 2018: member

#14555 (Move util files to directory by jimpo)
#14530 (Use RPCHelpMan to generate RPC doc strings by MarcoFalke)
#14481 (Add P2SH-P2WSH support to listunspent RPC by MeshCollider)
#14477 (Add ability to convert solvability info to descriptor by sipa)
#14468 ([wallet] Deprecate generate RPC method by jnewbery)
#14459 (More RPC help description fixes by ch4ot1c)
#14454 (Add SegWit support to importmulti by MeshCollider)
#14437 (Refactor: Start to separate wallet from node by ryanofsky)
#14411 ([wallet] Restore ability to list incoming transactions by label by ryanofsky)
#14410 (rpcwallet: ‘ischange’ field for ‘getaddressinfo’ RPC by mrwhythat)
#14350 (Add WalletLocation class by promag)
#14319 (doc: Fix PSBT howto and example parameters by priscoan)
#14296 ([wallet] Remove addwitnessaddress by jnewbery)
#14121 (Index for BIP 157 block filters by jimpo)
#14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
#14060 (ZMQ: add options to configure outbound message high water mark, aka SNDHWM by mruddy)
#14021 (Import key origin data through importmulti by achow101)
#13756 (wallet: -avoidreuse feature for improved privacy by kallewoof)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
#13743 (refactor: Replace boost::bind with std::bind by ken2812221)
#13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
#13381 (RPC: creates possibility to preserve labels on importprivkey by marcoagner)
#12911 (wallet: Show fee in results for signrawtransaction* when known by kallewoof)
#12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)
#12674 (RPC: Support addnode onetry without making the connection priviliged by luke-jr)
#12096 ([rpc] [wallet] Allow specifying the output index when using bumpfee by kallewoof)
#11484 (Optional update rescan option in importmulti RPC by pedrobranco)
#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.

MarcoFalke commented at 1:32 pm on October 20, 2018: member

Concept ACK. However, this seems to be a large change that is probably impossible to review without splitting it up in smaller chunks and/or making it a scripted diff.

While the change seems to touch every help text, it doesn’t yet make it meaningfully easier to actually produce the help text machine-generated. You split out the rpc method name, but then pass all arguments as a single string and then each of them again as a “Row”. This doens’t really solve the issue that the documentation is inconsistent in itself (Starting with the white space and formatting of the single line arg string [cf. #14459] and going on to just forgetting the single line arg string [as in importprunedfunds], …)

Ideally we’d either merge some exhaustive fix similar to #14459 and then a pure scripted-diff to make it auto-generated (and check that the resulting documentation does not differ after the scripted-diff) or we do the auto-generation without the scripted diff and check that the resulting diff in the documentation looks similar to #14459.

I have quickly hacked up a first step of how I think here: #14530

karelbilek commented at 8:38 am on October 23, 2018: contributor

Thanks for feedback.

ad script-diff: I can probably make it script-diffed, it will need some constistency fixed first so the text is parseable, but that can be done as a separate commit. (It is mostly whitespace, missing : after “Arguments” etc)

ad other notes:

Yes, this PR lets most of the arguments/“rows” be as-is, as the various styles seemed too different to me to make consistent, so I gave up and just let it be; I focused mainly on getting rid of the “space-formatting” and making it automatically consistent.

If the goal is indeed to make generation of, for example, the first-line argument etc more automatic, that can be done, but will take more time :) but sure, it will be a nicer code

If I got it correctly. Let’s say, walletfundedpsbt

https://bitcoincore.org/en/doc/0.17.0/rpc/wallet/walletcreatefundedpsbt/

The current RPC, at least start, is

 0walletcreatefundedpsbt [{"txid":"id","vout":n},...] [{"address":amount},{"data":"hex"},...] ( locktime ) ( replaceable ) ( options bip32derivs )
 1
 2Creates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough
 3Implements the Creator and Updater roles.
 4
 5Arguments:
 61. "inputs"                (array, required) A json array of json objects
 7     [
 8       {
 9         "txid":"id",      (string, required) The transaction id
10         "vout":n,         (numeric, required) The output number
11         "sequence":n      (numeric, optional) The sequence number
12       } 
13       ,...
14     ]
152. "outputs"               (array, required) a json array with outputs (key-value pairs)
16   [
17    {
18      "address": x.xxx,    (obj, optional) A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in BTC
19    },
20    {
21      "data": "hex"        (obj, optional) A key-value pair. The key must be "data", the value is hex encoded data
22    }
23    ,...                     More key-value pairs of the above form. For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also
24                             accepted as second parameter.
25   ]

Should the “subcodes” in inputs/outputs arguments, and their help, be also generated from some object (that seems to be m_inner in your example)? Including all the “,….” ? Should the first line stay like it is, or should it instead be walletcreatefundedpsbt inputs outputs ( locktime ) ( replaceable ) ( options bip32derivs )?

I am just saying it will be pretty hard to do this, that’s why I opted for “just” adding Row to each table (which also took care of all the Results, that have the same formatting logic). :)

but if the goal is indeed to make all the doc even more uniform, it’s probably better than this “naive” approach that just formats the spaces

edit: I look more closely at your #14530 and it indeed produces all the “pseudo-objects” on the left. OK

karelbilek commented at 12:58 pm on October 23, 2018: contributor

#14530 looks great, I will try to work on top of that

DrahtBot commented at 10:50 pm on October 23, 2018: member

DrahtBot added the label Needs rebase on Oct 23, 2018

karelbilek commented at 12:32 pm on October 24, 2018: contributor

Closing this, in support of approach similar to #14530

It will be harder to rewrite current source code to that, but ultimately will be more helpful.

karelbilek closed this on Oct 24, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

Rpc help helper class #14502