Merge bitcoin/bitcoin#31212: util: Improve documentation and negation of args

95a0104f2e test: Add tests for directories in place of config files (Hodlinator)
e85abe92c7 args: Catch directories in place of config files (Hodlinator)
e4b6b1822c test: Add tests for -noconf (Hodlinator)
483f0dacc4 args: Properly support -noconf (Hodlinator)
312ec64cc0 test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning (Hodlinator)
7402658bc2 test: -norpccookiefile (Hodlinator)
39cbd4f37c args: Support -norpccookiefile for bitcoind and bitcoin-cli (Hodlinator)
e82ad88452 logs: Use correct path and more appropriate macros in cookie-related code (Hodlinator)
6e28c76907 test: Harden testing of cookie file existence (Hodlinator)
75bacabb55 test: combine_logs.py - Output debug.log paths on error (Hodlinator)
bffd92f00f args: Support -nopid (Hodlinator)
12f8d848fd args: Disallow -nodatadir (Hodlinator)
6ff9662760 scripted-diff: Avoid printing version information for -noversion (Hodlinator)
e8a2054edc 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 https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013.

ACKs for top commit:
  l0rinc:
    utACK 95a0104f2e
  achow101:
    ACK 95a0104f2e
  ryanofsky:
    Code review ACK 95a0104f2e. Looks good! Thanks for all your work on this breaking the changes down and making them simple.

Tree-SHA512: 5174251e6b9196a9c6d135eddcb94130295c551bcfccc78e633d9e118ff91523b1be0d72828fb49603ceae312e6e1f8ee2651c6a2b9e0f195603a73a9a622785

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

src/qt/bitcoin.cpp

@@ -582,8 +582,8 @@ int GuiMain(int argc, char* argv[])
 
     // Show help message immediately after parsing command-line options (for "-lang") and setting locale,
     // but before showing splash screen.
-    if (HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
-        HelpMessageDialog help(nullptr, gArgs.IsArgSet("-version"));
+    if (HelpRequested(gArgs) || gArgs.GetBoolArg("-version", false)) {
+        HelpMessageDialog help(nullptr, gArgs.GetBoolArg("-version", false));
         help.showOrPrint();
         return EXIT_SUCCESS;
     }

src/rpc/request.cpp

@@ -86,6 +86,9 @@ static const char* const COOKIEAUTH_FILE = ".cookie";
 static fs::path GetAuthCookieFile(bool temp=false)
 {
     fs::path arg = gArgs.GetPathArg("-rpccookiefile", COOKIEAUTH_FILE);
+    if (arg.empty()) {
+        return {}; // -norpccookiefile was specified
+    }
     if (temp) {
         arg += ".tmp";
     }
@@ -106,9 +109,12 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie
      */
     std::ofstream file;
     fs::path filepath_tmp = GetAuthCookieFile(true);
+    if (filepath_tmp.empty()) {
+        return true; // -norpccookiefile
+    }
     file.open(filepath_tmp);
     if (!file.is_open()) {
-        LogInfo("Unable to open cookie authentication file %s for writing\n", fs::PathToString(filepath_tmp));
+        LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp));
         return false;
     }
     file << cookie;
@@ -116,14 +122,14 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie
 
     fs::path filepath = GetAuthCookieFile(false);
     if (!RenameOver(filepath_tmp, filepath)) {
-        LogInfo("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
+        LogWarning("Unable to rename cookie authentication file %s to %s", fs::PathToString(filepath_tmp), fs::PathToString(filepath));
         return false;
     }
     if (cookie_perms) {
         std::error_code code;
         fs::permissions(filepath, cookie_perms.value(), fs::perm_options::replace, code);
         if (code) {
-            LogInfo("Unable to set permissions on cookie authentication file %s\n", fs::PathToString(filepath_tmp));
+            LogWarning("Unable to set permissions on cookie authentication file %s", fs::PathToString(filepath));
             return false;
         }
     }
@@ -142,6 +148,9 @@ bool GetAuthCookie(std::string *cookie_out)
     std::ifstream file;
     std::string cookie;
     fs::path filepath = GetAuthCookieFile();
+    if (filepath.empty()) {
+        return true; // -norpccookiefile
+    }
     file.open(filepath);
     if (!file.is_open())
         return false;

test/functional/combine_logs.py

@@ -79,11 +79,12 @@ def read_logs(tmp_dir):
     Delegates to generator function get_log_events() to provide individual log events
     for each of the input log files."""
 
-    # Find out what the folder is called that holds the debug.log file
-    glob = pathlib.Path(tmp_dir).glob('node0/**/debug.log')
-    path = next(glob, None)
-    if path:
-        assert next(glob, None) is None #  more than one debug.log, should never happen
+    # Find out what the folder is called that holds node 0's debug.log file
+    debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
+    if len(debug_logs) > 0:
+        assert len(debug_logs) < 2, 'Max one debug.log is supported, ' \
+            'found several:\n\t' + '\n\t'.join([str(f) for f in debug_logs])
+        path = debug_logs[0]
         chain = re.search(r'node0/(.+?)/debug\.log$', path.as_posix()).group(1)  # extract the chain name
     else:
         chain = 'regtest'  # fallback to regtest (should only happen when none exists)

test/functional/feature_config_args.py

@@ -27,9 +27,74 @@ class ConfArgsTest(BitcoinTestFramework):
         self.wallet_names = []
         self.disable_autoconnect = False
 
+    # Overridden to avoid attempt to sync not yet started nodes.
+    def setup_network(self):
+        self.setup_nodes()
+
+    # Overriden to not start nodes automatically - doing so is the
+    # responsibility of each test function.
+    def setup_nodes(self):
+        self.add_nodes(self.num_nodes, self.extra_args)
+        # Ensure a log file exists as TestNode.assert_debug_log() expects it.
+        self.nodes[0].debug_log_path.parent.mkdir()
+        self.nodes[0].debug_log_path.touch()
+
+    def test_dir_config(self):
+        self.log.info('Error should be emitted if config file is a directory')
+        conf_path = self.nodes[0].datadir_path / 'bitcoin.conf'
+        os.rename(conf_path, conf_path.with_suffix('.confbkp'))
+        conf_path.mkdir()
+        self.stop_node(0)
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=['-regtest'],
+            expected_msg=f'Error: Error reading configuration file: Config file "{conf_path}" is a directory.',
+        )
+        conf_path.rmdir()
+        os.rename(conf_path.with_suffix('.confbkp'), conf_path)
+
+        self.log.debug('Verifying includeconf directive pointing to directory is caught')
+        with open(conf_path, 'a', encoding='utf-8') as conf:
+            conf.write(f'includeconf={self.nodes[0].datadir_path}\n')
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=['-regtest'],
+            expected_msg=f'Error: Error reading configuration file: Included config file "{self.nodes[0].datadir_path}" is a directory.',
+        )
+
+        self.nodes[0].replace_in_config([(f'includeconf={self.nodes[0].datadir_path}', '')])
+
+    def test_negated_config(self):
+        self.log.info('Disabling configuration via -noconf')
+
+        conf_path = self.nodes[0].datadir_path / 'bitcoin.conf'
+        with open(conf_path, encoding='utf-8') as conf:
+            settings = [f'-{line.rstrip()}' for line in conf if len(line) > 1 and line[0] != '[']
+        os.rename(conf_path, conf_path.with_suffix('.confbkp'))
+
+        self.log.debug('Verifying garbage in config can be detected')
+        with open(conf_path, 'a', encoding='utf-8') as conf:
+            conf.write(f'garbage\n')
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=['-regtest'],
+            expected_msg='Error: Error reading configuration file: parse error on line 1: garbage',
+        )
+
+        self.log.debug('Verifying that disabling of the config file means garbage inside of it does ' \
+            'not prevent the node from starting, and message about existing config file is logged')
+        ignored_file_message = [f'[InitConfig] Data directory "{self.nodes[0].datadir_path}" contains a "bitcoin.conf" file which is explicitly ignored using -noconf.']
+        with self.nodes[0].assert_debug_log(timeout=60, expected_msgs=ignored_file_message):
+            self.start_node(0, extra_args=settings + ['-noconf'])
+        self.stop_node(0)
+
+        self.log.debug('Verifying no message appears when removing config file')
+        os.remove(conf_path)
+        with self.nodes[0].assert_debug_log(timeout=60, expected_msgs=[], unexpected_msgs=ignored_file_message):
+            self.start_node(0, extra_args=settings + ['-noconf'])
+        self.stop_node(0)
+
+        os.rename(conf_path.with_suffix('.confbkp'), conf_path)
+
     def test_config_file_parser(self):
         self.log.info('Test config file parser')
-        self.stop_node(0)
 
         # Check that startup fails if conf= is set in bitcoin.conf or in an included conf file
         bad_conf_file_path = self.nodes[0].datadir_path / "bitcoin_bad.conf"
@@ -162,12 +227,11 @@ class ConfArgsTest(BitcoinTestFramework):
                 )
 
     def test_log_buffer(self):
-        self.stop_node(0)
         with self.nodes[0].assert_debug_log(expected_msgs=['Warning: parsed potentially confusing double-negative -connect=0\n']):
             self.start_node(0, extra_args=['-noconnect=0'])
+        self.stop_node(0)
 
     def test_args_log(self):
-        self.stop_node(0)
         self.log.info('Test config args logging')
         with self.nodes[0].assert_debug_log(
                 expected_msgs=[
@@ -196,10 +260,10 @@ class ConfArgsTest(BitcoinTestFramework):
                 '-rpcuser=secret-rpcuser',
                 '-torpassword=secret-torpassword',
             ])
+        self.stop_node(0)
 
     def test_networkactive(self):
         self.log.info('Test -networkactive option')
-        self.stop_node(0)
         with self.nodes[0].assert_debug_log(expected_msgs=['SetNetworkActive: true\n']):
             self.start_node(0)
 
@@ -222,16 +286,12 @@ class ConfArgsTest(BitcoinTestFramework):
         self.stop_node(0)
         with self.nodes[0].assert_debug_log(expected_msgs=['SetNetworkActive: false\n']):
             self.start_node(0, extra_args=['-nonetworkactive=1'])
+        self.stop_node(0)
 
     def test_seed_peers(self):
         self.log.info('Test seed peers')
         default_data_dir = self.nodes[0].datadir_path
         peer_dat = default_data_dir / 'peers.dat'
-        # Only regtest has no fixed seeds. To avoid connections to random
-        # nodes, regtest is the only network where it is safe to enable
-        # -fixedseeds in tests
-        util.assert_equal(self.nodes[0].getblockchaininfo()['chain'],'regtest')
-        self.stop_node(0)
 
         # No peers.dat exists and -dnsseed=1
         # We expect the node will use DNS Seeds, but Regtest mode does not have
@@ -248,6 +308,12 @@ class ConfArgsTest(BitcoinTestFramework):
                 timeout=10,
         ):
             self.start_node(0, extra_args=['-dnsseed=1', '-fixedseeds=1', f'-mocktime={start}'])
+
+        # Only regtest has no fixed seeds. To avoid connections to random
+        # nodes, regtest is the only network where it is safe to enable
+        # -fixedseeds in tests
+        util.assert_equal(self.nodes[0].getblockchaininfo()['chain'],'regtest')
+
         with self.nodes[0].assert_debug_log(expected_msgs=[
                 "Adding fixed seeds as 60 seconds have passed and addrman is empty",
         ]):
@@ -294,13 +360,13 @@ class ConfArgsTest(BitcoinTestFramework):
                 "Adding fixed seeds as 60 seconds have passed and addrman is empty",
         ]):
             self.nodes[0].setmocktime(start + 65)
+        self.stop_node(0)
 
     def test_connect_with_seednode(self):
         self.log.info('Test -connect with -seednode')
         seednode_ignored = ['-seednode is ignored when -connect is used\n']
         dnsseed_ignored = ['-dnsseed is ignored when -connect is used and -proxy is specified\n']
         addcon_thread_started = ['addcon thread start\n']
-        self.stop_node(0)
 
         # When -connect is supplied, expanding addrman via getaddr calls to ADDR_FETCH(-seednode)
         # nodes is irrelevant and -seednode is ignored.
@@ -325,6 +391,7 @@ class ConfArgsTest(BitcoinTestFramework):
             with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started,
                     unexpected_msgs=seednode_ignored):
                 self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2'])
+        self.stop_node(0)
 
     def test_ignored_conf(self):
         self.log.info('Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored '
@@ -423,6 +490,8 @@ class ConfArgsTest(BitcoinTestFramework):
         self.test_networkactive()
         self.test_connect_with_seednode()
 
+        self.test_dir_config()
+        self.test_negated_config()
         self.test_config_file_parser()
         self.test_config_file_log()
         self.test_invalid_command_line_options()

test/functional/interface_bitcoin_cli.py

@@ -164,6 +164,9 @@ class TestBitcoinCli(BitcoinTestFramework):
         self.log.info("Test connecting with non-existing RPC cookie file")
         assert_raises_process_error(1, "Could not locate RPC credentials", self.nodes[0].cli('-rpccookiefile=does-not-exist', '-rpcpassword=').echo)
 
+        self.log.info("Test connecting without RPC cookie file and with password arg")
+        assert_equal(BLOCKS, self.nodes[0].cli('-norpccookiefile', f'-rpcuser={user}', f'-rpcpassword={password}').getblockcount())
+
         self.log.info("Test -getinfo with arguments fails")
         assert_raises_process_error(1, "-getinfo takes no arguments", self.nodes[0].cli('-getinfo').help)
 

test/functional/rpc_users.py

@@ -22,13 +22,13 @@ import sys
 from typing import Optional
 
 
-def call_with_auth(node, user, password):
+def call_with_auth(node, user, password, method="getbestblockhash"):
     url = urllib.parse.urlparse(node.url)
     headers = {"Authorization": "Basic " + str_to_b64str('{}:{}'.format(user, password))}
 
     conn = http.client.HTTPConnection(url.hostname, url.port)
     conn.connect()
-    conn.request('POST', '/', '{"method": "getbestblockhash"}', headers)
+    conn.request('POST', '/', f'{{"method": "{method}"}}', headers)
     resp = conn.getresponse()
     conn.close()
     return resp
@@ -121,6 +121,25 @@ class HTTPBasicsTest(BitcoinTestFramework):
         for perm in ["owner", "group", "all"]:
             test_perm(perm)
 
+    def test_norpccookiefile(self, node0_cookie_path):
+        assert self.nodes[0].is_node_stopped(), "We expect previous test to stopped the node"
+        assert not node0_cookie_path.exists()
+
+        self.log.info('Starting with -norpccookiefile')
+        # Start, but don't wait for RPC connection as TestNode.wait_for_rpc_connection() requires the cookie.
+        with self.nodes[0].busy_wait_for_debug_log([b'init message: Done loading']):
+            self.nodes[0].start(extra_args=["-norpccookiefile"])
+        assert not node0_cookie_path.exists()
+
+        self.log.info('Testing user/password authentication still works without cookie file')
+        assert_equal(200, call_with_auth(self.nodes[0], "rt", self.rtpassword).status)
+        # After confirming that we could log in, check that cookie file does not exist.
+        assert not node0_cookie_path.exists()
+
+        # Need to shut down in slightly unorthodox way since cookie auth can't be used
+        assert_equal(200, call_with_auth(self.nodes[0], "rt", self.rtpassword, method="stop").status)
+        self.nodes[0].wait_until_stopped()
+
     def run_test(self):
         self.conf_setup()
         self.log.info('Check correctness of the rpcauth config option')
@@ -166,11 +185,19 @@ class HTTPBasicsTest(BitcoinTestFramework):
         self.stop_node(0)
 
         self.log.info('Check that failure to write cookie file will abort the node gracefully')
-        (self.nodes[0].chain_path / ".cookie.tmp").mkdir()
+        cookie_path =     self.nodes[0].chain_path / ".cookie"
+        cookie_path_tmp = self.nodes[0].chain_path / ".cookie.tmp"
+        cookie_path_tmp.mkdir()
         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error)
+        cookie_path_tmp.rmdir()
+        assert not cookie_path.exists()
+        self.restart_node(0)
+        assert cookie_path.exists()
+        self.stop_node(0)
 
         self.test_rpccookieperms()
 
+        self.test_norpccookiefile(cookie_path)
 
 if __name__ == '__main__':
     HTTPBasicsTest(__file__).main()