From fa03fbf7e31f384627230e1bc042f7f29c05ff68 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 17 Feb 2026 21:32:17 +0100 Subject: [PATCH] test: Fix broken --valgrind handling after bitcoin wrapper Prior to this commit, tool_bitcoin.py was failing: ```sh $ ./bld-cmake/test/functional/tool_bitcoin.py --valgrind TestFramework (ERROR): Unexpected exception Traceback (most recent call last): File "./test/functional/test_framework/test_framework.py", line 138, in main self.setup() ~~~~~~~~~~^^ File "./test/functional/test_framework/test_framework.py", line 269, in setup self.setup_network() ~~~~~~~~~~~~~~~~~~^^ File "./test/functional/tool_bitcoin.py", line 38, in setup_network assert all(node.args[:len(node_argv)] == node_argv for node in self.nodes) ~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError ``` This commit fixes this issue by running `bitcoin` under valgrind. Also, it comes with other improvements: * Drop the outdated valgrind 3.14 requirement, because there is no distro that ships a version that old anymore. * Drop the VALGRIND_SUPPRESSIONS_FILE env var handling, because it was presumably never used since it was introduced. Also, the use-case seems limited. Review note: The set_cmd_args was ignoring the --valgrind test option. In theory, this could be fixed by refactoring Binaries::node_argv() to be used here. However, for now, just re-implement the node_argv logic in set_cmd_args to prepend the valgrind cmd. --- .../test_framework/test_framework.py | 5 ++- test/functional/test_framework/test_node.py | 31 +++++++++++++------ test/functional/test_framework/util.py | 27 ++++++++++++---- test/functional/tool_bitcoin.py | 4 ++- 4 files changed, 47 insertions(+), 20 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 7970da52595..79fcfafde10 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -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 or binaries called from the bitcoin wrapper executable.") 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.""" @@ -493,7 +493,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, ) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index b56cf64ce99..aa3d2d0723e 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -91,7 +91,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 @@ -147,15 +167,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..3fabca8bf40 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -248,9 +248,18 @@ 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", + "--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 +291,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]