rpc: Assert that RPCArg names are equal to CRPCCommand ones (server) #19386

maflcko · 2020-06-26T17:50:56Z

This is split out from #18531 to just touch the RPC methods in server. Description from the main pr:

Motivation

RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the CRPCCommands and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.

Changes

The changes here add an assert in the CRPCCommand constructor that the RPCArg names are identical to the ones in the CRPCCommand.

Future work

Here or follow up, makes sense to also assert type of returned UniValue?

Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:

Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
Auto-formatting and sanity checking the RPCExamples with RPCMan
Checking passed-in json in self-check. Removing redundant checks
Checking returned json against documentation to avoid regressions or false documentation
Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static

Bugs found

The assert identified issue rpc: Fix named arguments in documentation #18607
The changes itself fixed bug wallet: Make RPC help compile-time static #19250

DrahtBot · 2020-06-26T20:26:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: checks that bitcoin-cli autocomplete is in sync #18606 (test: checks that bitcoin-cli autocomplete is in sync by pierreN)

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.

laanwj · 2020-06-29T14:27:33Z

Concept ACK.

Here or follow up, makes sense to also assert type of returned UniValue?

FWIW I dislike the idea of doing this at run-time. We don't want RPC to suddenly crash because an argument doesn't match its specified type in the help.
The extra checking also adds overhead on every RPC call.
It might make sense for something that is enabled with other heavier debug checks, enabled during the tests.

Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table

This is somewhat difficult. It was considered in the past but

It would be undesirable to link the server-side tables into the CLI, as this either pulls in the associated implementations or splits the table in yet another way.
The CLI table could be spit out by some command line flag to bitcoin, however because of cross-compilation we don't want the build process to depend on being able to run the generated executable (and it needs to be possible to build -cli without -d).
bitcoin-cli could stop hardcoding the table completely but request it, over the network, from bitcoind. This is the most elegant option in a certain regard, but, one would want to cache this table (and not load it on every request). Getting the cache invalidation strategy (when changes) right is non-trivia,l though.

maflcko · 2020-06-29T14:38:26Z

It might make sense for something that is enabled with other heavier debug checks, enabled during the tests.

Good idea. This could be hidden behind --enable-debug or some other option

The CLI table could be spit out by some command line flag to bitcoin, however because of cross-compilation we don't want the build process to depend on being able to run the generated executable (and it needs to be possible to build -cli without -d).

Makes sense. I think I will keep the linter/test around, but make it less fragile by removing the regex parsing, which has been broken in the past at least once.

ryanofsky

Code review ACK fac2edb. This is a great way of getting rid of redundant RPC metadata without requiring invasive changes. Also thanks from splitting this out from the more overwhelmingly sized #18531.

re: #19386 (comment)

FWIW I dislike the idea of doing this at run-time

I think it's important to distinguish three types of checking

Build time checking
Run time checking in unit tests or at startup
Run time checking at every method call

I think 1 & 2 are both perfectly good places to catch wrongly declared RPC methods. Only 3 is a bad place.

src/rpc/util.h

src/rpc/server.cpp

ryanofsky · 2020-06-30T15:05:02Z

src/rpc/util.h

@@ -329,6 +329,11 @@ class RPCHelpMan
 public:
    RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples);
    using RPCMethod = std::function<UniValue(const RPCHelpMan&, const JSONRPCRequest&)>;
+    struct HiddenArg {


In commit " rpc: Assert that passed arg names are equal to hardcoded ones (server)" (fac2edb)

It seems like it might be cleaner if instead of adding a new HiddenArg struct, and new RPCHelpMan constructor and special case code for m_hidden_arg in GetArgNames, this just added a new RPCArg::Type::HIDDEN type. This would also support multiple hidden args, or hidden args after non-hidden args.

Hidden args should not be a thing, because I can't imagine a use case for them.

If they are needed for testing, a new (hidden) RPC method with un-hidden RPC arguments should be added.

re: #19386 (comment)

Hidden args should not be a thing, because I can't imagine a use case for them.

If they are needed for testing, a new (hidden) RPC method with un-hidden RPC arguments should be added.

Beyond testing, undocumented hidden args also seem useful as a way of removing options we no longer want to support like wallet account and min_conf arguments in some places, and still maintaining compatibility when arguments are set to specific values (e.g. min_conf=1, account="*").

But regardless of whether hidden args will be used in the future, adding a Type::HIDDEN enum value seems like a simpler way to implement hidden args now than adding a HiddenArg struct and alternate code path.

Beyond testing, undocumented hidden args also seem useful as a way of removing options

I disagree. The RPC interface seems the wrong place to hide options in the documentation but then still accept them. For deprecation we use -rpcdeprecated and proper updates to the documentation to say an argument has been deprecated or is discouraged. (Edit: Making an option hidden will only hide the documentation that says the argument has been deprecated and thus will have the opposite effect of what was intended, no?)

I plan to remove HiddenArg before the next release, so the implementation shouldn't matter as long as it is minimal and correct.

promag

Concept ACK.

This allows the constructor to ask the rpc manager for the name of the rpc method or the rpc argument names instead of having it manually passed in.

maflcko · 2020-06-30T19:36:01Z

Addressed feedback. Sorry for the intermediate force-pushes

ryanofsky

Code review ACK fa87adf. Changes since previous review: removing redundant throw, renaming some things, adding RPCHelpMan::HandleRequest method to replace macro, changing commit order.

So suggestion from #19386 (comment) doesn't get lost, I think HandleRequest(request) method could call Check(request), so every individual method doesn't have to check its own arguments.

ryanofsky · 2020-07-02T19:04:09Z

src/rpc/util.h

@@ -329,6 +329,11 @@ class RPCHelpMan
 public:
    RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples);
    using RPCMethod = std::function<UniValue(const RPCHelpMan&, const JSONRPCRequest&)>;
+    struct HiddenArg {


re: #19386 (comment)

Hidden args should not be a thing, because I can't imagine a use case for them.

If they are needed for testing, a new (hidden) RPC method with un-hidden RPC arguments should be added.

Beyond testing, undocumented hidden args also seem useful as a way of removing options we no longer want to support like wallet account and min_conf arguments in some places, and still maintaining compatibility when arguments are set to specific values (e.g. min_conf=1, account="*").

But regardless of whether hidden args will be used in the future, adding a Type::HIDDEN enum value seems like a simpler way to implement hidden args now than adding a HiddenArg struct and alternate code path.

Also, update switch statments to our style guide

Also, move Check to inside HandleRequest

maflcko · 2020-07-03T15:54:31Z

So suggestion from #19386 (comment) doesn't get lost, I think HandleRequest(request) method could call Check(request), so every individual method doesn't have to check its own arguments.

Thanks. Fixed and force pushed

ryanofsky

Code review ACK fa7592b. Looks great! Just some hidden arg and Check() and comment cleanups since last review

laanwj · 2020-07-15T15:06:38Z

ACK fa7592b

Makes sense. I think I will keep the linter/test around, but make it less fragile by removing the regex parsing, which has been broken in the past at least once.

I agree. The regexs were a quick hack. More robust parsing would definitely make sense. It's too bad that there's no portable/low-dependency way to get the C++ syntax tree and extract information from a program without some monstriosity hand-rolled thing.

…ommand ones (server) fa7592b rpc: Update server to use new RPCHelpMan (MarcoFalke) aaaaad5 rpc: Add option to hide RPCArg (MarcoFalke) fa9708f rpc: Assert that passed arg names are equal to hardcoded ones (MarcoFalke) faaeb2b rpc: Add CRPCCommand constructor which takes RPCHelpMan (MarcoFalke) fa8ec00 rpc: Check that left section is not multiline (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch the RPC methods in server. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: laanwj: ACK fa7592b ryanofsky: Code review ACK fa7592b. Looks great! Just some hidden arg and Check() and comment cleanups since last review Tree-SHA512: e64b6a212f4a3aeedeee47557559bde104d5fd40cdc1746b27eb2f3d4c8885d5e6e4dd287595ea11cdbc6a939654fe103cae765fd505875444d851f0abb11308

Summary: PR description: > Motivation > > RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the CRPCCommands and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). > > Changes > > The changes here add an assert in the CRPCCommand constructor that the RPCArg names are identical to the ones in the CRPCCommand. This is a backport of [[bitcoin/bitcoin#19386 | core#19386]] [1/5] bitcoin/bitcoin@fa8ec00 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10006

Summary: This allows the constructor to ask the rpc manager for the name of the rpc method or the rpc argument names instead of having it manually passed in. Note: added underscores to some constructors' arguments to prevent shadowing fields, like previously done in D5906 This is a backport of [[bitcoin/bitcoin#19386 | core#19386]] [2/5] bitcoin/bitcoin@faaeb2b Depends on D10006 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10007

Summary: This is a backport of [[bitcoin/bitcoin#19386 | core#19386]] [3/5] bitcoin/bitcoin@fa9708f Depends on D10007 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10008

Summary: Also, update switch statments to our style guide This is a backport of [[bitcoin/bitcoin#19386 | core#19386]] [4/5] bitcoin/bitcoin@aaaaad5 Depends on D10008 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10009

Summary: Also, move Check to inside HandleRequest This is a backport of [[bitcoin/bitcoin#19386 | core#19386]] bitcoin/bitcoin@fa7592b Depends on D10009 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta, Fabien Reviewed By: #bitcoin_abc, majcosta, Fabien Subscribers: Fabien, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10010

rpc: Check that left section is not multiline

fa8ec00

maflcko force-pushed the 2006-rpcManServer branch from fa2b206 to fac2edb Compare June 26, 2020 18:15

DrahtBot added the RPC/REST/ZMQ label Jun 26, 2020

This was referenced Jun 27, 2020

test: checks that bitcoin-cli autocomplete is in sync #18606

Closed

rpc: remove deprecated CRPCCommand constructor #18531

Merged

ryanofsky approved these changes Jun 30, 2020

View reviewed changes

promag reviewed Jun 30, 2020

View reviewed changes

maflcko force-pushed the 2006-rpcManServer branch from fac2edb to fabb011 Compare June 30, 2020 17:55

rpc: Add CRPCCommand constructor which takes RPCHelpMan

faaeb2b

This allows the constructor to ask the rpc manager for the name of the rpc method or the rpc argument names instead of having it manually passed in.

maflcko force-pushed the 2006-rpcManServer branch 3 times, most recently from fa7276a to fa87adf Compare June 30, 2020 18:32

ryanofsky approved these changes Jul 2, 2020

View reviewed changes

rpc: Assert that passed arg names are equal to hardcoded ones

fa9708f

maflcko force-pushed the 2006-rpcManServer branch from fa87adf to faccf86 Compare July 3, 2020 14:59

MarcoFalke added 2 commits July 3, 2020 11:08

rpc: Add option to hide RPCArg

aaaaad5

Also, update switch statments to our style guide

rpc: Update server to use new RPCHelpMan

fa7592b

Also, move Check to inside HandleRequest

maflcko force-pushed the 2006-rpcManServer branch from faccf86 to fa7592b Compare July 3, 2020 15:09

ryanofsky approved these changes Jul 8, 2020

View reviewed changes

maflcko merged commit 804ca26 into bitcoin:master Jul 15, 2020

maflcko deleted the 2006-rpcManServer branch July 15, 2020 17:45

maflcko added the Refactoring label Jul 15, 2020

maflcko mentioned this pull request Aug 5, 2020

wallet: Replace -zapwallettxes with zapwallettxes RPC #19653

Closed

maflcko mentioned this pull request Aug 4, 2021

rpc: Add RPCContext #20017

Closed

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

rpc: Assert that RPCArg names are equal to CRPCCommand ones (server) #19386

rpc: Assert that RPCArg names are equal to CRPCCommand ones (server) #19386

maflcko commented Jun 26, 2020

DrahtBot commented Jun 26, 2020 •

edited

Loading

laanwj commented Jun 29, 2020

maflcko commented Jun 29, 2020

ryanofsky left a comment

ryanofsky Jun 30, 2020

maflcko Jun 30, 2020

ryanofsky Jul 2, 2020

maflcko Jul 2, 2020 •

edited

Loading

promag left a comment

maflcko commented Jun 30, 2020

ryanofsky left a comment

ryanofsky Jul 2, 2020

maflcko commented Jul 3, 2020

ryanofsky left a comment

laanwj commented Jul 15, 2020 •

edited

Loading

rpc: Assert that RPCArg names are equal to CRPCCommand ones (server) #19386

rpc: Assert that RPCArg names are equal to CRPCCommand ones (server) #19386

Conversation

maflcko commented Jun 26, 2020

Motivation

Changes

Future work

Bugs found

DrahtBot commented Jun 26, 2020 • edited Loading

Conflicts

laanwj commented Jun 29, 2020

maflcko commented Jun 29, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jun 30, 2020

Choose a reason for hiding this comment

maflcko Jun 30, 2020

Choose a reason for hiding this comment

ryanofsky Jul 2, 2020

Choose a reason for hiding this comment

maflcko Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

promag left a comment

Choose a reason for hiding this comment

maflcko commented Jun 30, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jul 2, 2020

Choose a reason for hiding this comment

maflcko commented Jul 3, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

laanwj commented Jul 15, 2020 • edited Loading

DrahtBot commented Jun 26, 2020 •

edited

Loading

maflcko Jul 2, 2020 •

edited

Loading

laanwj commented Jul 15, 2020 •

edited

Loading