cli: Improve error message on multiwallet cli-side commands #26990

pull pablomartin4btc wants to merge 1 commits into bitcoin:master from pablomartin4btc:bitcoin-cli-generate-validation-when-rpcwallet changing 4 files +15 −5
  1. pablomartin4btc commented at 5:16 am on January 30, 2023: member

    Running a CLI command when multiple wallets are loaded and -rpcwallet is not specified, should return a clearer error.

    Currently in master:

    0$ bitcoin-cli -regtest -generate 1
1error code: -19
2error message:
3Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).
4Try adding "-rpcwallet=<filename>" option to bitcoin-cli command line.

    With this change:

    0$ bitcoin-cli -regtest -generate 1
1error code: -19
2error message:
3Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<walletname> URI path. Or for the CLI, specify the "-rpcwallet=<walletname>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).
  2. DrahtBot commented at 5:16 am on January 30, 2023: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK furszy, maflcko, mzumsande, jonatack, achow101
    Stale ACK andrewtoth, hernanmarino

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. kouloumos commented at 10:48 am on January 30, 2023: contributor

    The behavior that you are describing is no different than running any wallet RPC with multiple loaded wallets but without adding the -rpcwallet= option.
    e.g. bitcoin-cli getnewaddress -rpcwallet=testWallet1 will fail because [options] should precede the command.

    I understand the confusion but I believe that its source is not -generate but our definition of [options]. The help text specifies usage of the cli as:

    0bitcoin-cli [options] <command> [params] Send command to Bitcoin Core

    Usually [options] refer to optional parameters or flags that allow the user to customize the execution of the command to either modify its default behavior or provide additional information.

    And that was true until client-side commands became part of [options]:

    The order of [options] shouldn’t matter, but right now those client-side commands are part of options and in the case of -generate (which accepts arguments) the order matters, therefore it creates confusion.

    I think that a better option could be to decoupled those from [options] . It doesn’t seem right that bitcoin-cli -generate -netinfo -addrinfo -getinfo is a valid command.

  4. pablomartin4btc commented at 8:44 pm on January 31, 2023: member

    Thanks @kouloumos for taking the time to review this PR, I agree with your comments and I appreciate you added more context into it so people could bring other thoughts.

    I just wanted to make a small change first and specifically to the -generate code block but since, as you said, the confusion applies to any other “client-side command” or [options] I could move the validation a bit up in the code around the line where the args variable is set, which contains the [params] of the command (or the args for -generate).

    I got another commit that I could publish that changes ArgsManager in order to accept, for example, -rpcwallet as an independent command or [option] after -generate so bitcoin-cli -generate 3 4 -rpcwallet=X works (so order won’t matter anymore) and I got something similar to the validation you mentioned in your last sentence, but this approach would change the behaviour of the current parsing and even if we can affirm that’s invalid or, as today, it “ignores” any [params] with “-” (slash) at the front when it’s specified after a non-optional argument, I’m not sure (yet) about all the implications this could bring to all the components where the ArgsManager is being used.

  5. pablomartin4btc force-pushed on Feb 1, 2023
  6. pablomartin4btc marked this as ready for review on Feb 2, 2023
  7. hernanmarino commented at 8:04 pm on February 6, 2023: contributor
    tested ACK. Changes provide more feedback to the user on error messages. Works as intended.
  8. fanquake requested review from jonatack on Feb 16, 2023
  9. fanquake renamed this:
    bitcoin:cli add validation to -generate command to avoid confusion when it's used with -rpcwallet
    cli: add validation to -generate command when it's used with -rpcwallet
    on Feb 16, 2023
  10. DrahtBot added the label Scripts and tools on Feb 16, 2023
  11. jonatack commented at 9:35 pm on February 16, 2023: member

    @pablomartin4btc as mentioned in #26990#pullrequestreview-1274911604, I think a general solution would be more helpful regarding the CLI/RPC error messages, for the case when multiple wallets are loaded and the user needs to specify which wallet to use, while also clarifying that the -rpcwallet option needs to be before the CLI command.

    What do you think about an approach like this: https://github.com/jonatack/bitcoin/commits/2023-02-improve-rpcwallet-error-messages

  12. pablomartin4btc force-pushed on Feb 27, 2023
  13. pablomartin4btc commented at 7:19 pm on February 27, 2023: member

    @jonatack thanks for reviewing; as you and @kouloumos recommended and as I commented here, I’ve made the validation more generic for any cli-command moving it up in the code. As part of this validation, I’ve added 3 more checks that were not verified before:

    • duplication of cli-command (at the moment you can specify -rpcwallet many times, only the last one will be taken into account,
    • only 1 cli-command can run at a time (as @kouloumos mentioned here),
    • no params starting with slash “-” will be accepted after a cli-command (this is the case for bitcoin-cli -generate 3 -rpcwallet=xyz)

    I’ve also incorporated @jonatack’s recommendation on the error description for WALLET_NOT_SPECIFIED.

  14. pablomartin4btc requested review from jonatack on Feb 27, 2023
  15. pablomartin4btc force-pushed on Feb 28, 2023
  16. pablomartin4btc force-pushed on Feb 28, 2023
  17. pablomartin4btc renamed this:
    cli: add validation to -generate command when it's used with -rpcwallet
    cli: add validation to cli side commands besides when it's used with -rpcwallet
    on Feb 28, 2023
  18. hernanmarino commented at 4:47 pm on March 1, 2023: contributor

    Light code review ACK 86c0442da27ed92855f9722eb10d94e72b197f2d Also tested ACK the use cases refered by author here The only thing i suggest changing is giving a more descriptive error on the following situation :

    0$ bitcoin-cli -regtest -generate 1 -rpcwallet=tst2
1error: Error: this is not a valid cli-command argument "-rpcwallet=tst2", please check the syntax of the "-generate" command.

    I would only show that error when the extra / invalid parameter does not start with “-” . When starting with “-” , perhaps an error similar to the following might be more useful :

    0$ bitcoin-cli -regtest -generate 1 -rpcwallet=tst2
1error: Error: this is not a valid cli-command argument "-rpcwallet=tst2", if that is a valid option you might try passing it before -generate

    This is just a sample text for the error, it might apply to other options requiring parameters and not only to -generate.

  19. pablomartin4btc commented at 2:23 pm on March 2, 2023: member

    The only thing i suggest changing is giving a more descriptive error on the following situation :

    0$ bitcoin-cli -regtest -generate 1 -rpcwallet=tst2
1error: Error: this is not a valid cli-command argument "-rpcwallet=tst2", if that is a valid option you might try passing it before -generate

    Thanks, I agree with it, updated it.

  20. pablomartin4btc force-pushed on Mar 2, 2023
  21. hernanmarino approved
  22. hernanmarino commented at 3:06 pm on March 2, 2023: contributor
    re ACK 7f5fd1e6999b955dbb0b382bbfdce90a02e1cd8e Looks good to me now
  23. in src/bitcoin-cli.cpp:890 in 7f5fd1e699 outdated 
    886@@ -887,7 +887,8 @@ static void ParseError(const UniValue& error, std::string& strPrint, int& nRet)
887             strPrint += ("error message:\n" + err_msg.get_str());
888         }
889         if (err_code.isNum() && err_code.getInt<int>() == RPC_WALLET_NOT_SPECIFIED) {
890-            strPrint += "\nTry adding \"-rpcwallet=<filename>\" option to bitcoin-cli command line.";
891+            strPrint += " For the CLI, specify the \"-rpcwallet=<filename>\" option before the command";
    andrewtoth commented at 1:09 am on March 7, 2023:
    I’m not sure if this new message makes it more clear. Specifically, “For the CLI” seems like it might confuse users because there’s nothing opposed to that in the message.
    jonatack commented at 2:42 am on March 7, 2023:

    The alternative is provided in the preceding sentence:

    Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<filename> URI path. Or for the CLI, specify the "-rpcwallet=<filename>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).

    The functional test could make that more clear if the full message is used.

    0index 0f2e076fe8b..58f6c593175 100755
1--- a/test/functional/interface_bitcoin_cli.py
2+++ b/test/functional/interface_bitcoin_cli.py
3@@ -28,7 +28,7 @@ JSON_PARSING_ERROR = 'error: Error parsing JSON: foo'
4 BLOCKS_VALUE_OF_ZERO = 'error: the first argument (number of blocks to generate, default: 1) must be an integer value greater than zero'
5 TOO_MANY_ARGS = 'error: too many arguments (maximum 2 for nblocks and maxtries)'
6 WALLET_NOT_LOADED = 'Requested wallet does not exist or is not loaded'
7-WALLET_NOT_SPECIFIED = 'for the CLI, specify the "-rpcwallet=<filename>" option before the command'
8+WALLET_NOT_SPECIFIED = 'Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<filename> URI path. Or for the CLI, specify the "-rpcwallet=<filename>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).'
    pablomartin4btc commented at 10:09 pm on March 9, 2023:

    I’ve updated the test with the full error message, also I’ve changed the text a bit:

    Multiple wallets are loaded. Please specify a wallet by requesting the RPC through the /wallet/<filename> URI path. Using "bitcoin-cli", add the "-rpcwallet=<filename>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).

    pablomartin4btc commented at 11:12 pm on March 12, 2023:

    I’ve re-tweaked it a bit, added a \n as current implementation and just set the error message that comes from the utils in the test’s constant:

    0Multiple wallets are loaded. Please specify a wallet to use by requesting the RPC through the /wallet/<filename> URI path.
1Using "bitcoin-cli", add the "-rpcwallet=<filename>" option before the command.
2Run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded.

    WALLET_NOT_SPECIFIED = ‘Multiple wallets are loaded. Please select a wallet to use’`

    pablomartin4btc commented at 10:07 pm on July 4, 2024:
    I’ve re-considered your preference above @jonatack.
  24. in src/bitcoin-cli.cpp:954 in 7f5fd1e699 outdated 
    886@@ -887,7 +887,8 @@ static void ParseError(const UniValue& error, std::string& strPrint, int& nRet)
887             strPrint += ("error message:\n" + err_msg.get_str());
888         }
889         if (err_code.isNum() && err_code.getInt<int>() == RPC_WALLET_NOT_SPECIFIED) {
890-            strPrint += "\nTry adding \"-rpcwallet=<filename>\" option to bitcoin-cli command line.";
891+            strPrint += " For the CLI, specify the \"-rpcwallet=<filename>\" option before the command";
892+            strPrint += " (run \"bitcoin-cli -h\" for help or \"bitcoin-cli listwallets\" to see which wallets are currently loaded).";
    andrewtoth commented at 1:10 am on March 7, 2023:
    Where does bitcoin-cli -h show how to solve this problem specifically? Telling users to run that here might not be very helpful. The second part could be useful information though.
    jonatack commented at 2:27 am on March 7, 2023:

    Where does bitcoin-cli -h show how to solve this problem specifically?

    ./src/bitcoin-cli -h | grep -A4 rpcwallet

    pablomartin4btc commented at 10:42 pm on March 9, 2023:
    @andrewtoth I’ve updated the error message, check if that looks better for you.
  25. in src/bitcoin-cli.cpp:1089 in 7f5fd1e699 outdated 
    1084+ * @param[in] cliCommand  Reference to const string cli-command.
1085+ * @returns true if cliCommand it's included in the set of the cli-commands.
1086+ */
1087+bool IsExclusivelyCliCommand(const std::string cliCommand)
1088+{
1089+    // Initialize array with cli-commands that run exclusively
    andrewtoth commented at 1:10 am on March 7, 2023:
    nit: Initialize set
    pablomartin4btc commented at 10:12 pm on March 8, 2023:
    fixed
  26. in src/wallet/rpc/util.cpp:94 in 7f5fd1e699 outdated 
    88@@ -89,7 +89,7 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
89             RPC_WALLET_NOT_FOUND, "No wallet is loaded. Load a wallet using loadwallet or create a new one with createwallet. (Note: A default wallet is no longer automatically created)");
90     }
91     throw JSONRPCError(RPC_WALLET_NOT_SPECIFIED,
92-        "Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).");
93+        "Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<filename> URI path.");
    andrewtoth commented at 1:11 am on March 7, 2023: 
    0        "Multiple wallets are loaded. Specify the wallet by requesting the RPC through /wallet/<filename> URI path.");
  27. in test/functional/interface_bitcoin_cli.py:31 in 7f5fd1e699 outdated 
    27@@ -28,7 +28,10 @@
28 BLOCKS_VALUE_OF_ZERO = 'error: the first argument (number of blocks to generate, default: 1) must be an integer value greater than zero'
29 TOO_MANY_ARGS = 'error: too many arguments (maximum 2 for nblocks and maxtries)'
30 WALLET_NOT_LOADED = 'Requested wallet does not exist or is not loaded'
31-WALLET_NOT_SPECIFIED = 'Wallet file not specified'
32+WALLET_NOT_SPECIFIED = 'For the CLI, specify the "-rpcwallet=<filename>" option before the command'
    andrewtoth commented at 1:13 am on March 7, 2023:
    Again, I don’t think “For the CLI” message is useful if it doesn’t have a “as opposed to the “.
    pablomartin4btc commented at 10:16 pm on March 9, 2023:
    I’ve updated the error message as I’ve commented it here.
  28. in test/functional/interface_bitcoin_cli.py:32 in 7f5fd1e699 outdated 
    27@@ -28,7 +28,10 @@
28 BLOCKS_VALUE_OF_ZERO = 'error: the first argument (number of blocks to generate, default: 1) must be an integer value greater than zero'
29 TOO_MANY_ARGS = 'error: too many arguments (maximum 2 for nblocks and maxtries)'
30 WALLET_NOT_LOADED = 'Requested wallet does not exist or is not loaded'
31-WALLET_NOT_SPECIFIED = 'Wallet file not specified'
32+WALLET_NOT_SPECIFIED = 'For the CLI, specify the "-rpcwallet=<filename>" option before the command'
33+INVALID_CLI_COMMAND_ARGUMENT = 'Error: this is not a valid cli-command argument \"{}\", if that is a valid option you might try passing it before \"{}\" command.'
    andrewtoth commented at 1:14 am on March 7, 2023: 
    0INVALID_CLI_COMMAND_ARGUMENT = 'Error: \"{}\" is not a valid cli-command argument. If \"{}\" is a valid option try passing it before the \"{}\" command.'
    pablomartin4btc commented at 9:52 pm on March 8, 2023:
    done
    pablomartin4btc commented at 10:09 pm on March 8, 2023:

    On this one, when I raise the error or when I have to pass the strings for the insertion into the chain, it doesn’t look right to pass twice the same variable (e.g.: INVALID_CLI_COMMAND_ARGUMENT.format(rpcwallet3, rpcwallet3, "-generate").

    Would you accept:

    INVALID_CLI_COMMAND_ARGUMENT = 'Error: invalid cli-command argument. If \"{}\" is a valid option try passing it before the \"{}\" command.'

  29. in test/functional/interface_bitcoin_cli.py:34 in 7f5fd1e699 outdated 
    27@@ -28,7 +28,10 @@
28 BLOCKS_VALUE_OF_ZERO = 'error: the first argument (number of blocks to generate, default: 1) must be an integer value greater than zero'
29 TOO_MANY_ARGS = 'error: too many arguments (maximum 2 for nblocks and maxtries)'
30 WALLET_NOT_LOADED = 'Requested wallet does not exist or is not loaded'
31-WALLET_NOT_SPECIFIED = 'Wallet file not specified'
32+WALLET_NOT_SPECIFIED = 'For the CLI, specify the "-rpcwallet=<filename>" option before the command'
33+INVALID_CLI_COMMAND_ARGUMENT = 'Error: this is not a valid cli-command argument \"{}\", if that is a valid option you might try passing it before \"{}\" command.'
34+DUPLICATE_COMMAND_ERROR = 'Error: duplicate command line argument {}'
35+MULTIPLE_CLI_COMMAND_ERROR = "Error: you can run only one cli-command at a time, please choose one: {} or {}"
    andrewtoth commented at 1:15 am on March 7, 2023: 
    0MULTIPLE_CLI_COMMAND_ERROR = "Error: you can only run one cli-command at a time, either {} or {}"
    pablomartin4btc commented at 9:52 pm on March 8, 2023:
    done
  30. andrewtoth changes_requested
  31. pablomartin4btc force-pushed on Mar 8, 2023
  32. pablomartin4btc force-pushed on Mar 9, 2023
  33. pablomartin4btc force-pushed on Mar 9, 2023
  34. pablomartin4btc force-pushed on Mar 10, 2023
  35. pablomartin4btc force-pushed on Mar 12, 2023
  36. in src/bitcoin-cli.cpp:891 in 0fe610deba outdated 
    886@@ -887,7 +887,8 @@ static void ParseError(const UniValue& error, std::string& strPrint, int& nRet)
887             strPrint += ("error message:\n" + err_msg.get_str());
888         }
889         if (err_code.isNum() && err_code.getInt<int>() == RPC_WALLET_NOT_SPECIFIED) {
890-            strPrint += "\nTry adding \"-rpcwallet=<filename>\" option to bitcoin-cli command line.";
891+            strPrint += "\nUsing \"bitcoin-cli\", add the \"-rpcwallet=<filename>\" option before the command.";
892+            strPrint += "\nRun \"bitcoin-cli -h\" for help or \"bitcoin-cli listwallets\" to see which wallets are currently loaded).";
    andrewtoth commented at 3:02 pm on March 14, 2023: 
    0            strPrint += "\nRun \"bitcoin-cli -h\" for help or \"bitcoin-cli listwallets\" to see which wallets are currently loaded.";
    pablomartin4btc commented at 4:40 pm on March 14, 2023:
    fixed
  37. andrewtoth commented at 3:08 pm on March 14, 2023: contributor
    ACK 0fe610deba336f0370d68c130dc9a223b7db964e modulo the dangling closing parenthesis in the error message. Also, the first line of the commit message should be at most 80 chars.
  38. DrahtBot requested review from hernanmarino on Mar 14, 2023
  39. pablomartin4btc force-pushed on Mar 14, 2023
  40. pablomartin4btc force-pushed on Mar 14, 2023
  41. pablomartin4btc commented at 4:41 pm on March 14, 2023: member

    ACK 0fe610d modulo the dangling closing parenthesis in the error message. Also, the first line of the commit message should be at most 80 chars.

    I’ve corrected them both. Thanks!

  42. pablomartin4btc force-pushed on Mar 14, 2023
  43. hernanmarino approved
  44. hernanmarino commented at 1:48 am on March 16, 2023: contributor
    ACK a870f5affcfb85f91afe989db0824313985158d8
  45. DrahtBot requested review from andrewtoth on Mar 16, 2023
  46. in src/bitcoin-cli.cpp:1158 in a870f5affc outdated 
    1131+            }
1132+
1133+            // Check if the "-" command was recognised by the ArgsManager or interpreted as a parameter
1134+            // e.g.: bitcoin-cli -generate 1 -rpcwallet=xxx, is an invalid call
1135+            if (!gArgs.IsArgSet(command) && exclusivelyCliCommand != "") {
1136+                throw std::runtime_error(strprintf("Error: invalid cli-command argument. If \"%s\" is a valid option try passing it before the \"%s\" command.", argv[i], exclusivelyCliCommand));
    luke-jr commented at 3:14 am on July 27, 2023:
    Is there a reason we shouldn’t just support this? Normally it is okay to pass switch options in any order.
    pablomartin4btc commented at 9:27 pm on October 8, 2023:
    Perhaps we should, this issue has been here for a while it seems (#1044). I’ll ping you with an alternative I’ve mentioned in a comment above (3rd paragraph).
  47. DrahtBot removed review request from andrewtoth on Jul 27, 2023
  48. DrahtBot requested review from andrewtoth on Jul 27, 2023
  49. pablomartin4btc marked this as a draft on Sep 15, 2023
  50. DrahtBot removed review request from andrewtoth on Oct 8, 2023
  51. DrahtBot requested review from andrewtoth on Oct 8, 2023
  52. pablomartin4btc force-pushed on Oct 9, 2023
  53. pablomartin4btc force-pushed on Oct 9, 2023
  54. DrahtBot added the label CI failed on Oct 9, 2023
  55. pablomartin4btc renamed this:
    cli: add validation to cli side commands besides when it's used with -rpcwallet
    cli: improve error message on multiwallet and add validation to cli-side commands
    on Oct 9, 2023
  56. pablomartin4btc marked this as ready for review on Oct 9, 2023
  57. DrahtBot removed the label CI failed on Oct 9, 2023
  58. pablomartin4btc commented at 2:38 pm on October 9, 2023: member

    Rebased.

    Split the changes into 2 commits: 1. improve current message error; 2. New validation.

    Updated PR description accordingly.

  59. luke-jr referenced this in commit 4a313ce293 on Oct 19, 2023
  60. luke-jr referenced this in commit faeba4d9ca on Oct 19, 2023
  61. in src/bitcoin-cli.cpp:1115 in 11e0a80b19 outdated 
    1110+{
1111+    // Initialize set with cli-commands that run exclusively
1112+    const std::set<std::string> cliCommands{"-generate", "-netinfo", "-addrinfo", "-getinfo"};
1113+
1114+    // return if element was found or not
1115+    return (cliCommands.find(cliCommand) != cliCommands.end());
    andrewtoth commented at 6:27 pm on November 6, 2023:
    Here we are using std::set::find and below on line 1141 we are using std::set::count to determine if an element already exists. We should use a consistent method for each. I’m not sure if C++20 is supported in this codebase yet, but we should use std::set::contains if it is.
    pablomartin4btc commented at 6:55 pm on November 6, 2023:

    It makes sense, I’ll use std::set::find instead of std::set::count (L#1141).

    Regarding C++20. it’s not enabled yet but perhaps will be soon (#28349). If this PR doesn’t get merged first I’ll update the code as you suggested so I’m leaving this conversation as unresolved, otherwise I’ll do a follow-up.

    pablomartin4btc commented at 11:56 pm on April 20, 2024:
    @andrewtoth, now I’ve included std::set::contains as you and @jonatack recommended.
  62. andrewtoth commented at 6:28 pm on November 6, 2023: contributor
    style-nit: all variable and function parameter names should be snake_case https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c.
  63. pablomartin4btc commented at 6:49 pm on November 6, 2023: member

    style-nit: all variable and function names should be snake_case https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c.

    Thanks for checking that, you are right, I’ll fix them all ASAP.

  64. pablomartin4btc force-pushed on Nov 9, 2023
  65. pablomartin4btc commented at 6:17 pm on November 9, 2023: member
    I’ve addressed feedback from @andrewtoth - thank you!
  66. luke-jr referenced this in commit efd7ea0f2e on Nov 10, 2023
  67. luke-jr referenced this in commit 17415da9db on Nov 10, 2023
  68. in src/bitcoin-cli.cpp:1106 in 755320f75f outdated 
    1100@@ -1101,6 +1101,68 @@ static UniValue GetNewAddress()
1101     return ConnectAndCallRPC(&rh, "getnewaddress", /* args=*/{}, wallet_name);
1102 }
1103 
1104+/**
1105+ * IsExclusivelyCliCommand checks if a string is a cli-command that needs to run exclusively
1106+ * @param[in] cli_command  Reference to const string cli-command.
    andrewtoth commented at 5:28 pm on December 3, 2023:

    nit: This parameter is not a reference. It is being passed by value. This comment could be more descriptive eg:

    0[@param](/bitcoin-bitcoin/contributor/param/)[in] cli_command The string to check if it is a cli-command that needs to run exclusively.
    pablomartin4btc commented at 9:37 pm on December 3, 2023:
    Thanks, there was an extra space as well.
  69. in src/bitcoin-cli.cpp:1107 in 755320f75f outdated 
    1100@@ -1101,6 +1101,68 @@ static UniValue GetNewAddress()
1101     return ConnectAndCallRPC(&rh, "getnewaddress", /* args=*/{}, wallet_name);
1102 }
1103 
1104+/**
1105+ * IsExclusivelyCliCommand checks if a string is a cli-command that needs to run exclusively
1106+ * @param[in] cli_command  Reference to const string cli-command.
1107+ * @returns true if cli_command it's included in the set of the exclusively_commands.
    andrewtoth commented at 5:29 pm on December 3, 2023:

    nit:

    0* [@returns](/bitcoin-bitcoin/contributor/returns/) true if cli_command is included in the set of the exclusively_commands.
    pablomartin4btc commented at 9:37 pm on December 3, 2023:
    Right, thanks!
  70. in src/bitcoin-cli.cpp:1112 in 755320f75f outdated 
    1107+ * @returns true if cli_command it's included in the set of the exclusively_commands.
1108+ */
1109+bool IsExclusivelyCliCommand(const std::string cli_command)
1110+{
1111+    // Initialize set with cli-commands that run exclusively
1112+    const std::set<std::string> exclusively_commands{"-generate", "-netinfo", "-addrinfo", "-getinfo"};
    andrewtoth commented at 5:29 pm on December 3, 2023:
    nit: could be static.
    pablomartin4btc commented at 9:37 pm on December 3, 2023:
    I’d prefer to keep the definition as is since the function is called only once, thanks for the suggestion.
    andrewtoth commented at 9:55 pm on December 3, 2023:
    Sorry, not sure what you mean here. It looks to me like this function is called for every argument passed to bitcoin-cli, and therefore this set will be initialized for every argument instead of just once.
    pablomartin4btc commented at 10:09 pm on December 3, 2023:
    Right, sorry, I haven’t expressed myself correctly, I meant that the validation was only “used” once but you are right, the IsExclusivelyCliCommand function is called for every argument passed to bitcoin-cli, still, static would keep the set throughout the lifetime of the program.
  71. in src/bitcoin-cli.cpp:1121 in 755320f75f outdated 
    1116+}
1117+
1118+/**
1119+ * ValidateCliCommand checks for:
1120+ *  1. duplicate command line arguments
1121+ *  2. no multiple bitcoin-cli side commands allowance
    andrewtoth commented at 5:35 pm on December 3, 2023:
    nit: 2. multiple bitcoin-cli commands that need to run exclusively
    pablomartin4btc commented at 9:37 pm on December 3, 2023:
    Yeah, your version is clearer. Thanks!
  72. in src/bitcoin-cli.cpp:1122 in 755320f75f outdated 
    1117+
1118+/**
1119+ * ValidateCliCommand checks for:
1120+ *  1. duplicate command line arguments
1121+ *  2. no multiple bitcoin-cli side commands allowance
1122+ *  3. unrecognised cli-command or any other argument starting with a slash
    andrewtoth commented at 5:36 pm on December 3, 2023:
    nit: 3. unrecognized cli-commands or other arguments starting with a slash
    pablomartin4btc commented at 9:37 pm on December 3, 2023:
    Thanks, also removed the following line and tweaked the return statement.
  73. andrewtoth approved
  74. andrewtoth commented at 5:44 pm on December 3, 2023: contributor
    ACK 755320f75f2141909e84b62f420462c1c5b193e6
  75. DrahtBot requested review from hernanmarino on Dec 3, 2023
  76. DrahtBot removed review request from hernanmarino on Dec 3, 2023
  77. DrahtBot requested review from hernanmarino on Dec 3, 2023
  78. DrahtBot removed review request from hernanmarino on Dec 3, 2023
  79. DrahtBot requested review from hernanmarino on Dec 3, 2023
  80. DrahtBot removed review request from hernanmarino on Dec 3, 2023
  81. DrahtBot requested review from hernanmarino on Dec 3, 2023
  82. DrahtBot removed review request from hernanmarino on Dec 3, 2023
  83. DrahtBot requested review from hernanmarino on Dec 3, 2023
  84. DrahtBot removed review request from hernanmarino on Dec 3, 2023
  85. DrahtBot requested review from hernanmarino on Dec 3, 2023
  86. DrahtBot removed review request from hernanmarino on Dec 3, 2023
  87. DrahtBot requested review from hernanmarino on Dec 3, 2023
  88. pablomartin4btc force-pushed on Dec 3, 2023
  89. pablomartin4btc commented at 9:48 pm on December 3, 2023: member
    Updates: addressed @andrewtoth’s feedback, thanks again!
  90. andrewtoth commented at 4:55 pm on January 5, 2024: contributor
    It looks safe to me to replace all uses of std::string with std::string_view in this PR.
  91. pablomartin4btc force-pushed on Jan 8, 2024
  92. pablomartin4btc commented at 6:38 pm on January 8, 2024: member

    It looks safe to me to replace all uses of std::string with std::string_view in this PR.

    ~Addressed most, except for exclusively_command which involves dynamic changes with different string values on each cycle, making it unsuitable for a std::string_view. Therefore, std::string is retained for exclusively_command.~

    edited:

    • Addressed all (on exclusively_command, once it is copied from command, the new std::string_view references the same underlying data, not owning it).
    • Since I was there, I’ve replaced exclusively_command != "" by !exclusively_command.empty(). Thanks!
  93. pablomartin4btc force-pushed on Jan 8, 2024
  94. pablomartin4btc force-pushed on Jan 8, 2024
  95. pablomartin4btc force-pushed on Jan 8, 2024
  96. DrahtBot added the label CI failed on Jan 8, 2024
  97. DrahtBot removed the label CI failed on Jan 8, 2024
  98. DrahtBot added the label CI failed on Jan 13, 2024
  99. in src/bitcoin-cli.cpp:1141 in fa48d46033 outdated 
    1136+                command = command.substr(0, pos);
1137+            }
1138+
1139+            // Check if this command has already been seen
1140+            if (commands.find(command) != commands.end()) {
1141+                throw std::runtime_error(strprintf("Error: duplicate command line argument %s", command));
    jonatack commented at 9:31 pm on February 2, 2024:

    fa48d460334beef43fa73455aa9e30971d33c1b2 This would output error: Error: and the duplicate element would be the bitcoin-cli command.

    0                throw std::runtime_error(strprintf("duplicate bitcoin-cli command %s", command));
    pablomartin4btc commented at 11:52 pm on April 20, 2024:
    It makes sense. Thanks!
  100. in src/bitcoin-cli.cpp:1149 in fa48d46033 outdated 
    1144+            // Check if it's a cli-command that runs exclusively e.g. a user could run:
1145+            // "bitcoin-cli -regtest -generate -netinfo -addrinfo -getinfo" but it's confusing,
1146+            // only the last one will be executed so we make sure the user specify only one
1147+            if (IsExclusivelyCliCommand(command)) {
1148+                if (!exclusively_command.empty()) {
1149+                    throw std::runtime_error(strprintf("Error: you can only run one cli-command at a time, either %s or %s", exclusively_command, command));
    jonatack commented at 9:33 pm on February 2, 2024:

    fa48d460334beef43fa73455aa9e30971d33c1b2 This would output error: Error: and would be clearer as bitcoin-cli command.

    0                    throw std::runtime_error(strprintf("you can only run one bitcoin-cli command at a time, either %s or %s", exclusively_command, command));
    pablomartin4btc commented at 11:53 pm on April 20, 2024:
    Same. Done!
  101. in src/bitcoin-cli.cpp:1157 in fa48d46033 outdated 
    1152+            }
1153+
1154+            // Check if the "-" command was recognised by the ArgsManager or interpreted as a parameter
1155+            // e.g.: bitcoin-cli -generate 1 -rpcwallet=xxx, is an invalid call
1156+            if (!gArgs.IsArgSet(std::string(command)) && !exclusively_command.empty()) {
1157+                throw std::runtime_error(strprintf("Error: invalid cli-command argument. If \"%s\" is a valid option try passing it before the \"%s\" command.", argv[i], exclusively_command));
    jonatack commented at 9:34 pm on February 2, 2024:

    fa48d460334beef43fa73455aa9e30971d33c1b2 This would output error: Error: and would be clearer as bitcoin-cli command.

    0                throw std::runtime_error(strprintf("invalid bitcoin-cli argument; if \"%s\" is a valid option, try passing it before the \"%s\" command", argv[i], exclusively_command));
    pablomartin4btc commented at 11:53 pm on April 20, 2024:
    Same. Done!
  102. in src/bitcoin-cli.cpp:1140 in fa48d46033 outdated 
    1135+            if (pos != std::string::npos) {
1136+                command = command.substr(0, pos);
1137+            }
1138+
1139+            // Check if this command has already been seen
1140+            if (commands.find(command) != commands.end()) {
    jonatack commented at 9:36 pm on February 2, 2024:

    https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 Can now use C++20 std::set#contains method.

    0            if (commands.contains(command)) {
    jonatack commented at 10:18 pm on February 2, 2024:
    Using an unordered set instead of a set, find/contains complexity can be reduced from O(log n) to O(1) - O(n) (constant on average, worst case linear to the size of the container). Remember to add the #include <unordered_set> header in this case.
    pablomartin4btc commented at 0:01 am on April 21, 2024:
    It was also suggested by @andrewtoth, but C++20 wasn’t supported at that time. Done!
  103. in src/bitcoin-cli.cpp:1130 in fa48d46033 outdated 
    1125+static void ValidateCliCommand(int argc, char *argv[])
1126+{
1127+    std::set<std::string_view> commands;
1128+    std::string_view exclusively_command;
1129+    for (int i = 1; i < argc; ++i) {
1130+        if (IsSwitchChar(argv[i][0])) {
    jonatack commented at 9:39 pm on February 2, 2024:

    https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 continue here would be more idiomatic C++ and avoid indenting the next 30 lines.

    0        if (!IsSwitchChar(argv[i][0])) continue;
    pablomartin4btc commented at 0:04 am on April 21, 2024:
    Nice. Done!
  104. in src/bitcoin-cli.cpp:1135 in fa48d46033 outdated 
    1130+        if (IsSwitchChar(argv[i][0])) {
1131+            std::string_view command(argv[i]);
1132+            // Check if the command line argument has an =, in that case, ignore what's after;
1133+            // e.g.: -color=never; we just want to check "-color"
1134+            std::size_t pos = command.find('=');
1135+            if (pos != std::string::npos) {
    jonatack commented at 9:43 pm on February 2, 2024:

    https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 These two lines could be reduced to the following to keep pos scoped to the conditional only (and use string_view::npos and the faster find_first_of that can stop at the first found result).

    0-            std::size_t pos = command.find('=');
1-            if (pos != std::string::npos) {
2+            if (const auto pos{command.find_first_of('=')}; pos != std::string_view::npos) {
3                 command = command.substr(0, pos);
4             }
    pablomartin4btc commented at 0:02 am on April 21, 2024:
    I like it. Done!
  105. in src/bitcoin-cli.cpp:1116 in fa48d46033 outdated 
    1111+    // Initialize set with cli-commands that run exclusively
1112+    const std::set<std::string_view> exclusively_commands{"-generate", "-netinfo", "-addrinfo", "-getinfo"};
1113+
1114+    // return if element was found or not
1115+    return (exclusively_commands.find(cli_command) != exclusively_commands.end());
1116+}
    jonatack commented at 9:54 pm on February 2, 2024:

    https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 This seems like a lot of code for what could be 2 lines instead, and you could make the commands data structure an constexpr array constant and have it run in O(n) instead of O(log n) (and order the commands by probable frequency of invocation). The resulting code is self-explanatory, so could drop the comments too.

     0@@ -53,6 +53,7 @@ using CliClock = std::chrono::system_clock;
 1 
 2+constexpr std::array<std::string_view, 4> CLI_COMMANDS{"-netinfo", "-getinfo", "-addrinfo", "-generate"};
 3 static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
 4
 5@@ -1105,20 +1106,6 @@ static UniValue GetNewAddress()
 6 
 7-/**
 8- * IsExclusivelyCliCommand checks if a string is a cli-command that needs to run exclusively
 9- * [@param](/bitcoin-bitcoin/contributor/param/)[in] cli_command The string to check if it is a cli-command that needs to run exclusively.
10- * [@returns](/bitcoin-bitcoin/contributor/returns/) true if cli_command is included in the set of the exclusively_commands.
11- */
12-bool IsExclusivelyCliCommand(const std::string_view cli_command)
13-{
14-    // Initialize set with cli-commands that run exclusively
15-    const std::set<std::string_view> exclusively_commands{"-generate", "-netinfo", "-addrinfo", "-getinfo"};
16-
17-    // return if element was found or not
18-    return (exclusively_commands.find(cli_command) != exclusively_commands.end());
19-}
20-
21
22@@ -1131,38 +1118,26 @@ static void ValidateCliCommand(int argc, char *argv[])
23-            // Check if it's a cli-command that runs exclusively e.g. a user could run:
24-            // "bitcoin-cli -regtest -generate -netinfo -addrinfo -getinfo" but it's confusing,
25-            // only the last one will be executed so we make sure the user specify only one
26-            if (IsExclusivelyCliCommand(command)) {
27+            if (std::find(std::begin(CLI_COMMANDS), std::end(CLI_COMMANDS), command) != std::end(CLI_COMMANDS)) {
    pablomartin4btc commented at 0:02 am on April 21, 2024:
    It makes more sense, thanks! Done!
  106. in src/bitcoin-cli.cpp:1156 in fa48d46033 outdated 
    1151+                exclusively_command = command;
1152+            }
1153+
1154+            // Check if the "-" command was recognised by the ArgsManager or interpreted as a parameter
1155+            // e.g.: bitcoin-cli -generate 1 -rpcwallet=xxx, is an invalid call
1156+            if (!gArgs.IsArgSet(std::string(command)) && !exclusively_command.empty()) {
    jonatack commented at 9:56 pm on February 2, 2024:

    https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 could do the cheaper check first

    0            if (!exclusively_command.empty() && !gArgs.IsArgSet(std::string(command))) {
    pablomartin4btc commented at 0:02 am on April 21, 2024:
    True. Done!
  107. jonatack commented at 9:59 pm on February 2, 2024: member

    Reviewed the first commit fa48d460334beef43fa73455aa9e30971d33c1b2. It looks like the prefix in its commit name should be cli instead of gui. Will review the second commit soon.

      0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
  1index a53c2a1b0d0..b342382fc96 100644
  2--- a/src/bitcoin-cli.cpp
  3+++ b/src/bitcoin-cli.cpp
  4@@ -53,6 +53,7 @@ using CliClock = std::chrono::system_clock;
  5 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
  6 UrlDecodeFn* const URL_DECODE = urlDecode;
  7 
  8+constexpr std::array<std::string_view, 4> CLI_COMMANDS{"-netinfo", "-getinfo", "-addrinfo", "-generate"};
  9 static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
 10 static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
 11 static constexpr int DEFAULT_WAIT_CLIENT_TIMEOUT = 0;
 12@@ -1105,20 +1106,6 @@ static UniValue GetNewAddress()
 13     return ConnectAndCallRPC(&rh, "getnewaddress", /* args=*/{}, wallet_name);
 14 }
 15 
 16-/**
 17- * IsExclusivelyCliCommand checks if a string is a cli-command that needs to run exclusively
 18- * [@param](/bitcoin-bitcoin/contributor/param/)[in] cli_command The string to check if it is a cli-command that needs to run exclusively.
 19- * [@returns](/bitcoin-bitcoin/contributor/returns/) true if cli_command is included in the set of the exclusively_commands.
 20- */
 21-bool IsExclusivelyCliCommand(const std::string_view cli_command)
 22-{
 23-    // Initialize set with cli-commands that run exclusively
 24-    const std::set<std::string_view> exclusively_commands{"-generate", "-netinfo", "-addrinfo", "-getinfo"};
 25-
 26-    // return if element was found or not
 27-    return (exclusively_commands.find(cli_command) != exclusively_commands.end());
 28-}
 29-
 30 /**
 31  * ValidateCliCommand checks for:
 32  *  1. duplicate command line arguments
 33@@ -1131,38 +1118,26 @@ static void ValidateCliCommand(int argc, char *argv[])
 34     std::set<std::string_view> commands;
 35     std::string_view exclusively_command;
 36     for (int i = 1; i < argc; ++i) {
 37-        if (IsSwitchChar(argv[i][0])) {
 38-            std::string_view command(argv[i]);
 39-            // Check if the command line argument has an =, in that case, ignore what's after;
 40-            // e.g.: -color=never; we just want to check "-color"
 41-            std::size_t pos = command.find('=');
 42-            if (pos != std::string::npos) {
 43-                command = command.substr(0, pos);
 44-            }
 45-
 46-            // Check if this command has already been seen
 47-            if (commands.find(command) != commands.end()) {
 48-                throw std::runtime_error(strprintf("Error: duplicate command line argument %s", command));
 49-            }
 50-
 51-            // Check if it's a cli-command that runs exclusively e.g. a user could run:
 52-            // "bitcoin-cli -regtest -generate -netinfo -addrinfo -getinfo" but it's confusing,
 53-            // only the last one will be executed so we make sure the user specify only one
 54-            if (IsExclusivelyCliCommand(command)) {
 55-                if (!exclusively_command.empty()) {
 56-                    throw std::runtime_error(strprintf("Error: you can only run one cli-command at a time, either %s or %s", exclusively_command, command));
 57-                }
 58-                exclusively_command = command;
 59-            }
 60-
 61-            // Check if the "-" command was recognised by the ArgsManager or interpreted as a parameter
 62-            // e.g.: bitcoin-cli -generate 1 -rpcwallet=xxx, is an invalid call
 63-            if (!gArgs.IsArgSet(std::string(command)) && !exclusively_command.empty()) {
 64-                throw std::runtime_error(strprintf("Error: invalid cli-command argument. If \"%s\" is a valid option try passing it before the \"%s\" command.", argv[i], exclusively_command));
 65+        if (!IsSwitchChar(argv[i][0])) continue;
 66+        std::string_view command(argv[i]);
 67+        if (const auto pos{command.find_first_of('=')}; pos != std::string_view::npos) {
 68+            command = command.substr(0, pos);
 69+        }
 70+        if (commands.contains(command)) {
 71+            throw std::runtime_error(strprintf("duplicate bitcoin-cli command %s", command));
 72+        }
 73+        if (std::find(std::begin(CLI_COMMANDS), std::end(CLI_COMMANDS), command) != std::end(CLI_COMMANDS)) {
 74+            if (!exclusively_command.empty()) {
 75+                throw std::runtime_error(strprintf("you can only run one bitcoin-cli command at a time, either %s or %s", exclusively_command, command));
 76             }
 77-
 78-            commands.insert(command);
 79+            exclusively_command = command;
 80+        }
 81+        // Check if the "-" command was recognised by the ArgsManager or interpreted as a parameter
 82+        // e.g.: bitcoin-cli -generate 1 -rpcwallet=xxx, is an invalid call
 83+        if (!exclusively_command.empty() && !gArgs.IsArgSet(std::string(command))) {
 84+            throw std::runtime_error(strprintf("invalid bitcoin-cli argument; if \"%s\" is a valid option, try passing it before the \"%s\" command", argv[i], exclusively_command));
 85         }
 86+        commands.insert(command);
 87     }
 88 }
 89 
 90diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py
 91index 2a64c78f216..057e4a2f389 100755
 92--- a/test/functional/interface_bitcoin_cli.py
 93+++ b/test/functional/interface_bitcoin_cli.py
 94@@ -29,9 +29,9 @@ BLOCKS_VALUE_OF_ZERO = 'error: the first argument (number of blocks to generate,
 95 TOO_MANY_ARGS = 'error: too many arguments (maximum 2 for nblocks and maxtries)'
 96 WALLET_NOT_LOADED = 'Requested wallet does not exist or is not loaded'
 97 WALLET_NOT_SPECIFIED = "Multiple wallets are loaded. Please specify a wallet to use"
 98-INVALID_CLI_COMMAND_ARGUMENT = 'Error: invalid cli-command argument. If \"{}\" is a valid option try passing it before the \"{}\" command.'
 99-DUPLICATE_COMMAND_ERROR = 'Error: duplicate command line argument {}'
100-MULTIPLE_CLI_COMMAND_ERROR = "Error: you can only run one cli-command at a time, either {} or {}"
101+INVALID_CLI_COMMAND_ARGUMENT = 'error: invalid bitcoin-cli argument; if \"{}\" is a valid option, try passing it before the \"{}\" command'
102+DUPLICATE_COMMAND_ERROR = 'error: duplicate bitcoin-cli command {}'
103+MULTIPLE_CLI_COMMAND_ERROR = "error: you can only run one bitcoin-cli command at a time, either {} or {}"
104 
105 
106 def cli_get_info_string_to_dict(cli_get_info_string):
107@@ -327,7 +327,7 @@ class TestBitcoinCli(BitcoinTestFramework):
108             self.log.info('Test -generate with bad args & without -rpcwallet in multiwallet mode')
109             assert_raises_rpc_error(-19, WALLET_NOT_SPECIFIED, self.nodes[0].cli('-generate', 1, 2, 3).echo)
110 
111-            self.log.info('Test running multiple cli-commands in one line')
112+            self.log.info('Test running multiple CLI commands in one line')
  108. in src/bitcoin-cli.cpp:911 in bf48c40c0e outdated 
    907@@ -908,7 +908,8 @@ static void ParseError(const UniValue& error, std::string& strPrint, int& nRet)
908             strPrint += ("error message:\n" + err_msg.get_str());
909         }
910         if (err_code.isNum() && err_code.getInt<int>() == RPC_WALLET_NOT_SPECIFIED) {
911-            strPrint += "\nTry adding \"-rpcwallet=<filename>\" option to bitcoin-cli command line.";
912+            strPrint += "\nUsing \"bitcoin-cli\", add the \"-rpcwallet=<filename>\" option before the command.";
    jonatack commented at 8:19 pm on February 8, 2024:
    In commit bf48c40c0ee62e, I suggest the documentation in https://github.com/jonatack/bitcoin/commit/610397665795b702e380f879db43a000ed81aad3 (have updated it to use the best elements of both bf48c40c0ee62e and https://github.com/jonatack/bitcoin/commit/135395c885e72444d9e90b495b101525da827650).
    pablomartin4btc commented at 11:51 pm on April 20, 2024:
    I’ve made a combination of current state (master) and your improvements, based on the feedback provided by @andrewtoth (1, 2). Please let me know what you think.
    jonatack commented at 9:13 pm on May 24, 2024:

    I prefer the documentation in https://github.com/jonatack/bitcoin/commit/610397665795b702e380f879db43a000ed81aad3 because it makes clear that for the CLI a different method is needed (-rpcwallet).

    (Please see also #26990 (review). The CLI error message is appended to the RPC error and isn’t in isolation. The functional tests should verify that.)

    pablomartin4btc commented at 9:35 am on July 1, 2024:
    Ok, I’ll work on the full error message verification in the test and will consider your preference on the error documentation. Thanks again!
    pablomartin4btc commented at 10:20 pm on July 4, 2024:
    Done.
  109. jonatack commented at 8:34 pm on February 8, 2024: member
    Approach ACK modulo the review suggestions.
  110. hernanmarino commented at 10:14 pm on February 13, 2024: contributor
    re ACK fa48d460334beef43fa73455aa9e30971d33c1b2 Just a nit (only if you update) : your second commit title seems to suggest that it’s gui related, instead of cli. Also, and although not invalidating for me , I think that @jonatack ’s suggestions are worth following.
  111. DrahtBot requested review from andrewtoth on Feb 13, 2024
  112. DrahtBot requested review from jonatack on Feb 13, 2024
  113. pablomartin4btc commented at 10:39 pm on February 13, 2024: member

    re ACK fa48d46 Just a nit (only if you update) : your second commit title seems to suggest that it’s gui related, instead of cli.

    Yeah, there was an unconscious typo somewhere since the beginning and I think it’s what @jonatack meant on his latest review as well.

    Also, and although not invalidating for me , I think that @jonatack ’s suggestions are worth following.

    That’s the plan, cheers.

  114. DrahtBot removed review request from andrewtoth on Feb 13, 2024
  115. DrahtBot requested review from andrewtoth on Feb 13, 2024
  116. maflcko commented at 6:18 pm on April 17, 2024: member
    Are you still working on this?
  117. pablomartin4btc commented at 6:57 pm on April 17, 2024: member

    Are you still working on this?

    I’m about to continue, sorry for the delay!

  118. pablomartin4btc commented at 1:56 pm on April 19, 2024: member

    Updates:

    • I’ve been working on @jonatack’s latest feedback, I’m performing some tests at the moment and will update the code soon.
  119. pablomartin4btc force-pushed on Apr 19, 2024
  120. DrahtBot removed the label CI failed on Apr 19, 2024
  121. pablomartin4btc commented at 0:14 am on April 21, 2024: member

    Updates:

    • Addressed feedback provided by @jonatack regarding first commit 6898219043f42ae42e4703c549e656f56d276d13.
    • Updated second commit cf7dd3564a3ace4153a32930f36bd78432b59097 following some directions given by @jonatack and discussed with @andrewtoth.
      • Fixed incorrect commit title prefix (gui: => cli:).
  122. jonatack commented at 4:54 pm on May 15, 2024: member

    Addressed feedback provided by @jonatack regarding first commit 6898219.

    Updated second commit cf7dd35 following some directions given by @jonatack and discussed with @andrewtoth.

    Thank you – just saw this, will review.

  123. in src/bitcoin-cli.cpp:1136 in cf7dd3564a outdated 
    1131+            command = command.substr(0, pos);
1132+        }
1133+
1134+        // Check if this command has already been seen
1135+        if (commands.contains(command)) {
1136+            throw std::runtime_error(strprintf("duplicate bitcoin-cli command %s.", command));
    jonatack commented at 9:16 pm on May 24, 2024:

    nit, here and lines 1141 and 1149, can drop the trailing . as the other CLI error messages don’t have one

    0            throw std::runtime_error(strprintf("duplicate bitcoin-cli command %s", command));
    pablomartin4btc commented at 9:31 am on July 1, 2024:
    Ok, thanks, will do.
    pablomartin4btc commented at 10:20 pm on July 4, 2024:
    Done.
  124. jonatack commented at 9:21 pm on May 24, 2024: member

    Approach ACK, modulo #26990 (review) and the other comment.

    Also, “protocol.h” in the first commit message body seems wrong or out-of-date here: bringing the error message in line with the definition established in protocol.h ("error when there are multiple wallets loaded"). I’m not sure, but did you mean “src/wallet/rpc/util.cpp” instead?

    Edit: Apologies for how long I took to re-review, will react quickly on this next iteration :)

  125. pablomartin4btc commented at 8:44 pm on July 1, 2024: member

    Also, “protocol.h” in the first commit message body seems wrong or out-of-date here: bringing the error message in line with the definition established in protocol.h ("error when there are multiple wallets loaded"). I’m not sure, but did you mean “src/wallet/rpc/util.cpp” instead?

    The reference to “src/rpc/protocol.h” was intended to clarify the context of the error message alignment, specifically in the section of the code where the RPC error code “not wallet specified” is defined (enum RPCErrorCode) as related to the issue of multiple wallets being loaded. Here’s the relevant code snippet for reference:

    https://github.com/bitcoin/bitcoin/blob/fe70be537783aeadf9e3f72cad07efd66775285b/src/rpc/protocol.h#L82

    However, if it adds more confusion than clarity, I can certainly remove it.

  126. pablomartin4btc referenced this in commit 6103976657 on Jul 4, 2024
  127. pablomartin4btc force-pushed on Jul 4, 2024
  128. pablomartin4btc force-pushed on Jul 4, 2024
  129. pablomartin4btc force-pushed on Jul 4, 2024
  130. DrahtBot commented at 11:15 pm on July 4, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/27062656083

  131. DrahtBot added the label CI failed on Jul 4, 2024
  132. pablomartin4btc force-pushed on Jul 5, 2024
  133. pablomartin4btc commented at 0:16 am on July 5, 2024: member

    Updates:

  134. DrahtBot removed the label CI failed on Jul 5, 2024
  135. jonatack commented at 2:11 pm on July 12, 2024: member

    Thanks for updating! I think 6ccf7cb946bd3772b5fba92246be7e2281f59d09 is good to go.

    WIP review on commit 3d63fc976d616436d64335b15a918ffba1883b9a but I initially find it confusing. The diffs below are between your commit and testing some modifications to it.

    For instance, the tests that pass a wallet CLI option to a non-wallet CLI command (-generate) seem to be mixing issues under test…

    0-            # Single or Multi wallet modes don't matter here as -generate command validation is performed before we send to RPC
1-            # Note also that -rpcwallet arg intentionally adds some confusion but could be any arg starting with "-"
2-            self.log.info('Test -generate with nblocks and -rpcwallet afterwards')
3-            assert_raises_process_error(1, INVALID_CLI_COMMAND_ARGUMENT.format(rpcwallet3, "-generate"), self.nodes[0].cli('-generate', 1, rpcwallet3).send_cli)
4+            self.log.info('Test -generate followed by a CLI option (invalid order of arguments)')
5+            assert_raises_process_error(1, INVALID_CLI_COMMAND_ARGUMENT.format("-rpcwait", "-generate"), self.nodes[0].cli('-generate', 4, 1000, "-rpcwait").send_cli)

    …and at first glance I’m not sure if this is the correct behavior, but I need to look at it more

    0-            # Note, same as above, -rpcwallet arg intentionally adds some confusion but could be any arg starting with "-"
1-            self.log.info('Test -generate -rpcwallet with nblocks and -rpcwallet again afterwards')
2-            assert_raises_process_error(1, DUPLICATE_COMMAND_ERROR.format("-rpcwallet"), self.nodes[0].cli(rpcwallet3, '-generate', 1, rpcwallet3).send_cli)
3+            self.log.info('Test -generate with a duplicate CLI command')
4+            assert_raises_process_error(1, DUPLICATE_COMMAND_ERROR.format("-rpcwait"), self.nodes[0].cli('-rpcwait', '-generate', 4, 1000, "-rpcwait").send_cli)

    Will finishing reviewing that commit.

  136. pablomartin4btc commented at 1:22 pm on July 15, 2024: member

    For instance, the tests that pass a wallet CLI option to a non-wallet CLI command (-generate) seem to be mixing issues under test…

    0-            # Single or Multi wallet modes don't matter here as -generate command validation is performed before we send to RPC
1-            # Note also that -rpcwallet arg intentionally adds some confusion but could be any arg starting with "-"
2-            self.log.info('Test -generate with nblocks and -rpcwallet afterwards')
3-            assert_raises_process_error(1, INVALID_CLI_COMMAND_ARGUMENT.format(rpcwallet3, "-generate"), self.nodes[0].cli('-generate', 1, rpcwallet3).send_cli)
4+            self.log.info('Test -generate followed by a CLI option (invalid order of arguments)')
5+            assert_raises_process_error(1, INVALID_CLI_COMMAND_ARGUMENT.format("-rpcwait", "-generate"), self.nodes[0].cli('-generate', 4, 1000, "-rpcwait").send_cli)

    Thanks for double checking the tests. For the one above, the idea was to test the proper use case of the issue described at the very top of the PR’s desciption, would fail with any “-” option, as explained, I’ve also clarified that could add confusion but it was intentional. Again, if you think it adds more confusion, I’m happy to remove it (or keep it) and add your suggestion anyways.

    …and at first glance I’m not sure if this is the correct behavior, but I need to look at it more

    0-            # Note, same as above, -rpcwallet arg intentionally adds some confusion but could be any arg starting with "-"
1-            self.log.info('Test -generate -rpcwallet with nblocks and -rpcwallet again afterwards')
2-            assert_raises_process_error(1, DUPLICATE_COMMAND_ERROR.format("-rpcwallet"), self.nodes[0].cli(rpcwallet3, '-generate', 1, rpcwallet3).send_cli)
3+            self.log.info('Test -generate with a duplicate CLI command')
4+            assert_raises_process_error(1, DUPLICATE_COMMAND_ERROR.format("-rpcwait"), self.nodes[0].cli('-rpcwait', '-generate', 4, 1000, "-rpcwait").send_cli)

    In this case, similar to the previous, without taking into account the order, wanted to use -rpcwallet option but could take (replace/ add?) the -rpcwait case as you are suggesting.

  137. DrahtBot added the label CI failed on Sep 6, 2024
  138. DrahtBot commented at 11:54 am on September 15, 2024: contributor
    Needs rebase, if still relevant.
  139. jonatack commented at 9:26 pm on September 16, 2024: member
    On my short list to re-review. Edit: will wait for an update for #30148 or for #26990 (comment).
  140. pablomartin4btc commented at 9:39 pm on September 16, 2024: member

    On my short list to re-review.

    Thanks, need to rebase but first I have to remove the validation for the CLI commands that seems it has been improved in #30148 which I missed to review.

  141. jonatack commented at 9:52 pm on September 16, 2024: member
    (I think this could be merged much more quickly if it only contained 6ccf7cb946bd3772b5fba92246be7e2281f59d09 and proposed the validation in a separate pull.)
  142. pablomartin4btc commented at 10:20 pm on September 16, 2024: member

    (I think this could be merged much more quickly if it only contained 6ccf7cb and proposed the validation in a separate pull.)

    I do agree and thought many times about it haha… I’ll split it out tomorrow. Thanks.

  143. rpc, cli: improve error message on multiwallet mode 
    The primary objective is to provide users with clearer
and more informative error messages when encountering
the RPC_WALLET_NOT_SPECIFIED error, which occurs when
multiple wallets are loadad.

This commit also rectifies the error message consistency
by bringing the error message in line with the definition
established in protocol.h ("error when there are multiple
wallets loaded").
    54227e681a
  144. in src/bitcoin-cli.cpp:950 in 6ccf7cb946 outdated 
    946@@ -947,7 +947,8 @@ static void ParseError(const UniValue& error, std::string& strPrint, int& nRet)
947             strPrint += ("error message:\n" + err_msg.get_str());
948         }
949         if (err_code.isNum() && err_code.getInt<int>() == RPC_WALLET_NOT_SPECIFIED) {
950-            strPrint += "\nTry adding \"-rpcwallet=<filename>\" option to bitcoin-cli command line.";
951+            strPrint += " Or for the CLI, specify the \"-rpcwallet=<filename>\" option before the command";
    mzumsande commented at 11:33 pm on September 16, 2024:
    I found “filename” confusing, see #30912 - if there isn’t a deeper reason behind it that I’m unaware of, it could be renamed to walletname.
    jonatack commented at 0:13 am on September 17, 2024:
    Good point. I was curious, as I’ve moved this error message around a few times for different refactorings; it looks like it originated back in #10931.
    jonatack commented at 0:18 am on September 17, 2024:
    @pablomartin4btc maybe a test could be added to interface_bitcoin_cli.py that passing a filename (edit: like “wallet.dat”) to the CLI raises (error code -18 “Requested wallet does not exist or is not loaded”).
    furszy commented at 0:41 am on September 17, 2024:

    I think it would be beneficial to explain the alternatives. The -rpcwallet parameter depends on how the wallet is loaded. It can either be the wallet directory name located inside the -walletdir (the default value is datadir/net/wallets/wallet_name, in which case only wallet_name is used in this option), or the absolute path to the wallet directory, which is the complete path datadir/net/wallets/wallet_name.

    You can note the difference by loading an existing wallet in two different ways:

    1. loadwallet "test_wallet"
    2. loadwallet "datadir/regtest/wallets/test_wallet"

    Then, call getwalletinfo(). The wallet name will differ, and as a result, the -rpcwallet=<arg> argument for the very same wallet will be different as well.

    mzumsande commented at 1:34 am on September 17, 2024:

    … and I think in both of these cases walletname would be correct and filename incorrect, it would just be a different wallet name depending on how it’s loaded.

    It could actually also be a filename: Put a legacy wallet oldwallet.dat into the datadir, run with -deprecatedrpc=create_bdb and loadwallet "oldwallet.dat" will load it successfully, in this special case the filename is the walletname. But even here, where filename would make sense, walletname is correct too, so it just seem better overall.

  145. pablomartin4btc force-pushed on Sep 17, 2024
  146. pablomartin4btc renamed this:
    cli: improve error message on multiwallet and add validation to cli-side commands
    cli: Improve error message on multiwallet cli-side commands
    on Sep 17, 2024
  147. pablomartin4btc commented at 7:39 pm on September 17, 2024: member

    Updates:

    • Removed validation commit which I’ll added in a separate PR as suggested by @jonatack.
    • Updated description and title accordingly with above.
    • Added test checking -rpcwallet=<filename> does not work as suggested by @jonatack.
    • Replaced -rpcwallet=<filename> which is incorrect with -rpcwallet=<walletname> as suggested by @mzumsande.
  148. furszy commented at 8:39 pm on September 17, 2024: member
    utACK 54227e681a4
  149. DrahtBot requested review from jonatack on Sep 17, 2024
  150. DrahtBot requested review from hernanmarino on Sep 17, 2024
  151. DrahtBot removed the label CI failed on Sep 17, 2024
  152. maflcko commented at 8:18 am on September 18, 2024: member
    review ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
  153. mzumsande commented at 4:01 pm on September 18, 2024: contributor
    Code Review ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
  154. in test/functional/interface_bitcoin_cli.py:339 in 54227e681a 
    335@@ -331,6 +336,10 @@ def run_test(self):
336             n4 = 10
337             blocks = self.nodes[0].getblockcount()
338 
339+            self.log.info('Test -generate -rpcwallet=<filename> raise RPC error')
    jonatack commented at 4:12 pm on September 18, 2024:

    nit if you retouch: s/raise/raises/ and perhaps explain the difference between the failing filename passed and the wallet name

    0            self.log.info('Test -generate -rpcwallet=<filename> ("wallet.dat" instead of "wallet") raises RPC error')
    pablomartin4btc commented at 11:00 pm on September 19, 2024:
    I’ll do this on the follow-up where I’ll be adding the validation from the 2nd commit that was dropped from this PR.
  155. jonatack commented at 4:14 pm on September 18, 2024: member
    ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
  156. jonatack commented at 4:16 pm on September 18, 2024: member
    @pablomartin4btc maybe further update (or just shorten) the PR description (also, please don’t use @-prefixed github names in PR descriptions, as these can cause many notifications for those concerned; an update to the merge script now warns maintainers if a merge message contains a @username, but it’s better not to add them to begin with).
  157. jonatack commented at 5:12 pm on September 18, 2024: member
    PR description much improved now, thanks.
  158. achow101 commented at 3:48 pm on September 20, 2024: member
    ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
  159. achow101 merged this on Sep 20, 2024
  160. achow101 closed this on Sep 20, 2024


pablomartin4btc DrahtBot kouloumos hernanmarino jonatack andrewtoth luke-jr maflcko mzumsande furszy achow101


andrewtoth hernanmarino

Labels
Scripts and tools

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