Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Mar 13, 2024

Issue being fixed or feature implemented

This batch of backports asserts that RPCArg names are equal to CRPCCommand ones.

What was done?

done backports:

Beside that same changes are applied for src/coinjoin's rpc.

There's also applied multiple fixes for various rpcs for cases when RPCArg names are mismatched with CPCCommand

Please, note, that this PR is not final fix for all RPCArgs. There's a lot of dash's rpc that is not refactored that.
That it is not easy to implement for quorum command because the list of arguments (and even their numbers) are different for each sub-command. This fixes are out-of scope of this PR and should be done before bitcoin#18531 is backported.

See also relevant bitcoin#21035.

How Has This Been Tested?

I used this helper to see which exactly args are specified wrongly:

diff --git a/src/rpc/server.h b/src/rpc/server.h
index d4a7ba60eb..cdfd741f54 100644
--- a/src/rpc/server.h
+++ b/src/rpc/server.h
@@ -16,6 +16,7 @@
 #include <string>
 
 #include <univalue.h>
+#include <logging.h>
 
 class CRPCCommand;
 
@@ -110,6 +111,19 @@ public:
               fn().GetArgNames(),
               intptr_t(fn))
     {
+        if (fn().m_name != name_in || fn().GetArgNames() != args_in) {
+            std::cerr << "names:  " << fn().m_name << ' ' << name_in << std::endl;
+            std::cerr << "arg names: " << fn().GetArgNames().size() << std::endl;
+            for (const auto& i : fn().GetArgNames()) {
+                std::cerr << "arg: " << i << std::endl;
+            }
+            std::cerr << "FIASCO FIASCO FIASCO FIASCO FIASCO FIASCO" << std::endl;
+        }
         CHECK_NONFATAL(fn().m_name == name_in);
         CHECK_NONFATAL(fn().GetArgNames() == args_in);
     }

Breaking Changes

N/A

Some arguments are renamed in RPC but they have been broken (used incorrect name not same as in docs)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst knst force-pushed the bp-v21-p26-rpc-args branch from 2bf1be2 to ea40973 Compare March 13, 2024 21:30
@github-actions
Copy link

This pull request has conflicts, please rebase.

@knst knst force-pushed the bp-v21-p26-rpc-args branch 2 times, most recently from a7599ae to 44f17d5 Compare March 14, 2024 11:30
@knst knst marked this pull request as ready for review March 14, 2024 12:08
@knst knst requested review from PastaPastaPasta and UdjinM6 March 14, 2024 12:08
@UdjinM6 UdjinM6 added this to the 21 milestone Mar 14, 2024
UdjinM6
UdjinM6 previously approved these changes Mar 14, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@knst knst added the RPC Some notable changes to RPC params/behaviour/descriptions label Mar 14, 2024
@github-actions
Copy link

This pull request has conflicts, please rebase.

UdjinM6
UdjinM6 previously approved these changes Mar 15, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@github-actions
Copy link

This pull request has conflicts, please rebase.

MarcoFalke and others added 9 commits March 17, 2024 13:02
…ommand ones (misc)

fa77de2 rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke)
fa50bdc rpc: Limit echo to 10 args (MarcoFalke)
fa89ca9 refactor: Use C++11 range based for loops to simplify rpc code (MarcoFalke)
fa459bd rpc: Treat all args after a hidden arg as hidden as well (MarcoFalke)

Pull request description:

  This is split out from bitcoin#18531 to just touch the RPC methods in misc. 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:
  laanwj:
    Code review ACK fa77de2
  fjahr:
    tested ACK fa77de2
  theStack:
    ACK bitcoin@fa77de2
  ryanofsky:
    Code review ACK fa77de2. Pretty straightfoward changes

Tree-SHA512: badae1606518c0b55ce2c0bb9025d14f05556532375eb20fd6f3bfadae1e5e6568860bff8599d037e655bf1d23f1f464ca17f4db10a6ab3d502b6e9e61c7b3d3
f0aa8ae test: add rpc_generate functional test (Jon Atack)
92d94ff rpc: print useful help and error message for generate (Jon Atack)
8d32d20 test: consider generate covered in _get_uncovered_rpc_commands() (Jon Atack)

Pull request description:

  This was a requested follow-up to bitcoin#19133 and bitcoin#17700 to alleviate confusion and head-scratching by people following tutorials that use `generate`. See bitcoin#19455 (comment) below, bitcoin#19133 (comment) and bitcoin#17700 (comment).

  before
  ```
  $ bitcoin-cli help generate
  help: unknown command: generate

  $ bitcoin-cli generate
  error code: -32601
  error message:
  Method not found
  ```

  after
  ```
  $ bitcoin-cli help generate
  generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.

  $ bitcoin-cli generate
  error code: -32601
  error message:
  generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.
  ```

  In the general help it remains hidden, as requested by laanwj.
  ```
  $ bitcoin-cli help

  == Generating ==
  generateblock "output" ["rawtx/txid",...]
  generatetoaddress nblocks "address" ( maxtries )
  generatetodescriptor num_blocks "descriptor" ( maxtries )
  ```

ACKs for top commit:
  adamjonas:
    utACK f0aa8ae
  pinheadmz:
    ACK f0aa8ae

Tree-SHA512: d083652589ad3e8228c733455245001db22397559c3946e7e573cf9bd01c46e9e88b72d934728ec7f4361436ae4c74adb8f579670b09f479011924357e729af5
…ommand ones (mining,zmq,rpcdump)

fa3d9ce rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke)
fa32c1d rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke)
faaa46d rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke)
fa93bc1 rpc: Remove unused return type from appendCommand (MarcoFalke)

Pull request description:

  This is split out from bitcoin#18531 to just touch the RPC methods in misc. 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:
    tested ACK fa3d9ce
  promag:
    Code review ACK fa3d9ce.

Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719
…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
…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
@PastaPastaPasta
Copy link
Member

@knst I rebased this for you; range-diff provided below; feel free to no-diff rebase to re-enshrine your GPG sigs on these commits if you'd like

 -:  ---------- >  1:  f5642281cc Merge #20282: wallet: change upgradewallet return type to be an object
 -:  ---------- >  2:  19aba38cab Merge #19200: rpc: remove deprecated getaddressinfo fields
 -:  ---------- >  3:  9c54cb16de Merge #19405: rpc, cli: add network in/out connections to `getnetworkinfo` and `-getinfo`
 1:  6353aae4e9 =  4:  58d923cd5b Merge #19528: rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc)
 2:  8044dd4c91 =  5:  41c35fd8dc fix: adjust missing arguments and help for misc rpc: debug, echo, mnsync
 3:  9950451b37 =  6:  860d31f504 Merge #19455: rpc generate: print useful help and error message
 4:  f99734e3e3 =  7:  7ac1ee0fb4 Merge #19717: rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining,zmq,rpcdump)
 5:  c0439e91ad =  8:  f525f574b0 fix: follow-up missing changes from Merge #18607: rpc: Fix named arguments in documentation
 6:  d631b7a9ba =  9:  c30c8f22dd Merge #19849: Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction)
 7:  a827d5f357 = 10:  af9eb81e56 fix: wrong name of arguments for RPC
 8:  36eb6f6082 ! 11:  0e1a31159f Merge #19994: Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet)
    @@ src/wallet/rpcwallet.cpp: static UniValue upgradewallet(const JSONRPCRequest& re
          if (!wallet) return NullUniValue;
          CWallet* const pwallet = wallet.get();
     @@ src/wallet/rpcwallet.cpp: static UniValue upgradewallet(const JSONRPCRequest& request)
    -         throw JSONRPCError(RPC_WALLET_ERROR, error.original);
    +         obj.pushKV("error", error.original);
          }
    -     return error.original;
    +     return obj;
     +},
     +    };
      }
 9:  106d69a45f = 12:  d0163543d9 refactor: use new format CPCCommand for rpc/coinjoin
10:  efcbc91de1 = 13:  b54f03a0c1 fix: wrong name of argument for coinjoin
11:  6ae994ba36 = 14:  3621966f12 feat: add todo to drop Throw() from rpc/util.h
12:  1f3d7c6b1c = 15:  c5031685bc fix: rename arguments for 'voteraw'

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(self)utACK c503168

Copy link
Collaborator Author

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase LGTM c503168

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@PastaPastaPasta PastaPastaPasta merged commit 1c20180 into dashpay:develop Mar 18, 2024
@knst knst deleted the bp-v21-p26-rpc-args branch March 18, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants