JSONRPCRequest-aware RPCHelpMan #16240

kallewoof · 2019-06-19T04:29:36Z

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

sipa · 2019-06-19T07:07:31Z

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?

promag · 2019-06-19T07:18:37Z

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

DrahtBot · 2019-06-19T07:19:40Z

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

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.

kallewoof · 2019-06-19T07:43:06Z

@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 kallewoof@b969457 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 kallewoof@b969457?w=1 -- that's almost as good as a scripted diff..

fanquake · 2019-06-19T08:26:25Z

Concept ACK - nice cleanup / deduplication.

kallewoof · 2019-06-19T09:42:55Z

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

promag · 2019-06-19T10:08:04Z

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

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

kallewoof · 2019-06-19T11:47:42Z

@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

maflcko

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

src/rpc/util.h

promag · 2019-06-19T13:29:00Z

Makes sense @MarcoFalke, not worth it.

kallewoof · 2019-06-19T13:32:13Z

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

maflcko · 2019-06-19T13:55:51Z

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.

kallewoof · 2019-06-19T14:03:30Z

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

src/rpc/mining.cpp

kallewoof · 2019-06-19T17:01:02Z

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

src/rpc/server.h

promag

ACK d9c41cc, 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 · 2019-07-06T15:54:49Z

src/rpc/util.h

+     * the user is asking for help information, and throw help when appropriate.
+     */
+    inline void Check(const JSONRPCRequest& request) const {
+        if (request.fHelp || !IsValidNumArgs(request.params.size())) throw std::runtime_error(ToString());


09f0bf5

nit, add { }.

kallewoof · 2019-07-08T00:56:58Z

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

laanwj · 2019-07-08T15:16:45Z

code rview and lightly tested ACK b6fb617

maflcko · 2019-07-08T16:29:50Z

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

maflcko · 2019-07-09T23:31:30Z

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

Show signature and timestamp

Signature:

Timestamp of file with hash 890e34cddd2f763e5555c79809bdd129b142d59574aa8799ab8f1a06a0c076c6 -

b6fb617 rpc: switch to using RPCHelpMan.Check() (Karl-Johan Alm) c7a9fc2 Make the RPCHelpMan aware of JSONRPCRequest and add Check() helper (Karl-Johan Alm) 5c5e32b rpc: migrate JSONRPCRequest functionality into request.cpp (Karl-Johan Alm) 0ab8ba1 rpc: fix RPC help requirements for getblocktemplate (Karl-Johan Alm) Pull request description: 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. ```C++ const RPCHelpMan help{...}; if (request.fHelp || !help.IsValidNumArgs(request.params.size())) { throw std::runtime_error(help.ToString()); } ``` or (older version) ```C++ 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 ```C++ 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`). ACKs for top commit: laanwj: code rview and lightly tested ACK b6fb617 MarcoFalke: ACK b6fb617, looked at the diff, verified move-only where applicable Tree-SHA512: eb73f47f812512905b852e313281d1c8df803db40a6188aa39d5a7586631664db6764491152a8a96769946c796dc56d38c6e3a66ddd06ba3fb9d20050e6274e1

…t action 7d26357 rpc: require second argument only for scantxoutset start action (Andrew Chow) Pull request description: It was reported on [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-11.html#l-377) that `scantxoutset`'s API was broken in 0.19.0: ``` <belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18 <belcher> im on regtest, in case its important <harding> I can confirm `scantxoutset abort` returns the help doc on latest master. Waiting for 0.18.1 to start now to attempt to reproduce there. <harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort"). <jonatack> Same for me as well <harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1. ``` As noted in the conversation, previously, the second argument of `scanobjects` is only required for the `start` action. `Stop` and `abort` actions did not and could work without them. It appears that this was broken by #16240 which enforced the size of the arguments to match the listed required arguments. To fix this issue, this PR makes the `scanobjects` argument an optional argument. Then only in the `start` action do we check whether the `scanobjects` argument is there and throw an informative error about that. Also a test is added for this case. ACKs for top commit: laanwj: ACK 7d26357 promag: ACK 7d26357. Tree-SHA512: 828bdfe47f4fffa5d00a2cf88db6cea4a2714d9c49276841ca5cbdd1603b87bb6862147b86edcf36d7b40314ddb80b1a07fd399faf288572c55cc788c5cf9526

…et start action 7d26357 rpc: require second argument only for scantxoutset start action (Andrew Chow) Pull request description: It was reported on [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-11.html#l-377) that `scantxoutset`'s API was broken in 0.19.0: ``` <belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18 <belcher> im on regtest, in case its important <harding> I can confirm `scantxoutset abort` returns the help doc on latest master. Waiting for 0.18.1 to start now to attempt to reproduce there. <harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort"). <jonatack> Same for me as well <harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1. ``` As noted in the conversation, previously, the second argument of `scanobjects` is only required for the `start` action. `Stop` and `abort` actions did not and could work without them. It appears that this was broken by bitcoin#16240 which enforced the size of the arguments to match the listed required arguments. To fix this issue, this PR makes the `scanobjects` argument an optional argument. Then only in the `start` action do we check whether the `scanobjects` argument is there and throw an informative error about that. Also a test is added for this case. ACKs for top commit: laanwj: ACK 7d26357 promag: ACK 7d26357. Tree-SHA512: 828bdfe47f4fffa5d00a2cf88db6cea4a2714d9c49276841ca5cbdd1603b87bb6862147b86edcf36d7b40314ddb80b1a07fd399faf288572c55cc788c5cf9526

Summary: This ensure names are consistent with [[bitcoin/bitcoin#16240 | PR16240]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6496

Summary: This si a partial backport of Core [[bitcoin/bitcoin#16240 | PR16240]] : bitcoin/bitcoin@5c5e32b Depends on D6496 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6497

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16240 | PR16240]] : bitcoin/bitcoin@c7a9fc2 Depends on D6497 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6498

…et start action 7d26357 rpc: require second argument only for scantxoutset start action (Andrew Chow) Pull request description: It was reported on [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-11.html#l-377) that `scantxoutset`'s API was broken in 0.19.0: ``` <belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18 <belcher> im on regtest, in case its important <harding> I can confirm `scantxoutset abort` returns the help doc on latest master. Waiting for 0.18.1 to start now to attempt to reproduce there. <harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort"). <jonatack> Same for me as well <harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1. ``` As noted in the conversation, previously, the second argument of `scanobjects` is only required for the `start` action. `Stop` and `abort` actions did not and could work without them. It appears that this was broken by bitcoin#16240 which enforced the size of the arguments to match the listed required arguments. To fix this issue, this PR makes the `scanobjects` argument an optional argument. Then only in the `start` action do we check whether the `scanobjects` argument is there and throw an informative error about that. Also a test is added for this case. ACKs for top commit: laanwj: ACK 7d26357 promag: ACK 7d26357. Tree-SHA512: 828bdfe47f4fffa5d00a2cf88db6cea4a2714d9c49276841ca5cbdd1603b87bb6862147b86edcf36d7b40314ddb80b1a07fd399faf288572c55cc788c5cf9526

b6fb617 rpc: switch to using RPCHelpMan.Check() (Karl-Johan Alm) c7a9fc2 Make the RPCHelpMan aware of JSONRPCRequest and add Check() helper (Karl-Johan Alm) 5c5e32b rpc: migrate JSONRPCRequest functionality into request.cpp (Karl-Johan Alm) 0ab8ba1 rpc: fix RPC help requirements for getblocktemplate (Karl-Johan Alm) Pull request description: 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. ```C++ const RPCHelpMan help{...}; if (request.fHelp || !help.IsValidNumArgs(request.params.size())) { throw std::runtime_error(help.ToString()); } ``` or (older version) ```C++ 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 ```C++ 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`). ACKs for top commit: laanwj: code rview and lightly tested ACK b6fb617 MarcoFalke: ACK b6fb617, looked at the diff, verified move-only where applicable Tree-SHA512: eb73f47f812512905b852e313281d1c8df803db40a6188aa39d5a7586631664db6764491152a8a96769946c796dc56d38c6e3a66ddd06ba3fb9d20050e6274e1

fanquake added RPC/REST/ZMQ Refactoring labels Jun 19, 2019

kallewoof force-pushed the 2019-06-json-aware-helper branch from 9096017 to 8f70239 Compare June 19, 2019 05:00

kallewoof changed the title ~~JSONRPCRequest-aware RPCHelpMan~~ RPCHelpMan-aware JSONRPCRequest Jun 19, 2019

kallewoof force-pushed the 2019-06-json-aware-helper branch from 8f70239 to 76b9be3 Compare June 19, 2019 05:37

kallewoof force-pushed the 2019-06-json-aware-helper branch from f2bb974 to 31599d8 Compare June 19, 2019 09:40

kallewoof changed the title ~~RPCHelpMan-aware JSONRPCRequest~~ JSONRPCRequest-aware RPCHelpMan Jun 19, 2019

kallewoof force-pushed the 2019-06-json-aware-helper branch from e7124e3 to a3b264a Compare June 19, 2019 11:47

maflcko reviewed Jun 19, 2019

View reviewed changes

src/rpc/util.h Outdated Show resolved Hide resolved

kallewoof force-pushed the 2019-06-json-aware-helper branch from a3b264a to 6da27e8 Compare June 19, 2019 13:31

kallewoof force-pushed the 2019-06-json-aware-helper branch from 6da27e8 to 5a3f382 Compare June 19, 2019 13:34

kallewoof force-pushed the 2019-06-json-aware-helper branch from 5a3f382 to 2b4e540 Compare June 19, 2019 14:02

maflcko reviewed Jun 19, 2019

View reviewed changes

src/rpc/mining.cpp Outdated Show resolved Hide resolved

kallewoof force-pushed the 2019-06-json-aware-helper branch from 2b4e540 to a388c8f Compare June 19, 2019 17:00

maflcko reviewed Jun 19, 2019

View reviewed changes

src/rpc/server.h Outdated Show resolved Hide resolved

kallewoof force-pushed the 2019-06-json-aware-helper branch 2 times, most recently from 79cb80a to b93d7ab Compare June 19, 2019 17:40

promag reviewed Jul 6, 2019

View reviewed changes

kallewoof added 2 commits July 8, 2019 09:53

Make the RPCHelpMan aware of JSONRPCRequest and add Check() helper

c7a9fc2

rpc: switch to using RPCHelpMan.Check()

b6fb617

kallewoof force-pushed the 2019-06-json-aware-helper branch from d9c41cc to b6fb617 Compare July 8, 2019 00:54

maflcko merged commit b6fb617 into bitcoin:master Jul 9, 2019

kallewoof deleted the 2019-06-json-aware-helper branch October 17, 2019 08:32

achow101 mentioned this pull request Dec 11, 2019

rpc: require second argument only for scantxoutset start action #17728

Merged

laanwj added the Needs backport (0.19) label Dec 12, 2019

fanquake removed the Needs backport (0.19) label Jan 6, 2020

CathCathub approved these changes Oct 6, 2020

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSONRPCRequest-aware RPCHelpMan #16240

JSONRPCRequest-aware RPCHelpMan #16240

kallewoof commented Jun 19, 2019 •

edited

Loading

sipa commented Jun 19, 2019 •

edited

Loading

promag commented Jun 19, 2019

DrahtBot commented Jun 19, 2019 •

edited

Loading

kallewoof commented Jun 19, 2019 •

edited

Loading

fanquake commented Jun 19, 2019

kallewoof commented Jun 19, 2019 •

edited

Loading

promag commented Jun 19, 2019

kallewoof commented Jun 19, 2019 •

edited

Loading

maflcko left a comment

promag commented Jun 19, 2019

kallewoof commented Jun 19, 2019

maflcko commented Jun 19, 2019

kallewoof commented Jun 19, 2019

kallewoof commented Jun 19, 2019

promag left a comment

promag Jul 6, 2019

kallewoof Jul 8, 2019

kallewoof commented Jul 8, 2019

laanwj commented Jul 8, 2019

maflcko commented Jul 8, 2019

maflcko commented Jul 9, 2019

JSONRPCRequest-aware RPCHelpMan #16240

JSONRPCRequest-aware RPCHelpMan #16240

Conversation

kallewoof commented Jun 19, 2019 • edited Loading

sipa commented Jun 19, 2019 • edited Loading

promag commented Jun 19, 2019

DrahtBot commented Jun 19, 2019 • edited Loading

Conflicts

kallewoof commented Jun 19, 2019 • edited Loading

fanquake commented Jun 19, 2019

kallewoof commented Jun 19, 2019 • edited Loading

promag commented Jun 19, 2019

kallewoof commented Jun 19, 2019 • edited Loading

maflcko left a comment

Choose a reason for hiding this comment

promag commented Jun 19, 2019

kallewoof commented Jun 19, 2019

maflcko commented Jun 19, 2019

kallewoof commented Jun 19, 2019

kallewoof commented Jun 19, 2019

promag left a comment

Choose a reason for hiding this comment

promag Jul 6, 2019

Choose a reason for hiding this comment

kallewoof Jul 8, 2019

Choose a reason for hiding this comment

kallewoof commented Jul 8, 2019

laanwj commented Jul 8, 2019

maflcko commented Jul 8, 2019

maflcko commented Jul 9, 2019

kallewoof commented Jun 19, 2019 •

edited

Loading

sipa commented Jun 19, 2019 •

edited

Loading

DrahtBot commented Jun 19, 2019 •

edited

Loading

kallewoof commented Jun 19, 2019 •

edited

Loading

kallewoof commented Jun 19, 2019 •

edited

Loading

kallewoof commented Jun 19, 2019 •

edited

Loading