rpc: remove deprecated CRPCCommand constructor #18531

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2004-rpcMan changing 6 files +20 −51
  1. MarcoFalke commented at 3:58 pm on April 5, 2020: member

    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
  2. DrahtBot added the label GUI on Apr 5, 2020
  3. DrahtBot added the label Mining on Apr 5, 2020
  4. DrahtBot added the label RPC/REST/ZMQ on Apr 5, 2020
  5. DrahtBot added the label Tests on Apr 5, 2020
  6. DrahtBot added the label Wallet on Apr 5, 2020
  7. MarcoFalke removed the label GUI on Apr 5, 2020
  8. MarcoFalke removed the label Mining on Apr 5, 2020
  9. MarcoFalke removed the label Tests on Apr 5, 2020
  10. MarcoFalke removed the label Wallet on Apr 5, 2020
  11. DrahtBot commented at 7:37 pm on April 5, 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:

    • #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.

  12. promag commented at 9:01 am on April 6, 2020: member

    Concept ACK.

    Here or follow up, makes sense to also assert type of returned UniValue?

  13. MarcoFalke force-pushed on Apr 6, 2020
  14. MarcoFalke referenced this in commit 1b151e3ffc on Apr 7, 2020
  15. sidhujag referenced this in commit 121b214536 on Apr 8, 2020
  16. DrahtBot added the label Needs rebase on Apr 10, 2020
  17. MarcoFalke force-pushed on Apr 12, 2020
  18. MarcoFalke commented at 1:37 pm on April 12, 2020: member

    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
  19. DrahtBot removed the label Needs rebase on Apr 12, 2020
  20. DrahtBot added the label Needs rebase on Apr 14, 2020
  21. MarcoFalke referenced this in commit 244daa4821 on Apr 17, 2020
  22. sidhujag referenced this in commit 645c5f7960 on Apr 17, 2020
  23. MarcoFalke force-pushed on Apr 17, 2020
  24. DrahtBot removed the label Needs rebase on Apr 17, 2020
  25. in src/rpc/server.cpp:149 in fa45fbcd65 outdated 
    142+        [&](const RPCMan& self, const JSONRPCRequest& jsonRequest) -> UniValue
143+{
144+    self.Check(jsonRequest);
145+    if (jsonRequest.fHelp || jsonRequest.params.size() > 1)
146+        throw std::runtime_error(
147+            self.ToString()
    pierreN commented at 2:32 pm on April 18, 2020:
    Since you add a call to RPCMan::Check, why then recheck again for fHelp flag/params size? The if statement could be removed, right?
  26. pierreN approved
  27. pierreN commented at 2:53 pm on April 18, 2020: contributor

    Tested ACK fa45fbc

    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?

    I tried to refactor a bit the changes on top of your PR so that it’s easier for users to use RPCMan: https://github.com/pierreN/bitcoin/commit/b1633b0e449177ca5eea382409e3a3af064fde08 (splits the lambda in 2, add a constructor helper to automatically call Check)

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

  28. DrahtBot added the label Needs rebase on Apr 19, 2020
  29. 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.
  30. MarcoFalke force-pushed on Apr 30, 2020
  31. DrahtBot removed the label Needs rebase on Apr 30, 2020
  32. MarcoFalke closed this on Apr 30, 2020
  33. MarcoFalke reopened this on Apr 30, 2020
  34. MarcoFalke closed this on Apr 30, 2020
  35. MarcoFalke reopened this on Apr 30, 2020
  36. 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:

  37. DrahtBot added the label Needs rebase on May 1, 2020
  38. MarcoFalke force-pushed on May 1, 2020
  39. DrahtBot removed the label Needs rebase on May 1, 2020
  40. DrahtBot added the label Needs rebase on May 4, 2020
  41. MarcoFalke force-pushed on May 4, 2020
  42. DrahtBot removed the label Needs rebase on May 5, 2020
  43. DrahtBot added the label Needs rebase on May 14, 2020
  44. MarcoFalke force-pushed on May 14, 2020
  45. DrahtBot removed the label Needs rebase on May 15, 2020
  46. DrahtBot added the label Needs rebase on May 21, 2020
  47. MarcoFalke force-pushed on May 21, 2020
  48. DrahtBot removed the label Needs rebase on May 21, 2020
  49. DrahtBot added the label Needs rebase on May 23, 2020
  50. MarcoFalke force-pushed on May 23, 2020
  51. DrahtBot removed the label Needs rebase on May 23, 2020
  52. DrahtBot added the label Needs rebase on Jun 11, 2020
  53. MarcoFalke force-pushed on Jun 12, 2020
  54. MarcoFalke commented at 9:55 pm on June 12, 2020: member

    Rebased to reduce the diff and get rid of the already merged bugfixes.

    Not sure why GitHub is hiding the comments (there are only 8 in total), but for reference this has one Concept ACK and one (stale) tested ACK.

  55. DrahtBot removed the label Needs rebase on Jun 12, 2020
  56. DrahtBot added the label Needs rebase on Jun 21, 2020
  57. MarcoFalke force-pushed on Jun 21, 2020
  58. DrahtBot removed the label Needs rebase on Jun 21, 2020
  59. jonatack commented at 3:31 pm on June 21, 2020: member

    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.

  60. MarcoFalke added the label Refactoring on Jun 21, 2020
  61. DrahtBot added the label Needs rebase on Jun 25, 2020
  62. MarcoFalke force-pushed on Jun 26, 2020
  63. MarcoFalke force-pushed on Jun 26, 2020
  64. DrahtBot removed the label Needs rebase on Jun 26, 2020
  65. DrahtBot added the label Needs rebase on Jul 7, 2020
  66. MarcoFalke referenced this in commit 804ca26629 on Jul 15, 2020
  67. MarcoFalke force-pushed on Jul 15, 2020
  68. DrahtBot removed the label Needs rebase on Jul 15, 2020
  69. MarcoFalke force-pushed on Jul 15, 2020
  70. sidhujag referenced this in commit ea73ed0a45 on Jul 16, 2020
  71. DrahtBot added the label Needs rebase on Jul 21, 2020
  72. MarcoFalke referenced this in commit 31760bb7c9 on Aug 14, 2020
  73. MarcoFalke force-pushed on Aug 14, 2020
  74. DrahtBot removed the label Needs rebase on Aug 14, 2020
  75. DrahtBot added the label Needs rebase on Aug 14, 2020
  76. MarcoFalke force-pushed on Aug 14, 2020
  77. DrahtBot removed the label Needs rebase on Aug 14, 2020
  78. DrahtBot added the label Needs rebase on Aug 15, 2020
  79. sidhujag referenced this in commit b761dae549 on Aug 16, 2020
  80. sidhujag referenced this in commit 010a3815db on Aug 16, 2020
  81. in src/rpc/blockchain.cpp:2467 in b224f98033 outdated 
    2434@@ -2435,39 +2435,39 @@ void RegisterBlockchainRPCCommands(CRPCTable &t)
2435 static const CRPCCommand commands[] =
2436 { //  category              name                      actor (function)         argNames
    fjahr commented at 3:38 pm on August 30, 2020:
    Missed fixing the headline here.
  82. fjahr commented at 3:45 pm on August 30, 2020: member

    Concept ACK

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

  83. MarcoFalke referenced this in commit 89a8299a14 on Aug 31, 2020
  84. MarcoFalke force-pushed on Aug 31, 2020
  85. DrahtBot removed the label Needs rebase on Aug 31, 2020
  86. MarcoFalke force-pushed on Aug 31, 2020
  87. sidhujag referenced this in commit 4c2ef86224 on Aug 31, 2020
  88. DrahtBot added the label Needs rebase on Sep 3, 2020
  89. domob1812 referenced this in commit 68bbfff8f8 on Sep 7, 2020
  90. domob1812 referenced this in commit 198ced101f on Sep 7, 2020
  91. domob1812 referenced this in commit 887e7b450a on Sep 7, 2020
  92. MarcoFalke referenced this in commit d692d192cd on Sep 22, 2020
  93. MarcoFalke force-pushed on Sep 22, 2020
  94. MarcoFalke force-pushed on Sep 22, 2020
  95. DrahtBot removed the label Needs rebase on Sep 22, 2020
  96. MarcoFalke force-pushed on Sep 22, 2020
  97. MarcoFalke force-pushed on Sep 22, 2020
  98. sidhujag referenced this in commit f769a0d8b6 on Sep 23, 2020
  99. DrahtBot added the label Needs rebase on Sep 23, 2020
  100. MarcoFalke referenced this in commit 5e14fafb31 on Sep 23, 2020
  101. sidhujag referenced this in commit 63306f71f5 on Sep 23, 2020
  102. 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
  103. remove CRPCCommand constructor that takes rpcfn_type function pointer faaf9c58e4
  104. MarcoFalke force-pushed on Sep 24, 2020
  105. MarcoFalke renamed this:
    rpc: Assert that RPCArg names are equal to CRPCCommand ones
    rpc: remove deprecated CRPCCommand constructor
    on Sep 24, 2020
  106. MarcoFalke commented at 7:04 pm on September 24, 2020: member
    This is ready for review now
  107. DrahtBot removed the label Needs rebase on Sep 24, 2020
  108. 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.
  109. in test/lint/lint-rpc-help.sh:15 in faaf9c58e4 
     9-
10-EXIT_CODE=0
11-
12-# Assume that all multiline strings passed into a runtime_error are help texts.
13-# This is potentially fragile, but the linter is only temporary and can safely
14-# be removed early 2019.
    promag commented at 8:45 am on September 25, 2020:
    😄
  110. in src/rpc/server.h:88 in faaf9c58e4 
    84@@ -85,7 +85,6 @@ void RPCUnsetTimerInterface(RPCTimerInterface *iface);
85  */
86 void RPCRunLater(const std::string& name, std::function<void()> func, int64_t nSeconds);
87 
88-typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);
89 typedef RPCHelpMan (*RpcMethodFnType)();
    promag commented at 8:50 am on September 25, 2020:
    I wouldn’t mind adding a commit to ditch this definition.
    MarcoFalke commented at 10:07 am on September 25, 2020:
    The def is also used in #20012
  111. promag commented at 9:15 am on September 25, 2020: member

    Tested ACK faaf9c58e4aa809019d4ca12747dd47411988e37.

    Verified 1st commit claims.

  112. fjahr commented at 10:06 am on September 25, 2020: member
    tested ACK faaf9c58e4aa809019d4ca12747dd47411988e37
  113. ryanofsky approved
  114. ryanofsky commented at 1:00 pm on November 19, 2020: member

    Code review ACK faaf9c58e4aa809019d4ca12747dd47411988e37. Two obviously good simplifications.

    re: #18531#issue-399144769

    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

  115. MarcoFalke merged this on Nov 19, 2020
  116. MarcoFalke closed this on Nov 19, 2020
  117. MarcoFalke deleted the branch on Nov 19, 2020
  118. 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

    Indeed. Done in #20012

  119. sidhujag referenced this in commit 92bd89791a on Nov 19, 2020
  120. Fabcien referenced this in commit 3d7fdc544a on Jan 3, 2022
  121. Fabcien referenced this in commit e74ec8b0f2 on Jan 3, 2022
  122. Fabcien referenced this in commit 9f54c180f5 on Jan 3, 2022
  123. DrahtBot locked this on Feb 15, 2022


MarcoFalke DrahtBot promag pierreN jonatack fjahr ryanofsky

Labels
Refactoring RPC/REST/ZMQ

github-metadata-mirror

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: 2024-11-21 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me