JSONRPCRequest-aware RPCHelpMan #16240

Every single RPC call has a helper-section at the start, which throws a help string if the user asks for help or if the user provided too few/many arguments.

const RPCHelpMan help{...};
if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
    throw std::runtime_error(help.ToString());
}

or (older version)

if (request.fHelp || request.params.size() < min || request.params.size() > max)
    throw std::runtime_error(
        RPCHelpMan{...}.ToString()
    );

It seems like an obvious improvement, and less copy-pasting, to make RPCHelpMan aware of JSONRPCRequest, and to let it handle the checks instead. Both of the above become

RPCHelpMan{...}.Check(request);

which means we save roughly 3 lines per RPC command, and the RPCHelpMan instance is never referenced afterwards, so the approach is a tiny fraction cleaner.

This is a complete update, sans a few special case locations that had special rules. 623 lines turn into 284 (which includes the addition to RPCHelpMan).

Concept ACK, though it seems more appropriate that the RPC help object would gain a method the check RPC arguments, than the RPC handling code to be aware of RPC help. I'm sure the circular dependency is resolvable; don't let it stop you from cleaning things up.

Also, if these lines are all touched anyway, could you add a scripted diff to fix their indentation?

Agree with @sipa, although the request already knows about fHelp.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
• #16362 (gui: Bilingual translation by hebasto)
• #16244 (Move wallet creation out of the createwallet rpc into its own function by achow101)
• #16224 (gui: Bilingual GUI error messages by hebasto)
• #16185 (gettransaction: add an argument to decode the transaction by darosior)
• #15974 (refactor: Avoid UniValue copy constructor by promag)
• #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
• #15836 (Add feerate histogram to getmempoolinfo by jonasschnelli)
• #15729 (rpc: Raise error in getbalance if minconf is not zero by promag)
• #15450 ([GUI] Create wallet menu option by achow101)
• #14898 (rpc listtransactions new argument options (paginatebypointer impl) by hosseinamin)
• #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

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.

@sipa I initially added one, but it needs to know about JSONRPCRequest to fetch the fHelp and params.size(), but if I #include <rpc/server.h>, the linters detect a circular dependency util -> server -> util. I definitely think this would be better in RPCHelpMan, but alas..

I would love to fix the indentation, but not sure how a scripted-diff would be used in this case. (I started working on that, with a sample commit at https://github.com/kallewoof/bitcoin/commit/b9694578fa6401508e18761954ded33ed053df94 in https://github.com/kallewoof/bitcoin/commits/json-aware-helper-whitespaced-sample but got stuck on how to do scripted-diff part).

Edit: it may be acceptable to link to commit with ?w=1 which should show it empty, like https://github.com/kallewoof/bitcoin/commit/b9694578fa6401508e18761954ded33ed053df94?w=1 -- that's almost as good as a scripted diff..

Concept ACK - nice cleanup / deduplication.

@sipa @promag I realized I could inline the helper method and implement it after JSONRPCRequest to get around linker issues, so this is now an RPCHelpMan method, as you recommended.

There is also a whitespace only commit (e7124e3) which blows the diff up a bit, but if reviewers check per-commit and use -w/?w=1 properly, it should be manageable. I can drop the whitespace commit too.

@kallewoof see dfdf7f213cd84756f8f17bb3c7cb25a1ee7989e2 for a scripted diff, produced by running on your 31599d8:

git diff -U0 HEAD~3.. | ./contrib/devtools/clang-format-diff.py -p0 -i -v

@promag I had no idea you could do that. Thanks, very cool!

Edit: not sure what is going wrong. Running this from cli gives me

Formatting b/src/rpc/blockchain.cpp
No such file or directory

Tend to NACK on fixing the code style in lines that are not touched otherwise. This breaks git blame and all cherry-pick/rebase/merge operations by default (see the list of conflicts).

Makes sense @MarcoFalke, not worth it.

@MarcoFalke

Tend to NACK on fixing the code style in lines that are not touched otherwise. This breaks git blame and all cherry-pick/rebase/merge operations by default (see the list of conflicts).

I would normally agree, but the RPC code is out-of-this-world awful with indentation. I think it may be worth to run through it once, especially since the review effort required is near-nil.

especially since the review effort required is near-nil.

Just because something is easy to review doesn't mean it should be done. Beside the things it breaks that I mentioned previously, it will also conflict with a bunch of my local branches, where I restructure the help to make it more consistent and compile-time-safe (use compile time checks and compile-time generated help). Those changes are going to actually touch the lines, so a reformat could make sense at that point in time (maybe).

Also, I'd say it is not clear if the rpc help must live in the scope of the rpc method. Maybe the help should live at the same scope as the rpc method? I'd say unclear right now, but if it is ever going to change, it would need another reformat. Doing another project-wide break of git blame/cherry-pick/merge/rebase

I think you underestimate the cost of merge conflicts. If you feel strong about the reformat, it should probably be split up into a separate pull request to be evaluated on its own. Otherwise, my NACK will still hold on this pull.

Understood. Would have been a nice clean up but I'm not gonna lose sleep over it. :) Dropped the whitespace commit.

@MarcoFalke: Updated getblocktemplate to display default as {} rather than {"mode":"template"}.

ACK 36ad069e8a2641fc7a04af3b924ca05b8b91b6f0.

Concept ACK on the approach, I think making this checking data-driven is, overall, a good idea (it enforces consistency), and this seems to be a good way to go about it. I do think this is slightly risky, as it gets rid of a lot of special-case code, is our test coverage of RPC good enough to be confident about changing this for all methods at once?

@laanwj

I do think this is slightly risky, as it gets rid of a lot of special-case code, is our test coverage of RPC good enough to be confident about changing this for all methods at once?

I'm not sure about "a lot" of special-case code, to be honest. There were a few exceptions but I mostly left special case stuff alone.

I'm not sure about "a lot" of special-case code, to be honest. There were a few exceptions but I mostly left special case stuff alone.

OK, I just wanted to bring it up.

ACK d9c41cc4cb, github commit sort ftw.. Yet another simple tidy up to RPC documentation. Renaming rpc/protocol.{h.cpp} LGTM - thanks to @laanwj. Also confirmed moved code.

@promag Thanks for the review! Note that I only renamed protocol.cpp, and moved some parts out of protocol.h into request.h -- protocol.h is still there, just smaller.

code rview and lightly tested ACK b6fb617aaaad5f9cdd7f2ad2825b253ca792055d

I do think this is slightly risky, as it gets rid of a lot of special-case code, is our test coverage of RPC good enough to be confident about changing this for all methods at once?

I checked all call-sites as part of

• rpc: Actually throw help when passed invalid number of params #15401

so this should only be refactoring. Though, you are right, most of it is not covered by tests.

ACK b6fb617aaa, looked at the diff, verified move-only where applicable

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK b6fb617aaa, looked at the diff, verified move-only where applicable
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhX0AwAtwImLc9meZaRYasAtXwhpkTQpZ9bC0l6hK6uvUGSr4N8n3v63bezavGZ
pbddzccHclbs5gAtbJMsWkFmR7vKbfDgZi02MZLQqLwc5r+X8/rJdgscfFhMeHNy
WCQ8ZCJy5VAC5GA6SfshB8VWK+oQpx9GHCKc5OEMFRr7Sc8go0osRxWbMaNuJQdg
RGgzZ0/e2FS17czydBb5O3k2XTawUUfIa49zFq69kxSntpJdnmMJrqbAg8l0cKDK
OYd/HkWIVgx1LBtTBmLEF0sKuymhmqMfPbEoFOUXYqiGSTaQsZeS9cJDpc+Cy8Qa
+icW5+C23plWHC/+N89AeE6g3zcdxG1P1pIRP0YfRjQMx1lKxCuYeCep8vr2pNw7
X+SpZaoyRJWaoQnL4mPjtGP/7n+QXXjYj2MCq8yNz2uPzyqrvcfq7ZaP+IW5SBmY
Yl+J9K6FwTkneVkhDMjYMzoJLDwDH4yVrgChwMHYN7HEVY2M0WZhsSE9/HDgscqg
jEii4j6x
=rpAy
-----END PGP SIGNATURE-----

Timestamp of file with hash 890e34cddd2f763e5555c79809bdd129b142d59574aa8799ab8f1a06a0c076c6 -

</details>