pytest: Use valgrind target suppressions instead of skipping tests

Having a list of very targeted suppressions allows us to still run the
majority of tests with valgrind checking, and not fail when Rust does
some trickery. This is for example the case with `std::sync::Once`
which uses `num_procs` calling out to the cgroups subsystem, sometimes
with a null path.

Suggested-by: Rusty Russell <@rustyrussell>

doc/HACKING.md

@@ -242,6 +242,37 @@ TEST_DB_PROVIDER=[sqlite3|postgres] - Selects the database to use when running
 EXPERIMENTAL_DUAL_FUND=[0|1]	    - Enable dual-funding tests.
 ```
 
+#### Troubleshooting
+
+##### Valgrind complains about code we don't control
+
+Sometimes `valgrind` will complain about code we do not control
+ourselves, either because it's in a library we use or it's a false
+positive. There are generally three ways to address these issues
+(in descending order of preference):
+
+ 1. Add a suppression for the one specific call that is causing the
+    issue. Upon finding an issue `valgrind` is instructed in the
+    testing framework to print filters that'd match the issue. These
+    can be added to the suppressions file under
+    `tests/valgrind-suppressions.txt` in order to explicitly skip
+    reporting these in future. This is preferred over the other
+    solutions since it only disables reporting selectively for things
+    that were manually checked. See the [valgrind docs][vg-supp] for
+    details.
+ 2. Add the process that `valgrind` is complaining about to the
+    `--trace-children-skip` argument in `pyln-testing`. This is used
+    in cases of full binaries not being under our control, such as the
+    `python3` interpreter used in tests that run plugins. Do not use
+    this for binaries that are compiled from our code, as it tends to
+    mask real issues.
+ 3. Mark the test as skipped if running under `valgrind`. It's mostly
+    used to skip tests that otherwise would take considerably too long
+    to test on CI. We discourage this for suppressions, since it is a
+    very blunt tool.
+
+[vg-supp]: https://valgrind.org/docs/manual/manual-core.html#manual-core.suppress
+
 Making BOLT Modifications
 -------------------------
 

tests/fixtures.py

@@ -1,7 +1,8 @@
-from utils import DEVELOPER, TEST_NETWORK  # noqa: F401,F403
+from utils import DEVELOPER, TEST_NETWORK, VALGRIND  # noqa: F401,F403
 from pyln.testing.fixtures import directory, test_base_dir, test_name, chainparams, node_factory, bitcoind, teardown_checks, throttler, db_provider, executor, setup_logging, jsonschemas  # noqa: F401,F403
 from pyln.testing import utils
 from utils import COMPAT
+from pathlib import Path
 
 import os
 import pytest
@@ -17,6 +18,18 @@ class LightningNode(utils.LightningNode):
     def __init__(self, *args, **kwargs):
         utils.LightningNode.__init__(self, *args, **kwargs)
 
+        # We have some valgrind suppressions in the `tests/`
+        # directory, so we can add these to the valgrind configuration
+        # (not generally true when running pyln-testing, hence why
+        # it's being done in this specialization, and not in the
+        # library).
+        if self.daemon.cmd_line[0] == 'valgrind':
+            suppressions_path = Path(__file__).parent / "valgrind-suppressions.txt"
+            self.daemon.cmd_prefix += [
+                f"--suppressions={suppressions_path}",
+                "--gen-suppressions=all"
+            ]
+
         # If we opted into checking the DB statements we will attach the dblog
         # plugin before starting the node
         check_dblog = os.environ.get("TEST_CHECK_DBSTMTS", None) == "1"

tests/test_cln_rs.py

@@ -5,19 +5,11 @@
 import pytest
 
 
-# Skip the entire module if we don't have Rust. The same is true for
-# VALGRIND, since it sometimes causes false positives in
-# `std::sync::Once`
-pytestmark = [
-    pytest.mark.skipif(
-        env('RUST') != '1',
-        reason='RUST is not enabled skipping rust-dependent tests'
-    ),
-    pytest.mark.skipif(
-        env('VALGRIND') == '1',
-        reason='VALGRIND is enabled skipping rust-dependent tests, as they may report false positives.'
-    ),
-]
+# Skip the entire module if we don't have Rust.
+pytestmark = pytest.mark.skipif(
+    env('RUST') != '1',
+    reason='RUST is not enabled skipping rust-dependent tests'
+)
 
 
 def test_rpc_client(node_factory):

tests/valgrind-suppressions.txt

@@ -0,0 +1,14 @@
+{
+   rust__std_sync_once__try_statx_01
+   Memcheck:Param
+   statx(buf)
+   fun:statx
+   ...
+}
+{
+   rust__std_sync_once__try_statx_02
+   Memcheck:Param
+   statx(file_name)
+   fun:statx
+   ...
+}