From fa03fbf7e31f384627230e1bc042f7f29c05ff68 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 17 Feb 2026 21:32:17 +0100 Subject: [PATCH 1/3] 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] From fa29fb72cb21bec798ddae49b842c78ac84a5a3b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 18 Feb 2026 08:43:14 +0100 Subject: [PATCH 2/3] test: Remove redundant warning about missing binaries The error was added in commit 1ea7e45a1f445d32a2b690d52befb2e63418653b, because there was an additional confusing `AssertionError: [node 0] Error: no RPC connection` instead of just a single `FileNotFoundError: [Errno 2] No such file or directory`. This is no longer needed on current master. Also, the test is incomplete, because it was just checking bitcoind and bitcoin-cli, not any other missing binaries. Also, after the previous commit, it would not work in combination with --valgrind. Instead of trying to make it complete, and work in all combinations, just remove it, because the already existing error will be clear in any case. This can be tested via: ```sh ./test/get_previous_releases.py mv releases releases_backup # Confirm the test is skipped due to missing releases ./bld-cmake/test/functional/wallet_migration.py # Confirm the test fails due to missing releases ./bld-cmake/test/functional/wallet_migration.py --previous-releases mv releases_backup releases mv ./releases/v28.2 ./releases/v28.2_backup # Confirm the test fails with a single FileNotFoundError ./bld-cmake/test/functional/wallet_migration.py mv ./releases/v28.2_backup ./releases/v28.2 # Confirm the test runs and passes ./bld-cmake/test/functional/wallet_migration.py rm ./bld-cmake/bin/bitcoind # Confirm the test fails with a single "No such file or directory", # testing with and without --valgrind ./bld-cmake/test/functional/wallet_migration.py ./bld-cmake/test/functional/wallet_migration.py --valgrind ``` --- .../test_framework/test_framework.py | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 79fcfafde10..e3893eb4c4f 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", @@ -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] @@ -968,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): From fa5d478853a88ce7d7095b6abfe8112390298b65 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 18 Feb 2026 15:25:09 +0100 Subject: [PATCH 3/3] test: valgrind --trace-children=yes for bitcoin wrapper --- test/functional/test_framework/test_framework.py | 2 +- test/functional/test_framework/util.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index e3893eb4c4f..9e05d227fa7 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 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.") + 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") diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 3fabca8bf40..b6aa3568a58 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -256,6 +256,7 @@ class Binaries: "valgrind", f"--suppressions={suppressions_file}", "--gen-suppressions=all", + "--trace-children=yes", # Needed for 'bitcoin' wrapper "--exit-on-first-error=yes", "--error-exitcode=1", "--quiet",