Truthier error message when rpcpassword is missing#5318
Truthier error message when rpcpassword is missing#5318laanwj merged 1 commit intobitcoin:masterfrom
Conversation
|
Nit: 'bitcoin-core' isn't very well defined, and if it is, it already includes bitcoind. Writing bitcoin-qt would be more correct. |
|
@sipa Fixed, thanks. (Is best practice around here to push another commit to the PR, or replace my old commit?) |
|
You can update your old commit. Unless there are significant portions of finished and reviewed work, but you're just adding something mostly independent. |
|
A unifide error message is also better because it's more easily found via search. ACK. |
There was a problem hiding this comment.
I'd suggest: To use bitcoind, or the -server option with bitcoin-qt.
Perhaps also bitcoin-qt should be replaced by Bitcoin Core GUI?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Nit: -server is an option to bitcoind: you can do -server=0 to run without RPC
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
killall -SIGINT bitcoind shuts down cleanly, afaik.
There was a problem hiding this comment.
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)
|
And it makes the code simpler too! great, |
77c38bb Truthier error message when rpcpassword is missing (Glenn Willen)
5 participants
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.)