test: Fix broken --valgrind handling after bitcoin wrapper
What changed, and why it matters
This is a test-only bug fix. The --valgrind option used in Bitcoin Core's functional tests had stopped working after a previous change introduced a 'bitcoin' wrapper executable. The patch moves valgrind handling into the test framework's binary helper so the wrapper is launched under valgrind, and it removes an outdated minimum valgrind version and an unused environment variable. There is no change to production Bitcoin node code, no user-facing behavior change, and no security vulnerability in the network software itself.
No security action required. Treat as a normal test infrastructure fix. If running functional tests with --valgrind, verify tool_bitcoin.py now passes.
Security signals we found
No security signal: change is entirely in test framework code
No modification of consensus, networking, wallet, or RPC code
No input parsing, privilege, or memory management changes in production code
Evidence from the diff
The commit fixes functional test argument construction so –valgrind again wraps the bitcoin wrapper executable. Previously, TestNode.init prepended the valgrind command to self.args, but tool_bitcoin.py later overwrote node.args with a manually constructed bitcoin wrapper invocation, dropping valgrind and causing an assertion failure. The fix moves valgrind_cmd into the Binaries helper and uses it in Binaries._argv() and in tool_bitcoin.py’s set_cmd_args(). It also drops the valgrind 3.14 requirement and VALGRIND_SUPPRESSIONS_FILE environment variable handling. All changes are in test/functional/test_framework/.
Changed components
test/functional/test_framework/test_framework.pytest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/functional/tool_bitcoin.pyInspect captured patch +47 / −20
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 7970da52..79fcfafd 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 b56cf64c..aa3d2d07 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 03ece6f0..3fabca8b 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 1112e02e..d59c75de 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]
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.