Merge #2948: [Tests] Actually wait for nodes to connect

5f8c06e test: Add possibility to skip awaiting for the connection (Alessandro Rezzi)
5374229 Merge bitcoin#30252: test: Remove redundant verack check (merge-script)
e7c21c7 Merge bitcoin#30118: test: improve robustness of connect_nodes() (Ava Chow)
9beef8d Merge bitcoin#26854: test: Fix intermittent timeout in p2p_permissions.py (MarcoFalke)
b3a3d78 Merge bitcoin#25443: test: Fail if connect_nodes fails (laanwj)
72709b9 test: Avoid connecting a peer to himself (Alessandro Rezzi)
77697b8 test: Do not connect the nodes in parallel (Alessandro Rezzi)
17c065d test: Avoid race after connect_nodes (MarcoFalke)
d73ac82 test: refactor connect_nodes and disconnect_nodes to take two indices instead of index + node (Alessandro Rezzi)
e19d72f Merge bitcoin#18866: test: Fix verack race to avoid intermittent test failures (MarcoFalke)

Pull request description:

  This PR solves the failures of `wallet_listtransactions.py`, like the one of  https://github.com/PIVX-Project/PIVX/actions/runs/11705208154/job/32601252220?pr=2947.

  They happen due to mempool sync timeout:
  ```
  2024-11-06T14:58:03.8489793Z AssertionError: Mempool sync timed out after 60s:
  2024-11-06T14:58:03.8490785Z   {'7364836e7eae24a75378b920373e303b99b4ff18db758defc4c057d784a43905'}
  ```

  The issue is that the connection between nodes is established after the transaction is sent, as we can see from the logs:
  ```
  2024-11-06T14:58:03.9085473Z 2024-11-06T14:57:02Z (mocktime: 2019-10-31T18:21:20Z) Relaying wtx 7364836e7eae24a75378b920373e303b99b4ff18db758defc4c057d784a43905
  ...
  2024-11-06T14:58:03.9103287Z 2024-11-06T14:57:02Z (mocktime: 2019-10-31T18:21:20Z) New outbound peer connected: version: 70927, blocks=200, peer=0
  ```

  Hence the newly connected node will never receive the transaction and the mempool will never be synced.

  This bug is fixed by ensuring that `connect_nodes` actually wait for the connection to be established. As a consequence of those checks we cannot anymore connect nodes in parallel in `connect_nodes_clique` (which will make tests run slightly slower)

ACKs for top commit: 5f8c06e
  Fuzzbawls:
    utACK 5f8c06e
  Duddino:
    utACK 5f8c06e
  Liquid369:
    tACK 5f8c06e

Tree-SHA512: 88007d7302f3b7c3c5b9d446e7d8acc959cd03b0bb27409b1633cb86c57905a4814be148e2e5f6ccaa1da17eccdd44f68f81d00299696d1f47f52f9b12b32ec7

test/functional/example_test.py

@@ -26,7 +26,6 @@
 from test_framework.test_framework import PivxTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes,
     wait_until,
 )
 
@@ -111,7 +110,7 @@ def setup_network(self):
         # In this test, we're not connecting node2 to node0 or node1. Calls to
         # sync_all() should not include node2, since we're not expecting it to
         # sync.
-        connect_nodes(self.nodes[0], 1)
+        self.connect_nodes(0, 1)
         self.sync_all(self.nodes[0:1])
 
     # Use setup_nodes() to customize the node start behaviour (for example if
@@ -182,7 +181,7 @@ def run_test(self):
         self.nodes[1].waitforblockheight(11)
 
         self.log.info("Connect node2 and node1")
-        connect_nodes(self.nodes[1], 2)
+        self.connect_nodes(1, 2)
 
         self.log.info("Add P2P connection to node2")
         self.nodes[0].disconnect_p2ps()

test/functional/feature_abortnode.py

@@ -13,7 +13,7 @@
 import os
 
 from test_framework.test_framework import PivxTestFramework
-from test_framework.util import wait_until, get_datadir_path, connect_nodes
+from test_framework.util import wait_until, get_datadir_path
 
 
 class AbortNodeTest(PivxTestFramework):
@@ -37,7 +37,7 @@ def run_test(self):
         # attempt.
         self.nodes[1].generate(3)
         with self.nodes[0].assert_debug_log(["Failed to disconnect block"]):
-            connect_nodes(self.nodes[0], 1)
+            self.connect_nodes(0, 1)
             self.nodes[1].generate(1)
 
             # Check that node0 aborted

test/functional/feature_fee_estimation.py

@@ -10,7 +10,7 @@
 from test_framework.messages import CTransaction, CTxIn, CTxOut, COutPoint, ToHex, COIN
 from test_framework.script import CScript, OP_1, OP_DROP, OP_2, OP_HASH160, OP_EQUAL, hash160, OP_TRUE
 from test_framework.test_framework import PivxTestFramework
-from test_framework.util import connect_nodes, satoshi_round
+from test_framework.util import satoshi_round
 
 
 # Use as minTxFee
@@ -247,9 +247,9 @@ def run_test(self):
         # so the estimates would not be affected by the splitting transactions
         self.start_node(1)
         self.start_node(2)
-        connect_nodes(self.nodes[1], 0)
-        connect_nodes(self.nodes[0], 2)
-        connect_nodes(self.nodes[2], 1)
+        self.connect_nodes(1, 0)
+        self.connect_nodes(0, 2)
+        self.connect_nodes(2, 1)
 
         self.sync_all()
 

test/functional/feature_minchainwork.py

@@ -18,7 +18,7 @@
 import time
 
 from test_framework.test_framework import PivxTestFramework
-from test_framework.util import connect_nodes, assert_equal
+from test_framework.util import assert_equal
 
 
 # 2 hashes required per regtest block (with no difficulty adjustment)
@@ -40,7 +40,7 @@ def setup_network(self):
         # block relay to inbound peers.
         self.setup_nodes()
         for i in range(self.num_nodes-1):
-            connect_nodes(self.nodes[i+1], i)
+            self.connect_nodes(i+1, i)
 
     def run_test(self):
         # Start building a chain on node0.  node2 shouldn't be able to sync until node1's

test/functional/feature_notifications.py

@@ -9,8 +9,7 @@
 from test_framework.test_framework import PivxTestFramework
 from test_framework.util import (
     assert_equal,
-    wait_until,
-    connect_nodes,
+    wait_until
 )
 
 
@@ -66,7 +65,7 @@ def run_test(self):
             self.log.info("test -walletnotify after rescan")
             # restart node to rescan to force wallet notifications
             self.start_node(1)
-            connect_nodes(self.nodes[0], 1)
+            self.connect_nodes(0, 1)
 
             wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10)
 

test/functional/interface_rest.py

@@ -13,7 +13,7 @@
 import urllib.parse
 
 from test_framework.test_framework import PivxTestFramework
-from test_framework.util import assert_equal, assert_greater_than, connect_nodes, hex_str_to_bytes
+from test_framework.util import assert_equal, assert_greater_than, hex_str_to_bytes
 
 
 def deser_uint256(f):
@@ -52,7 +52,7 @@ def set_test_params(self):
 
     def setup_network(self, split=False):
         super().setup_network()
-        connect_nodes(self.nodes[0], 2)
+        self.connect_nodes(0, 2)
 
     def run_test(self):
         url = urllib.parse.urlparse(self.nodes[0].url)

test/functional/mining_pos_reorg.py

@@ -7,9 +7,6 @@
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
-    connect_nodes,
-    connect_nodes_clique,
-    disconnect_nodes,
     set_node_times,
     DecimalAmt,
 )
@@ -28,7 +25,7 @@ def setup_chain(self):
     def setup_network(self):
         # connect all nodes between each other
         self.setup_nodes()
-        connect_nodes_clique(self.nodes)
+        self.connect_nodes_clique(self.nodes)
         self.sync_all()
 
     def log_title(self):
@@ -42,7 +39,7 @@ def disconnect_all(self):
         for i in range(self.num_nodes):
             for j in range(self.num_nodes):
                 if j != i:
-                    disconnect_nodes(self.nodes[i], j)
+                    self.disconnect_nodes(i, j)
         self.log.info("Nodes disconnected")
 
     def get_tot_balance(self, nodeid):
@@ -109,7 +106,7 @@ def findUtxoInList(txid, vout, utxo_list):
 
         # Connect with node 2 and sync
         self.log.info("Reconnecting node 0 and node 2")
-        connect_nodes(self.nodes[0], 2)
+        self.connect_nodes(0, 2)
         self.sync_blocks([self.nodes[i] for i in [0, 2]])
 
         # verify that the stakeinput can't be spent
@@ -140,7 +137,7 @@ def findUtxoInList(txid, vout, utxo_list):
         new_best_hash = self.nodes[1].getbestblockhash()
         self.log.info("Connecting and syncing nodes...")
         set_node_times(self.nodes, self.mocktime)
-        connect_nodes_clique(self.nodes)
+        self.connect_nodes_clique(self.nodes)
         self.sync_blocks()
         for i in [0, 2]:
             assert_equal(self.nodes[i].getbestblockhash(), new_best_hash)

test/functional/p2p_disconnect_ban.py

@@ -9,7 +9,6 @@
 from test_framework.test_framework import PivxTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes,
     assert_raises_rpc_error,
     wait_until,
 )
@@ -21,8 +20,8 @@ def set_test_params(self):
 
     def run_test(self):
         self.log.info("Connect nodes both way")
-        connect_nodes(self.nodes[0], 1)
-        connect_nodes(self.nodes[1], 0)
+        self.connect_nodes(0, 1)
+        self.connect_nodes(1, 0)
 
         self.log.info("Test setban and listbanned RPCs")
 
@@ -81,8 +80,8 @@ def run_test(self):
         # Clear ban lists
         self.nodes[1].clearbanned()
         self.log.info("Connect nodes both way")
-        connect_nodes(self.nodes[0], 1)
-        connect_nodes(self.nodes[1], 0)
+        self.connect_nodes(0, 1)
+        self.connect_nodes(1, 0)
 
         self.log.info("Test disconnectnode RPCs")
 
@@ -101,7 +100,7 @@ def run_test(self):
         assert not [node for node in self.nodes[0].getpeerinfo() if node['addr'] == address1]
 
         self.log.info("disconnectnode: successfully reconnect node")
-        connect_nodes(self.nodes[0], 1)  # reconnect the node
+        self.connect_nodes(0, 1)  # reconnect the node
         assert_equal(len(self.nodes[0].getpeerinfo()), 2)
         assert [node for node in self.nodes[0].getpeerinfo() if node['addr'] == address1]
 

test/functional/p2p_quorum_connect.py

@@ -13,7 +13,6 @@
 from test_framework.messages import msg_version
 from test_framework.util import (
     assert_equal,
-    connect_nodes,
     hash256,
     wait_until,
 )
@@ -174,7 +173,7 @@ def run_test(self):
         self.clean_conns_and_disconnect(mn6_node)
 
         # Create the regular connection
-        connect_nodes(mn5_node, mn6.idx)
+        self.connect_nodes(mn5.idx, mn6.idx)
         self.wait_for_peers_count([mn5_node], 1)
         assert self.has_single_regular_connection(mn5_node)
         assert self.has_single_regular_connection(mn6_node)

test/functional/p2p_sendheaders.py

@@ -88,7 +88,7 @@
 )
 from test_framework.mininode import mininode_lock, NetworkThread, P2PInterface
 from test_framework.test_framework import PivxTestFramework
-from test_framework.util import assert_equal, wait_until, connect_nodes, p2p_port
+from test_framework.util import assert_equal, wait_until, p2p_port
 
 
 direct_fetch_response_time = 0.05
@@ -242,7 +242,7 @@ def __init__(self):
     def setup_network(self):
         self.nodes = []
         self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [["-debug", "-logtimemicros=1"]]*2)
-        connect_nodes(self.nodes[0], 1)
+        self.connect_nodes(0, 1)
 
     # mine count blocks and return the new tip
     def mine_blocks(self, count):

test/functional/p2p_time_offset.py

@@ -8,15 +8,11 @@
 from test_framework.test_framework import PivxTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes,
     set_node_times,
+    wait_until,
 )
 
 
-def connect_nodes_bi(nodes, a, b):
-    connect_nodes(nodes[a], b)
-    connect_nodes(nodes[b], a)
-
 class TimeOffsetTest(PivxTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
@@ -27,6 +23,10 @@ def setup_network(self):
         # don't connect nodes yet
         self.setup_nodes()
 
+    def connect_nodes_bi(self, a, b, wait_for_connect = True):
+        self.connect_nodes(a, b, wait_for_connect)
+        self.connect_nodes(b, a, wait_for_connect)
+
     def check_connected_nodes(self):
         ni = [node.getnetworkinfo() for node in self.connected_nodes]
         assert_equal([x['connections'] for x in ni], [2] * len(ni))
@@ -52,8 +52,8 @@ def run_test(self):
 
         # connect nodes 1 and 2
         self.log.info("Connecting with node-1 (+10 s) and node-2 (+15 s)...")
-        connect_nodes_bi(self.nodes, 0, 1)
-        connect_nodes_bi(self.nodes, 0, 2)
+        self.connect_nodes_bi(0, 1)
+        self.connect_nodes_bi(0, 2)
         self.log.info("--> samples = [+0, +10, (+10), +15, +15]")
         ni = self.nodes[0].getnetworkinfo()
         assert_equal(ni['connections'], 4)
@@ -64,7 +64,7 @@ def run_test(self):
 
         # connect node 3
         self.log.info("Connecting with node-3 (+20 s). This will print the warning...")
-        connect_nodes_bi(self.nodes, 0, 3)
+        self.connect_nodes_bi(0, 3)
         self.log.info("--> samples = [+0, +10, +10, (+15), +15, +20, +20]")
         ni = self.nodes[0].getnetworkinfo()
         assert_equal(ni['connections'], 6)
@@ -75,7 +75,7 @@ def run_test(self):
 
         # connect node 6
         self.log.info("Connecting with node-6 (-5 s)...")
-        connect_nodes_bi(self.nodes, 0, 6)
+        self.connect_nodes_bi(0, 6)
         self.log.info("--> samples = [-5, -5, +0, +10, (+10), +15, +15, +20, +20]")
         ni = self.nodes[0].getnetworkinfo()
         assert_equal(ni['connections'], 8)
@@ -86,7 +86,7 @@ def run_test(self):
 
         # connect node 4
         self.log.info("Connecting with node-4 (+25 s). This will print the warning...")
-        connect_nodes_bi(self.nodes, 0, 4)
+        self.connect_nodes_bi(0, 4)
         self.log.info("--> samples = [-5, -5, +0, +10, +10, (+15), +15, +20, +20, +25, +25]")
         ni = self.nodes[0].getnetworkinfo()
         assert_equal(ni['connections'], 10)
@@ -97,16 +97,16 @@ def run_test(self):
 
         # try to connect node 5 and check that it can't
         self.log.info("Trying to connect with node-5 (+30 s)...")
-        connect_nodes_bi(self.nodes, 0, 5)
-        ni = self.nodes[0].getnetworkinfo()
-        assert_equal(ni['connections'], 10)
+        # Don't wait for a connection that will never be established.
+        self.connect_nodes_bi(0, 5, False)
+        wait_until(lambda: self.nodes[0].getnetworkinfo()['connections'] == 10)
         assert_equal(ni['timeoffset'], 15)
         self.log.info("Not connected.")
         self.log.info("Node-0 nTimeOffset: +%d seconds" % ni['timeoffset'])
 
         # connect node 7
         self.log.info("Connecting with node-7 (-10 s)...")
-        connect_nodes_bi(self.nodes, 0, 7)
+        self.connect_nodes_bi(0, 7)
         self.log.info("--> samples = [-10, -10, -5, -5, +0, +10, (+10), +15, +15, +20, +20, +25, +25]")
         ni = self.nodes[0].getnetworkinfo()
         assert_equal(ni['connections'], 12)

test/functional/p2p_unrequested_blocks.py

@@ -58,7 +58,7 @@
 from test_framework.messages import CBlockHeader, CInv, msg_block, msg_headers, msg_inv
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.test_framework import PivxTestFramework
-from test_framework.util import assert_equal, assert_raises_rpc_error, connect_nodes
+from test_framework.util import assert_equal, assert_raises_rpc_error
 
 
 class AcceptBlockTest(PivxTestFramework):
@@ -311,7 +311,7 @@ def run_test(self):
         test_node.wait_for_disconnect()
 
         # 9. Connect node1 to node0 and ensure it is able to sync
-        connect_nodes(self.nodes[0], 1)
+        self.connect_nodes(0, 1)
         self.sync_blocks([self.nodes[0], self.nodes[1]])
         self.log.info("Successfully synced nodes 1 and 0")
 

test/functional/rpc_invalidateblock.py

@@ -5,7 +5,7 @@
 """Test the invalidateblock RPC."""
 
 from test_framework.test_framework import PivxTestFramework
-from test_framework.util import assert_equal, connect_nodes, wait_until
+from test_framework.util import assert_equal, wait_until
 
 
 class InvalidateTest(PivxTestFramework):
@@ -29,7 +29,7 @@ def run_test(self):
         assert_equal(self.nodes[1].getblockcount(), 6)
 
         self.log.info("Connect nodes to force a reorg")
-        connect_nodes(self.nodes[0], 1)
+        self.connect_nodes(0, 1)
         self.sync_blocks(self.nodes[0:2])
         assert_equal(self.nodes[0].getblockcount(), 6)
         badhash = self.nodes[1].getblockhash(2)
@@ -40,7 +40,7 @@ def run_test(self):
         assert_equal(self.nodes[0].getbestblockhash(), besthash_n0)
 
         self.log.info("Make sure we won't reorg to a lower work chain:")
-        connect_nodes(self.nodes[1], 2)
+        self.connect_nodes(1, 2)
         self.log.info("Sync node 2 to node 1 so both have 6 blocks")
         self.sync_blocks(self.nodes[1:3])
         assert_equal(self.nodes[2].getblockcount(), 6)