From 0d2eefca8bf3cb64e3f2f912ed32118524430967 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 26 Nov 2024 11:51:41 -0500 Subject: [PATCH 1/2] 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 7596fca7b9a..c4b10af16c7 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 @@ -239,9 +283,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"), @@ -254,7 +299,11 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): "bin", 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.""" @@ -265,8 +314,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): config = self.config - self.set_binary_paths() - os.environ['PATH'] = os.pathsep.join([ os.path.join(config['environment']['BUILDDIR'], 'bin'), os.environ['PATH'] @@ -473,14 +520,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. @@ -498,7 +545,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): ), ), 'bin', - bin_name, ) if self.bind_to_localhost_only: @@ -513,13 +559,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 @@ -529,8 +574,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( @@ -540,8 +584,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, @@ -852,8 +895,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 52efadb6e22..84c32cafac9 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 @@ -870,16 +869,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 @@ -900,7 +899,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: @@ -916,7 +915,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 0d65b0cd177..387ba276dd9 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -48,7 +48,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 d190f0facc8379da7610d7161e532d57c6a7eb96 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 26 Nov 2024 11:51:41 -0500 Subject: [PATCH 2/2] 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',