rpc: Remove duplicate name and argNames from CRPCCommand

MarcoFalke commented at 7:28 am on September 25, 2020: member

Currently, the RPC argument names are specified twice to simplify consistency linting. To avoid having to specify the argnames twice when adding new arguments, remove the linter and add an equivalent test based on RPCHelpMan.

MarcoFalke force-pushed on Sep 25, 2020

DrahtBot added the label GUI on Sep 25, 2020

DrahtBot added the label Mining on Sep 25, 2020

DrahtBot added the label RPC/REST/ZMQ on Sep 25, 2020

DrahtBot added the label Wallet on Sep 25, 2020

promag commented at 9:24 am on September 25, 2020: member

Concept ACK

DrahtBot commented at 1:05 pm on September 25, 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:

#20664 (Add scanblocks RPC call by jonasschnelli)
#20459 (rpc: Fail to return undocumented return values by MarcoFalke)
#20429 (refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size by theStack)
#20391 (wallet: introduce setfeerate (an improved settxfee, in sat/vB) by jonatack)
#20295 (rpc: getblockfrompeer by Sjors)
#19825 (rpc: simpler setban and new ban manipulation commands by dhruv)
#19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
#19521 (Coinstats Index (without UTXO set hash) by fjahr)
#19443 (rpc: Add options argument to listtransactions, with paginatebypointer options by kristapsk)
#19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#16546 (External signer support - Wallet Box edition by Sjors)
#16439 (cli/gui: support “@height” in place of blockhash for getblock on client side by ajtowns)
#15129 (rpc: Added ability to remove watch only addresses by benthecarman)
#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.

MarcoFalke force-pushed on Nov 19, 2020

MarcoFalke removed the label GUI on Nov 19, 2020

MarcoFalke removed the label Mining on Nov 19, 2020

MarcoFalke removed the label Wallet on Nov 19, 2020

MarcoFalke added the label Refactoring on Nov 19, 2020

MarcoFalke marked this as ready for review on Nov 19, 2020

MarcoFalke force-pushed on Nov 20, 2020

stackman27 commented at 1:44 am on December 29, 2020: contributor

ACK Code review and Built successful. This definitely looks cleaner than the old version and eliminates lots of repetitions

DrahtBot added the label Needs rebase on Jan 11, 2021

MarcoFalke force-pushed on Jan 12, 2021

DrahtBot removed the label Needs rebase on Jan 12, 2021

MarcoFalke commented at 9:34 am on January 12, 2021: member

Rebased

ryanofsky approved

ryanofsky commented at 4:10 pm on January 21, 2021: member

Code review ACK fafd58e4dc4089d5aa450feefa9af27f9b34c297. Nice cleanup which gets rid of some parameter repetition dealing with RPCs. It’s also nice to drop some c++ parsing code. The previous asserts and new test updates seem to make this pretty foolproof and ensure there’s no change in behavior.

jonasschnelli commented at 7:52 am on January 25, 2021: contributor

Much tidier! Concept ACK

DrahtBot added the label Needs rebase on Jan 28, 2021

rpc: [refactor] Use concise C++11 code in CRPCConvertTable constructor faf835680b

rpc: Use RPCHelpMan for check-rpc-mappings linter fa92912b4b

MarcoFalke force-pushed on Jan 28, 2021

rpc: Remove duplicate name and argNames from CRPCCommand fa04f9b4dd

MarcoFalke force-pushed on Jan 28, 2021

MarcoFalke commented at 7:43 am on January 28, 2021: member

Rebased, should be trivial to re-ACK

MarcoFalke commented at 7:43 am on January 28, 2021: member

@laanwj As the author of test/lint/check-rpc-mappings.py, could you take a look here on the python changes?

DrahtBot removed the label Needs rebase on Jan 28, 2021

laanwj commented at 6:24 pm on January 28, 2021: member

ACK fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204 Not sad to see that script go.

laanwj merged this on Jan 28, 2021

laanwj closed this on Jan 28, 2021

MarcoFalke deleted the branch on Jan 28, 2021

sidhujag referenced this in commit 30a8d4142f on Jan 29, 2021

sidhujag referenced this in commit 48442dd499 on Jan 29, 2021

in src/rpc/server.cpp:492 in fa92912b4b outdated

483@@ -479,6 +484,18 @@ std::vector<std::string> CRPCTable::listCommands() const
484     return commandList;
485 }
486 
487+UniValue CRPCTable::dumpArgMap() const
488+{
489+    UniValue ret{UniValue::VARR};
490+    for (const auto& cmd : mapCommands) {
491+        for (const auto& c : cmd.second) {
492+            const auto help = RpcMethodFnType(c->unique_id)();

ryanofsky commented at 11:58 pm on January 29, 2021:

In commit “rpc: Use RPCHelpMan for check-rpc-mappings linter” (fa92912b4bb4629addcbfdfb7cc000be701614af)

I missed this in initial review, but this cast from unique_id to function pointer isn’t really safe. Or at least it results in segfaults with #10102 (https://cirrus-ci.com/task/5469433013469184?command=ci#L2275). The unique_id field was really only supposed to be used to detect RPC aliases, not be converted to a function pointer and called. #21035 updates this to take a different approach making arg retrieval work more like help retrieval.

domob1812 referenced this in commit dd4f6f1a8b on Feb 1, 2021

domob1812 referenced this in commit c4af2fd596 on Feb 1, 2021

Fabcien referenced this in commit 427e5fc8ea on Jan 5, 2022

Fabcien referenced this in commit 25e918ce93 on Jan 5, 2022

Fabcien referenced this in commit 27d3f1aaa0 on Jan 5, 2022

DrahtBot locked this on Aug 16, 2022

rpc: Remove duplicate name and argNames from CRPCCommand #20012

Conflicts