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

willcl-ark · 2024-03-21T10:31:26Z

Closes: #29555

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

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

DrahtBot · 2024-03-21T10:31:29Z

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.

maflcko · 2024-03-21T10:57:53Z

lgtm ACK 69d6fd6

cbergqvist

This is one of the first errors users are likely to encounter, so I support making it a bit more helpful.

cbergqvist · 2024-03-21T11:48:38Z

src/bitcoin-cli.cpp

+        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));


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.

Suggested change

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

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

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.

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

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

Sure, no worries :)

cbergqvist · 2024-03-21T14:59:53Z

src/bitcoin-cli.cpp

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


nit: ++specificity

Suggested change

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

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

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

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.

Adds a string suggestion `bitcoin-cli -help` as an additional source of information. Github-Pull: bitcoin#29687 Rebased-From: 69d6fd6

tdb3

ACK for 69d6fd6

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

ack 69d6fd6

itornaza · 2024-03-29T18:46:22Z

tested ACK 69d6fd6

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

fa3e115 doc: Correct pull request prefix for scripts and tools (MarcoFalke) Pull request description: `script` is confusing, because in the context of Bitcoin, it usually means Bitcoin script (c.f. `CScript` in `script.h`, or pull requests such as #27122 using the prefix). This could be fixed by renaming it to `scripts` (with a plural `s` at the end), however, looking at the current usage `contrib` and `cli` seem more common (#29687, #26953, #26584, #24864, #30074, #29433 ...) ACKs for top commit: fanquake: ACK fa3e115 willcl-ark: ACK fa3e115 hebasto: ACK fa3e115. theuni: ACK fa3e115 Tree-SHA512: fb3a3892ca5f859e590c8a620350c397ef1f9eafd9e174c70ef50095d401a396758d6c93ad41888da8025c41e25e691f30c18f9e974af13597f2266bb2c53b6d

cli: improve bitcoin-cli error when not connected

69d6fd6

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

DrahtBot added the Scripts and tools label Mar 21, 2024

cbergqvist reviewed Mar 21, 2024

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 23, 2024

cli: improve bitcoin-cli error when not connected

c8e1382

Adds a string suggestion `bitcoin-cli -help` as an additional source of information. Github-Pull: bitcoin#29687 Rebased-From: 69d6fd6

tdb3 reviewed Mar 23, 2024

View reviewed changes

MarnixCroes approved these changes Mar 26, 2024

View reviewed changes

fanquake merged commit c407caa into bitcoin:master Apr 2, 2024

maflcko mentioned this pull request May 22, 2024

doc: Correct pull request prefix for scripts and tools #30150

Merged

Pttn added a commit to Pttn/Bitcoin that referenced this pull request Jan 10, 2025

Merge bitcoin#29687

ae1802d

bitcoin locked and limited conversation to collaborators Apr 2, 2025

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

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

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

Uh oh!

Conversation

willcl-ark commented Mar 21, 2024

Uh oh!

DrahtBot commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

maflcko commented Mar 21, 2024

Uh oh!

cbergqvist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

MarnixCroes left a comment

Choose a reason for hiding this comment

Uh oh!

itornaza commented Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Output

Tests

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Mar 21, 2024 •

edited

Loading

itornaza commented Mar 29, 2024 •

edited

Loading