pytest: Remove valgrind opt-out via tests marker

Reviewing ElementsProject#4391 I noticed that we have a sneaky way of opting out of
valgrind using the `slow_test` marker. I consider this dangerous as it
causes a false sense of security ("this test is being run under
valgrind" when really it isn't), as exemplified by some of the tests
even branching on whether we run under valgrind or not.

This is my attempt of making tests explicitly opt out of valgrind
testing via the `@unittest.skipIf` decorator, which is more honest, as
it doesn't run the test in a tweaked configuration, and reports a
skipped test. The tests are all already being run on CI without
valgrind, so we don't lose any coverage by this, but are more
respectful of the resources we have on CI.

tests/test_closing.py

@@ -5,6 +5,7 @@
 from utils import (
     only_one, sync_blockheight, wait_for, DEVELOPER, TIMEOUT,
     account_balance, first_channel_id, basic_fee, TEST_NETWORK,
+    VALGRIND,
 )
 
 import os
@@ -169,7 +170,7 @@ def test_closing_id(node_factory):
     wait_for(lambda: not only_one(l2.rpc.listpeers(l1.info['id'])['peers'])['connected'])
 
 
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Many processes being spawned")
 def test_closing_torture(node_factory, executor, bitcoind):
     # We set up a fully-connected mesh of N nodes, then try
     # closing them all at once.
@@ -231,7 +232,7 @@ def test_closing_torture(node_factory, executor, bitcoind):
         wait_for(lambda: n.rpc.listpeers()['peers'] == [])
 
 
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Many processes")
 def test_closing_different_fees(node_factory, bitcoind, executor):
     l1 = node_factory.get_node()
 
@@ -722,7 +723,7 @@ def test_penalty_outhtlc(node_factory, bitcoind, executor, chainparams):
 
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
 @unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "Makes use of the sqlite3 db")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too resource hungry")
 def test_penalty_htlc_tx_fulfill(node_factory, bitcoind, chainparams):
     """ Test that the penalizing node claims any published
         HTLC transactions
@@ -855,7 +856,7 @@ def test_penalty_htlc_tx_fulfill(node_factory, bitcoind, chainparams):
 
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
 @unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "Makes use of the sqlite3 db")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too resource hungry")
 def test_penalty_htlc_tx_timeout(node_factory, bitcoind, chainparams):
     """ Test that the penalizing node claims any published
         HTLC transactions
@@ -2131,7 +2132,7 @@ def setup_multihtlc_test(node_factory, bitcoind):
 
 
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too resource hungry")
 def test_onchain_multihtlc_our_unilateral(node_factory, bitcoind):
     """Node pushes a channel onchain with multiple HTLCs with same payment_hash """
     h, nodes = setup_multihtlc_test(node_factory, bitcoind)
@@ -2223,7 +2224,7 @@ def test_onchain_multihtlc_our_unilateral(node_factory, bitcoind):
 
 
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too resource hungry")
 def test_onchain_multihtlc_their_unilateral(node_factory, bitcoind):
     """Node pushes a channel onchain with multiple HTLCs with same payment_hash """
     h, nodes = setup_multihtlc_test(node_factory, bitcoind)

tests/test_connection.py

@@ -166,7 +166,6 @@ def test_bad_opening(node_factory):
 
 @unittest.skipIf(not DEVELOPER, "gossip without DEVELOPER=1 is slow")
 @unittest.skipIf(TEST_NETWORK != 'regtest', "Fee computation and limits are network specific")
-@pytest.mark.slow_test
 def test_opening_tiny_channel(node_factory):
     # Test custom min-capacity-sat parameters
     #
@@ -2586,16 +2585,10 @@ def test_fulfill_incoming_first(node_factory, bitcoind):
 
 
 @unittest.skipIf(not DEVELOPER, "gossip without DEVELOPER=1 is slow")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too memory hungry under valgrind")
 def test_restart_many_payments(node_factory, bitcoind):
     l1 = node_factory.get_node(may_reconnect=True)
-
-    # On my laptop, these take 89 seconds and 12 seconds
-    if node_factory.valgrind:
-        num = 2
-    else:
-        num = 5
-
+    num = 5
     nodes = node_factory.get_nodes(num * 2, opts={'may_reconnect': True})
     innodes = nodes[:num]
     outnodes = nodes[num:]
@@ -2883,13 +2876,11 @@ def test_feerate_stress(node_factory, executor):
 
 
 @unittest.skipIf(not DEVELOPER, "need dev_disconnect")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Reconnection slow due to process spawning")
 def test_pay_disconnect_stress(node_factory, executor):
     """Expose race in htlc restoration in channeld: 50% chance of failure"""
-    if node_factory.valgrind:
-        NUM_RUNS = 2
-    else:
-        NUM_RUNS = 5
+    NUM_RUNS = 5
+
     for i in range(NUM_RUNS):
         l1, l2 = node_factory.line_graph(2, opts=[{'may_reconnect': True},
                                                   {'may_reconnect': True,

tests/test_pay.py

@@ -1333,7 +1333,7 @@ def test_forward_stats(node_factory, bitcoind):
 
 
 @unittest.skipIf(not DEVELOPER, "too slow without --dev-fast-gossip")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Many processes are memory hungry")
 def test_forward_local_failed_stats(node_factory, bitcoind, executor):
     """Check that we track forwarded payments correctly.
 
@@ -1554,7 +1554,7 @@ def test_forward_local_failed_stats(node_factory, bitcoind, executor):
 
 
 @unittest.skipIf(not DEVELOPER, "too slow without --dev-fast-gossip")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too resource hungry")
 def test_htlcs_cltv_only_difference(node_factory, bitcoind):
     # l1 -> l2 -> l3 -> l4
     # l4 ignores htlcs, so they stay.
@@ -1631,7 +1631,7 @@ def test_pay_variants(node_factory):
 
 
 @unittest.skipIf(not DEVELOPER, "gossip without DEVELOPER=1 is slow")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too resource hungry")
 def test_pay_retry(node_factory, bitcoind, executor, chainparams):
     """Make sure pay command retries properly. """
 
@@ -1715,7 +1715,7 @@ def listpays_nofail(b11):
 
 
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 otherwise gossip takes 5 minutes!")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too resource hungry")
 def test_pay_routeboost(node_factory, bitcoind, compat):
     """Make sure we can use routeboost information. """
     # l1->l2->l3--private-->l4
@@ -3516,7 +3516,7 @@ def pay(l1, inv):
 
 
 @unittest.skipIf(not DEVELOPER, "channel setup very slow (~10 minutes) if not DEVELOPER")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Too resource hungry")
 def test_mpp_interference_2(node_factory, bitcoind, executor):
     '''
     We create a "public network" that looks like so.
@@ -3672,7 +3672,7 @@ def test_large_mpp_presplit(node_factory):
 
 
 @unittest.skipIf(not DEVELOPER, "builds large network, which is slow if not DEVELOPER")
-@pytest.mark.slow_test
+@unittest.skipIf(VALGRIND, "Large network")
 def test_mpp_overload_payee(node_factory, bitcoind):
     """
     We had a bug where if the payer is unusually well-connected compared