rpc: remove deprecated CRPCCommand constructor by maflcko · Pull Request #18531 · bitcoin/bitcoin

maflcko · 2020-04-05T15:58:34Z

Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant

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 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-04-05T19:37:23Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

RPC: Internal named params #17356 (RPC: Internal named params by luke-jr)

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.

…ommand ones (misc) fa77de2 rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke) fa50bdc rpc: Limit echo to 10 args (MarcoFalke) fa89ca9 refactor: Use C++11 range based for loops to simplify rpc code (MarcoFalke) fa459bd rpc: Treat all args after a hidden arg as hidden as well (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch the RPC methods in misc. 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: Code review ACK fa77de2 fjahr: tested ACK fa77de2 theStack: ACK bitcoin@fa77de2 ryanofsky: Code review ACK fa77de2. Pretty straightfoward changes Tree-SHA512: badae1606518c0b55ce2c0bb9025d14f05556532375eb20fd6f3bfadae1e5e6568860bff8599d037e655bf1d23f1f464ca17f4db10a6ab3d502b6e9e61c7b3d3

…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

fjahr

Concept ACK

Will test when this is rebased and checks pass. Please re-check the commit messages, 97a419abaf673d072e3c5b23af078e403d1c5dc4 and 0bd51da9ecdc0c5f3834f8824d260f6f535ded92 seem to be outdated.

src/rpc/blockchain.cpp

…ommand ones (mining,zmq,rpcdump) fa3d9ce rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke) fa32c1d rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke) faaa46d rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke) fa93bc1 rpc: Remove unused return type from appendCommand (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch the RPC methods in misc. 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: fjahr: tested ACK fa3d9ce promag: Code review ACK fa3d9ce. Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719

Checking for fHelp, or the size of the args, is dead code because: * fHelp is always false (src/qt/test/rpcnestedtests.cpp) * It is already implicitly called by RPCHelpMan::Check (src/rpc/mining.cpp, src/rpc/misc.cpp, src/rpc/net.cpp)

maflcko · 2020-09-24T19:04:34Z

This is ready for review now

promag

On fa19bb2 you could rename fHelp so that if this gets rebased we don't miss new cases.

promag

Tested ACK faaf9c5.

Verified 1st commit claims.

promag · 2020-09-25T08:45:31Z

test/lint/lint-rpc-help.sh

-
-# Assume that all multiline strings passed into a runtime_error are help texts.
-# This is potentially fragile, but the linter is only temporary and can safely
-# be removed early 2019.


promag · 2020-09-25T08:50:58Z

src/rpc/server.h

 void RPCRunLater(const std::string& name, std::function<void()> func, int64_t nSeconds);

-typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);
 typedef RPCHelpMan (*RpcMethodFnType)();


I wouldn't mind adding a commit to ditch this definition.

The def is also used in #20012

fjahr · 2020-09-25T10:06:24Z

tested ACK faaf9c5

ryanofsky

Code review ACK faaf9c5. Two obviously good simplifications.

re: #18531 (comment)

Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant

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

I don't see this is mentioned in "future work" part of PR description, but it seems like a good followup would be to drop redundant name_in and args_in arguments to the other CRPCCommand constructor:

https://github.com/MarcoFalke/bitcoin-core/blob/faaf9c58e4aa809019d4ca12747dd47411988e37/src/rpc/server.h#L106-L116

since these are just checked for equality and discarded anyway. Or maybe that's what the PR was originally intended to do? The code changes are simple, but the PR description is a little hard to follow

maflcko · 2020-11-19T14:06:50Z

since these are just checked for equality and discarded anyway. Or maybe that's what the PR was originally intended to do? The code changes are simple, but the PR description is a little hard to follow

Indeed. Done in #20012

maflcko mentioned this pull request Apr 5, 2020

rpc: Avoid initialization-order-fiasco on static CRPCCommand tables #18532

Merged

DrahtBot added GUI Mining RPC/REST/ZMQ Tests Wallet labels Apr 5, 2020

maflcko removed GUI Mining Tests Wallet labels Apr 5, 2020

DrahtBot removed the Needs rebase label Aug 14, 2020

maflcko mentioned this pull request Aug 14, 2020

rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining,zmq,rpcdump) #19717

Merged

DrahtBot added the Needs rebase label Aug 14, 2020

maflcko force-pushed the 2004-rpcMan branch from dd5c092 to b224f98 Compare August 14, 2020 10:43

DrahtBot added Needs rebase and removed Needs rebase labels Aug 14, 2020

fjahr reviewed Aug 30, 2020

View reviewed changes

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

maflcko mentioned this pull request Aug 31, 2020

Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) #19849

Merged

maflcko mentioned this pull request Sep 22, 2020

Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) #19994

Merged

MarcoFalke added 2 commits September 24, 2020 19:53

remove dead rpc code

fa19bb2

Checking for fHelp, or the size of the args, is dead code because: * fHelp is always false (src/qt/test/rpcnestedtests.cpp) * It is already implicitly called by RPCHelpMan::Check (src/rpc/mining.cpp, src/rpc/misc.cpp, src/rpc/net.cpp)

remove CRPCCommand constructor that takes rpcfn_type function pointer

faaf9c5

maflcko mentioned this pull request Sep 25, 2020

rpc: Remove duplicate name and argNames from CRPCCommand #20012

Merged

promag reviewed Sep 25, 2020

View reviewed changes

DrahtBot mentioned this pull request Sep 30, 2020

upstream: add and use defaulted getters to UniValue #20045

Closed

ryanofsky approved these changes Nov 19, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: remove deprecated CRPCCommand constructor#18531

rpc: remove deprecated CRPCCommand constructor#18531
maflcko merged 2 commits intobitcoin:masterfrom
maflcko:2004-rpcMan

maflcko commented Apr 5, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Apr 5, 2020 •

edited

Loading

Uh oh!

fjahr left a comment

Uh oh!

Uh oh!

maflcko commented Sep 24, 2020

Uh oh!

promag left a comment

Uh oh!

promag left a comment

Uh oh!

promag Sep 25, 2020

Uh oh!

promag Sep 25, 2020

Uh oh!

maflcko Sep 25, 2020

Uh oh!

fjahr commented Sep 25, 2020

Uh oh!

ryanofsky left a comment

Uh oh!

maflcko commented Nov 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

maflcko commented Apr 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Future work

Bugs found

Uh oh!

DrahtBot commented Apr 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Sep 24, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

promag Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

fjahr commented Sep 25, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Future work

Bugs found

Uh oh!

maflcko commented Nov 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maflcko commented Apr 5, 2020 •

edited

Loading

DrahtBot commented Apr 5, 2020 •

edited

Loading