Skip to content

Commit 11f68cc

Browse files
committed
Merge bitcoin/bitcoin#31212: util: Improve documentation and negation of args
95a0104 test: Add tests for directories in place of config files (Hodlinator) e85abe9 args: Catch directories in place of config files (Hodlinator) e4b6b18 test: Add tests for -noconf (Hodlinator) 483f0da args: Properly support -noconf (Hodlinator) 312ec64 test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning (Hodlinator) 7402658 test: -norpccookiefile (Hodlinator) 39cbd4f args: Support -norpccookiefile for bitcoind and bitcoin-cli (Hodlinator) e82ad88 logs: Use correct path and more appropriate macros in cookie-related code (Hodlinator) 6e28c76 test: Harden testing of cookie file existence (Hodlinator) 75bacab test: combine_logs.py - Output debug.log paths on error (Hodlinator) bffd92f args: Support -nopid (Hodlinator) 12f8d84 args: Disallow -nodatadir (Hodlinator) 6ff9662 scripted-diff: Avoid printing version information for -noversion (Hodlinator) e8a2054 doc args: Document narrow scope of -color (Hodlinator) Pull request description: - Document `-color` as only applying to `-getinfo`, to be less confusing for bitcoin-cli users. - No longer print version information when getting passed `-noversion`. - Disallow `-nodatadir` as we cannot run without one. It was previously interpreted as a mix of unset and as a relative path of "0". - Support `-norpccookiefile` - Support `-nopid` - Properly support `-noconf` (instead of working by accident). Also detect when directories are specified instead of files. Prompted by investigation in bitcoin/bitcoin#16545 (review). ACKs for top commit: l0rinc: utACK 95a0104 achow101: ACK 95a0104 ryanofsky: Code review ACK 95a0104. Looks good! Thanks for all your work on this breaking the changes down and making them simple. Tree-SHA512: 5174251e6b9196a9c6d135eddcb94130295c551bcfccc78e633d9e118ff91523b1be0d72828fb49603ceae312e6e1f8ee2651c6a2b9e0f195603a73a9a622785
2 parents 893ccea + 95a0104 commit 11f68cc

16 files changed

+212
-75
lines changed

src/bitcoin-cli.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ static void SetupCliArgs(ArgsManager& argsman)
8282

8383
argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
8484
argsman.AddArg("-conf=<file>", strprintf("Specify configuration file. Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
85-
argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
85+
argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
8686
argsman.AddArg("-generate",
8787
strprintf("Generate blocks, equivalent to RPC getnewaddress followed by RPC generatetoaddress. Optional positional integer "
8888
"arguments are number of blocks to generate (default: %s) and maximum iterations to try (default: %s), equivalent to "
@@ -94,7 +94,7 @@ static void SetupCliArgs(ArgsManager& argsman)
9494
argsman.AddArg("-netinfo", strprintf("Get network peer connection information from the remote server. An optional argument from 0 to %d can be passed for different peers listings (default: 0). If a non-zero value is passed, an additional \"outonly\" (or \"o\") argument can be passed to see outbound peers only. Pass \"help\" (or \"h\") for detailed help documentation.", NETINFO_MAX_LEVEL), ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
9595

9696
SetupChainParamsBaseOptions(argsman);
97-
argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
97+
argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never. Only applies to the output of -getinfo.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
9898
argsman.AddArg("-named", strprintf("Pass named instead of positional arguments (default: %s)", DEFAULT_NAMED), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
9999
argsman.AddArg("-rpcclienttimeout=<n>", strprintf("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)", DEFAULT_HTTP_CLIENT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
100100
argsman.AddArg("-rpcconnect=<ip>", strprintf("Send commands to node running on <ip> (default: %s)", DEFAULT_RPCCONNECT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
@@ -145,10 +145,10 @@ static int AppInitRPC(int argc, char* argv[])
145145
tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
146146
return EXIT_FAILURE;
147147
}
148-
if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
148+
if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {
149149
std::string strUsage = CLIENT_NAME " RPC client version " + FormatFullVersion() + "\n";
150150

151-
if (gArgs.IsArgSet("-version")) {
151+
if (gArgs.GetBoolArg("-version", false)) {
152152
strUsage += FormatParagraph(LicenseInfo());
153153
} else {
154154
strUsage += "\n"

src/bitcoin-tx.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,11 @@ static int AppInitRawTx(int argc, char* argv[])
105105

106106
fCreateBlank = gArgs.GetBoolArg("-create", false);
107107

108-
if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
108+
if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {
109109
// First part of help message is specific to this utility
110110
std::string strUsage = CLIENT_NAME " bitcoin-tx utility version " + FormatFullVersion() + "\n";
111111

112-
if (gArgs.IsArgSet("-version")) {
112+
if (gArgs.GetBoolArg("-version", false)) {
113113
strUsage += FormatParagraph(LicenseInfo());
114114
} else {
115115
strUsage += "\n"

src/bitcoin-util.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,11 @@ static int AppInitUtil(ArgsManager& args, int argc, char* argv[])
5050
return EXIT_FAILURE;
5151
}
5252

53-
if (HelpRequested(args) || args.IsArgSet("-version")) {
53+
if (HelpRequested(args) || args.GetBoolArg("-version", false)) {
5454
// First part of help message is specific to this utility
5555
std::string strUsage = CLIENT_NAME " bitcoin-util utility version " + FormatFullVersion() + "\n";
5656

57-
if (args.IsArgSet("-version")) {
57+
if (args.GetBoolArg("-version", false)) {
5858
strUsage += FormatParagraph(LicenseInfo());
5959
} else {
6060
strUsage += "\n"

src/bitcoin-wallet.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ static void SetupWalletToolArgs(ArgsManager& argsman)
3434
SetupChainParamsBaseOptions(argsman);
3535

3636
argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
37-
argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
37+
argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
3838
argsman.AddArg("-wallet=<wallet-name>", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS);
3939
argsman.AddArg("-dumpfile=<file name>", "When used with 'dump', writes out the records to this file. When used with 'createfromdump', loads the records into a new wallet.", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
4040
argsman.AddArg("-debug=<category>", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
@@ -60,10 +60,10 @@ static std::optional<int> WalletAppInit(ArgsManager& args, int argc, char* argv[
6060
return EXIT_FAILURE;
6161
}
6262
const bool missing_args{argc < 2};
63-
if (missing_args || HelpRequested(args) || args.IsArgSet("-version")) {
63+
if (missing_args || HelpRequested(args) || args.GetBoolArg("-version", false)) {
6464
std::string strUsage = strprintf("%s bitcoin-wallet utility version", CLIENT_NAME) + " " + FormatFullVersion() + "\n";
6565

66-
if (args.IsArgSet("-version")) {
66+
if (args.GetBoolArg("-version", false)) {
6767
strUsage += FormatParagraph(LicenseInfo());
6868
} else {
6969
strUsage += "\n"

src/bitcoind.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,10 +135,10 @@ static bool ParseArgs(NodeContext& node, int argc, char* argv[])
135135
static bool ProcessInitCommands(ArgsManager& args)
136136
{
137137
// Process help and version before taking care about datadir
138-
if (HelpRequested(args) || args.IsArgSet("-version")) {
138+
if (HelpRequested(args) || args.GetBoolArg("-version", false)) {
139139
std::string strUsage = CLIENT_NAME " daemon version " + FormatFullVersion() + "\n";
140140

141-
if (args.IsArgSet("-version")) {
141+
if (args.GetBoolArg("-version", false)) {
142142
strUsage += FormatParagraph(LicenseInfo());
143143
} else {
144144
strUsage += "\n"

src/common/config.cpp

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include <algorithm>
1717
#include <cassert>
1818
#include <cstdlib>
19+
#include <filesystem>
1920
#include <fstream>
2021
#include <iostream>
2122
#include <list>
@@ -128,12 +129,18 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
128129
}
129130

130131
const auto conf_path{GetConfigFilePath()};
131-
std::ifstream stream{conf_path};
132-
133-
// not ok to have a config file specified that cannot be opened
134-
if (IsArgSet("-conf") && !stream.good()) {
135-
error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
136-
return false;
132+
std::ifstream stream;
133+
if (!conf_path.empty()) { // path is empty when -noconf is specified
134+
if (fs::is_directory(conf_path)) {
135+
error = strprintf("Config file \"%s\" is a directory.", fs::PathToString(conf_path));
136+
return false;
137+
}
138+
stream = std::ifstream{conf_path};
139+
// If the file is explicitly specified, it must be readable
140+
if (IsArgSet("-conf") && !stream.good()) {
141+
error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
142+
return false;
143+
}
137144
}
138145
// ok to not have a config file
139146
if (stream.good()) {
@@ -175,7 +182,12 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
175182
const size_t default_includes = add_includes({});
176183

177184
for (const std::string& conf_file_name : conf_file_names) {
178-
std::ifstream conf_file_stream{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)};
185+
const auto include_conf_path{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)};
186+
if (fs::is_directory(include_conf_path)) {
187+
error = strprintf("Included config file \"%s\" is a directory.", fs::PathToString(include_conf_path));
188+
return false;
189+
}
190+
std::ifstream conf_file_stream{include_conf_path};
179191
if (conf_file_stream.good()) {
180192
if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) {
181193
return false;
@@ -213,7 +225,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
213225

214226
fs::path AbsPathForConfigVal(const ArgsManager& args, const fs::path& path, bool net_specific)
215227
{
216-
if (path.is_absolute()) {
228+
if (path.is_absolute() || path.empty()) {
217229
return path;
218230
}
219231
return fsbridge::AbsPathJoin(net_specific ? args.GetDataDirNet() : args.GetDataDirBase(), path);

src/common/init.cpp

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -62,29 +62,36 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
6262
fs::create_directories(net_path / "wallets");
6363
}
6464

65-
// Show an error or warning if there is a bitcoin.conf file in the
65+
// Show an error or warn/log if there is a bitcoin.conf file in the
6666
// datadir that is being ignored.
6767
const fs::path base_config_path = base_path / BITCOIN_CONF_FILENAME;
68-
if (fs::exists(base_config_path) && !fs::equivalent(orig_config_path, base_config_path)) {
69-
const std::string cli_config_path = args.GetArg("-conf", "");
70-
const std::string config_source = cli_config_path.empty()
71-
? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path)))
72-
: strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path));
73-
const std::string error = strprintf(
74-
"Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
75-
"%3$s from %4$s is being used instead. Possible ways to address this would be to:\n"
76-
"- Delete or rename the %2$s file in data directory %1$s.\n"
77-
"- Change datadir= or conf= options to specify one configuration file, not two, and use "
78-
"includeconf= to include any other configuration files.\n"
79-
"- Set allowignoredconf=1 option to treat this condition as a warning, not an error.",
80-
fs::quoted(fs::PathToString(base_path)),
81-
fs::quoted(BITCOIN_CONF_FILENAME),
82-
fs::quoted(fs::PathToString(orig_config_path)),
83-
config_source);
84-
if (args.GetBoolArg("-allowignoredconf", false)) {
85-
LogPrintf("Warning: %s\n", error);
86-
} else {
87-
return ConfigError{ConfigStatus::FAILED, Untranslated(error)};
68+
if (fs::exists(base_config_path)) {
69+
if (orig_config_path.empty()) {
70+
LogInfo(
71+
"Data directory %s contains a %s file which is explicitly ignored using -noconf.",
72+
fs::quoted(fs::PathToString(base_path)),
73+
fs::quoted(BITCOIN_CONF_FILENAME));
74+
} else if (!fs::equivalent(orig_config_path, base_config_path)) {
75+
const std::string cli_config_path = args.GetArg("-conf", "");
76+
const std::string config_source = cli_config_path.empty()
77+
? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path)))
78+
: strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path));
79+
std::string error = strprintf(
80+
"Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
81+
"%3$s from %4$s is being used instead. Possible ways to address this would be to:\n"
82+
"- Delete or rename the %2$s file in data directory %1$s.\n"
83+
"- Change datadir= or conf= options to specify one configuration file, not two, and use "
84+
"includeconf= to include any other configuration files.",
85+
fs::quoted(fs::PathToString(base_path)),
86+
fs::quoted(BITCOIN_CONF_FILENAME),
87+
fs::quoted(fs::PathToString(orig_config_path)),
88+
config_source);
89+
if (args.GetBoolArg("-allowignoredconf", false)) {
90+
LogWarning("%s", error);
91+
} else {
92+
error += "\n- Set allowignoredconf=1 option to treat this condition as a warning, not an error.";
93+
return ConfigError{ConfigStatus::FAILED, Untranslated(error)};
94+
}
8895
}
8996
}
9097

src/httprpc.cpp

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,6 @@ static bool multiUserAuthorized(std::string strUserPass)
134134

135135
static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut)
136136
{
137-
if (strRPCUserColonPass.empty()) // Belt-and-suspenders measure if InitRPCAuthentication was not called
138-
return false;
139137
if (strAuth.substr(0, 6) != "Basic ")
140138
return false;
141139
std::string_view strUserPass64 = TrimStringView(std::string_view{strAuth}.substr(6));
@@ -147,8 +145,9 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna
147145
if (strUserPass.find(':') != std::string::npos)
148146
strAuthUsernameOut = strUserPass.substr(0, strUserPass.find(':'));
149147

150-
//Check if authorized under single-user field
151-
if (TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
148+
// Check if authorized under single-user field.
149+
// (strRPCUserColonPass is empty when -norpccookiefile is specified).
150+
if (!strRPCUserColonPass.empty() && TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
152151
return true;
153152
}
154153
return multiUserAuthorized(strUserPass);
@@ -294,22 +293,26 @@ static bool InitRPCAuthentication()
294293
{
295294
if (gArgs.GetArg("-rpcpassword", "") == "")
296295
{
297-
LogInfo("Using random cookie authentication.\n");
298-
299296
std::optional<fs::perms> cookie_perms{std::nullopt};
300297
auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
301298
if (cookie_perms_arg) {
302299
auto perm_opt = InterpretPermString(*cookie_perms_arg);
303300
if (!perm_opt) {
304-
LogInfo("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.\n", *cookie_perms_arg);
301+
LogError("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.", *cookie_perms_arg);
305302
return false;
306303
}
307304
cookie_perms = *perm_opt;
308305
}
309306

307+
assert(strRPCUserColonPass.empty()); // Only support initializing once
310308
if (!GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)) {
311309
return false;
312310
}
311+
if (strRPCUserColonPass.empty()) {
312+
LogInfo("RPC authentication cookie file generation is disabled.");
313+
} else {
314+
LogInfo("Using random cookie authentication.");
315+
}
313316
} else {
314317
LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");
315318
strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");

src/init.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,8 @@ static fs::path GetPidFile(const ArgsManager& args)
175175

176176
[[nodiscard]] static bool CreatePidFile(const ArgsManager& args)
177177
{
178+
if (args.IsArgNegated("-pid")) return true;
179+
178180
std::ofstream file{GetPidFile(args)};
179181
if (file) {
180182
#ifdef WIN32
@@ -483,7 +485,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
483485
argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Disables automatic broadcast and rebroadcast of transactions, unless the source peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
484486
argsman.AddArg("-coinstatsindex", strprintf("Maintain coinstats index used by the gettxoutsetinfo RPC (default: %u)", DEFAULT_COINSTATSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
485487
argsman.AddArg("-conf=<file>", strprintf("Specify path to read-only configuration file. Relative paths will be prefixed by datadir location (only useable from command line, not configuration file) (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
486-
argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
488+
argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
487489
argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
488490
argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (minimum %d, default: %d). Make sure you have enough RAM. In addition, unused memory allocated to the mempool is shared with this cache (see -maxmempool).", nMinDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
489491
argsman.AddArg("-includeconf=<file>", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

src/init/common.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <util/translation.h>
1818

1919
#include <algorithm>
20+
#include <filesystem>
2021
#include <string>
2122
#include <vector>
2223

@@ -122,10 +123,13 @@ bool StartLogging(const ArgsManager& args)
122123

123124
// Only log conf file usage message if conf file actually exists.
124125
fs::path config_file_path = args.GetConfigFilePath();
125-
if (fs::exists(config_file_path)) {
126+
if (args.IsArgNegated("-conf")) {
127+
LogInfo("Config file: <disabled>");
128+
} else if (fs::is_directory(config_file_path)) {
129+
LogWarning("Config file: %s (is directory, not file)", fs::PathToString(config_file_path));
130+
} else if (fs::exists(config_file_path)) {
126131
LogPrintf("Config file: %s\n", fs::PathToString(config_file_path));
127132
} else if (args.IsArgSet("-conf")) {
128-
// Warn if no conf file exists at path provided by user
129133
InitWarning(strprintf(_("The specified config file %s does not exist"), fs::PathToString(config_file_path)));
130134
} else {
131135
// Not categorizing as "Warning" because it's the default behavior

0 commit comments

Comments
 (0)