Merge bitcoin/bitcoin#34608: test: Fix broken --valgrind handling after bitcoin wrapper

fa5d478853 test: valgrind --trace-children=yes for bitcoin wrapper (MarcoFalke)
fa29fb72cb test: Remove redundant warning about missing binaries (MarcoFalke)
fa03fbf7e3 test: Fix broken --valgrind handling after bitcoin wrapper (MarcoFalke)

Pull request description:

  Currently, tool_bitcoin.py is failing under `--valgrind`:

  ```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
  ```

  Fix this issue by running `bitcoin` under valgrind.

ACKs for top commit:
  achow101:
    ACK fa5d478853
  ryanofsky:
    Code review ACK fa5d478853 just squashing commits since last review (Thanks!)

Tree-SHA512: 503685ac69e1ca3046958655bed4fe6d0aee1525c5ea58ebf098efd0332c0e16f138540baffaf9af1263a8c42ac6b150ed8bca5a5371a3c49802e21957ec6632

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(
+                raise AssertionError(f"Force test of previous releases but releases missing: {self.options.previous_releases_path}\n"
-                    self.options.previous_releases_path))
+                                     "Previous releases binaries can be downloaded via `test/get_previous_releases.py`.")
         return self.options.prev_releases
 
     def skip_if_no_external_signer(self):

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

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
+        """Return argv array that should be used to invoke the command.
-        either uses the bitcoin wrapper executable (if BITCOIN_CMD is set or
+
+        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):

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]