From e8a2054edc814b2c4661b96a3dce91da9be68fa4 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Fri, 8 Nov 2024 12:15:22 +0100 Subject: [PATCH 01/14] doc args: Document narrow scope of -color --- src/bitcoin-cli.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index dd7b21e567137..dd8f3b996dc5c 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -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=", 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=", 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=", 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=", strprintf("Send commands to node running on (default: %s)", DEFAULT_RPCCONNECT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); From 6ff9662760099c405cf13153dd1de22900045f5e Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Wed, 20 Nov 2024 22:55:23 +0100 Subject: [PATCH 02/14] scripted-diff: Avoid printing version information for -noversion -BEGIN VERIFY SCRIPT- sed -i --regexp-extended 's/\b(gArgs|args)\.IsArgSet\("-version"\)/\1.GetBoolArg("-version", false)/g' $(git grep -l '-version') -END VERIFY SCRIPT- --- src/bitcoin-cli.cpp | 4 ++-- src/bitcoin-tx.cpp | 4 ++-- src/bitcoin-util.cpp | 4 ++-- src/bitcoin-wallet.cpp | 4 ++-- src/bitcoind.cpp | 4 ++-- src/qt/bitcoin.cpp | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index dd8f3b996dc5c..f8d6fe8b2b2fa 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -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" diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index 1561510db8bf4..21fd4c437a565 100644 --- a/src/bitcoin-tx.cpp +++ b/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" diff --git a/src/bitcoin-util.cpp b/src/bitcoin-util.cpp index c7d10b6622b9e..ff2e4fb800560 100644 --- a/src/bitcoin-util.cpp +++ b/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" diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index be3162094555f..85910e56a7ffc 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -60,10 +60,10 @@ static std::optional 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" diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 09a35d4bad453..5d5f06127d213 100644 --- a/src/bitcoind.cpp +++ b/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" diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 5c1ae895b1d08..59eeeb02cd026 100644 --- a/src/qt/bitcoin.cpp +++ b/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; } From 12f8d848fd91b11d5cffe21dfc3ba124102ee236 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 4 Nov 2024 14:15:51 +0100 Subject: [PATCH 03/14] args: Disallow -nodatadir Does not make sense to run without a datadir. Prior to this change it would be interpreted as a mix of unset and as a relative path of "0". --- src/bitcoin-cli.cpp | 2 +- src/bitcoin-wallet.cpp | 2 +- src/init.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index f8d6fe8b2b2fa..5c5965245bc6a 100644 --- a/src/bitcoin-cli.cpp +++ b/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=", 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=", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-datadir=", "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 " diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index 85910e56a7ffc..033cf7a0f3932 100644 --- a/src/bitcoin-wallet.cpp +++ b/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=", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); argsman.AddArg("-wallet=", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-dumpfile=", "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=", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); diff --git a/src/init.cpp b/src/init.cpp index f79ebe881dc06..0d255db375f4d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -483,7 +483,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=", 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=", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-datadir=", "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=", strprintf("Maximum database cache size 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=", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); From bffd92f00f5bfbb5a622d0bd20bfeed9f8b10928 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Thu, 14 Nov 2024 22:14:24 +0100 Subject: [PATCH 04/14] args: Support -nopid --- src/init.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 0d255db375f4d..19efc69aaa2ba 100644 --- a/src/init.cpp +++ b/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 From 75bacabb55f3d54ad9b2c660cafc82c167e4f644 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:38:20 +0100 Subject: [PATCH 05/14] test: combine_logs.py - Output debug.log paths on error --- test/functional/combine_logs.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/functional/combine_logs.py b/test/functional/combine_logs.py index 33c81bde13180..57fd0710b6907 100755 --- a/test/functional/combine_logs.py +++ b/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) From 6e28c76907ca0012b1a4556d9a982dfffb5abaf6 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 25 Nov 2024 10:41:50 +0100 Subject: [PATCH 06/14] test: Harden testing of cookie file existence --- test/functional/rpc_users.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index 49eb64abada02..45ea7ab0cc5d6 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -166,8 +166,15 @@ def run_test(self): 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() From e82ad88452bce4132e4583727e610d52dcf9ad9e Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 2 Dec 2024 10:38:07 +0100 Subject: [PATCH 07/14] logs: Use correct path and more appropriate macros in cookie-related code filepath_tmp -> filepath in last message. More material changes to nearby code in next commit. --- src/httprpc.cpp | 2 +- src/rpc/request.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index 69dd821dc0442..dd4cd40ed67b4 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -301,7 +301,7 @@ static bool InitRPCAuthentication() 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; diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index afd98f88754ab..869941bbaae9d 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -108,7 +108,7 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional cookie fs::path filepath_tmp = GetAuthCookieFile(true); 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 +116,14 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional 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; } } From 39cbd4f37c3d3a32cd993cbc78052d53f700989b Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:20:31 +0100 Subject: [PATCH 08/14] args: Support -norpccookiefile for bitcoind and bitcoin-cli Replaces belt & suspenders check for initialization in RPCAuthorized() with not allowing empty passwords further down. --- src/httprpc.cpp | 15 +++++++++------ src/rpc/request.cpp | 9 +++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index dd4cd40ed67b4..5d906ffa0c244 100644 --- a/src/httprpc.cpp +++ b/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,8 +293,6 @@ static bool InitRPCAuthentication() { if (gArgs.GetArg("-rpcpassword", "") == "") { - LogInfo("Using random cookie authentication.\n"); - std::optional cookie_perms{std::nullopt}; auto cookie_perms_arg{gArgs.GetArg("-rpccookieperms")}; if (cookie_perms_arg) { @@ -307,9 +304,15 @@ static bool InitRPCAuthentication() 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", ""); diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index 869941bbaae9d..c0ae94bc4f287 100644 --- a/src/rpc/request.cpp +++ b/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,6 +109,9 @@ bool GenerateAuthCookie(std::string* cookie_out, std::optional 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()) { LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp)); @@ -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; From 7402658bc2b9d835b240edb9c1dac308859687c3 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 25 Nov 2024 10:50:40 +0100 Subject: [PATCH 09/14] test: -norpccookiefile Both bitcoind and bitcoin-cli. --- test/functional/interface_bitcoin_cli.py | 3 +++ test/functional/rpc_users.py | 24 ++++++++++++++++++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index 3fe6570dd16c3..134b4e566be41 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -164,6 +164,9 @@ def run_test(self): 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) diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index 45ea7ab0cc5d6..75507877d5456 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -22,13 +22,13 @@ 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 @@ def test_perm(perm: Optional[str]): 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') @@ -178,6 +197,7 @@ def run_test(self): self.test_rpccookieperms() + self.test_norpccookiefile(cookie_path) if __name__ == '__main__': HTTPBasicsTest(__file__).main() From 312ec64cc0619f58c6e8abc5855fd2fa0e920c3f Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:32:20 +0100 Subject: [PATCH 10/14] test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning This ensures we don't needlessly start the node, and reduces implicit dependencies between test functions. test_seed_peers() - Move assert calling RPC to verify correct chain after our own function actually started the node. --- test/functional/feature_config_args.py | 33 ++++++++++++++++++-------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index 44c7edf96267e..37585510d41d7 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -27,9 +27,20 @@ def set_test_params(self): 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_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 +173,11 @@ def test_invalid_command_line_options(self): ) 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 +206,10 @@ def test_args_log(self): '-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 +232,12 @@ def test_networkactive(self): 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 +254,12 @@ def test_seed_peers(self): 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 +306,13 @@ def test_seed_peers(self): "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 +337,7 @@ def test_connect_with_seednode(self): 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 ' From 483f0dacc413f4b1ba1b74c2429c4367b87e7f11 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 3 Dec 2024 09:45:47 +0100 Subject: [PATCH 11/14] args: Properly support -noconf -noconf would previously lead to an ifstream "successfully" being opened to the ".bitcoin"-directory (not a file). (Guards against the general case of directories as configs are added in grandchild commit to this one). Other users of AbsPathForConfigVal() in combination with negated args have been updated earlier in this PR ("args: Support -nopid" and "args: Support -norpccookiefile..."). --- src/common/config.cpp | 16 +++++++------- src/common/init.cpp | 49 ++++++++++++++++++++++++------------------- src/init/common.cpp | 6 ++++-- 3 files changed, 41 insertions(+), 30 deletions(-) diff --git a/src/common/config.cpp b/src/common/config.cpp index 98223fc3e3a59..13db98171ae03 100644 --- a/src/common/config.cpp +++ b/src/common/config.cpp @@ -128,12 +128,14 @@ 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 + 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()) { @@ -213,7 +215,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); diff --git a/src/common/init.cpp b/src/common/init.cpp index 412d73aec7016..5a704404689d8 100644 --- a/src/common/init.cpp +++ b/src/common/init.cpp @@ -62,29 +62,36 @@ std::optional 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)}; + } } } diff --git a/src/init/common.cpp b/src/init/common.cpp index 8d0b008fb52c1..8f7a86b688638 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -122,10 +123,11 @@ 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: "); + } 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 From e4b6b1822ce004365be11c54c8f5f02f95303fb0 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:55:13 +0100 Subject: [PATCH 12/14] test: Add tests for -noconf --- test/functional/feature_config_args.py | 32 ++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index 37585510d41d7..8778a25f08058 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -39,6 +39,37 @@ def setup_nodes(self): self.nodes[0].debug_log_path.parent.mkdir() self.nodes[0].debug_log_path.touch() + 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') @@ -436,6 +467,7 @@ def run_test(self): self.test_networkactive() self.test_connect_with_seednode() + self.test_negated_config() self.test_config_file_parser() self.test_config_file_log() self.test_invalid_command_line_options() From e85abe92c7cc5380489c028479f0d42f91827efd Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 2 Dec 2024 10:31:19 +0100 Subject: [PATCH 13/14] args: Catch directories in place of config files Previously passing a directory path as -conf would lead to an ifstream being opened for it, and would not trigger any errors. --- src/common/config.cpp | 12 +++++++++++- src/init/common.cpp | 2 ++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/common/config.cpp b/src/common/config.cpp index 13db98171ae03..fac4aa314c54a 100644 --- a/src/common/config.cpp +++ b/src/common/config.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -130,6 +131,10 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) const auto conf_path{GetConfigFilePath()}; 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()) { @@ -177,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; diff --git a/src/init/common.cpp b/src/init/common.cpp index 8f7a86b688638..3a8d75a626950 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -125,6 +125,8 @@ bool StartLogging(const ArgsManager& args) fs::path config_file_path = args.GetConfigFilePath(); if (args.IsArgNegated("-conf")) { LogInfo("Config file: "); + } 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")) { From 95a0104f2e9869799db84add108ae8c57b56d360 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Thu, 28 Nov 2024 10:46:37 +0100 Subject: [PATCH 14/14] test: Add tests for directories in place of config files --- test/functional/feature_config_args.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index 8778a25f08058..1852fd28215fa 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -39,6 +39,29 @@ def setup_nodes(self): 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') @@ -467,6 +490,7 @@ def run_test(self): 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()