diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 67def9b5668..fb0fc0af6b8 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -179,7 +179,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): help="The seed to use for assigning port numbers (default: current process id)") parser.add_argument("--previous-releases", dest="prev_releases", action="store_true", default=os.path.isdir(previous_releases_path) and bool(os.listdir(previous_releases_path)), - help="Force test of previous releases (default: %(default)s)") + help="Force test of previous releases (default: %(default)s). Previous releases binaries can be downloaded via `test/get_previous_releases.py`.") parser.add_argument("--coveragedir", dest="coveragedir", help="Write tested RPC commands into this directory") parser.add_argument("--configfile", dest="configfile", @@ -192,7 +192,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): parser.add_argument("--perf", dest="perf", default=False, action="store_true", help="profile running nodes with perf for the duration of the test") parser.add_argument("--valgrind", dest="valgrind", default=False, action="store_true", - help="run nodes under the valgrind memory error detector: expect at least a ~10x slowdown. valgrind 3.14 or later required. Does not apply to previous release binaries.") + help="Run binaries under the valgrind memory error detector: Expect at least a ~10x slowdown. Does not apply to previous release binaries.") parser.add_argument("--randomseed", type=int, help="set a random seed for deterministically reproducing a previous test run") parser.add_argument("--timeout-factor", dest="timeout_factor", type=float, help="adjust test timeouts by a factor. Setting it to 0 disables all timeouts") @@ -223,7 +223,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): PortSeed.n = self.options.port_seed def get_binaries(self, bin_dir=None): - return Binaries(self.binary_paths, bin_dir) + return Binaries(self.binary_paths, bin_dir, use_valgrind=self.options.valgrind) def setup(self): """Call this method to start up the test framework object with options set.""" @@ -458,18 +458,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): bin_dirs = [] for v in versions: bin_dir = bin_dir_from_version(v) - - # Fail test if any of the needed release binaries is missing - for bin_path in (argv[0] for binaries in (self.get_binaries(bin_dir),) - for argv in (binaries.node_argv(), binaries.rpc_argv())): - - if shutil.which(bin_path) is None: - self.log.error(f"Binary not found: {bin_path}") - if v is None: - raise AssertionError("At least one binary is missing, did you compile?") - raise AssertionError("At least one release binary is missing. " - "Previous releases binaries can be downloaded via `test/get_previous_releases.py`.") - bin_dirs.append(bin_dir) extra_init = [{}] * num_nodes if self.extra_init is None else self.extra_init # type: ignore[var-annotated] @@ -493,7 +481,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): extra_args=args, use_cli=self.options.usecli, start_perf=self.options.perf, - use_valgrind=self.options.valgrind, v2transport=self.options.v2transport, uses_wallet=self.uses_wallet, ) @@ -969,8 +956,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): """Checks whether previous releases are present and enabled.""" if not os.path.isdir(self.options.previous_releases_path): if self.options.prev_releases: - raise AssertionError("Force test of previous releases but releases missing: {}".format( - self.options.previous_releases_path)) + raise AssertionError(f"Force test of previous releases but releases missing: {self.options.previous_releases_path}\n" + "Previous releases binaries can be downloaded via `test/get_previous_releases.py`.") return self.options.prev_releases def skip_if_no_external_signer(self): diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 9f9ad33c451..4bcf01e37c0 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -92,7 +92,27 @@ 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, binaries, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, v2transport=False, uses_wallet=False, ipcbind=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, + version=None, + v2transport=False, + uses_wallet=False, + ipcbind=False, + ): """ Kwargs: start_perf (bool): If True, begin profiling the node with `perf` as soon as @@ -148,15 +168,6 @@ class TestNode(): self.ipc_socket_path = self.ipc_tmp_dir / "node.sock" self.args.append(f"-ipcbind=unix:{self.ipc_socket_path}") - # Use valgrind, expect for previous release binaries - if use_valgrind and version is None: - default_suppressions_file = Path(__file__).parents[3] / "contrib" / "valgrind.supp" - suppressions_file = os.getenv("VALGRIND_SUPPRESSIONS_FILE", - default_suppressions_file) - self.args = ["valgrind", "--suppressions={}".format(suppressions_file), - "--gen-suppressions=all", "--exit-on-first-error=yes", - "--error-exitcode=1", "--quiet"] + self.args - if self.version_is_at_least(190000): self.args.append("-logthreadnames") if self.version_is_at_least(219900): diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 03ece6f064a..b6aa3568a58 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -248,9 +248,19 @@ class Binaries: 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): + def __init__(self, paths, bin_dir, *, use_valgrind=False): self.paths = paths self.bin_dir = bin_dir + suppressions_file = pathlib.Path(__file__).parents[3] / "contrib" / "valgrind.supp" + self.valgrind_cmd = [ + "valgrind", + f"--suppressions={suppressions_file}", + "--gen-suppressions=all", + "--trace-children=yes", # Needed for 'bitcoin' wrapper + "--exit-on-first-error=yes", + "--error-exitcode=1", + "--quiet", + ] if use_valgrind else [] def node_argv(self, **kwargs): "Return argv array that should be used to invoke bitcoind" @@ -282,20 +292,26 @@ class Binaries: return self._argv("chainstate", self.paths.bitcoinchainstate) 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 + """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.""" + always uses the direct path. + + The returned args include valgrind, except when bin_dir is set + (previous releases). Also, valgrind will only apply to the bitcoin + wrapper executable directly, not to the commands that `bitcoin` calls. + """ 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: # 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] + return self.valgrind_cmd + bitcoin_cmd + (["-m"] if need_ipc else []) + [command] else: - return [bin_path] + return self.valgrind_cmd + [bin_path] def get_binary_paths(config): diff --git a/test/functional/tool_bitcoin.py b/test/functional/tool_bitcoin.py index 1112e02e090..d59c75dee53 100755 --- a/test/functional/tool_bitcoin.py +++ b/test/functional/tool_bitcoin.py @@ -39,7 +39,9 @@ class ToolBitcoinTest(BitcoinTestFramework): 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] + # Manually construct the `bitcoin node` command, similar to Binaries::node_argv() + bitcoin_cmd = node.binaries.valgrind_cmd + [node.binaries.paths.bitcoin_bin] + node.args = bitcoin_cmd + 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]