rpc: remove deprecated CRPCCommand constructor #18531

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.

…d ones (blockchain,rawtransaction) fa6bb0c Assert that RPCArg names are equal to CRPCCommand ones (rawtransaction) (MarcoFalke) fa80c81 Assert that RPCArg names are equal to CRPCCommand ones (blockchain) (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch some RPC methods. 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: utACK fa6bb0c tryphe: utACK fa6bb0c. Reducing data duplication is nice. Code changes are minimal and concise. Tree-SHA512: deb0edc3f999baf055526eaa199b98c500635e12502dece7aa3cad5319db330eb5ee7459a5c8f040a83671a7f20c560c19a2026fb76c8416f138aa332727cbce

…(net, rpcwallet) fa14f57 Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) (MarcoFalke) Pull request description: This is the last part split out from #18531 to just touch some RPC methods. 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 #18607 * The changes itself fixed bug #19250 ACKs for top commit: fjahr: tACK fa14f57 ryanofsky: Code review ACK fa14f57. Just straightforward replacements except code moved in `addnode`, and displatching updated in `bumpfee_helper` Tree-SHA512: e07af150f1d95a88e558256ce197a6b7dc6cd722a6d6c13c75d944c49c2e2441f8b8237e9f94b03db69fa18f9bda627b0781d5e1da70bf5415e09b38728a8cb1

…d ones (net, rpcwallet) fa14f57 Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) (MarcoFalke) Pull request description: This is the last part split out from bitcoin#18531 to just touch some RPC methods. 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: tACK fa14f57 ryanofsky: Code review ACK fa14f57. Just straightforward replacements except code moved in `addnode`, and displatching updated in `bumpfee_helper` Tree-SHA512: e07af150f1d95a88e558256ce197a6b7dc6cd722a6d6c13c75d944c49c2e2441f8b8237e9f94b03db69fa18f9bda627b0781d5e1da70bf5415e09b38728a8cb1

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

faaf9c5 remove CRPCCommand constructor that takes rpcfn_type function pointer (MarcoFalke) fa19bb2 remove dead rpc code (MarcoFalke) Pull request description: 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 bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: fjahr: tested ACK faaf9c5 promag: Tested ACK faaf9c5. ryanofsky: Code review ACK faaf9c5. Two obviously good simplifications. Tree-SHA512: 5de3b440f7b2ed2c3e86655d4f0e2e5df9c67e8ce3c7817d5ea5311d1a38690f2f3e28fab41aad6936be9fc884326d037e5f19e85d4d2fe281474dada13911ee

Summary: > 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 D10741 for a list of bugs that have been found as a result of the changes here. > > The changes here add an assert in the CRPCCommand constructor that the RPCArg names are identical to the ones in the CRPCCommand. This should be the final diff related to [[bitcoin/bitcoin#18531 | core#18531]]. Depends on D10741 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10742

Summary: 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) This is a backport of [[bitcoin/bitcoin#18531 | core#18531]] [1/2] bitcoin/bitcoin@fa19bb2 I confirmed with a `grep` that `fHelp` is not used in any other RPC command. Depends on D10742 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10743

Summary: Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant Future work - 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 This concludes backport of [[bitcoin/bitcoin#18531 | core#18531]] bitcoin/bitcoin@faaf9c5 Depends on D10743 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10744

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

maflcko force-pushed the 2004-rpcMan branch from ed13e77 to c4c656b Compare September 22, 2020 15:40

maflcko mentioned this pull request Sep 22, 2020

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

Merged

maflcko force-pushed the 2004-rpcMan branch from c4c656b to 79792fb Compare September 22, 2020 16:58

DrahtBot removed the Needs rebase label Sep 22, 2020

maflcko force-pushed the 2004-rpcMan branch 2 times, most recently from d8e5fdf to 928193e Compare September 22, 2020 20:21

DrahtBot added the Needs rebase label Sep 23, 2020

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 force-pushed the 2004-rpcMan branch from 928193e to faaf9c5 Compare September 24, 2020 17:55

maflcko changed the title ~~rpc: Assert that RPCArg names are equal to CRPCCommand ones~~ rpc: remove deprecated CRPCCommand constructor Sep 24, 2020

DrahtBot removed the Needs rebase label Sep 24, 2020

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

maflcko merged commit 71d068d into bitcoin:master Nov 19, 2020

maflcko deleted the 2004-rpcMan branch November 19, 2020 13:21

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

Uh oh!

rpc: remove deprecated CRPCCommand constructor #18531

rpc: remove deprecated CRPCCommand constructor #18531

Uh oh!

maflcko commented Apr 5, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Apr 5, 2020 •

edited

Loading

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

rpc: remove deprecated CRPCCommand constructor #18531

rpc: remove deprecated CRPCCommand constructor #18531

Uh oh!

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!

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