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.
Since you add a call to RPCMan::Check, why then recheck again for fHelp flag/params size? The if statement could be removed, right?
pierreN approved
pierreN
commented at 2:53 PM on April 18, 2020:
contributor
Tested ACKfa45fbc
This may be out of scope for this PR, but something that might bother RPCMan users a bit is to have to call Check every time (as most do now).
I think this issue is reflected in the lambda function signature: to do the actual RPC work, why would you need a reference to RPCMan?
IMHO this makes the class a tad easier to use (this might be personal taste though?) and shaves off ~500 lines.
If you think this refactor is a good idea, please do add it to the PR. Or if you think this is too out of scope, I could do a PR later once this one is merged (or we could just let things as is).
DrahtBot added the label Needs rebase on Apr 19, 2020
MarcoFalke
commented at 11:25 PM on April 29, 2020:
member
@pierreN Thanks for the review. I do like your suggestion to pass in the checks into RPCMan and execute them there. However, I think this can be done nicely in a follow-up and would bloat this pull a bit too much.
pierreN
commented at 4:47 AM on May 1, 2020:
contributor
I do like your suggestion to pass in the checks into RPCMan and execute them there. However, I think this can be done nicely in a follow-up and would bloat this pull a bit too much.
Thanks, indeed. I'll send a PR once this one is merged then :smiley:
DrahtBot added the label Needs rebase on May 1, 2020
DrahtBot added the label Needs rebase on Jun 21, 2020
MarcoFalke force-pushed on Jun 21, 2020
DrahtBot removed the label Needs rebase on Jun 21, 2020
jonatack
commented at 3:31 PM on June 21, 2020:
contributor
Concept ACK, will review.
For testing the printed JSON of CAmounts, I propose assert_scale in test_framework/util.py in 741d4b94f4f30ca2d7e4886bc98a9faa8d5cc8c2 used for testing bitcoin balances in #19089 and #19092.
MarcoFalke added the label Refactoring on Jun 21, 2020
fjahr
commented at 3:45 PM on August 30, 2020:
contributor
Concept ACK
Will test when this is rebased and checks pass. Please re-check the commit messages, 97a419abaf673d072e3c5b23af078e403d1c5dc4 and 0bd51da9ecdc0c5f3834f8824d260f6f535ded92 seem to be outdated.
MarcoFalke referenced this in commit 89a8299a14 on Aug 31, 2020
DrahtBot removed the label Needs rebase on Sep 22, 2020
MarcoFalke force-pushed on Sep 22, 2020
MarcoFalke force-pushed on Sep 22, 2020
sidhujag referenced this in commit f769a0d8b6 on Sep 23, 2020
DrahtBot added the label Needs rebase on Sep 23, 2020
MarcoFalke referenced this in commit 5e14fafb31 on Sep 23, 2020
sidhujag referenced this in commit 63306f71f5 on Sep 23, 2020
remove dead rpc code
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)
fa19bb2cd8
remove CRPCCommand constructor that takes rpcfn_type function pointerfaaf9c58e4
MarcoFalke force-pushed on Sep 24, 2020
MarcoFalke renamed this: rpc: Assert that RPCArg names are equal to CRPCCommand ones rpc: remove deprecated CRPCCommand constructor on Sep 24, 2020
MarcoFalke
commented at 7:04 PM on September 24, 2020:
member
This is ready for review now
DrahtBot removed the label Needs rebase on Sep 24, 2020
promag
commented at 8:42 AM on September 25, 2020:
member
On fa19bb2cd8c575593583138a84e6bb3444d6196d you could rename fHelp so that if this gets rebased we don't miss new cases.
in
test/lint/lint-rpc-help.sh:15
in
faaf9c58e4
10 | - 11 | -EXIT_CODE=0 12 | - 13 | -# Assume that all multiline strings passed into a runtime_error are help texts. 14 | -# This is potentially fragile, but the linter is only temporary and can safely 15 | -# be removed early 2019.
promag
commented at 8:45 AM on September 25, 2020:
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:
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
MarcoFalke merged this on Nov 19, 2020
MarcoFalke closed this on Nov 19, 2020
MarcoFalke deleted the branch on Nov 19, 2020
MarcoFalke
commented at 2:06 PM on November 19, 2020:
member
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
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC
