From 47721451a9fd1f5083061bb99bf8da21e7e78937 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 20 Aug 2025 13:24:35 -0400 Subject: [PATCH 1/4] init: add exe name to bitcoind, bitcoin-node -version output to be able to distinguish these in tests --- src/bitcoind.cpp | 11 ++++++++--- src/init/bitcoin-gui.cpp | 1 + src/init/bitcoin-node.cpp | 1 + src/init/bitcoin-qt.cpp | 3 +++ src/init/bitcoind.cpp | 3 +++ src/interfaces/init.h | 1 + 6 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index ceb3c99410ca1..a4373dafdf233 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -132,11 +132,16 @@ static bool ParseArgs(NodeContext& node, int argc, char* argv[]) return true; } -static bool ProcessInitCommands(ArgsManager& args) +static bool ProcessInitCommands(interfaces::Init& init, ArgsManager& args) { // Process help and version before taking care about datadir if (HelpRequested(args) || args.GetBoolArg("-version", false)) { - std::string strUsage = CLIENT_NAME " daemon version " + FormatFullVersion() + "\n"; + std::string strUsage = CLIENT_NAME " daemon version " + FormatFullVersion(); + if (const char* exe_name{init.exeName()}) { + strUsage += " "; + strUsage += exe_name; + } + strUsage += "\n"; if (args.GetBoolArg("-version", false)) { strUsage += FormatParagraph(LicenseInfo()); @@ -277,7 +282,7 @@ MAIN_FUNCTION ArgsManager& args = *Assert(node.args); if (!ParseArgs(node, argc, argv)) return EXIT_FAILURE; // Process early info return commands such as -help or -version - if (ProcessInitCommands(args)) return EXIT_SUCCESS; + if (ProcessInitCommands(*init, args)) return EXIT_SUCCESS; // Start application if (!AppInit(node) || !Assert(node.shutdown_signal)->wait()) { diff --git a/src/init/bitcoin-gui.cpp b/src/init/bitcoin-gui.cpp index eae30bc995ae6..aca3fbe1c4452 100644 --- a/src/init/bitcoin-gui.cpp +++ b/src/init/bitcoin-gui.cpp @@ -39,6 +39,7 @@ class BitcoinGuiInit : public interfaces::Init // bitcoin-node accepts the option, and bitcoin-gui accepts all bitcoin-node // options and will start the node with those options. bool canListenIpc() override { return true; } + const char* exeName() override { return EXE_NAME; } node::NodeContext m_node; std::unique_ptr m_ipc; }; diff --git a/src/init/bitcoin-node.cpp b/src/init/bitcoin-node.cpp index 3f8c50b8d66c9..e5f1411fdb576 100644 --- a/src/init/bitcoin-node.cpp +++ b/src/init/bitcoin-node.cpp @@ -38,6 +38,7 @@ class BitcoinNodeInit : public interfaces::Init std::unique_ptr makeEcho() override { return interfaces::MakeEcho(); } interfaces::Ipc* ipc() override { return m_ipc.get(); } bool canListenIpc() override { return true; } + const char* exeName() override { return EXE_NAME; } node::NodeContext& m_node; std::unique_ptr m_ipc; }; diff --git a/src/init/bitcoin-qt.cpp b/src/init/bitcoin-qt.cpp index 5209c72973176..05f3bc32d7d41 100644 --- a/src/init/bitcoin-qt.cpp +++ b/src/init/bitcoin-qt.cpp @@ -16,6 +16,8 @@ namespace init { namespace { +const char* EXE_NAME = "bitcoin-qt"; + class BitcoinQtInit : public interfaces::Init { public: @@ -32,6 +34,7 @@ class BitcoinQtInit : public interfaces::Init return MakeWalletLoader(chain, *Assert(m_node.args)); } std::unique_ptr makeEcho() override { return interfaces::MakeEcho(); } + const char* exeName() override { return EXE_NAME; } node::NodeContext m_node; }; } // namespace diff --git a/src/init/bitcoind.cpp b/src/init/bitcoind.cpp index 48be8831d25df..0b0ab3f8fa874 100644 --- a/src/init/bitcoind.cpp +++ b/src/init/bitcoind.cpp @@ -18,6 +18,8 @@ using node::NodeContext; namespace init { namespace { +const char* EXE_NAME = "bitcoind"; + class BitcoindInit : public interfaces::Init { public: @@ -34,6 +36,7 @@ class BitcoindInit : public interfaces::Init return MakeWalletLoader(chain, *Assert(m_node.args)); } std::unique_ptr makeEcho() override { return interfaces::MakeEcho(); } + const char* exeName() override { return EXE_NAME; } NodeContext& m_node; }; } // namespace diff --git a/src/interfaces/init.h b/src/interfaces/init.h index b496ada05f4ab..030ce306c00a9 100644 --- a/src/interfaces/init.h +++ b/src/interfaces/init.h @@ -38,6 +38,7 @@ class Init virtual std::unique_ptr makeEcho() { return nullptr; } virtual Ipc* ipc() { return nullptr; } virtual bool canListenIpc() { return false; } + virtual const char* exeName() { return nullptr; } }; //! Return implementation of Init interface for the node process. If the argv From a013a02a5a95238a4eb09c691afbc32dd06a2751 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 20 Aug 2025 14:13:17 -0400 Subject: [PATCH 2/4] test: add tool_bitcoin to test bitcoin wrapper behavior --- test/functional/test_runner.py | 1 + test/functional/tool_bitcoin.py | 102 ++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100755 test/functional/tool_bitcoin.py diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 31c9805e8ebf0..81daa4239a5ac 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -335,6 +335,7 @@ 'p2p_tx_privacy.py', 'rpc_getdescriptoractivity.py', 'rpc_scanblocks.py', + 'tool_bitcoin.py', 'p2p_sendtxrcncl.py', 'rpc_scantxoutset.py', 'feature_unsupported_utxo_db.py', diff --git a/test/functional/tool_bitcoin.py b/test/functional/tool_bitcoin.py new file mode 100755 index 0000000000000..dcfdb2d774b2b --- /dev/null +++ b/test/functional/tool_bitcoin.py @@ -0,0 +1,102 @@ +#!/usr/bin/env python3 +# Copyright (c) The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test the bitcoin wrapper tool.""" +from test_framework.test_framework import ( + BitcoinTestFramework, + SkipTest, +) +from test_framework.util import assert_equal + +import platform +import re + + +class ToolBitcoinTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 1 + + def skip_test_if_missing_module(self): + # Skip test on windows because currently when `bitcoin node -version` is + # run on windows, python doesn't capture output from the child + # `bitcoind` and `bitcoin-node` process started with _wexecvp, and + # stdout/stderr are always empty. See + # https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3265524908 + if platform.system() == "Windows": + raise SkipTest("Test does not currently work on windows") + + def setup_network(self): + """Set up nodes normally, but save a copy of their arguments before starting them.""" + self.add_nodes(self.num_nodes, self.extra_args) + node_argv = self.get_binaries().node_argv() + self.node_options = [node.args[len(node_argv):] for node in self.nodes] + assert all(node.args[:len(node_argv)] == node_argv for node in self.nodes) + + def set_cmd_args(self, node, args): + """Set up node so it will be started through bitcoin wrapper command with specified arguments.""" + node.args = [self.binary_paths.bitcoin_bin] + args + ["node"] + self.node_options[node.index] + + def test_args(self, cmd_args, node_args, expect_exe=None, expect_error=None): + node = self.nodes[0] + self.set_cmd_args(node, cmd_args) + extra_args = node_args + ["-version"] + if expect_error is not None: + node.assert_start_raises_init_error(expected_msg=expect_error, extra_args=extra_args) + else: + assert expect_exe + node.start(extra_args=extra_args) + ret, out, err = get_node_output(node) + try: + assert_equal(get_exe_name(out), expect_exe.encode()) + assert_equal(err, b"") + except Exception as e: + raise RuntimeError(f"Unexpected output from {node.args + extra_args}: {out=!r} {err=!r} {ret=!r}") from e + + def run_test(self): + self.log.info("Ensure bitcoin node command invokes bitcoind by default") + self.test_args([], [], expect_exe="bitcoind") + + self.log.info("Ensure bitcoin command does not accept -ipcbind by default") + self.test_args(["-M"], ["-ipcbind=unix"], expect_error='Error: Error parsing command line arguments: Invalid parameter -ipcbind=unix') + + self.log.info("Ensure bitcoin -M invokes bitcoind") + self.test_args(["-M"], [], expect_exe="bitcoind") + + self.log.info("Ensure bitcoin -M does not accept -ipcbind") + self.test_args(["-M"], ["-ipcbind=unix"], expect_error='Error: Error parsing command line arguments: Invalid parameter -ipcbind=unix') + + if self.is_ipc_compiled(): + self.log.info("Ensure bitcoin -m invokes bitcoin-node") + self.test_args(["-m"], [], expect_exe="bitcoin-node") + + self.log.info("Ensure bitcoin -m does accept -ipcbind") + self.test_args(["-m"], ["-ipcbind=unix"], expect_exe="bitcoin-node") + + +def get_node_output(node): + ret = node.process.wait(timeout=60) + node.stdout.seek(0) + node.stderr.seek(0) + out = node.stdout.read() + err = node.stderr.read() + node.stdout.close() + node.stderr.close() + + # Clean up TestNode state + node.running = False + node.process = None + node.rpc_connected = False + node.rpc = None + + return ret, out, err + + +def get_exe_name(version_str): + """Get exe name from last word of first line of version string.""" + return re.match(rb".*?(\S+)\s*?(?:\n|$)", version_str.strip()).group(1) + + +if __name__ == '__main__': + ToolBitcoinTest(__file__).main() From f9685d6a1389938b0cceb31d9eef201ab3dd2e86 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 20 Aug 2025 15:52:21 -0400 Subject: [PATCH 3/4] bitcoin: Make wrapper not require -m Choose the right binary by default if an IPC option is specified --- src/CMakeLists.txt | 2 +- src/bitcoin.cpp | 32 +++++++++++++++++++++++++++++--- src/common/args.cpp | 8 +++++++- src/common/args.h | 8 +++++++- test/functional/tool_bitcoin.py | 17 +++++++++++++---- test/lint/check-doc.py | 2 +- 6 files changed, 58 insertions(+), 11 deletions(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a6fb12c009758..fc4083324b751 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -292,7 +292,7 @@ if(BUILD_BITCOIN_BIN) add_executable(bitcoin bitcoin.cpp) add_windows_resources(bitcoin bitcoin-res.rc) add_windows_application_manifest(bitcoin) - target_link_libraries(bitcoin core_interface bitcoin_util) + target_link_libraries(bitcoin core_interface bitcoin_common bitcoin_util) install_binary_component(bitcoin) endif() diff --git a/src/bitcoin.cpp b/src/bitcoin.cpp index c327827415337..c1a5fce33af70 100644 --- a/src/bitcoin.cpp +++ b/src/bitcoin.cpp @@ -5,6 +5,7 @@ #include // IWYU pragma: keep #include +#include #include #include #include @@ -47,7 +48,7 @@ Run '%s help' to see additional commands (e.g. for testing and debugging). )"; struct CommandLine { - bool use_multiprocess{false}; + std::optional use_multiprocess; bool show_version{false}; bool show_help{false}; std::string_view command; @@ -55,6 +56,7 @@ struct CommandLine { }; CommandLine ParseCommandLine(int argc, char* argv[]); +bool UseMultiprocess(const CommandLine& cmd); static void ExecCommand(const std::vector& args, std::string_view argv0); int main(int argc, char* argv[]) @@ -78,9 +80,9 @@ int main(int argc, char* argv[]) return EXIT_FAILURE; } } else if (cmd.command == "gui") { - args.emplace_back(cmd.use_multiprocess ? "bitcoin-gui" : "bitcoin-qt"); + args.emplace_back(UseMultiprocess(cmd) ? "bitcoin-gui" : "bitcoin-qt"); } else if (cmd.command == "node") { - args.emplace_back(cmd.use_multiprocess ? "bitcoin-node" : "bitcoind"); + args.emplace_back(UseMultiprocess(cmd) ? "bitcoin-node" : "bitcoind"); } else if (cmd.command == "rpc") { args.emplace_back("bitcoin-cli"); // Since "bitcoin rpc" is a new interface that doesn't need to be @@ -143,6 +145,30 @@ CommandLine ParseCommandLine(int argc, char* argv[]) return cmd; } +bool UseMultiprocess(const CommandLine& cmd) +{ + // If -m or -M options were explicitly specified, there is no need to + // further parse arguments to determine which to use. + if (cmd.use_multiprocess) return *cmd.use_multiprocess; + + ArgsManager args; + args.SetDefaultFlags(ArgsManager::ALLOW_ANY); + std::string error_message; + auto argv{cmd.args}; + argv.insert(argv.begin(), nullptr); + if (!args.ParseParameters(argv.size(), argv.data(), error_message)) { + tfm::format(std::cerr, "Warning: failed to parse subcommand command line options: %s\n", error_message); + } + if (!args.ReadConfigFiles(error_message, true)) { + tfm::format(std::cerr, "Warning: failed to parse subcommand config: %s\n", error_message); + } + args.SelectConfigNetwork(args.GetChainTypeString()); + + // If any -ipc* options are set these need to be processed by a + // multiprocess-capable binary. + return args.IsArgSet("-ipcbind") || args.IsArgSet("-ipcconnect") || args.IsArgSet("-ipcfd"); +} + //! Execute the specified bitcoind, bitcoin-qt or other command line in `args` //! using src, bin and libexec directory paths relative to this executable, where //! the path to this executable is specified in `wrapper_argv0`. diff --git a/src/common/args.cpp b/src/common/args.cpp index ff30ec5b8c1ee..d44cd4319b6aa 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -266,7 +266,13 @@ std::optional ArgsManager::GetArgFlags(const std::string& name) co return search->second.m_flags; } } - return std::nullopt; + return m_default_flags; +} + +void ArgsManager::SetDefaultFlags(std::optional flags) +{ + LOCK(cs_args); + m_default_flags = flags; } fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) const diff --git a/src/common/args.h b/src/common/args.h index da19cbda66faf..d907ad7663de3 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -137,6 +137,7 @@ class ArgsManager std::string m_network GUARDED_BY(cs_args); std::set m_network_only_args GUARDED_BY(cs_args); std::map> m_available_args GUARDED_BY(cs_args); + std::optional m_default_flags GUARDED_BY(cs_args){}; bool m_accept_any_command GUARDED_BY(cs_args){true}; std::list m_config_sections GUARDED_BY(cs_args); std::optional m_config_path GUARDED_BY(cs_args); @@ -375,10 +376,15 @@ class ArgsManager /** * Return Flags for known arg. - * Return nullopt for unknown arg. + * Return default flags for unknown arg. */ std::optional GetArgFlags(const std::string& name) const; + /** + * Set default flags to return for an unknown arg. + */ + void SetDefaultFlags(std::optional); + /** * Get settings file path, or return false if read-write settings were * disabled with -nosettings. diff --git a/test/functional/tool_bitcoin.py b/test/functional/tool_bitcoin.py index dcfdb2d774b2b..1112e02e09097 100755 --- a/test/functional/tool_bitcoin.py +++ b/test/functional/tool_bitcoin.py @@ -7,7 +7,10 @@ BitcoinTestFramework, SkipTest, ) -from test_framework.util import assert_equal +from test_framework.util import ( + append_config, + assert_equal, +) import platform import re @@ -55,12 +58,11 @@ def test_args(self, cmd_args, node_args, expect_exe=None, expect_error=None): raise RuntimeError(f"Unexpected output from {node.args + extra_args}: {out=!r} {err=!r} {ret=!r}") from e def run_test(self): + node = self.nodes[0] + self.log.info("Ensure bitcoin node command invokes bitcoind by default") self.test_args([], [], expect_exe="bitcoind") - self.log.info("Ensure bitcoin command does not accept -ipcbind by default") - self.test_args(["-M"], ["-ipcbind=unix"], expect_error='Error: Error parsing command line arguments: Invalid parameter -ipcbind=unix') - self.log.info("Ensure bitcoin -M invokes bitcoind") self.test_args(["-M"], [], expect_exe="bitcoind") @@ -74,6 +76,13 @@ def run_test(self): self.log.info("Ensure bitcoin -m does accept -ipcbind") self.test_args(["-m"], ["-ipcbind=unix"], expect_exe="bitcoin-node") + self.log.info("Ensure bitcoin accepts -ipcbind by default") + self.test_args([], ["-ipcbind=unix"], expect_exe="bitcoin-node") + + self.log.info("Ensure bitcoin recognizes -ipcbind in config file") + append_config(node.datadir_path, ["ipcbind=unix"]) + self.test_args([], [], expect_exe="bitcoin-node") + def get_node_output(node): ret = node.process.wait(timeout=60) diff --git a/test/lint/check-doc.py b/test/lint/check-doc.py index 3e9e5ba230a5e..e47ff30f31cf5 100755 --- a/test/lint/check-doc.py +++ b/test/lint/check-doc.py @@ -23,7 +23,7 @@ CMD_GREP_WALLET_HIDDEN_ARGS = r"git grep --function-context 'void DummyWalletInit::AddWalletOptions' -- {}".format(CMD_ROOT_DIR) CMD_GREP_DOCS = r"git grep --perl-regexp '{}' {}".format(REGEX_DOC, CMD_ROOT_DIR) # list unsupported, deprecated and duplicate args as they need no documentation -SET_DOC_OPTIONAL = set(['-h', '-?', '-dbcrashratio', '-forcecompactdb']) +SET_DOC_OPTIONAL = set(['-h', '-?', '-dbcrashratio', '-forcecompactdb', '-ipcconnect', '-ipcfd']) def lint_missing_argument_documentation(): From cf08bc93debc5fde1a3139bad472bfd54736603a Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 9 Sep 2025 11:24:27 +0200 Subject: [PATCH 4/4] test: automatically pick bitcoind or bitcoin-node allows the bitcoin executable to switch between bitcoind and bitcoin-node as needed. This commit simplifies the test suite to take advantage of that, but also retain some coverage of direct bitcoind usage. The latter is achieved by only the wrapper if ipc functionality is required. --- ci/test/00_setup_env_mac_native.sh | 1 - ci/test/00_setup_env_native_centos.sh | 1 - doc/multiprocess.md | 1 - .../test_framework/test_framework.py | 25 ++++++++----------- 4 files changed, 11 insertions(+), 17 deletions(-) diff --git a/ci/test/00_setup_env_mac_native.sh b/ci/test/00_setup_env_mac_native.sh index 41a3bc4587779..eb89adeae08e4 100755 --- a/ci/test/00_setup_env_mac_native.sh +++ b/ci/test/00_setup_env_mac_native.sh @@ -16,4 +16,3 @@ export BITCOIN_CONFIG="-DBUILD_GUI=ON -DWITH_ZMQ=ON -DREDUCE_EXPORTS=ON -DCMAKE_ export CI_OS_NAME="macos" export NO_DEPENDS=1 export OSX_SDK="" -export BITCOIN_CMD="bitcoin -m" # Used in functional tests diff --git a/ci/test/00_setup_env_native_centos.sh b/ci/test/00_setup_env_native_centos.sh index 998ddaf45f825..022eb057d2e05 100755 --- a/ci/test/00_setup_env_native_centos.sh +++ b/ci/test/00_setup_env_native_centos.sh @@ -18,4 +18,3 @@ export BITCOIN_CONFIG="\ -DREDUCE_EXPORTS=ON \ -DCMAKE_BUILD_TYPE=Debug \ " -export BITCOIN_CMD="bitcoin -m" # Used in functional tests diff --git a/doc/multiprocess.md b/doc/multiprocess.md index fd047c09b71fb..a632ec1da18b2 100644 --- a/doc/multiprocess.md +++ b/doc/multiprocess.md @@ -26,7 +26,6 @@ HOST_PLATFORM="x86_64-pc-linux-gnu" cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake cmake --build build build/bin/bitcoin -m node -regtest -printtoconsole -debug=ipc -BITCOIN_CMD="bitcoin -m" build/test/functional/test_runner.py ``` The `cmake` build will pick up settings and library locations from the depends directory, so there is no need to pass `-DENABLE_IPC=ON` as a separate flag when using the depends system (it's controlled by the `NO_IPC=1` option). diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 0b9295cef536f..81cd93f136231 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -15,7 +15,6 @@ import pdb import random import re -import shlex import shutil import subprocess import sys @@ -103,20 +102,21 @@ def chainstate_argv(self): def _argv(self, command, bin_path, need_ipc=False): """Return argv array that should be used to invoke the command. It - either uses the bitcoin wrapper executable (if BITCOIN_CMD is set or - need_ipc is True), or the direct binary path (bitcoind, etc). When - bin_dir is set (by tests calling binaries from previous releases) it - always uses the direct path.""" + uses the bitcoin wrapper executable need_ipc is True or any command + other than 'node' (e.g. 'rpc'). Otherwise it uses bitcoind. + When bin_dir is set (by tests calling binaries from previous releases) + it always uses the direct binary path (bitcoind, etc).""" if self.bin_dir is not None: return [os.path.join(self.bin_dir, os.path.basename(bin_path))] - elif self.paths.bitcoin_cmd is not None or need_ipc: + elif need_ipc or command != "node": # If the current test needs IPC functionality, use the bitcoin - # wrapper binary and append -m so it calls multiprocess binaries. - bitcoin_cmd = self.paths.bitcoin_cmd or [self.paths.bitcoin_bin] - return bitcoin_cmd + (["-m"] if need_ipc else []) + [command] + # wrapper binary. It will decide based on arguments whether to + # launch the bitcoind or bitcoin-node binary. + # The wrapper binary is also used for RPC calls (bitcoin-cli). + return [self.paths.bitcoin_bin, command] else: - return [bin_path] - + # Use bitcoind otherwise to retain coverage. + return [self.paths.bitcoind] class BitcoinTestMetaClass(type): """Metaclass for BitcoinTestFramework. @@ -299,9 +299,6 @@ def get_binary_paths(self): binary + self.config["environment"]["EXEEXT"], ) setattr(paths, env_variable_name.lower(), os.getenv(env_variable_name, default=default_filename)) - # BITCOIN_CMD environment variable can be specified to invoke bitcoin - # wrapper binary instead of other executables. - paths.bitcoin_cmd = shlex.split(os.getenv("BITCOIN_CMD", "")) or None return paths def get_binaries(self, bin_dir=None):