Merge 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#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

Author: achow101

src/bitcoin-cli.cpp

@@ -82,7 +82,7 @@ static void SetupCliArgs(ArgsManager& argsman)
 
     argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     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);
-    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
     argsman.AddArg("-generate",
                    strprintf("Generate blocks, equivalent to RPC getnewaddress followed by RPC generatetoaddress. Optional positional integer "
                              "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)
     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);
 
     SetupChainParamsBaseOptions(argsman);
-    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);
+    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);
     argsman.AddArg("-named", strprintf("Pass named instead of positional arguments (default: %s)", DEFAULT_NAMED), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     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);
     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[])
         tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
         return EXIT_FAILURE;
     }
-    if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
+    if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {
         std::string strUsage = CLIENT_NAME " RPC client version " + FormatFullVersion() + "\n";
 
-        if (gArgs.IsArgSet("-version")) {
+        if (gArgs.GetBoolArg("-version", false)) {
             strUsage += FormatParagraph(LicenseInfo());
         } else {
             strUsage += "\n"

src/bitcoin-tx.cpp

@@ -105,11 +105,11 @@ static int AppInitRawTx(int argc, char* argv[])
 
     fCreateBlank = gArgs.GetBoolArg("-create", false);
 
-    if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
+    if (argc < 2 || HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {
         // First part of help message is specific to this utility
         std::string strUsage = CLIENT_NAME " bitcoin-tx utility version " + FormatFullVersion() + "\n";
 
-        if (gArgs.IsArgSet("-version")) {
+        if (gArgs.GetBoolArg("-version", false)) {
             strUsage += FormatParagraph(LicenseInfo());
         } else {
             strUsage += "\n"

src/bitcoin-util.cpp

@@ -50,11 +50,11 @@ static int AppInitUtil(ArgsManager& args, int argc, char* argv[])
         return EXIT_FAILURE;
     }
 
-    if (HelpRequested(args) || args.IsArgSet("-version")) {
+    if (HelpRequested(args) || args.GetBoolArg("-version", false)) {
         // First part of help message is specific to this utility
         std::string strUsage = CLIENT_NAME " bitcoin-util utility version " + FormatFullVersion() + "\n";
 
-        if (args.IsArgSet("-version")) {
+        if (args.GetBoolArg("-version", false)) {
             strUsage += FormatParagraph(LicenseInfo());
         } else {
             strUsage += "\n"

src/bitcoin-wallet.cpp

@@ -34,7 +34,7 @@ static void SetupWalletToolArgs(ArgsManager& argsman)
     SetupChainParamsBaseOptions(argsman);
 
     argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
     argsman.AddArg("-wallet=<wallet-name>", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS);
     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);
     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[
         return EXIT_FAILURE;
     }
     const bool missing_args{argc < 2};
-    if (missing_args || HelpRequested(args) || args.IsArgSet("-version")) {
+    if (missing_args || HelpRequested(args) || args.GetBoolArg("-version", false)) {
         std::string strUsage = strprintf("%s bitcoin-wallet utility version", CLIENT_NAME) + " " + FormatFullVersion() + "\n";
 
-        if (args.IsArgSet("-version")) {
+        if (args.GetBoolArg("-version", false)) {
             strUsage += FormatParagraph(LicenseInfo());
         } else {
             strUsage += "\n"

src/bitcoind.cpp

@@ -135,10 +135,10 @@ static bool ParseArgs(NodeContext& node, int argc, char* argv[])
 static bool ProcessInitCommands(ArgsManager& args)
 {
     // Process help and version before taking care about datadir
-    if (HelpRequested(args) || args.IsArgSet("-version")) {
+    if (HelpRequested(args) || args.GetBoolArg("-version", false)) {
         std::string strUsage = CLIENT_NAME " daemon version " + FormatFullVersion() + "\n";
 
-        if (args.IsArgSet("-version")) {
+        if (args.GetBoolArg("-version", false)) {
             strUsage += FormatParagraph(LicenseInfo());
         } else {
             strUsage += "\n"

src/common/config.cpp

@@ -16,6 +16,7 @@
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
+#include <filesystem>
 #include <fstream>
 #include <iostream>
 #include <list>
@@ -128,12 +129,18 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
     }
 
     const auto conf_path{GetConfigFilePath()};
-    std::ifstream stream{conf_path};
-
-    // not ok to have a config file specified that cannot be opened
-    if (IsArgSet("-conf") && !stream.good()) {
-        error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
-        return false;
+    std::ifstream stream;
+    if (!conf_path.empty()) { // path is empty when -noconf is specified
+        if (fs::is_directory(conf_path)) {
+            error = strprintf("Config file \"%s\" is a directory.", fs::PathToString(conf_path));
+            return false;
+        }
+        stream = std::ifstream{conf_path};
+        // If the file is explicitly specified, it must be readable
+        if (IsArgSet("-conf") && !stream.good()) {
+            error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path));
+            return false;
+        }
     }
     // ok to not have a config file
     if (stream.good()) {
@@ -175,7 +182,12 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
             const size_t default_includes = add_includes({});
 
             for (const std::string& conf_file_name : conf_file_names) {
-                std::ifstream conf_file_stream{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)};
+                const auto include_conf_path{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)};
+                if (fs::is_directory(include_conf_path)) {
+                    error = strprintf("Included config file \"%s\" is a directory.", fs::PathToString(include_conf_path));
+                    return false;
+                }
+                std::ifstream conf_file_stream{include_conf_path};
                 if (conf_file_stream.good()) {
                     if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) {
                         return false;
@@ -213,7 +225,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
 
 fs::path AbsPathForConfigVal(const ArgsManager& args, const fs::path& path, bool net_specific)
 {
-    if (path.is_absolute()) {
+    if (path.is_absolute() || path.empty()) {
         return path;
     }
     return fsbridge::AbsPathJoin(net_specific ? args.GetDataDirNet() : args.GetDataDirBase(), path);

src/common/init.cpp

@@ -62,29 +62,36 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
             fs::create_directories(net_path / "wallets");
         }
 
-        // Show an error or warning if there is a bitcoin.conf file in the
+        // Show an error or warn/log if there is a bitcoin.conf file in the
         // datadir that is being ignored.
         const fs::path base_config_path = base_path / BITCOIN_CONF_FILENAME;
-        if (fs::exists(base_config_path) && !fs::equivalent(orig_config_path, base_config_path)) {
-            const std::string cli_config_path = args.GetArg("-conf", "");
-            const std::string config_source = cli_config_path.empty()
-                ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path)))
-                : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path));
-            const std::string error = strprintf(
-                "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
-                "%3$s from %4$s is being used instead. Possible ways to address this would be to:\n"
-                "- Delete or rename the %2$s file in data directory %1$s.\n"
-                "- Change datadir= or conf= options to specify one configuration file, not two, and use "
-                "includeconf= to include any other configuration files.\n"
-                "- Set allowignoredconf=1 option to treat this condition as a warning, not an error.",
-                fs::quoted(fs::PathToString(base_path)),
-                fs::quoted(BITCOIN_CONF_FILENAME),
-                fs::quoted(fs::PathToString(orig_config_path)),
-                config_source);
-            if (args.GetBoolArg("-allowignoredconf", false)) {
-                LogPrintf("Warning: %s\n", error);
-            } else {
-                return ConfigError{ConfigStatus::FAILED, Untranslated(error)};
+        if (fs::exists(base_config_path)) {
+            if (orig_config_path.empty()) {
+                LogInfo(
+                    "Data directory %s contains a %s file which is explicitly ignored using -noconf.",
+                    fs::quoted(fs::PathToString(base_path)),
+                    fs::quoted(BITCOIN_CONF_FILENAME));
+            } else if (!fs::equivalent(orig_config_path, base_config_path)) {
+                const std::string cli_config_path = args.GetArg("-conf", "");
+                const std::string config_source = cli_config_path.empty()
+                    ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path)))
+                    : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path));
+                std::string error = strprintf(
+                    "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
+                    "%3$s from %4$s is being used instead. Possible ways to address this would be to:\n"
+                    "- Delete or rename the %2$s file in data directory %1$s.\n"
+                    "- Change datadir= or conf= options to specify one configuration file, not two, and use "
+                    "includeconf= to include any other configuration files.",
+                    fs::quoted(fs::PathToString(base_path)),
+                    fs::quoted(BITCOIN_CONF_FILENAME),
+                    fs::quoted(fs::PathToString(orig_config_path)),
+                    config_source);
+                if (args.GetBoolArg("-allowignoredconf", false)) {
+                    LogWarning("%s", error);
+                } else {
+                    error += "\n- Set allowignoredconf=1 option to treat this condition as a warning, not an error.";
+                    return ConfigError{ConfigStatus::FAILED, Untranslated(error)};
+                }
             }
         }
 

src/httprpc.cpp

@@ -134,8 +134,6 @@ static bool multiUserAuthorized(std::string strUserPass)
 
 static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut)
 {
-    if (strRPCUserColonPass.empty()) // Belt-and-suspenders measure if InitRPCAuthentication was not called
-        return false;
     if (strAuth.substr(0, 6) != "Basic ")
         return false;
     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
     if (strUserPass.find(':') != std::string::npos)
         strAuthUsernameOut = strUserPass.substr(0, strUserPass.find(':'));
 
-    //Check if authorized under single-user field
-    if (TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
+    // Check if authorized under single-user field.
+    // (strRPCUserColonPass is empty when -norpccookiefile is specified).
+    if (!strRPCUserColonPass.empty() && TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
         return true;
     }
     return multiUserAuthorized(strUserPass);
@@ -294,22 +293,26 @@ static bool InitRPCAuthentication()
 {
     if (gArgs.GetArg("-rpcpassword", "") == "")
     {
-        LogInfo("Using random cookie authentication.\n");
-
         std::optional<fs::perms> cookie_perms{std::nullopt};
         auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")};
         if (cookie_perms_arg) {
             auto perm_opt = InterpretPermString(*cookie_perms_arg);
             if (!perm_opt) {
-                LogInfo("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.\n", *cookie_perms_arg);
+                LogError("Invalid -rpccookieperms=%s; must be one of 'owner', 'group', or 'all'.", *cookie_perms_arg);
                 return false;
             }
             cookie_perms = *perm_opt;
         }
 
+        assert(strRPCUserColonPass.empty()); // Only support initializing once
         if (!GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)) {
             return false;
         }
+        if (strRPCUserColonPass.empty()) {
+            LogInfo("RPC authentication cookie file generation is disabled.");
+        } else {
+            LogInfo("Using random cookie authentication.");
+        }
     } else {
         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");
         strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");

src/init.cpp

@@ -175,6 +175,8 @@ static fs::path GetPidFile(const ArgsManager& args)
 
 [[nodiscard]] static bool CreatePidFile(const ArgsManager& args)
 {
+    if (args.IsArgNegated("-pid")) return true;
+
     std::ofstream file{GetPidFile(args)};
     if (file) {
 #ifdef WIN32
@@ -483,7 +485,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
     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);
     argsman.AddArg("-coinstatsindex", strprintf("Maintain coinstats index used by the gettxoutsetinfo RPC (default: %u)", DEFAULT_COINSTATSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     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);
-    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
     argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
     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);
     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

@@ -17,6 +17,7 @@
 #include <util/translation.h>
 
 #include <algorithm>
+#include <filesystem>
 #include <string>
 #include <vector>
 
@@ -122,10 +123,13 @@ bool StartLogging(const ArgsManager& args)
 
     // Only log conf file usage message if conf file actually exists.
     fs::path config_file_path = args.GetConfigFilePath();
-    if (fs::exists(config_file_path)) {
+    if (args.IsArgNegated("-conf")) {
+        LogInfo("Config file: <disabled>");
+    } else if (fs::is_directory(config_file_path)) {
+        LogWarning("Config file: %s (is directory, not file)", fs::PathToString(config_file_path));
+    } else if (fs::exists(config_file_path)) {
         LogPrintf("Config file: %s\n", fs::PathToString(config_file_path));
     } else if (args.IsArgSet("-conf")) {
-        // Warn if no conf file exists at path provided by user
         InitWarning(strprintf(_("The specified config file %s does not exist"), fs::PathToString(config_file_path)));
     } else {
         // Not categorizing as "Warning" because it's the default behavior