refactor: CLI cleanups

jonatack commented at 7:45 pm on February 16, 2025: member

These are simple and have been accumulating over the past few years.

Each is described in its commit message.

cli, refactor: simplify DetailsRequested()

The upper bound check is no longer needed after the merge of PR 21192.

176cd02785

cli, refactor: deduplicate NetworkStringToId() 9184ec9e8e

DrahtBot commented at 7:45 pm on February 16, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31887.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept ACK	yancyribbens
Stale ACK	hodlinator

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:

#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 Refactoring on Feb 16, 2025

cli, refactor: simplify public-only classes with structs 3252f3bab0

jonatack force-pushed on Feb 16, 2025

DrahtBot added the label CI failed on Feb 16, 2025

DrahtBot removed the label CI failed on Feb 17, 2025

in src/bitcoin-cli.cpp:1 in 3252f3bab0

maflcko commented at 2:57 pm on February 18, 2025:

As the pull title is “CLI cleanups”, just a random comment: It seems odd to treat -getinfo different than -netinfo (and everything else). It would be good for consistency to treat them the same:

 0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1index defe5e4c4e..18bd6b1f91 100644
 2--- a/src/bitcoin-cli.cpp
 3+++ b/src/bitcoin-cli.cpp
 4@@ -1267,7 +1267,7 @@ static int CommandLineRPC(int argc, char *argv[])
 5         gArgs.CheckMultipleCLIArgs();
 6         std::unique_ptr<BaseRequestHandler> rh;
 7         std::string method;
 8-        if (gArgs.IsArgSet("-getinfo")) {
 9+        if (gArgs.GetBoolArg("-getinfo", false)) {
10             rh.reset(new GetinfoRequestHandler());
11         } else if (gArgs.GetBoolArg("-netinfo", false)) {
12             if (!args.empty() && (args.at(0) == "h" || args.at(0) == "help")) {

Unrelated, negation probably does not make sense for any of them, so ArgsManager::DISALLOW_NEGATION could be set.

But up to you. Feel free to ignore.

ryanofsky commented at 9:43 pm on February 18, 2025:

re: #31887 (review)

Mostly agree with this suggestion. It would be nice to remove this IsArgSet call (in a separate commit or PR), because like many other IsArgSet calls, this one introduces a bug and doesn’t handle negation properly. A common sense interpretation of -nogetinfo would be to not run the getinfo command instead of running it.

Only part of suggestion above I’d disagree with is the part about also adding the DISALLOW_NEGATION flag. I think the DISALLOW_NEGATION flag is useful for non-boolean options accepting string/int/path/list values where not having a value isn’t supported or doesn’t make sense (e.g. -nodatadir). For other options, it is better IMO to stay consistent and allow negation so code does not fill up with special cases and users do not have to guess which options allow negation and which will trigger errors for no particular reason.

yancyribbens commented at 10:40 pm on February 18, 2025:

It would be nice to remove this IsArgSet call (in a separate commit or PR)

Looks like that was followed up on already? https://github.com/bitcoin/bitcoin/pull/31896

in src/bitcoin-cli.cpp:133 in 3252f3bab0

129@@ -130,14 +130,11 @@ static void libevent_log_cb(int severity, const char *msg)
130 // Exception thrown on connection error.  This error is used to determine
131 // when to wait if -rpcwait is given.
132 //
133-class CConnectionFailed : public std::runtime_error
134+struct CConnectionFailed : std::runtime_error

hodlinator commented at 3:38 pm on February 18, 2025:

Weird to change inheritance from class -> class to struct -> class.

in src/bitcoin-cli.cpp:270 in 3252f3bab0

263@@ -267,21 +264,19 @@ static int8_t NetworkStringToId(const std::string& str)
264     return UNKNOWN_NETWORK;
265 }
266 
267-/** Class that handles the conversion from a command-line to a JSON-RPC request,
268+/** Handle the conversion from a command-line to a JSON-RPC request,
269  * as well as converting back to a JSON object that can be shown as result.
270  */
271-class BaseRequestHandler
272+struct BaseRequestHandler

hodlinator commented at 3:43 pm on February 18, 2025:

Makes more sense to use classes for these as they use virtual functions. Not worth changing just to be able to remove public IMO.

hodlinator commented at 3:50 pm on February 18, 2025: contributor

Code reviewed 3252f3bab02605129a568112f650ef3cb29703b7

ACK 9184ec9e8ea6efd4d0d3625a06f223156f940243

Advise dropping 3rd commit, the others seem good.

ryanofsky approved

ryanofsky commented at 9:52 pm on February 18, 2025: contributor

Code review ACK 3252f3bab02605129a568112f650ef3cb29703b7. All changes look good. No real opinion on the third commit, seems fine to keep or drop

DrahtBot requested review from hodlinator on Feb 18, 2025

in src/bitcoin-cli.cpp:407 in 176cd02785 outdated

403@@ -404,7 +404,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
404         return UNKNOWN_NETWORK;
405     }
406     uint8_t m_details_level{0}; //!< Optional user-supplied arg to set dashboard details level
407-    bool DetailsRequested() const { return m_details_level > 0 && m_details_level < 5; }

yancyribbens commented at 10:47 pm on February 18, 2025:

The upper bound check is no longer needed after the merge of PR 21192.

Nit, looks like the bound check is for upper and lower bounds (above zero and bellow 5).

yancyribbens commented at 10:53 pm on February 18, 2025: contributor

Concept ACK https://github.com/bitcoin/bitcoin/commit/3252f3bab02605129a568112f650ef3cb29703b7

refactor: CLI cleanups #31887

Code Coverage & Benchmarks

Reviews

Conflicts