Merge #17633: tests: Add option --valgrind to run the functional tests under Valgrind

5db506ba5943868cc2c845f717508739b7f05714 tests: Add option --valgrind to run nodes under valgrind in the functional tests (practicalswift)

Pull request description:

  What is better than fixing bugs? Fixing entire bug classes of course! :)

  Add option `--valgrind` to run the functional tests under Valgrind.

  Regular functional testing under Valgrind would have caught many of the uninitialized reads we've seen historically.

  Let's kill this bug class once and for all: let's never use an uninitialized value ever again. Or at least not one that would be triggered by running the functional tests! :)

  My hope is that this addition will make it super-easy to run the functional tests under Valgrind and thus increase the probability of people making use of it :)

  Hopefully `test/functional/test_runner.py --valgrind` will become a natural part of the pre-release QA process.

  **Usage:**

  ```
  $ test/functional/test_runner.py --help
  …
    --valgrind            run nodes under the valgrind memory error detector:
                          expect at least a ~10x slowdown, valgrind 3.14 or
                          later required
  ```

  **Live demo:**

  First, let's re-introduce a memory bug by reverting the recent P2P uninitialized read bug fix from PR #17624 ("net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have").

  ```
  $ git diff
  diff --git a/src/consensus/validation.h b/src/consensus/validation.h
  index 3401eb64c..940adea33 100644
  --- a/src/consensus/validation.h
  +++ b/src/consensus/validation.h
  @@ -114,7 +114,7 @@ inline ValidationState::~ValidationState() {};

   class TxValidationState : public ValidationState {
   private:
  -    TxValidationResult m_result = TxValidationResult::TX_RESULT_UNSET;
  +    TxValidationResult m_result;
   public:
       bool Invalid(TxValidationResult result,
                    const std::string &reject_reason="",
  ```

  Second, let's test as normal without Valgrind:

  ```
  $ test/functional/p2p_segwit.py -l INFO
  2019-11-28T09:30:42.810000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__fc8q3qo
  …
  2019-11-28T09:31:57.187000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
  …
  2019-11-28T09:32:08.265000Z TestFramework (INFO): Tests successful
  ```

  Third, let's test with `--valgrind` and see if the test fail (as we expect) when the unitialized value is used:

  ```
  $ test/functional/p2p_segwit.py -l INFO --valgrind
  2019-11-28T09:32:33.018000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_gtjecx2l
  …
  2019-11-28T09:40:36.702000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
  2019-11-28T09:40:37.813000Z TestFramework (ERROR): Assertion failed
  ConnectionRefusedError: [Errno 111] Connection refused
  ```

ACKs for top commit:
  MarcoFalke:
    ACK 5db506ba5943868cc2c845f717508739b7f05714
  jonatack:
    ACK 5db506ba5943868cc2c845f717508739b7f05714

Tree-SHA512: 2eaecacf4da166febad88b2a8ee6d7ac2bcd38d4c1892ca39516b6343e8f8c8814edf5eaf14c90f11a069a0389d24f0713076112ac284de987e72fc5f6cc3795

2 files changed, 13 insertions, 1 deletions

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index a17e7709f2..da92c6325a 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -161,6 +161,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
                             help="use bitcoin-cli instead of RPC for all commands")
         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")
         parser.add_argument("--randomseed", type=int,
                             help="set a random seed for deterministically reproducing a previous test run")
         self.add_options(parser)
@@ -398,6 +400,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
                 extra_args=extra_args[i],
                 use_cli=self.options.usecli,
                 start_perf=self.options.perf,
+                use_valgrind=self.options.valgrind,
             ))
 
     def start_node(self, i, *args, **kwargs):
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index 1c9628264f..40681790b9 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -59,7 +59,7 @@ 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, *, chain, rpchost, timewait, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False):
+    def __init__(self, i, datadir, *, chain, rpchost, timewait, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False):
         """
         Kwargs:
             start_perf (bool): If True, begin profiling the node with `perf` as soon as
@@ -96,6 +96,15 @@ class TestNode():
             "-debugexclude=leveldb",
             "-uacomment=testnode%d" % i,
         ]
+        if use_valgrind:
+            default_suppressions_file = os.path.join(
+                os.path.dirname(os.path.realpath(__file__)),
+                "..", "..", "..", "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
 
         self.cli = TestNodeCLI(bitcoin_cli, self.datadir)
         self.use_cli = use_cli