From e60ff180c29dde6015f21132e9bc3846a608edf6 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 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 8f5228d9239464ece06f3a56372aeec29685801c 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',