cli: improve bitcoin-cli error when not connected #29687

pull willcl-ark wants to merge 1 commits into bitcoin:master from willcl-ark:improve-cli-error changing 1 files +4 −1

willcl-ark commented at 10:31 AM on March 21, 2024: member

Closes: #29555

Simply adds an additional suggestion to check bitcoin-cli -help.

cli: improve bitcoin-cli error when not connected

Adds a string suggestion `bitcoin-cli -help` as an additional source of
information.

69d6fd676e

DrahtBot commented at 10:31 AM on March 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 maflcko, tdb3, itornaza

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Scripts and tools on Mar 21, 2024
maflcko commented at 10:57 AM on March 21, 2024: member

lgtm ACK 69d6fd676e9c15ef41a03722d51ed0b13c3e4320

Type	Reviewers
ACK	maflcko, tdb3, itornaza

in src/bitcoin-cli.cpp:833 in 69d6fd676e

 826 | @@ -827,7 +827,10 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
 827 |          if (response.error != -1) {
 828 |              responseErrorMessage = strprintf(" (error code %d - \"%s\")", response.error, http_errorstring(response.error));
 829 |          }
 830 | -        throw CConnectionFailed(strprintf("Could not connect to the server %s:%d%s\n\nMake sure the bitcoind server is running and that you are connecting to the correct RPC port.", host, port, responseErrorMessage));
 831 | +        throw CConnectionFailed(strprintf("Could not connect to the server %s:%d%s\n\n"
 832 | +                    "Make sure the bitcoind server is running and that you are connecting to the correct RPC port.\n"
 833 | +                    "Use \"bitcoin-cli -help\" for more info.",
 834 | +                    host, port, responseErrorMessage));

cbergqvist commented at 11:48 AM on March 21, 2024:

3 tabs eh? :) I suggest conforming with the throw-statement following the one being changed - putting the initial string literal on a new line and keep to one tab indentation.

        throw CConnectionFailed(strprintf(
            "Could not connect to the server %s:%d%s\n\n"
            "Make sure the bitcoind server is running and that you are connecting to the correct RPC port.\n"
            "Use \"bitcoin-cli -help\" for more info.",
            host, port, responseErrorMessage));

willcl-ark commented at 3:38 PM on March 21, 2024:

clang-format-diff has other ideas:

        throw CConnectionFailed(strprintf("Could not connect to the server %s:%d%s\n\n"
                                          "Make sure the bitcoind server is running and that you are connecting to the correct RPC port.\n"
                                          "Use \"bitcoin-cli -help\" for more info.",
                                          host, port, responseErrorMessage));

Not sure where the 3 tabs came from, as my check script is supposed to run that... I guess was my editor default somehow though.

maflcko commented at 4:29 PM on March 21, 2024:

@cbergqvist Generally, the style is left to the pull request author and usually not changed, unless someone else touches it. Also, tabs \t are not allowed in C++ code in this repo. If there were any, the CI should have failed.

cbergqvist commented at 6:02 PM on March 21, 2024:

I regret the initial tone of my comment but I'm happy clang-format came up with something reasonable.

maflcko commented at 6:51 PM on March 21, 2024:

Sure, no worries :)

in src/bitcoin-cli.cpp:832 in 69d6fd676e

 826 | @@ -827,7 +827,10 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
 827 |          if (response.error != -1) {
 828 |              responseErrorMessage = strprintf(" (error code %d - \"%s\")", response.error, http_errorstring(response.error));
 829 |          }
 830 | -        throw CConnectionFailed(strprintf("Could not connect to the server %s:%d%s\n\nMake sure the bitcoind server is running and that you are connecting to the correct RPC port.", host, port, responseErrorMessage));
 831 | +        throw CConnectionFailed(strprintf("Could not connect to the server %s:%d%s\n\n"
 832 | +                    "Make sure the bitcoind server is running and that you are connecting to the correct RPC port.\n"
 833 | +                    "Use \"bitcoin-cli -help\" for more info.",

cbergqvist commented at 2:59 PM on March 21, 2024:

nit: ++specificity

                    "Use \"bitcoin-cli -help\" for more connection info.",

willcl-ark commented at 3:39 PM on March 21, 2024:

I think "connection info" might be less accurate here?

cbergqvist commented at 6:12 PM on March 21, 2024:

Was thinking the user might be missing -rpcport, -rpcconnect or -signet (different default port), but not -generate -netinfo or -stdin. Maybe -signet is not something one would classify as "connection info" though, making it less accurate as you say.

cbergqvist commented at 3:01 PM on March 21, 2024: contributor

This is one of the first errors users are likely to encounter, so I support making it a bit more helpful.
luke-jr referenced this in commit c8e1382472 on Mar 23, 2024
tdb3 commented at 3:34 PM on March 23, 2024: contributor
ACK for 69d6fd676e9c15ef41a03722d51ed0b13c3e4320

Good light-touch approach to address Issue #29555. Executed, and the output looks good:
```
$ src/bitcoin-cli help
error: timeout on transient error: Could not connect to the server 127.0.0.1:8332

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
Use "bitcoin-cli -help" for more info.
```
Just to be thorough: Pulled the pr branch, built, ran all unit and functional tests (all passed) before executing.
MarnixCroes approved
MarnixCroes commented at 7:58 AM on March 26, 2024: contributor

ack 69d6fd676e9c15ef41a03722d51ed0b13c3e4320
itornaza commented at 6:46 PM on March 29, 2024: contributor
tested ACK 69d6fd676e9c15ef41a03722d51ed0b13c3e4320

This a reasonable way for both reporting the connection failure, plus guiding the user on how to get more help on bitcoin-cli.

Output
- The output upon trying to connect without having the bitcoind already running is confirmed:
```
% src/bitcoin-cli help
error: timeout on transient error: Could not connect to the server 127.0.0.1:18332

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
Use "bitcoin-cli -help" for more info.
```
Tests
- Configured with --with-incompatible-bdb and --enable-suppress-external-warnings
- Run unit tests with make check and all tests pass
- Run all functional tests at test/functional/test_runner.py --extended and all tests pass
fanquake merged this on Apr 2, 2024
fanquake closed this on Apr 2, 2024
fanquake referenced this in commit 6f36624147 on May 22, 2024
Pttn referenced this in commit ae1802d062 on Jan 10, 2025
bitcoin locked this on Apr 2, 2025