Merge bitcoin/bitcoin#31866: test, refactor: Add TestNode.binaries to hold binary paths

d190f0facc8379da7610d7161e532d57c6a7eb96 test, contrib: Fix signer/miner command line escaping (Ryan Ofsky)
0d2eefca8bf3cb64e3f2f912ed32118524430967 test, refactor: Add TestNode.binaries to hold binary paths (Ryan Ofsky)

Pull request description:

  Add new `TestNode.binaries` object to manage paths to bitcoin binaries.

  The `binaries` 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 in general to add new binaries, and new 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.

  These changes were originally part of #31375 but made that PR harder to review because they were unrelated to the other changes there. If this PR can get merged first, python changes in #31375 will be simple, and the test framework changes here should also get a higher quality review.

ACKs for top commit:
  maflcko:
    re-review-ACK d190f0facc8379da7610d7161e532d57c6a7eb96 🍓
  Sjors:
    ACK d190f0facc8379da7610d7161e532d57c6a7eb96
  vasild:
    ACK d190f0facc8379da7610d7161e532d57c6a7eb96

Tree-SHA512: 5a6c0553cd2822585810d827ef1c1772cbf3097d3336daf733f8378dd3da79c00fc3721e50ed0f7455908fbd7a509e9739f9be33f588d6bc1aaa400b9d75c650

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 = {}

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,

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):

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
@@ -49,13 +50,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={shlex.join(rpc_argv)}',
                 'generate',
                 f'--address={node.getnewaddress()}',
-                f'--grind-cmd={self.options.bitcoinutil} grind',
+                f'--grind-cmd={shlex.join(util_argv)}',
                 f'--nbits={DIFF_1_N_BITS:08x}',
                 f'--set-block-time={int(time.time())}',
                 '--poolnum=99',

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)

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,