cli: Improve error message on multiwallet cli-side commands

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

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.

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]:

First,-getinfo was introduced with #10871 as a way to offer client-side the capabilities of the now removed getinfo RPC (history of getinfo removal).
Then -generate was added with #19133 based on the same reasoning, as a way to offer a client-side version of the deprecated generate RPC.
And then followed by the additions of -netinfo and -addinfo

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.

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.

pablomartin4btc force-pushed on Feb 1, 2023

pablomartin4btc marked this as ready for review on Feb 2, 2023

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.

fanquake requested review from jonatack on Feb 16, 2023

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

DrahtBot added the label Scripts and tools on Feb 16, 2023

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

pablomartin4btc force-pushed on Feb 27, 2023

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.

pablomartin4btc requested review from jonatack on Feb 27, 2023

pablomartin4btc force-pushed on Feb 28, 2023

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

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.

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.

pablomartin4btc force-pushed on Mar 2, 2023

hernanmarino approved

hernanmarino commented at 3:06 pm on March 2, 2023: contributor

re ACK 7f5fd1e6999b955dbb0b382bbfdce90a02e1cd8e Looks good to me now

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.

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.

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

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

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.

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

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

andrewtoth changes_requested

pablomartin4btc force-pushed on Mar 8, 2023

pablomartin4btc force-pushed on Mar 9, 2023

pablomartin4btc force-pushed on Mar 10, 2023

pablomartin4btc force-pushed on Mar 12, 2023

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

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.

DrahtBot requested review from hernanmarino on Mar 14, 2023

pablomartin4btc force-pushed on Mar 14, 2023

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!

pablomartin4btc force-pushed on Mar 14, 2023

hernanmarino approved

hernanmarino commented at 1:48 am on March 16, 2023: contributor

ACK a870f5affcfb85f91afe989db0824313985158d8

DrahtBot requested review from andrewtoth on Mar 16, 2023

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

DrahtBot removed review request from andrewtoth on Jul 27, 2023

DrahtBot requested review from andrewtoth on Jul 27, 2023

pablomartin4btc marked this as a draft on Sep 15, 2023

DrahtBot removed review request from andrewtoth on Oct 8, 2023

DrahtBot requested review from andrewtoth on Oct 8, 2023

pablomartin4btc force-pushed on Oct 9, 2023

DrahtBot added the label CI failed on Oct 9, 2023

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

pablomartin4btc marked this as ready for review on Oct 9, 2023

DrahtBot removed the label CI failed on Oct 9, 2023

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.

luke-jr referenced this in commit 4a313ce293 on Oct 19, 2023

luke-jr referenced this in commit faeba4d9ca on Oct 19, 2023

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.

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.

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.

pablomartin4btc force-pushed on Nov 9, 2023

pablomartin4btc commented at 6:17 pm on November 9, 2023: member

I’ve addressed feedback from @andrewtoth - thank you!

luke-jr referenced this in commit efd7ea0f2e on Nov 10, 2023

luke-jr referenced this in commit 17415da9db on Nov 10, 2023

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.

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!

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.

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!

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.

andrewtoth approved

andrewtoth commented at 5:44 pm on December 3, 2023: contributor

ACK 755320f75f2141909e84b62f420462c1c5b193e6