rpc: Remove duplicate name and argNames from CRPCCommand #20012

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2009-rpcCheckMapping changing 16 files +284 −349

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
MarcoFalke cross-referenced this on Sep 25, 2020 from issue rpc: remove deprecated CRPCCommand constructor by MarcoFalke
DrahtBot commented at 1:05 PM on September 25, 2020: contributor


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.
DrahtBot cross-referenced this on Sep 25, 2020 from issue rpc: Add dumpcoinstats by fjahr
DrahtBot cross-referenced this on Sep 25, 2020 from issue wallet: Migrate legacy wallets to descriptor wallets by achow101
DrahtBot cross-referenced this on Sep 25, 2020 from issue Coinstats Index by fjahr
DrahtBot cross-referenced this on Sep 25, 2020 from issue send* RPCs in the wallet returns the "fee reason" by stackman27
DrahtBot cross-referenced this on Sep 25, 2020 from issue multiprocess: Add basic spawn and IPC support by ryanofsky
DrahtBot cross-referenced this on Sep 25, 2020 from issue RPC: Internal named params by luke-jr
DrahtBot cross-referenced this on Sep 25, 2020 from issue Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101
DrahtBot cross-referenced this on Sep 25, 2020 from issue External signer support - Wallet Box edition by Sjors
DrahtBot cross-referenced this on Sep 25, 2020 from issue rpc: Added ability to remove watch only addresses by benthecarman
DrahtBot cross-referenced this on Sep 25, 2020 from issue [tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar
DrahtBot cross-referenced this on Sep 26, 2020 from issue rpc: Add options argument to listtransactions, with paginatebypointer options by kristapsk
DrahtBot cross-referenced this on Sep 29, 2020 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
DrahtBot cross-referenced this on Sep 29, 2020 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
DrahtBot cross-referenced this on Sep 29, 2020 from issue Multiprocess bitcoin by ryanofsky
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
DrahtBot cross-referenced this on Nov 19, 2020 from issue rpc: getblockfrompeer by Sjors
DrahtBot cross-referenced this on Nov 19, 2020 from issue wallet, rpc: add listdescriptors command by S3RK
DrahtBot cross-referenced this on Nov 19, 2020 from issue BIP-322 support by kallewoof
DrahtBot cross-referenced this on Nov 19, 2020 from issue rpc: Add RPCContext by promag
DrahtBot cross-referenced this on Nov 19, 2020 from issue Prune locks by luke-jr
DrahtBot cross-referenced this on Nov 20, 2020 from issue Add address-based index (attempt 4?) by marcinja
MarcoFalke force-pushed on Nov 20, 2020
MarcoFalke force-pushed on Nov 20, 2020
MarcoFalke force-pushed on Nov 20, 2020
MarcoFalke force-pushed on Nov 20, 2020
DrahtBot cross-referenced this on Nov 23, 2020 from issue rpc: Fail to return undocumented return values by MarcoFalke
DrahtBot cross-referenced this on Nov 28, 2020 from issue wallet: introduce setfeerate (an improved settxfee, in sat/vB) by jonatack
DrahtBot cross-referenced this on Dec 3, 2020 from issue RPC/Net: Allow changing the connection_type for addnode onetry by luke-jr
DrahtBot cross-referenced this on Dec 14, 2020 from issue rpc: simpler setban and new ban manipulation commands by dhruv
DrahtBot cross-referenced this on Dec 16, 2020 from issue Add scanblocks RPC call by jonasschnelli
DrahtBot cross-referenced this on Dec 18, 2020 from issue [RPC] Include coinbase transactions in receivedby RPCs by andrewtoth
DrahtBot cross-referenced this on Dec 18, 2020 from issue rpc: Add getblocklocations call by romanz
DrahtBot cross-referenced this on Dec 23, 2020 from issue rpc: Allow to ignore specific policy reject reasons by MarcoFalke
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
DrahtBot cross-referenced this on Jan 12, 2021 from issue refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size by theStack
DrahtBot cross-referenced this on Jan 12, 2021 from issue cli/gui: support "@height" in place of blockhash for getblock on client side by ajtowns
ryanofsky approved
ryanofsky commented at 4:10 PM on January 21, 2021: contributor

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.
MarcoFalke cross-referenced this on Jan 22, 2021 from issue doc: add instructions for generating RPC docs by ben-kaufman
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
luke-jr cross-referenced this on Jul 7, 2021 from issue Bugfix: Workaround UniValue push_back(bool) limitation with push_back(UniValue(bool)) by luke-jr
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
bitcoin locked this on Aug 16, 2022