From 025c0c3695ffc0d825eadcba04e7ec1cfc0de00e Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 13 Feb 2025 16:00:15 -0500 Subject: [PATCH 1/7] util: Add cross-platform ExecVp and GetExePath functions These functions are just meant to serve the needs of the bitcoin wrapper executable, and are intentionally not very general purpose so they can be simple. --- src/util/CMakeLists.txt | 1 + src/util/exec.cpp | 107 ++++++++++++++++++++++++++++++++++++++++ src/util/exec.h | 19 +++++++ 3 files changed, 127 insertions(+) create mode 100644 src/util/exec.cpp create mode 100644 src/util/exec.h diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 9ca26a9e27b..491e651127a 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -9,6 +9,7 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL bytevectorhash.cpp chaintype.cpp check.cpp + exec.cpp exception.cpp feefrac.cpp fs.cpp diff --git a/src/util/exec.cpp b/src/util/exec.cpp new file mode 100644 index 00000000000..68fab9b935f --- /dev/null +++ b/src/util/exec.cpp @@ -0,0 +1,107 @@ +// Copyright (c) 2025 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include + +#ifdef WIN32 +#include +#include +#else +#include +#endif + +namespace util { +int ExecVp(const char *file, char *const argv[]) +{ +#ifndef WIN32 + return execvp(file, argv); +#else + std::vector new_argv; + std::vector escaped_args; + for (char* const* arg_ptr{argv}; *arg_ptr; ++arg_ptr) { + std::string_view arg{*arg_ptr}; + if (arg.find_first_of(" \t\"") == std::string_view::npos) { + // Argument has no quotes or spaces so escaping not necessary. + new_argv.push_back(*arg_ptr); + } else { + // Add escaping to the command line that the executable being + // invoked will split up using the CommandLineToArgvW function, + // which expects arguments with spaces to be quoted, quote + // characters to be backslash-escaped, and backslashes to also be + // backslash-escaped, but only if they precede a quote character. + std::string escaped{'"'}; // Start with a quote + for (size_t i = 0; i < arg.size(); ++i) { + if (arg[i] == '\\') { + // Count consecutive backslashes + size_t backslash_count = 0; + while (i < arg.size() && arg[i] == '\\') { + ++backslash_count; + ++i; + } + if (i < arg.size() && arg[i] == '"') { + // Backslashes before a quote need to be doubled + escaped.append(backslash_count * 2 + 1, '\\'); + escaped.push_back('"'); + } else { + // Otherwise, backslashes remain as-is + escaped.append(backslash_count, '\\'); + --i; // Compensate for the outer loop's increment + } + } else if (arg[i] == '"') { + // Escape double quotes with a backslash + escaped.push_back('\\'); + escaped.push_back('"'); + } else { + escaped.push_back(arg[i]); + } + } + escaped.push_back('"'); // End with a quote + escaped_args.emplace_back(std::move(escaped)); + new_argv.push_back((char *)escaped_args.back().c_str()); + } + } + new_argv.push_back(nullptr); + return _execvp(file, new_argv.data()); +#endif +} + +fs::path GetExePath(std::string_view argv0) +{ + // Try to figure out where executable is located. This does a simplified + // search that won't work perfectly on every platform and doesn't need to, + // as it is only currently being used in a convenience wrapper binary to try + // to prioritize locally built or installed executables over system + // executables. + const fs::path argv0_path{fs::PathFromString(std::string{argv0})}; + fs::path path{argv0_path}; + std::error_code ec; +#ifndef WIN32 + // If argv0 doesn't contain a path separator, it was invoked from the system + // PATH and can be searched for there. + if (!argv0_path.has_parent_path()) { + if (const char* path_env = std::getenv("PATH")) { + size_t start{0}, end{0}; + for (std::string_view paths{path_env}; end != std::string_view::npos; start = end + 1) { + end = paths.find(':', start); + fs::path candidate = fs::path(paths.substr(start, end - start)) / argv0_path; + if (fs::is_regular_file(candidate, ec)) { + path = candidate; + break; + } + } + } + } +#else + wchar_t module_path[MAX_PATH]; + if (GetModuleFileNameW(nullptr, module_path, MAX_PATH) > 0) { + path = fs::path{module_path}; + } +#endif + return path; +} + +} // namespace util diff --git a/src/util/exec.h b/src/util/exec.h new file mode 100644 index 00000000000..a43ab046504 --- /dev/null +++ b/src/util/exec.h @@ -0,0 +1,19 @@ +// Copyright (c) 2025 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_UTIL_EXEC_H +#define BITCOIN_UTIL_EXEC_H + +#include + +#include + +namespace util { +//! Cross-platform wrapper for POSIX execvp function. +int ExecVp(const char *file, char *const argv[]); +//! Return path to current executable assuming it was invoked with argv0. +fs::path GetExePath(std::string_view argv0); +} // namespace util + +#endif // BITCOIN_UTIL_EXEC_H From 2b50b42dfe48ae714f7fd57318eade7b746aa98f Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 1 Oct 2024 16:06:28 -0400 Subject: [PATCH 2/7] multiprocess: Add bitcoin wrapper executable Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion. Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983 Co-authored-by: Sjors Provoost --- CMakeLists.txt | 3 + contrib/devtools/security-check.py | 3 + src/CMakeLists.txt | 6 + src/bitcoin.cpp | 196 +++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+) create mode 100644 src/bitcoin.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index cd388570f0c..4c3fa77eb41 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -84,6 +84,7 @@ endif() #============================= include(CMakeDependentOption) # When adding a new option, end the with a full stop for consistency. +option(BUILD_BITCOIN_BIN "Build bitcoin executable." ON) option(BUILD_DAEMON "Build bitcoind executable." ON) option(BUILD_GUI "Build bitcoin-qt executable." OFF) option(BUILD_CLI "Build bitcoin-cli executable." ON) @@ -218,6 +219,7 @@ target_link_libraries(core_interface INTERFACE if(BUILD_FOR_FUZZING) message(WARNING "BUILD_FOR_FUZZING=ON will disable all other targets and force BUILD_FUZZ_BINARY=ON.") + set(BUILD_BITCOIN_BIN OFF) set(BUILD_DAEMON OFF) set(BUILD_CLI OFF) set(BUILD_TX OFF) @@ -631,6 +633,7 @@ message("\n") message("Configure summary") message("=================") message("Executables:") +message(" bitcoin ............................. ${BUILD_BITCOIN_BIN}") message(" bitcoind ............................ ${BUILD_DAEMON}") if(BUILD_DAEMON AND WITH_MULTIPROCESS) set(bitcoin_daemon_status ON) diff --git a/contrib/devtools/security-check.py b/contrib/devtools/security-check.py index 4c20685b51c..6f07dbca580 100755 --- a/contrib/devtools/security-check.py +++ b/contrib/devtools/security-check.py @@ -122,6 +122,9 @@ def check_ELF_FORTIFY(binary) -> bool: # bitcoin-util does not currently contain any fortified functions if 'Bitcoin Core bitcoin-util utility version ' in binary.strings: return True + # bitcoin wrapper does not currently contain any fortified functions + if '--monolithic' in binary.strings: + return True chk_funcs = set() diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 07544f59cfa..7c543951dc2 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -304,6 +304,12 @@ target_link_libraries(bitcoin_node $ ) +# Bitcoin wrapper executable that can call other executables. +if(BUILD_BITCOIN_BIN) + add_executable(bitcoin bitcoin.cpp) + target_link_libraries(bitcoin core_interface bitcoin_util) + install_binary_component(bitcoin) +endif() # Bitcoin Core bitcoind. if(BUILD_DAEMON) diff --git a/src/bitcoin.cpp b/src/bitcoin.cpp new file mode 100644 index 00000000000..7bb2a45933a --- /dev/null +++ b/src/bitcoin.cpp @@ -0,0 +1,196 @@ +// Copyright (c) 2024 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include // IWYU pragma: keep + +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +const TranslateFn G_TRANSLATION_FUN{nullptr}; + +static constexpr auto HELP_USAGE = R"(Usage: %s [OPTIONS] COMMAND... + +Options: + -m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui. + -M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior) + -v, --version Show version information + -h, --help Show this help message + +Commands: + gui [ARGS] Start GUI, equivalent to running 'bitcoin-qt [ARGS]' or 'bitcoin-gui [ARGS]'. + daemon [ARGS] Start daemon, equivalent to running 'bitcoind [ARGS]' or 'bitcoin-node [ARGS]'. + rpc [ARGS] Call RPC method, equivalent to running 'bitcoin-cli -named [ARGS]'. + wallet [ARGS] Call wallet command, equivalent to running 'bitcoin-wallet [ARGS]'. + tx [ARGS] Manipulate hex-encoded transactions, equivalent to running 'bitcoin-tx [ARGS]'. + help [-a] Show this help message. Include -a or --all to show additional internal commands. +)"; + +static constexpr auto HELP_INTERNAL = R"( +Additional internal commands: + bench [ARGS] Run bench command, equivalent to running 'bench_bitcoin [ARGS]'. + test [ARGS] Run unit tests, equivalent to running 'test_bitcoin [ARGS]'. + test-gui [ARGS] Run GUI unit tests, equivalent to running 'test_bitcoin-qt [ARGS]'. +)"; + +struct CommandLine { + bool use_multiprocess{false}; + bool show_version{false}; + bool show_help{false}; + bool show_help_all{false}; + std::string_view command; + std::vector args; +}; + +CommandLine ParseCommandLine(int argc, char* argv[]); +static void ExecCommand(const std::vector& args, std::string_view argv0); + +int main(int argc, char* argv[]) +{ + try { + CommandLine cmd{ParseCommandLine(argc, argv)}; + if (cmd.show_version) { + tfm::format(std::cout, "%s version %s\n%s", CLIENT_NAME, FormatFullVersion(), FormatParagraph(LicenseInfo())); + return EXIT_SUCCESS; + } + + std::vector args; + if (cmd.show_help || cmd.command.empty()) { + tfm::format(std::cout, HELP_USAGE, argv[0]); + if (cmd.show_help_all) tfm::format(std::cout, HELP_INTERNAL); + return cmd.show_help ? EXIT_SUCCESS : EXIT_FAILURE; + } else if (cmd.command == "gui") { + args.emplace_back(cmd.use_multiprocess ? "qt/bitcoin-gui" : "qt/bitcoin-qt"); + } else if (cmd.command == "daemon") { + args.emplace_back(cmd.use_multiprocess ? "bitcoin-node" : "bitcoind"); + } else if (cmd.command == "rpc") { + args.emplace_back("bitcoin-cli"); + args.emplace_back("-named"); + } else if (cmd.command == "wallet") { + args.emplace_back("bitcoin-wallet"); + } else if (cmd.command == "tx") { + args.emplace_back("bitcoin-tx"); + } else if (cmd.command == "bench") { + args.emplace_back("bench/bench_bitcoin"); + } else if (cmd.command == "test") { + args.emplace_back("test/test_bitcoin"); + } else if (cmd.command == "test-gui") { + args.emplace_back("qt/test/test_bitcoin-qt"); + } else if (cmd.command == "util") { + args.emplace_back("bitcoin-util"); + } else { + throw std::runtime_error(strprintf("Unrecognized command: '%s'", cmd.command)); + } + if (!args.empty()) { + args.insert(args.end(), cmd.args.begin(), cmd.args.end()); + ExecCommand(args, argv[0]); + } + } catch (const std::exception& e) { + tfm::format(std::cerr, "Error: %s\nTry '%s --help' for more information.\n", e.what(), argv[0]); + return EXIT_FAILURE; + } + return EXIT_SUCCESS; +} + +CommandLine ParseCommandLine(int argc, char* argv[]) +{ + CommandLine cmd; + cmd.args.reserve(argc); + for (int i = 1; i < argc; ++i) { + std::string_view arg = argv[i]; + if (!cmd.command.empty()) { + cmd.args.emplace_back(argv[i]); + } else if (arg == "-m" || arg == "--multiprocess") { + cmd.use_multiprocess = true; + } else if (arg == "-M" || arg == "--monolithic") { + cmd.use_multiprocess = false; + } else if (arg == "-v" || arg == "--version") { + cmd.show_version = true; + } else if (arg == "-h" || arg == "--help" || arg == "help") { + cmd.show_help = true; + } else if (cmd.show_help && (arg == "-a" || arg == "--all")) { + cmd.show_help_all = true; + } else if (arg.starts_with("-")) { + throw std::runtime_error(strprintf("Unknown option: %s", arg)); + } else if (!arg.empty()) { + cmd.command = arg; + } + } + return cmd; +} + +//! 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`. +//! +//! @param args Command line arguments to execute, where first argument should +//! be a relative path to a bitcoind, bitcoin-qt or other executable +//! that will be located on the PATH or relative to wrapper_argv0. +//! +//! @param wrapper_argv0 String containing first command line argument passed to +//! main() to run the current executable. This is used to +//! help determine the path to the current executable and +//! how to look for new executables. +// +//! @note This function doesn't currently print anything but can be debugged +//! from the command line using strace or dtrace like: +//! +//! strace -e trace=execve -s 10000 build/src/bitcoin ... +//! dtrace -n 'proc:::exec-success /pid == $target/ { trace(curpsinfo->pr_psargs); }' -c ... +static void ExecCommand(const std::vector& args, std::string_view wrapper_argv0) +{ + // Construct argument string for execvp + std::vector exec_args{args}; + exec_args.emplace_back(nullptr); + + // Try to call ExecVp with given exe path. + auto try_exec = [&](fs::path exe_path, bool allow_notfound = true) { + std::string exe_path_str{fs::PathToString(exe_path)}; + exec_args[0] = exe_path_str.c_str(); + if (util::ExecVp(exec_args[0], (char*const*)exec_args.data()) == -1) { + if (allow_notfound && errno == ENOENT) return false; + throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0])); + } + return true; // In practice, this line should not be reached if execvp succeeds + }; + + // Get the wrapper executable path. + const fs::path wrapper_path{util::GetExePath(wrapper_argv0)}; + + // Try to resolve any symlinks and figure out the directory containing the wrapper executable. + std::error_code ec; + fs::path wrapper_dir{fs::weakly_canonical(wrapper_path, ec)}; + if (wrapper_dir.empty()) wrapper_dir = wrapper_path; // Restore previous path if weakly_canonical failed. + wrapper_dir = wrapper_dir.parent_path(); + + // Get path of the executable to be invoked. + const fs::path arg0{fs::PathFromString(args[0])}; + + // Decide whether to fall back to the operating system to search for the + // specified executable. Avoid doing this if it looks like the wrapper + // executable was invoked by path, rather than by search, to avoid + // unintentionally launching system executables in a local build. + // (https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1861814807) + const bool fallback_os_search{!fs::PathFromString(std::string{wrapper_argv0}).has_parent_path()}; + + // If wrapper is in a CMake build tree, first look for target executable + // relative to it. + (wrapper_dir.filename() == "src" && try_exec(wrapper_dir / arg0)) || + // Otherwise if wrapper is installed in a bin/ directory, look for + // target executable in libexec/ + (wrapper_dir.filename() == "bin" && try_exec(fs::path{wrapper_dir.parent_path()} / "libexec" / arg0.filename())) || + // Otherwise check the "daemon" subdirectory in a windows install. + try_exec(wrapper_dir / "daemon" / arg0.filename()) || + // Otherwise look for target executable next to current wrapper + try_exec(wrapper_dir / arg0.filename(), fallback_os_search) || + // Otherwise just look on the system path. + (fallback_os_search && try_exec(arg0.filename(), false)); +}; From a92ecaba9521fd1d28fb0018f0708499f3354ad4 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 26 Nov 2024 11:51:41 -0500 Subject: [PATCH 3/7] test, refactor: Add TestNode.binaries to hold binary paths Add new TestNode.binaries object to manage paths to bitcoin binaries. Having this object makes it possible for the test framework to exercise the bitcoin wrapper executable introduced in https://github.com/bitcoin/bitcoin/pull/31375 and also makes it easier to add new binaries and options and environment variables controlling how they are invoked, because logic for invoking them that was previously spread out is now consolidated in one place. Co-authored-by: Sjors Provoost --- .../test_framework/test_framework.py | 82 ++++++++++++++----- test/functional/test_framework/test_node.py | 19 ++--- test/functional/tool_signet_miner.py | 6 +- test/functional/tool_wallet.py | 2 +- test/functional/wallet_encryption.py | 2 +- 5 files changed, 77 insertions(+), 34 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 921f12d9fb4..02be8b19f85 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -18,6 +18,7 @@ import subprocess import sys import tempfile import time +import types from .address import create_deterministic_address_bcrt1_p2tr_op_true from .authproxy import JSONRPCException @@ -56,6 +57,48 @@ class SkipTest(Exception): self.message = message +class Binaries: + """Helper class to provide information about bitcoin binaries + + Attributes: + paths: Object returned from get_binary_paths() containing information + which binaries and command lines to use from environment variables and + the config file. + bin_dir: An optional string containing a directory path to look for + binaries, which takes precedence over the paths above, if specified. + This is used by tests calling binaries from previous releases. + """ + def __init__(self, paths, bin_dir): + self.paths = paths + self.bin_dir = bin_dir + + def daemon_argv(self): + "Return argv array that should be used to invoke bitcoind" + return self._argv(self.paths.bitcoind) + + def rpc_argv(self): + "Return argv array that should be used to invoke bitcoin-cli" + return self._argv(self.paths.bitcoincli) + + def util_argv(self): + "Return argv array that should be used to invoke bitcoin-util" + return self._argv(self.paths.bitcoinutil) + + def wallet_argv(self): + "Return argv array that should be used to invoke bitcoin-wallet" + return self._argv(self.paths.bitcoinwallet) + + def _argv(self, bin_path): + """Return argv array that should be used to invoke the command. + Normally this will return binary paths directly from the paths object, + but when bin_dir is set (by tests calling binaries from previous + releases) it will return paths relative to bin_dir instead.""" + if self.bin_dir is not None: + return [os.path.join(self.bin_dir, os.path.basename(bin_path))] + else: + return [bin_path] + + class BitcoinTestMetaClass(type): """Metaclass for BitcoinTestFramework. @@ -220,6 +263,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): config = configparser.ConfigParser() config.read_file(open(self.options.configfile)) self.config = config + self.binary_paths = self.get_binary_paths() if self.options.v1transport: self.options.v2transport=False @@ -243,9 +287,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): PortSeed.n = self.options.port_seed - def set_binary_paths(self): - """Update self.options with the paths of all binaries from environment variables or their default values""" + def get_binary_paths(self): + """Get paths of all binaries from environment variables or their default values""" + paths = types.SimpleNamespace() binaries = { "bitcoind": ("bitcoind", "BITCOIND"), "bitcoin-cli": ("bitcoincli", "BITCOINCLI"), @@ -258,7 +303,11 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): "src", binary + self.config["environment"]["EXEEXT"], ) - setattr(self.options, attribute_name, os.getenv(env_variable_name, default=default_filename)) + setattr(paths, attribute_name, os.getenv(env_variable_name, default=default_filename)) + return paths + + def get_binaries(self, bin_dir=None): + return Binaries(self.binary_paths, bin_dir) def setup(self): """Call this method to start up the test framework object with options set.""" @@ -269,8 +318,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): config = self.config - self.set_binary_paths() - os.environ['PATH'] = os.pathsep.join([ os.path.join(config['environment']['BUILDDIR'], 'src'), os.path.join(config['environment']['BUILDDIR'], 'src', 'qt'), os.environ['PATH'] @@ -477,14 +524,14 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): group.add_argument("--legacy-wallet", action='store_const', const=False, **kwargs, help="Run test using legacy wallets", dest='descriptors') - def add_nodes(self, num_nodes: int, extra_args=None, *, rpchost=None, binary=None, binary_cli=None, versions=None): + def add_nodes(self, num_nodes: int, extra_args=None, *, rpchost=None, versions=None): """Instantiate TestNode objects. Should only be called once after the nodes have been specified in set_test_params().""" - def get_bin_from_version(version, bin_name, bin_default): + def bin_dir_from_version(version): if not version: - return bin_default + return None if version > 219999: # Starting at client version 220000 the first two digits represent # the major version, e.g. v22.0 instead of v0.22.0. @@ -502,7 +549,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): ), ), 'bin', - bin_name, ) if self.bind_to_localhost_only: @@ -517,13 +563,12 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): extra_args[i] = extra_args[i] + ["-whitelist=noban,in,out@127.0.0.1"] if versions is None: versions = [None] * num_nodes - if binary is None: - binary = [get_bin_from_version(v, 'bitcoind', self.options.bitcoind) for v in versions] - if binary_cli is None: - binary_cli = [get_bin_from_version(v, 'bitcoin-cli', self.options.bitcoincli) for v in versions] + bin_dirs = [bin_dir_from_version(v) for v in versions] # Fail test if any of the needed release binaries is missing bins_missing = False - for bin_path in binary + binary_cli: + for bin_path in (argv[0] for bin_dir in bin_dirs + for binaries in (self.get_binaries(bin_dir),) + for argv in (binaries.daemon_argv(), binaries.rpc_argv())): if shutil.which(bin_path) is None: self.log.error(f"Binary not found: {bin_path}") bins_missing = True @@ -533,8 +578,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): assert_equal(len(extra_confs), num_nodes) assert_equal(len(extra_args), num_nodes) assert_equal(len(versions), num_nodes) - assert_equal(len(binary), num_nodes) - assert_equal(len(binary_cli), num_nodes) + assert_equal(len(bin_dirs), num_nodes) for i in range(num_nodes): args = list(extra_args[i]) test_node_i = TestNode( @@ -544,8 +588,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): rpchost=rpchost, timewait=self.rpc_timeout, timeout_factor=self.options.timeout_factor, - bitcoind=binary[i], - bitcoin_cli=binary_cli[i], + binaries=self.get_binaries(bin_dirs[i]), version=versions[i], coverage_dir=self.options.coveragedir, cwd=self.options.tmpdir, @@ -856,8 +899,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): rpchost=None, timewait=self.rpc_timeout, timeout_factor=self.options.timeout_factor, - bitcoind=self.options.bitcoind, - bitcoin_cli=self.options.bitcoincli, + binaries=self.get_binaries(), coverage_dir=None, cwd=self.options.tmpdir, descriptors=self.options.descriptors, diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index f7d6ba78d23..28ffb95a962 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -76,7 +76,7 @@ class TestNode(): To make things easier for the test writer, any unrecognised messages will be dispatched to the RPC connection.""" - def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False, v2transport=False): + def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, binaries, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False, v2transport=False): """ Kwargs: start_perf (bool): If True, begin profiling the node with `perf` as soon as @@ -92,7 +92,7 @@ class TestNode(): self.chain = chain self.rpchost = rpchost self.rpc_timeout = timewait - self.binary = bitcoind + self.binaries = binaries self.coverage_dir = coverage_dir self.cwd = cwd self.descriptors = descriptors @@ -109,8 +109,7 @@ class TestNode(): # Configuration for logging is set as command-line args rather than in the bitcoin.conf file. # This means that starting a bitcoind using the temp dir to debug a failed test won't # spam debug.log. - self.args = [ - self.binary, + self.args = self.binaries.daemon_argv() + [ f"-datadir={self.datadir_path}", "-logtimemicros", "-debug", @@ -149,7 +148,7 @@ class TestNode(): self.args.append("-v2transport=0") # if v2transport is requested via global flag but not supported for node version, ignore it - self.cli = TestNodeCLI(bitcoin_cli, self.datadir_path) + self.cli = TestNodeCLI(binaries, self.datadir_path) self.use_cli = use_cli self.start_perf = start_perf @@ -865,16 +864,16 @@ def arg_to_cli(arg): class TestNodeCLI(): """Interface to bitcoin-cli for an individual node""" - def __init__(self, binary, datadir): + def __init__(self, binaries, datadir): self.options = [] - self.binary = binary + self.binaries = binaries self.datadir = datadir self.input = None self.log = logging.getLogger('TestFramework.bitcoincli') def __call__(self, *options, input=None): # TestNodeCLI is callable with bitcoin-cli command-line options - cli = TestNodeCLI(self.binary, self.datadir) + cli = TestNodeCLI(self.binaries, self.datadir) cli.options = [str(o) for o in options] cli.input = input return cli @@ -895,7 +894,7 @@ class TestNodeCLI(): """Run bitcoin-cli command. Deserializes returned string as python object.""" pos_args = [arg_to_cli(arg) for arg in args] named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items()] - p_args = [self.binary, f"-datadir={self.datadir}"] + self.options + p_args = self.binaries.rpc_argv() + [f"-datadir={self.datadir}"] + self.options if named_args: p_args += ["-named"] if clicommand is not None: @@ -911,7 +910,7 @@ class TestNodeCLI(): code, message = match.groups() raise JSONRPCException(dict(code=int(code), message=message)) # Ignore cli_stdout, raise with cli_stderr - raise subprocess.CalledProcessError(returncode, self.binary, output=cli_stderr) + raise subprocess.CalledProcessError(returncode, p_args, output=cli_stderr) try: return json.loads(cli_stdout, parse_float=decimal.Decimal) except (json.JSONDecodeError, decimal.InvalidOperation): diff --git a/test/functional/tool_signet_miner.py b/test/functional/tool_signet_miner.py index 00841585548..16386f76d8b 100755 --- a/test/functional/tool_signet_miner.py +++ b/test/functional/tool_signet_miner.py @@ -49,13 +49,15 @@ class SignetMinerTest(BitcoinTestFramework): # generate block with signet miner tool base_dir = self.config["environment"]["SRCDIR"] signet_miner_path = os.path.join(base_dir, "contrib", "signet", "miner") + rpc_argv = node.binaries.rpc_argv() + [f"-datadir={node.cli.datadir}"] + util_argv = node.binaries.util_argv() + ["grind"] subprocess.run([ sys.executable, signet_miner_path, - f'--cli={node.cli.binary} -datadir={node.cli.datadir}', + f'--cli={" ".join(rpc_argv)}', 'generate', f'--address={node.getnewaddress()}', - f'--grind-cmd={self.options.bitcoinutil} grind', + f'--grind-cmd={" ".join(util_argv)}', f'--nbits={DIFF_1_N_BITS:08x}', f'--set-block-time={int(time.time())}', '--poolnum=99', diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index b0b9adab879..276e0227bb4 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -49,7 +49,7 @@ class ToolWalletTest(BitcoinTestFramework): if "dump" in args and self.options.bdbro: default_args.append("-withinternalbdb") - return subprocess.Popen([self.options.bitcoinwallet] + default_args + list(args), stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + return subprocess.Popen(self.get_binaries().wallet_argv() + default_args + list(args), stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) def assert_raises_tool_error(self, error, *args): p = self.bitcoin_wallet_process(*args) diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py index 5e131405f1c..4171951cf89 100755 --- a/test/functional/wallet_encryption.py +++ b/test/functional/wallet_encryption.py @@ -112,7 +112,7 @@ class WalletEncryptionTest(BitcoinTestFramework): def do_wallet_tool(*args): proc = subprocess.Popen( - [self.options.bitcoinwallet, f"-datadir={self.nodes[0].datadir_path}", f"-chain={self.chain}"] + list(args), + self.get_binaries().wallet_argv() + [f"-datadir={self.nodes[0].datadir_path}", f"-chain={self.chain}"] + list(args), stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, From 6c457f40436a681bd42f81d28ea7b14c1ed3aa47 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 26 Nov 2024 11:51:41 -0500 Subject: [PATCH 4/7] test, contrib: Fix signer/miner command line escaping Pass bitcoin binary command lines from test framework to signet/miner utility using shell escaping so they are unambigous and don't get mangled if they contain spaces. This change is not needed for tests to pass currently, but is a useful change to avoid CI failures in followup PR https://github.com/bitcoin/bitcoin/pull/31375 and to avoid other bugs. --- contrib/signet/miner | 8 ++++---- test/functional/tool_signet_miner.py | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/contrib/signet/miner b/contrib/signet/miner index 3c90fe96a1d..e020c4589c1 100755 --- a/contrib/signet/miner +++ b/contrib/signet/miner @@ -9,7 +9,7 @@ import logging import math import os import re -import struct +import shlex import sys import time import subprocess @@ -86,7 +86,7 @@ def finish_block(block, signet_solution, grind_cmd): block.solve() else: headhex = CBlockHeader.serialize(block).hex() - cmd = grind_cmd.split(" ") + [headhex] + cmd = shlex.split(grind_cmd) + [headhex] newheadhex = subprocess.run(cmd, stdout=subprocess.PIPE, input=b"", check=True).stdout.strip() newhead = from_hex(CBlockHeader(), newheadhex.decode('utf8')) block.nNonce = newhead.nNonce @@ -479,7 +479,7 @@ def do_calibrate(args): header.nTime = i header.nNonce = 0 headhex = header.serialize().hex() - cmd = args.grind_cmd.split(" ") + [headhex] + cmd = shlex.split(args.grind_cmd) + [headhex] newheadhex = subprocess.run(cmd, stdout=subprocess.PIPE, input=b"", check=True).stdout.strip() avg = (time.time() - start) * 1.0 / TRIALS @@ -549,7 +549,7 @@ def main(): args = parser.parse_args(sys.argv[1:]) - args.bcli = lambda *a, input=b"", **kwargs: bitcoin_cli(args.cli.split(" "), list(a), input=input, **kwargs) + args.bcli = lambda *a, input=b"", **kwargs: bitcoin_cli(shlex.split(args.cli), list(a), input=input, **kwargs) if hasattr(args, "address") and hasattr(args, "descriptor"): args.derived_addresses = {} diff --git a/test/functional/tool_signet_miner.py b/test/functional/tool_signet_miner.py index 16386f76d8b..11b6af4e9dd 100755 --- a/test/functional/tool_signet_miner.py +++ b/test/functional/tool_signet_miner.py @@ -5,6 +5,7 @@ """Test signet miner tool""" import os.path +import shlex import subprocess import sys import time @@ -54,10 +55,10 @@ class SignetMinerTest(BitcoinTestFramework): subprocess.run([ sys.executable, signet_miner_path, - f'--cli={" ".join(rpc_argv)}', + f'--cli={shlex.join(rpc_argv)}', 'generate', f'--address={node.getnewaddress()}', - f'--grind-cmd={" ".join(util_argv)}', + f'--grind-cmd={shlex.join(util_argv)}', f'--nbits={DIFF_1_N_BITS:08x}', f'--set-block-time={int(time.time())}', '--poolnum=99', From c10edb869adcd1079e59c538d933a1e57bbc33a8 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 26 Nov 2024 11:51:41 -0500 Subject: [PATCH 5/7] test: Support BITCOIN_CMD environment variable Support new BITCOIN_CMD environment variable in functional test to be able to test the new bitcoin wrapper executable and run other commands through it instead of calling them directly. Co-authored-by: Sjors Provoost --- .../test_framework/test_framework.py | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 02be8b19f85..8fcee195804 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -13,6 +13,7 @@ import platform import pdb import random import re +import shlex import shutil import subprocess import sys @@ -74,27 +75,31 @@ class Binaries: def daemon_argv(self): "Return argv array that should be used to invoke bitcoind" - return self._argv(self.paths.bitcoind) + return self._argv("daemon", self.paths.bitcoind) def rpc_argv(self): "Return argv array that should be used to invoke bitcoin-cli" - return self._argv(self.paths.bitcoincli) + # Add -nonamed because "bitcoin rpc" enables -named by default, but bitcoin-cli doesn't + return self._argv("rpc", self.paths.bitcoincli) + ["-nonamed"] def util_argv(self): "Return argv array that should be used to invoke bitcoin-util" - return self._argv(self.paths.bitcoinutil) + return self._argv("util", self.paths.bitcoinutil) def wallet_argv(self): "Return argv array that should be used to invoke bitcoin-wallet" - return self._argv(self.paths.bitcoinwallet) + return self._argv("wallet", self.paths.bitcoinwallet) - def _argv(self, bin_path): - """Return argv array that should be used to invoke the command. - Normally this will return binary paths directly from the paths object, - but when bin_dir is set (by tests calling binaries from previous - releases) it will return paths relative to bin_dir instead.""" + def _argv(self, command, bin_path): + """Return argv array that should be used to invoke the command. It + either uses the bitcoin wrapper executable (if BITCOIN_CMD is set), 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.""" 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: + return self.paths.bitcoin_cmd + [command] else: return [bin_path] @@ -304,6 +309,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): binary + self.config["environment"]["EXEEXT"], ) setattr(paths, attribute_name, 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): From b1670d28a6a18385cc8740bce25f737428d3925c Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 26 Nov 2024 11:58:16 -0500 Subject: [PATCH 6/7] ci: Run multiprocess tests through wrapper executable --- ci/test/00_setup_env_i686_multiprocess.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/test/00_setup_env_i686_multiprocess.sh b/ci/test/00_setup_env_i686_multiprocess.sh index 1aa143a7f08..cbc5b14a406 100755 --- a/ci/test/00_setup_env_i686_multiprocess.sh +++ b/ci/test/00_setup_env_i686_multiprocess.sh @@ -20,4 +20,4 @@ export BITCOIN_CONFIG="\ -DCMAKE_CXX_COMPILER='clang++;-m32' \ -DAPPEND_CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE' \ " -export BITCOIND=bitcoin-node # Used in functional tests +export BITCOIN_CMD="bitcoin -m" # Used in functional tests From 623c0f0df878968dabbaa271a7654c638603f369 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 10 Jan 2025 15:33:52 +0100 Subject: [PATCH 7/7] build: add bitcoin.exe to windows installer --- cmake/module/GenerateSetupNsi.cmake | 1 + cmake/module/Maintenance.cmake | 5 +++-- share/setup.nsi.in | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cmake/module/GenerateSetupNsi.cmake b/cmake/module/GenerateSetupNsi.cmake index 97a53b071db..c8d5bd67c5f 100644 --- a/cmake/module/GenerateSetupNsi.cmake +++ b/cmake/module/GenerateSetupNsi.cmake @@ -7,6 +7,7 @@ function(generate_setup_nsi) set(abs_top_builddir ${PROJECT_BINARY_DIR}) set(CLIENT_URL ${PROJECT_HOMEPAGE_URL}) set(CLIENT_TARNAME "bitcoin") + set(BITCOIN_WRAPPER_NAME "bitcoin") set(BITCOIN_GUI_NAME "bitcoin-qt") set(BITCOIN_DAEMON_NAME "bitcoind") set(BITCOIN_CLI_NAME "bitcoin-cli") diff --git a/cmake/module/Maintenance.cmake b/cmake/module/Maintenance.cmake index d337625abc8..bd37f18bde2 100644 --- a/cmake/module/Maintenance.cmake +++ b/cmake/module/Maintenance.cmake @@ -23,7 +23,7 @@ function(add_maintenance_targets) return() endif() - foreach(target IN ITEMS bitcoind bitcoin-qt bitcoin-cli bitcoin-tx bitcoin-util bitcoin-wallet test_bitcoin bench_bitcoin) + foreach(target IN ITEMS bitcoin bitcoind bitcoin-qt bitcoin-cli bitcoin-tx bitcoin-util bitcoin-wallet test_bitcoin bench_bitcoin) if(TARGET ${target}) list(APPEND executables $) endif() @@ -47,7 +47,7 @@ function(add_maintenance_targets) endfunction() function(add_windows_deploy_target) - if(MINGW AND TARGET bitcoin-qt AND TARGET bitcoind AND TARGET bitcoin-cli AND TARGET bitcoin-tx AND TARGET bitcoin-wallet AND TARGET bitcoin-util AND TARGET test_bitcoin) + if(MINGW AND TARGET bitcoin AND TARGET bitcoin-qt AND TARGET bitcoind AND TARGET bitcoin-cli AND TARGET bitcoin-tx AND TARGET bitcoin-wallet AND TARGET bitcoin-util AND TARGET test_bitcoin) # TODO: Consider replacing this code with the CPack NSIS Generator. # See https://cmake.org/cmake/help/latest/cpack_gen/nsis.html include(GenerateSetupNsi) @@ -55,6 +55,7 @@ function(add_windows_deploy_target) add_custom_command( OUTPUT ${PROJECT_BINARY_DIR}/bitcoin-win64-setup.exe COMMAND ${CMAKE_COMMAND} -E make_directory ${PROJECT_BINARY_DIR}/release + COMMAND ${CMAKE_STRIP} $ -o ${PROJECT_BINARY_DIR}/release/$ COMMAND ${CMAKE_STRIP} $ -o ${PROJECT_BINARY_DIR}/release/$ COMMAND ${CMAKE_STRIP} $ -o ${PROJECT_BINARY_DIR}/release/$ COMMAND ${CMAKE_STRIP} $ -o ${PROJECT_BINARY_DIR}/release/$ diff --git a/share/setup.nsi.in b/share/setup.nsi.in index d1a85cdb7f3..1e10e0a65e9 100644 --- a/share/setup.nsi.in +++ b/share/setup.nsi.in @@ -73,6 +73,7 @@ Section -Main SEC0000 SetOutPath $INSTDIR SetOverwrite on File @abs_top_builddir@/release/@BITCOIN_GUI_NAME@@EXEEXT@ + File @abs_top_builddir@/release/@BITCOIN_WRAPPER_NAME@@EXEEXT@ File /oname=COPYING.txt @abs_top_srcdir@/COPYING File /oname=readme.txt @abs_top_srcdir@/doc/README_windows.txt File @abs_top_srcdir@/share/examples/bitcoin.conf