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

MarcoFalke commented at 5:50 pm on June 26, 2020: member

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 #18607
The changes itself fixed bug #19250

rpc: Check that left section is not multiline fa8ec00061

MarcoFalke force-pushed on Jun 26, 2020

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

DrahtBot commented at 8:26 pm on June 26, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 commented at 2:27 pm on June 29, 2020: member

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.

MarcoFalke commented at 2:38 pm on June 29, 2020: member

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.

in src/rpc/util.h:325 in fac1bf1a08 outdated

321@@ -322,10 +322,14 @@ struct RPCExamples {
322     std::string ToDescriptionString() const;
323 };
324 
325+#define CALL_RPC_METHOD(rpc_name, json_request) [&] { const RPCHelpMan& man = rpc_name(); return man.m_fun(man, json_request); }()

ryanofsky commented at 2:43 pm on June 30, 2020:

In commit “rpc: Add CRPCCommand constructor which takes RPCHelpMan” (fac1bf1a08ea9cb5a62889e61696d9503338c105)

IMO, it’d be nicer if this were not a macro. Seems like this could be a RPCHelpMan method or standalone function:

0UniValue HandleRequest(const RPCHelpMan& man, const JSONRPCRequest& request)
1{
2    return man.m_fun(request);
3}

Also, maybe in the future it would be nice to add man.Check(request); here so it can be removed from all the individual RPC methods.

MarcoFalke commented at 6:14 pm on June 30, 2020:

thx. Done in faaeb2b0b347b40ce456a951eec5e820587e5b02

in src/rpc/util.h:347 in fac1bf1a08 outdated

343@@ -340,6 +344,8 @@ class RPCHelpMan
344         }
345     }
346 
347+    const RPCMethod m_fun;

ryanofsky commented at 2:48 pm on June 30, 2020:

In commit “rpc: Add CRPCCommand constructor which takes RPCHelpMan” (fac1bf1a08ea9cb5a62889e61696d9503338c105)

I think RPCMethod m_method would be more obvious naming than RPCMethod m_fun. Also in the future I’d consider a broader name cleanup, like renaming RPCHelpMan to RPCMethod and renaming RPCMethod to RPCMethodImpl

MarcoFalke commented at 6:15 pm on June 30, 2020:

Thanks, renamed to RPCMethodImpl in faaeb2b0b347b40ce456a951eec5e820587e5b02

in src/rpc/server.cpp:147 in fa0ae5063f outdated

140@@ -143,44 +141,54 @@ UniValue help(const JSONRPCRequest& jsonRequest)
141                     RPCResult::Type::STR, "", "The help text"
142                 },
143                 RPCExamples{""},
144-            }.ToString()
145+        [&](const RPCHelpMan& self, const JSONRPCRequest& jsonRequest) -> UniValue
146+{
147+    self.Check(jsonRequest);
148+    if (jsonRequest.fHelp || jsonRequest.params.size() > 1)

ryanofsky commented at 2:56 pm on June 30, 2020:

In commit “rpc: Update server to use new RPCHelpMan” (fa0ae5063f4d4eae429b33d4375fd8101adb8150)

This is just being kept for consistency right? It seems like it should be redundant with the self.Check call immediately above.

MarcoFalke commented at 6:21 pm on June 30, 2020:

thx, done in fa87adf7c6433517a8ed56f0c93560bc9837ad64

in src/rpc/util.h:330 in fac2edbd5f outdated

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

ryanofsky commented at 3:05 pm on June 30, 2020:

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

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.

MarcoFalke commented at 5:19 pm on June 30, 2020:

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.

ryanofsky commented at 7:04 pm on July 2, 2020:

re: #19386 (review)

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.

MarcoFalke commented at 8:26 pm on July 2, 2020:

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.

ryanofsky approved

ryanofsky commented at 3:19 pm on June 30, 2020: member

Code review ACK fac2edbd5fc8075647e90e90d49b3cd910bdbbb7. 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.

promag commented at 3:42 pm on June 30, 2020: member

Concept ACK.

MarcoFalke force-pushed on Jun 30, 2020

rpc: Add CRPCCommand constructor which takes RPCHelpMan

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.

faaeb2b0b3

MarcoFalke force-pushed on Jun 30, 2020

MarcoFalke commented at 7:36 pm on June 30, 2020: member

Addressed feedback. Sorry for the intermediate force-pushes

ryanofsky approved

ryanofsky commented at 7:31 pm on July 2, 2020: member

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

So suggestion from #19386 (review) 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.

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

MarcoFalke force-pushed on Jul 3, 2020

rpc: Add option to hide RPCArg

Also, update switch statments to our style guide

aaaaad5627

rpc: Update server to use new RPCHelpMan

Also, move Check to inside HandleRequest

fa7592bfa8

MarcoFalke force-pushed on Jul 3, 2020

MarcoFalke commented at 3:54 pm on July 3, 2020: member

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 approved

ryanofsky commented at 5:46 pm on July 8, 2020: member

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

MarcoFalke added this to the "Blockers" column in a project

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke added this to the "Blockers" column in a project

laanwj commented at 3:06 pm on July 15, 2020: member

ACK fa7592bfa8691eb0289b21da3571709a18391b0f

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.

MarcoFalke merged this on Jul 15, 2020

MarcoFalke closed this on Jul 15, 2020

MarcoFalke deleted the branch on Jul 15, 2020

laanwj removed this from the "Blockers" column in a project

MarcoFalke added the label Refactoring on Jul 15, 2020

sidhujag referenced this in commit ea73ed0a45 on Jul 16, 2020

sidhujag referenced this in commit 010a3815db on Aug 16, 2020

Fabcien referenced this in commit 4f6a3afbc4 on Sep 1, 2021

Fabcien referenced this in commit b568655c01 on Sep 10, 2021

Fabcien referenced this in commit f726df976d on Sep 10, 2021

Fabcien referenced this in commit 924c5621d6 on Sep 10, 2021

Fabcien referenced this in commit c6a3bec649 on Sep 10, 2021

DrahtBot locked this on Feb 15, 2022

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

Motivation

Changes

Future work

Bugs found

Conflicts