Truthier error message when rpcpassword is missing #5318

pull gwillen wants to merge 1 commits into bitcoin:master from gwillen:master changing 1 files +1 −7

gwillen commented at 11:22 PM on November 19, 2014: contributor

See #5178 -- this change makes the error slightly more honest / less confusing. The message was always being displayed as "To use the -server option". The check for "-server" did not work as intended, because SoftSetBoolArg is used to enable -server anytime we run as bitcoind, so there's no way for this code to tell whether -server was actually passed by the user or not.

If there's some nice way to tell whether we're bitcoind or bitcoin-core, we could customize the error message accordingly; but it's probably not worth adding code in order to do that, so I've made a single error message that covers both.

I have removed all references to -daemon because as far as I can tell that flag has no effect on whether RPC is enabled or not. (RPC-enabling happens only on init.cpp:768, as far as I can tell, and is conditional only on whether -server is set, which happens if the user passes "-server", or if we are bitcoind. I couldn't find any way for -daemon to affect this.)

Previously, one of the possible messages was localized and the other was not. I have left the message localized, but I don't know what else I might need to touch since I changed it. (gmaxwell opined to me that it's better not to localize error messages at all; I'm happy to remove that here if it's undesired.)
sipa commented at 11:24 PM on November 19, 2014: member

Nit: 'bitcoin-core' isn't very well defined, and if it is, it already includes bitcoind. Writing bitcoin-qt would be more correct.
gwillen commented at 11:30 PM on November 19, 2014: contributor

@sipa Fixed, thanks. (Is best practice around here to push another commit to the PR, or replace my old commit?)
sipa commented at 11:33 PM on November 19, 2014: member

You can update your old commit. Unless there are significant portions of finished and reviewed work, but you're just adding something mostly independent.
Truthier error message when rpcpassword is missing 77c38bb5cc
gwillen force-pushed on Nov 19, 2014
gmaxwell commented at 1:44 AM on November 20, 2014: contributor

A unifide error message is also better because it's more easily found via search. ACK.
in src/rpcserver.cpp:None in 77c38bb5cc
```
 570 | -            strWhatAmI = strprintf(_("To use the %s option"), "\"-server\"");
 571 | -        else if (mapArgs.count("-daemon"))
 572 | -            strWhatAmI = strprintf(_("To use the %s option"), "\"-daemon\"");
 573 |          uiInterface.ThreadSafeMessageBox(strprintf(
 574 | -            _("%s, you must set a rpcpassword in the configuration file:\n"
 575 | +            _("To use bitcoind, or the -server option to bitcoin-qt, you must set an rpcpassword in the configuration file:\n"
```
Diapolo commented at 12:00 PM on November 20, 2014:

I'd suggest: To use bitcoind, or the -server option with bitcoin-qt. Perhaps also bitcoin-qt should be replaced by Bitcoin Core GUI?

gmaxwell commented at 4:23 PM on November 20, 2014:

IMO authors choice here, but I think to is more pedantically correct than with. (-server isn't actually an option to bitcoind and it kind of makes it sould like you have the option of bitcoind without -server in that clause).

laanwj commented at 4:42 PM on November 20, 2014:

Nit: -server is an option to bitcoind: you can do -server=0 to run without RPC

gwillen commented at 10:44 PM on November 20, 2014:

Could reword it to "To use bitcoind, or 'bitcoin-qt -server'", to use fewer and perhaps clearer words. (That's assuming that the binary can be invoked as bitcoin-qt on all platforms; I don't know how you invoke it on windows. I don't know that it makes sense to say Bitcoin Core Gui, since anybody who's passing commandline arguments to it will be familiar with the name of the binary.)

(I'm assuming the nit was directed purely at gmaxwell; the -server option to bitcoind isn't relevant to the wording of this error message, I think.)

Failing that, I think I'll just stick with my original wording barring major objections.

gmaxwell commented at 12:04 AM on November 21, 2014:

I guess the point of the nit is that potentially you could bypass the rpc password requirement by running bitcoind -server=0 ... not that doing so is all that useful. It's a pretty bad configuration though, since you have no way to cleanly shut down the daemon even. :)

sipa commented at 12:08 AM on November 21, 2014:

killall -SIGINT bitcoind shuts down cleanly, afaik.

gmaxwell commented at 12:09 AM on November 21, 2014:

I stand corrected. (I think it should be TERM, since a unix norm is to term then kill, but I don't think we reliably shutdown fast enough for that to help)

sipa commented at 12:10 AM on November 21, 2014:

We shutdown on either of them.
laanwj commented at 12:53 PM on November 21, 2014: member

And it makes the code simpler too! great, Tested ACK 77c38bb5cc7349b5b0159f04e769f863e76aad7b https://dev.visucore.com/bitcoin/acks/5318
laanwj merged this on Nov 21, 2014
laanwj closed this on Nov 21, 2014
laanwj referenced this in commit cb83af9937 on Nov 21, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#5178 bitcoind and -server error message at the first start