cli: restrict multiple exclusive argument usage in bitcoin-cli

naiyoma commented at 7:41 pm on May 21, 2024: contributor

This PR is a continuation of the validation suggested here to ensure that only one Request Handler can be specified at a time.

DrahtBot commented at 7:41 pm on May 21, 2024: 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	tdb3, stratospher, achow101
Concept ACK	theuni, kristapsk

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28802 (ArgsManager: support subcommand-specific options by ajtowns)
#26988 (cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency by stratospher)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Scripts and tools on May 21, 2024

theuni commented at 9:01 pm on May 21, 2024: member

Concept ACK.

This is very close to @Brotcrunsher’s original commit. At the very least they should be a co-author.

kristapsk commented at 0:44 am on May 22, 2024: contributor

Concept ACK

naiyoma force-pushed on May 22, 2024

naiyoma commented at 8:03 am on May 22, 2024: contributor

This is very close to @Brotcrunsher’s original commit. At the very least they should be a co-author.

I’ve added the co-author.

in src/bitcoin-cli.cpp:1179 in 628d2d4173 outdated

1174@@ -1175,6 +1175,12 @@ static int CommandLineRPC(int argc, char *argv[])
1175                 fputc('\n', stdout);
1176             }
1177         }
1178+        if (gArgs.IsArgSet("-getinfo") +
1179+            gArgs.GetBoolArg("-netinfo", false) +

tdb3 commented at 3:42 pm on May 25, 2024:

What’s the rationale for using IsArgSet() for getinfo but GetBoolArg() for the others? Maybe I’m overlooking something simple.

Tried the following locally and didn’t see any issues with it:

 0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1index 4002424af0..b682544fa2 100644
 2--- a/src/bitcoin-cli.cpp
 3+++ b/src/bitcoin-cli.cpp
 4@@ -1175,10 +1175,11 @@ static int CommandLineRPC(int argc, char *argv[])
 5                 fputc('\n', stdout);
 6             }
 7         }
 8+
 9         if (gArgs.IsArgSet("-getinfo") +
10-            gArgs.GetBoolArg("-netinfo", false) +
11-            gArgs.GetBoolArg("-generate", false) +
12-            gArgs.GetBoolArg("-addrinfo", false) > 1) {
13+            gArgs.IsArgSet("-netinfo") +
14+            gArgs.IsArgSet("-generate") +
15+            gArgs.IsArgSet("-addrinfo") > 1) {
16             throw std::runtime_error("Only one of \"-getinfo\", \"-netinfo\", \"-generate\", and \"-addrinfo\" may be specified.");
17         }
18         std::unique_ptr<BaseRequestHandler> rh;

naiyoma commented at 9:17 pm on May 30, 2024:

I wasn’t keen on choosing between IsArgSet() and GetBoolArg() since both achieve the same desired outcome, but you are certainly right; using either of the two, for all would be a better approach.

maflcko commented at 9:28 pm on May 30, 2024:

No, they are not the same. One checks whether the arg is set at all (it could be set to 0), the other checks if the arg is set to 1.

in src/bitcoin-cli.cpp:1181 in 628d2d4173 outdated

1174@@ -1175,6 +1175,12 @@ static int CommandLineRPC(int argc, char *argv[])
1175                 fputc('\n', stdout);
1176             }
1177         }
1178+        if (gArgs.IsArgSet("-getinfo") +
1179+            gArgs.GetBoolArg("-netinfo", false) +
1180+            gArgs.GetBoolArg("-generate", false) +
1181+            gArgs.GetBoolArg("-addrinfo", false) > 1) {

tdb3 commented at 3:43 pm on May 25, 2024:

The implicit conversions from bool to int seem safe (at least at first glance, https://en.cppreference.com/w/cpp/language/implicit_conversion), but I didn’t get a change to dig into C++ standard docs to check. It is a tiny bit unintuitive for a reader, but this doesn’t seem too cryptic and seems cleaner than the separate if statements and temp variable used in 27815.

naiyoma commented at 9:05 pm on May 30, 2024:

Thanks for pointing this out, the reference specifies an implicit conversion from bool to int is unsafe for T::operator bool() const;, but for this scenario, since we are not using a custom class it could be safe

tdb3 approved

tdb3 commented at 3:46 pm on May 25, 2024: contributor

ACK for 628d2d4173660c1ad35b2d84524f71b92593d7cd

Thanks for picking this up. Built and ran all functional tests (all passed). Manually executed bitcoin-cli with one and multiple instances of the argument group covered by this PR. The error was seen (as expected) if more than one argument of the group was specified.

Left a few notes.

DrahtBot requested review from theuni on May 25, 2024

ajtowns commented at 8:11 am on May 27, 2024: contributor

Rather than checking each one of these options individually, why not have an OptionsCategory for them, and iterate over that? That has the advantage that it lists them separately when you invoke bitcoin-cli -help, eg:

 0...
 1  -testnet
 2       Use the test chain. Equivalent to -chain=test.
 3
 4CLI Commands:
 5
 6  -addrinfo
 7       Get the number of addresses known to the node, per network and total,
 8       after filtering for quality and recency. The total number of
 9       addresses known to the node may be higher.
10
11  -generate
12       ...
13
14  -getinfo
15       ...
16
17  -netinfo
18       ...

Example patches at https://github.com/ajtowns/bitcoin/commits/202405-clihelpers/ . Example output:

0$ ./bitcoin-cli -netinfo -getinfo
1error: Only one of -getinfo, -netinfo may be specified.
2$ ./bitcoin-cli -netinfo=3 -getinfo -addrinfo
3error: Only one of -addrinfo, -getinfo, -netinfo may be specified.

naiyoma commented at 7:55 pm on June 9, 2024: contributor

Rather than checking each one of these options individually, why not have an OptionsCategory for them, and iterate over that? That has the advantage that it lists them separately when you invoke bitcoin-cli -help,

Thanks for this suggestion, I have applied the patch and pushed an update

tdb3 commented at 2:20 am on June 10, 2024: contributor

Looks like commits 628d2d4173660c1ad35b2d84524f71b92593d7cd and ac0d2e474b5df93c8e5824d4d326eda977aade3d overlap a bit. For example:

https://github.com/bitcoin/bitcoin/blob/628d2d4173660c1ad35b2d84524f71b92593d7cd/src/bitcoin-cli.cpp#L1178-L1182

How about squashing some of these changes to help keep the master branch commit log cleaner?

naiyoma force-pushed on Jun 12, 2024

naiyoma commented at 8:49 pm on June 12, 2024: contributor

How about squashing some of these changes to help keep the master branch commit log cleaner?

Done

DrahtBot added the label CI failed on Jun 13, 2024

DrahtBot commented at 5:56 am on June 13, 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/26150993024

naiyoma force-pushed on Jun 24, 2024

DrahtBot removed the label CI failed on Jun 24, 2024

naiyoma force-pushed on Jun 24, 2024

in src/common/args.cpp:656 in d2e86117bc outdated

651@@ -634,6 +652,9 @@ std::string ArgsManager::GetHelpMessage() const
652             case OptionsCategory::REGISTER_COMMANDS:
653                 usage += HelpMessageGroup("Register Commands:");
654                 break;
655+            case OptionsCategory::CLI_COMMANDS:
656+                usage += HelpMessageGroup("Cli Commands:");

stratospher commented at 8:31 am on June 26, 2024:

b382bf4: nit: s/Cli/CLI

stratospher commented at 6:16 am on June 27, 2024: contributor

tested ACK d2e8611.

2 more suggestions:

include new OptionsCategory in the fuzz test too - https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/src/test/fuzz/system.cpp#L56
add functional test coverage for this error in test/functional/interface_bitcoin_cli.py

DrahtBot requested review from tdb3 on Jun 27, 2024

tdb3 commented at 6:14 pm on June 28, 2024: contributor

How about squashing some of these changes to help keep the master branch commit log cleaner?

Done

Looks like b382bf4755e2b503cb1ee2f87a7b0f66dc5b76f4 adds the if, then d2e86117bc6004515ec72e55b2ec57304a31c0eb removes it. This seems extraneous. Could these commits be squashed?

0        if (gArgs.IsArgSet("-getinfo") +
1            gArgs.GetBoolArg("-netinfo", false) +
2            gArgs.GetBoolArg("-generate", false) +
3            gArgs.GetBoolArg("-addrinfo", false) > 1) {
4            throw std::runtime_error("Only one of \"-getinfo\", \"-netinfo\", \"-generate\", and \"-addrinfo\" may be specified.");
5        }

naiyoma force-pushed on Jul 26, 2024

naiyoma commented at 6:09 pm on July 26, 2024: contributor

add functional test coverage for this error in test/functional/interface_bitcoin_cli.py @stratospher added two assertions here https://github.com/bitcoin/bitcoin/pull/30148/commits/36b21dd8b68b0cb5cbf26c0401181139ef0d91fe

in test/functional/interface_bitcoin_cli.py:385 in 36b21dd8b6 outdated

380@@ -381,6 +381,10 @@ def run_test(self):
381         assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcwait', '-rpcwaittimeout=5').echo)
382         assert_greater_than_or_equal(time.time(), start_time + 5)
383 
384+        self.log.info("Test that only one of -addrinfo, -generate, -getinfo, -netinfo may be specified at a time")
385+        assert_raises_process_error(1, "Only one of -getinfo, -netinfo may be specified", self.nodes[0].cli('-getinfo', '-netinfo').send_cli)

stratospher commented at 9:49 am on July 27, 2024:

36b21dd: nit: I think any 1 assertion would suffice since there’s no extra coverage from the second assertion?

naiyoma commented at 1:45 pm on July 29, 2024:

initially added two assertions to cover different edge cases, but I see how that can be redundant, I’ve deleted one.

stratospher commented at 9:51 am on July 27, 2024: contributor

reACK 36b21dd.

naiyoma force-pushed on Jul 29, 2024

in src/common/args.h:373 in 563f4126f2 outdated

363@@ -363,6 +364,12 @@ class ArgsManager
364         m_network_only_args.clear();
365     }
366 
367+    /**
368+     * Check CLI command args
369+     * (throws std::runtime_error when multiple CLI_COMMAND arguments are specified)
370+     */
371+    void CheckMultipleCLIArgs() const;
372+

tdb3 commented at 0:44 am on July 31, 2024:

nit: If other changes are being made, could use @throws doxygen syntax instead.

Currently renders as:

Using @throws for the exception:

 0diff --git a/src/common/args.h b/src/common/args.h
 1index 73a4cca1441..f2489ea7446 100644
 2--- a/src/common/args.h
 3+++ b/src/common/args.h
 4@@ -366,7 +366,8 @@ protected:
 5 
 6     /**
 7      * Check CLI command args
 8-     * (throws std::runtime_error when multiple CLI_COMMAND arguments are specified)
 9+     * 
10+     * [@throws](/bitcoin-bitcoin/contributor/throws/) std::runtime_error when multiple CLI_COMMAND arguments are specified
11      */
12     void CheckMultipleCLIArgs() const;

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-doxygen-compatible-comments

naiyoma commented at 12:00 pm on August 6, 2024:

thanks, added https://github.com/bitcoin/bitcoin/pull/30148/commits/8838c4f171f3c3e568d237b216167fddb521d0a7

tdb3 approved

tdb3 commented at 0:49 am on July 31, 2024: contributor

re ACK 563f4126f20504aa582c58c87547f02c8820c5fa Tested by executing a few combinations to see that exclusivity is enforced. Appeared to work well:

0  src/bitcoin-cli -addrinfo -generate
1  src/bitcoin-cli -addrinfo -generate -netinfo
2  src/bitcoin-cli -addrinfo -generate -netinfo -getinfo
3  src/bitcoin-cli -addrinfo -generate -getinfo
4  src/bitcoin-cli -addrinfo -netinfo -getinfo
5  src/bitcoin-cli -generate -netinfo -getinfo
6  src/bitcoin-cli -generate -netinfo

DrahtBot requested review from stratospher on Jul 31, 2024

naiyoma force-pushed on Aug 6, 2024

common/args.h: automate check for multiple cli commands

Co-authored-by: Anthony Towns <aj@erisian.com.au>

8838c4f171

test: restrict multiple CLI arguments c8e6771af0

naiyoma force-pushed on Aug 6, 2024

tdb3 approved

tdb3 commented at 3:14 pm on August 10, 2024: contributor

cr re ACK c8e6771af002eaf15567794fcdc57fdb0e3fb140 Diff showed minor changes to comment header of CheckMultipleCLIArgs()

stratospher commented at 4:30 pm on August 26, 2024: contributor

reACK c8e6771.

achow101 commented at 6:20 pm on September 4, 2024: member

ACK c8e6771af002eaf15567794fcdc57fdb0e3fb140

achow101 merged this on Sep 4, 2024

achow101 closed this on Sep 4, 2024

TheCharlatan referenced this in commit 69282950aa on Sep 16, 2024

TheCharlatan referenced this in commit dfe0cd4ec5 on Sep 16, 2024

bitcoin deleted a comment on Oct 18, 2024

cli: restrict multiple exclusive argument usage in bitcoin-cli #30148

Code Coverage

Reviews

Conflicts