CLI cleanups #31887

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

These have been accumulating over the past few years.

Each is described in its commit message.

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	pablomartin4btc, hodlinator, l0rinc, ryanofsky
Concept ACK	yancyribbens

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

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:

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index defe5e4c4e..18bd6b1f91 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -1267,7 +1267,7 @@ static int CommandLineRPC(int argc, char *argv[])
         gArgs.CheckMultipleCLIArgs();
         std::unique_ptr<BaseRequestHandler> rh;
         std::string method;
-        if (gArgs.IsArgSet("-getinfo")) {
+        if (gArgs.GetBoolArg("-getinfo", false)) {
             rh.reset(new GetinfoRequestHandler());
         } else if (gArgs.GetBoolArg("-netinfo", false)) {
             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

jonatack commented at 8:05 PM on February 22, 2025:

It seems odd to treat -getinfo different than -netinfo (and everything else). It would be good for consistency to treat them the same

It would be nice to remove this IsArgSet call (in a separate commit or PR), because like #30529 https://github.com/bitcoin/bitcoin/pull/31212 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.

Thanks, done.

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

 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.

jonatack commented at 5:45 PM on February 22, 2025:

Thanks for having a look! Replied below in #31887 (review).

hodlinator commented at 11:19 PM on February 22, 2025:

I consider mixing class/struct in the inheritance hierarchy to be a separate issue.

jonatack commented at 8:20 PM on March 12, 2025:

See https://stackoverflow.com/questions/3574040/c-can-a-struct-inherit-from-a-class in addition to my response above.

hodlinator commented at 8:34 PM on March 12, 2025:

My objection does not come from a knowledge gap in what C++ can and cannot do. It is about sticking to certain conventions.

As I was checking to see if the Core Guidelines had influenced me in the matter I only found weak counter evidence: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c122-use-abstract-classes-as-interfaces-when-complete-separation-of-interface-and-implementation-is-needed In the example code class D1 inherits from struct Device, full of virtual methods.

jonatack commented at 8:42 PM on March 12, 2025:

I believe I already addressed the conventions comment with #31887 (review).

hodlinator commented at 10:36 PM on March 12, 2025:

Thread #31887 (review):

I still read #31887 (review) as what can be done technically, not what should be done (convention). What comes close to indicating convention is the SO-link which shows that some random guy on the internet also prioritizes default visibility over other conventions. His example of taking a struct from a C library and inheriting from it into a class is far from convincing to me - it goes against Core Guidelines: "Do not use inheritance when simply having a data member will do." The way you use inheritance is better than his example in this aspect.

jonatack commented at 12:33 AM on March 13, 2025:

Yes, here it seems cleaner, fitting the use case and requiring less code to do the same thing. I appreciate your indulgence here. Edit: and that commit looks even cleaner now after taking your formatting suggestion.

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

 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.

jonatack commented at 5:44 PM on February 22, 2025:

If I'm understanding the feedback, it comes from the idea of C structs being for POD. My understanding is that in C++, classes and structs are identical except for their default inheritance and member access levels, and for the cases here, structs seem to be the appropriate choice that allows simpler code. See also https://stackoverflow.com/a/46339933. I don't mind dropping this change if there is a compelling reason that I'm not aware of, though.

hodlinator commented at 11:48 PM on February 22, 2025:

If I'm understanding the feedback, it comes from the idea of C structs being for POD. My understanding is that in C++, classes and structs are identical except for their default inheritance and member access levels, and for the cases here, structs seem to be the appropriate choice that allows simpler code.

I agree that C++ classes and structs are essentially the same, no confusion there. (C# disallows inheritance for struct but not class, Rust doesn't allow inheritance, period. I know we're in C++, but still aware of the tendencies).

What contributed to giving me a sense that this project attempts to use struct/class as a shorthand for distinct things (like POD/vs non-POD), beyond the code that passed me by so far, was dev-notes.md:

https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/doc/developer-notes.md?plain=1#L70-L71

I want to maintain a fairly equal bar for review and 3252f3bab02605129a568112f650ef3cb29703b7 (the topic of this thread) stands out to me as having been shot down instantly if done by a relative newcomer. I've benefited from your review guide and am sure you've provided and provide much value to the project.

I really hope I can build a reputation for other things than nits. :)

If you insist on keeping structs, maybe hoist up the braces to conform with the dev-notes? Also, you have inconsistent spacing: struct AddrinfoRequestHandler : BaseRequestHandler vs struct GetinfoRequestHandler: BaseRequestHandler

ryanofsky commented at 4:01 PM on February 28, 2025:

re: #31887 (review)

If you insist on keeping structs, maybe hoist up the braces to conform with the dev-notes?

Not important but do agree it would be good to use recommended style for structs with opening brace on the same line (can just run clang-format).

Also agree classes make more sense than structs here. Compilers may not care about the difference but they mean different things to developers. Structs usually provide data definitions while classes provide interface definitions, so they have different uses and follow different conventions. In this case, I can see the appeal of dropping some lines of code though so don't have a strong opinion.

jonatack commented at 8:09 PM on March 12, 2025:

hoist up the braces to conform with the dev-notes? Also...inconsistent spacing

it would be good to use recommended style (can just run clang-format)

Good idea, done.

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

jonatack commented at 8:14 PM on February 22, 2025:

Thank you for taking a look! m_details_level is capped at a maximum value of 4 since https://github.com/bitcoin/bitcoin/pull/21192/files; that line is currently m_details_level = std::min(n, NETINFO_MAX_LEVEL);

yancyribbens commented at 11:34 PM on February 22, 2025:

I think you misunderstood my comment (or I am not understanding yours). min provides both an upper an lower bound here, so it's just bounded. In the commit you say "upper bound" but I'm saying it should just read "bound".

jonatack commented at 8:11 PM on March 12, 2025:

Oh ok, updated the commit message per s/upper bound/bound/ as you suggested.

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

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

pablomartin4btc commented at 2:44 PM on February 25, 2025: member

cr ACK ccc34a26170ef8c63a6f211eda4464e954873e89

Regarding the 3rd commit:

for the exceptions case, perhaps this can be done in a separate PR as I can see other places where we are using class outside CLI if we want to be consistent (and if we think we won't require encapsulation for them).
for the *RequestHandler, are we sure the internal representation of these objects and what they expose won't change at any time in the future? It looks like it should be ok, but I'm not too sure if at some point we would need to.

DrahtBot requested review from ryanofsky on Feb 25, 2025

DrahtBot requested review from yancyribbens on Feb 25, 2025

in src/bitcoin-cli.cpp:1251 in ccc34a2617 outdated

1251 | @@ -1252,7 +1252,7 @@ static int CommandLineRPC(int argc, char *argv[])
1252 |          gArgs.CheckMultipleCLIArgs();
1253 |          std::unique_ptr<BaseRequestHandler> rh;
1254 |          std::string method;
1255 | -        if (gArgs.IsArgSet("-getinfo")) {
1256 | +        if (gArgs.GetBoolArg("-getinfo", false)) {

ryanofsky commented at 2:31 PM on February 28, 2025:

In commit "cli, refactor: for -getinfo, replace IsArgSet() with GetBoolArg()" (ccc34a26170ef8c63a6f211eda4464e954873e89)

Would be good to drop "refactor" from commit subject since this is a small change in behavior, not a pure refactoring. It could be described as a bugfix: no longer treating -nogetinfo and -getinfo=0 the same as -getinfo and -getinfo=1, instead treating them like getinfo was not specified.

jonatack commented at 8:25 PM on March 12, 2025:

Thank you for clarifying the change. Done.

l0rinc commented at 10:58 AM on March 13, 2025:

This is a surprising change, the PR states this is a refactor - I would prefer doing this separately

jonatack commented at 3:01 PM on March 13, 2025:

It was added at the request of multiple reviewers and doesn't seem worth splitting out to another PR for that one line and requiring an additional merge script run. Instead, I've dropped the word "refactor" from the PR title.

ryanofsky approved

ryanofsky commented at 5:27 PM on February 28, 2025: contributor

Code review ACK ccc34a26170ef8c63a6f211eda4464e954873e89. Since last review just added a commit replacing IsArgSet call. I left some suggestions that could be followed up, but they are not very important.

cli, refactor: simplify DetailsRequested()

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

be82139b2a

cli, refactor: deduplicate NetworkStringToId() fdbfd250fb

jonatack commented at 8:32 PM on March 12, 2025: member

Thank you for reviewing, @pablomartin4btc. I prefer to keep this pull scoped only to this file and not increase the scope, as these changes are local to it and it is a file I know reasonably well since 2019 (the RequestHandler representation hasn't changed over that time period).

cli, refactor: simplify public-only classes with structs

and run clang-format on it

e99e41b307

cli, bugfix: for -getinfo, replace IsArgSet() with GetBoolArg()

for consistency with the other CLI commands (-netinfo, -addrinfo, -generate).

This can be considered a bugfix because IsArgSet() returns whether an arg has
been set even if it has been negated. After this change, we no longer treat
-nogetinfo and -getinfo=0 the same as -getinfo and -getinfo=1, and instead as if
-getinfo was not specified.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

d423fd9ec8

jonatack force-pushed on Mar 12, 2025

jonatack commented at 8:39 PM on March 12, 2025: member

Thanks for the reviews, everyone. Ran clang-format on the third commit and adjusted a few commit messages per the feedback. Should be trivial to re-ack (thanks!)

pablomartin4btc commented at 9:04 PM on March 12, 2025: member

re-ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4

DrahtBot requested review from ryanofsky on Mar 12, 2025

in src/bitcoin-cli.cpp:258 in d423fd9ec8

 254 | @@ -259,30 +255,25 @@ static void http_error_cb(enum evhttp_request_error err, void *ctx)
 255 |      reply->error = err;
 256 |  }
 257 |  
 258 | -/** Class that handles the conversion from a command-line to a JSON-RPC request,
 259 | +static int8_t NetworkStringToId(const std::string& str)

hodlinator commented at 10:00 PM on March 12, 2025:

info: The NetworkStringToId() duplication was introduced in db4d2c282afd46709792aaf2d36ffbfc1745b776. Could add a reference in the commit message if you retouch.

jonatack commented at 12:37 AM on March 13, 2025:

Your review comment provides that context (thanks). I wouldn't usually add that to the commit message for the same reason as #31887 (review). In this case, however, where the PR and discussion are locked, maybe it wouldn't add notifications to that PR. Not sure.

in src/bitcoin-cli.cpp:389 in d423fd9ec8

 392 | -        }
 393 | -        return UNKNOWN_NETWORK;
 394 | -    }
 395 |      uint8_t m_details_level{0}; //!< Optional user-supplied arg to set dashboard details level
 396 | -    bool DetailsRequested() const { return m_details_level > 0 && m_details_level < 5; }
 397 | +    bool DetailsRequested() const { return m_details_level; }

hodlinator commented at 10:05 PM on March 12, 2025:

nit: If you re-touch it might be good to add the missing # to the commit message (for tooling/consistency). Unless "PR <number>" is also a convention for tooling unbeknownst to me.

-PR 21192
+PR [#21192](/bitcoin-bitcoin/21192/)

jonatack commented at 12:30 AM on March 13, 2025:

I omit the # in cases like this, where it isn't really important to link them here, so that GitHub does not add a notification in that PR every time this branch is (re)pushed.

hodlinator approved

hodlinator commented at 11:04 PM on March 12, 2025: contributor

Code review ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4

Good to see -getinfo negation behavior fixed. I encountered it when exploring args in #16545#pullrequestreview-2316714013 (in the "Expand/collapse lengthy exploration"-section).

Still think the 3rd commit is out of character compared to my brief experience with this project as it is purely stylistic, but at least the formatting was fixed since my previous review.

in src/bitcoin-cli.cpp:407 in be82139b2a 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; }
 408 | +    bool DetailsRequested() const { return m_details_level; }

l0rinc commented at 10:38 AM on March 13, 2025:

This is better than before, but having a NETINFO_MAX_LEVEL and a checking the bounded values in methods and having DetailsRequested, but also checking it manually in https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L499-L501 where we're checking the raw n value seems quite hacky.

Seems the code is begging for a bounded type - if we're cleaning it up, we might as well try an enum here:

enum class NetinfoDetailLevel : uint8_t {
    BASIC = 0,
    PEERS,
    PEERS_WITH_ADDR,
    PEERS_WITH_VER,
    FULL
};

jonatack commented at 2:29 PM on March 13, 2025:

where we're checking the raw n value

That is to check user input.

I think the abstraction level here is fine, but happy to review if you propose a further abstraction in a follow-up.

in src/bitcoin-cli.cpp:262 in fdbfd250fb outdated

 258 | @@ -259,6 +259,14 @@ static void http_error_cb(enum evhttp_request_error err, void *ctx)
 259 |      reply->error = err;
 260 |  }
 261 |  
 262 | +static int8_t NetworkStringToId(const std::string& str)

l0rinc commented at 10:49 AM on March 13, 2025:

from the usages it seems we can modernize this in a few more ways:

UNKNOWN_NETWORK seems to serve as a value-missing proxy - we could use an optional type instead
const std::string& could be modernized to static std::optional<int8_t> NetworkStringToId(std::string_view network)

This would allow us to change the usages to something like:

if (auto network_id{NetworkStringToId(network_name)}) {
    ++counts.at(*network_id);
}

jonatack commented at 2:45 PM on March 13, 2025:

Yes, IIRC std::optional wasn't available when this was added, as the codebase was using C++11 at that time. I think this would be more of a modernization rewrite than a cleanup, but happy to review if you want to propose it.

in src/bitcoin-cli.cpp:264 in fdbfd250fb outdated

 258 | @@ -259,6 +259,14 @@ static void http_error_cb(enum evhttp_request_error err, void *ctx)
 259 |      reply->error = err;
 260 |  }
 261 |  
 262 | +static int8_t NetworkStringToId(const std::string& str)
 263 | +{
 264 | +    for (size_t i = 0; i < NETWORKS.size(); ++i) {

l0rinc commented at 10:53 AM on March 13, 2025:

we're returning an int8_t, we might as well iterate over one

    for (int8_t i{0}; i < NETWORKS.size(); ++i) {

jonatack commented at 2:39 PM on March 13, 2025:

Not sure but I think this would cause a conversion from size_t to int8_t on each iteration, rather than once for return i. Doesn't seem worth it.

l0rinc commented at 4:15 PM on March 13, 2025:

the problem is likely that the method returns int8_t for an index - unnecessary, should likely be size_t or int - not important

jonatack commented at 4:21 PM on March 13, 2025:

That's true about the index being int.

in src/bitcoin-cli.cpp:276 in e99e41b307 outdated

 278 |  
 279 |  /** Process addrinfo requests */
 280 | -class AddrinfoRequestHandler : public BaseRequestHandler
 281 | -{
 282 | -public:
 283 | +struct AddrinfoRequestHandler : BaseRequestHandler {

l0rinc commented at 10:56 AM on March 13, 2025:

simple structs are fine, but I'm not a fan of struct inheritance, I like the previous version more in the non-trivial cases

jonatack commented at 2:54 PM on March 13, 2025:

If I'm understanding the comment ("simple structs are fine"), it comes from the idea of structs being for POD, say, in C. In C++, classes and structs are identical except for their default inheritance and member access levels, and for the use cases here, a struct appears to be the more appropriate choice that allows simpler, cleaner code (see also stackoverflow.com/a/46339933. Struct inheritance is also fine (see discussions like stackoverflow.com/questions/3574040/c-can-a-struct-inherit-from-a-class. If there is a technical issue beyond style nits, happy to re-evaluate.

l0rinc changes_requested

l0rinc commented at 10:59 AM on March 13, 2025: contributor

I like these cleanups, ans we should go a step further and modernize the code more when touching them. I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"

jonatack renamed this:
~~refactor: CLI cleanups~~
CLI cleanups
on Mar 13, 2025

jonatack commented at 2:57 PM on March 13, 2025: member

I like these cleanups, ans we should go a step further and modernize the code more when touching them. I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"

Thanks for reviewing. The last commit was added at the request of multiple reviewers, but I've removed "refactor" from the pull title. Addressed the other feedback individually and happy to look at any modernization follow-ups you'd like to propose.

l0rinc approved

l0rinc commented at 4:14 PM on March 13, 2025: contributor

I would prefer doing the modernizations here to get rid of more technical debt, but this is already better than before so:

ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4

ryanofsky approved

ryanofsky commented at 4:05 PM on March 23, 2025: contributor

Code review ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4, just running clang-format and updating commit messages since last review

ryanofsky assigned ryanofsky on Mar 23, 2025

ryanofsky merged this on Mar 23, 2025

ryanofsky closed this on Mar 23, 2025

jonatack deleted the branch on Mar 23, 2025