Merge #19967: test: Replace (dis)?connect_nodes globals with TestFramework methods

3c7d9ab test: Move (dis)?connect_nodes globals into TestFramework as helpers (Elliott Jin)
4b16c61 scripted-diff: test: Replace uses of (dis)?connect_nodes global (Prayank)
be38684 test: Replace use of (dis)?connect_nodes globals (Elliott Jin)

Pull request description:

  `util.py` defines global helper functions `connect_nodes` and `disconnect_nodes`; however, these functions are confusing because they take a node and an index (instead of two indexes).

  The `TestFramework` object has enough context to convert from `i` to `self.nodes[i]`, so we can replace all instances of `connect_nodes(self.nodes[a], b)` with `self.connect_nodes(a, b)`. Similarly, we can replace instances of `disconnect_nodes`.

  The approach taken in this PR builds on #19945 but uses a scripted-diff for the majority of the changes.

  Fixes: #19821

ACKs for top commit:
  MarcoFalke:
    ACK 3c7d9ab
  guggero:
    ACK 3c7d9ab

Tree-SHA512: e027092748602904abcd986d7299624c8754c3236314b6d8e392e306741c212f266c2207e385adfb194f67ae6559a585ee7b15d639b1d65c4651dbf503e5931a

Author: MarcoFalke

test/functional/example_test.py

@@ -25,7 +25,6 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes,
 )
 
 # P2PInterface is a class containing callbacks to be executed when a P2P
@@ -115,7 +114,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:2])
 
     # Use setup_nodes() to customize the node start behaviour (for example if
@@ -183,7 +182,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("Wait for node2 to receive all the blocks from node1")
         self.sync_all()

test/functional/feature_abortnode.py

@@ -11,7 +11,7 @@
 """
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import get_datadir_path, connect_nodes
+from test_framework.util import get_datadir_path
 import os
 
 
@@ -36,7 +36,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

@@ -13,7 +13,6 @@
     assert_equal,
     assert_greater_than,
     assert_greater_than_or_equal,
-    connect_nodes,
     satoshi_round,
 )
 
@@ -232,9 +231,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 BitcoinTestFramework
-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)
 REGTEST_WORK_PER_BLOCK = 2
@@ -39,7 +39,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,6 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes,
-    disconnect_nodes,
     hex_str_to_bytes,
 )
 
@@ -75,7 +73,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)
 
             self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10)
 
@@ -126,12 +124,12 @@ def run_test(self):
             # Bump tx2 as bump2 and generate a block on node 0 while
             # disconnected, then reconnect and check for notifications on node 1
             # about newly confirmed bump2 and newly conflicted tx2.
-            disconnect_nodes(self.nodes[0], 1)
+            self.disconnect_nodes(0, 1)
             bump2 = self.nodes[0].bumpfee(tx2)["txid"]
             self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)
             assert_equal(self.nodes[0].gettransaction(bump2)["confirmations"], 1)
             assert_equal(tx2 in self.nodes[1].getrawmempool(), True)
-            connect_nodes(self.nodes[0], 1)
+            self.connect_nodes(0, 1)
             self.sync_blocks()
             self.expect_wallet_notify([bump2, tx2])
             assert_equal(self.nodes[1].gettransaction(bump2)["confirmations"], 1)

test/functional/feature_pruning.py

@@ -18,8 +18,6 @@
     assert_equal,
     assert_greater_than,
     assert_raises_rpc_error,
-    connect_nodes,
-    disconnect_nodes,
 )
 
 # Rescans start at the earliest block up to 2 hours before a key timestamp, so
@@ -102,11 +100,11 @@ def setup_network(self):
 
         self.prunedir = os.path.join(self.nodes[2].datadir, self.chain, 'blocks', '')
 
-        connect_nodes(self.nodes[0], 1)
-        connect_nodes(self.nodes[1], 2)
-        connect_nodes(self.nodes[0], 2)
-        connect_nodes(self.nodes[0], 3)
-        connect_nodes(self.nodes[0], 4)
+        self.connect_nodes(0, 1)
+        self.connect_nodes(1, 2)
+        self.connect_nodes(0, 2)
+        self.connect_nodes(0, 3)
+        self.connect_nodes(0, 4)
         self.sync_blocks(self.nodes[0:5])
 
     def setup_nodes(self):
@@ -148,17 +146,17 @@ def create_chain_with_staleblocks(self):
         for _ in range(12):
             # Disconnect node 0 so it can mine a longer reorg chain without knowing about node 1's soon-to-be-stale chain
             # Node 2 stays connected, so it hears about the stale blocks and then reorg's when node0 reconnects
-            disconnect_nodes(self.nodes[0], 1)
-            disconnect_nodes(self.nodes[0], 2)
+            self.disconnect_nodes(0, 1)
+            self.disconnect_nodes(0, 2)
             # Mine 24 blocks in node 1
             mine_large_blocks(self.nodes[1], 24)
 
             # Reorg back with 25 block chain from node 0
             mine_large_blocks(self.nodes[0], 25)
 
             # Create connections in the order so both nodes can see the reorg at the same time
-            connect_nodes(self.nodes[0], 1)
-            connect_nodes(self.nodes[0], 2)
+            self.connect_nodes(0, 1)
+            self.connect_nodes(0, 2)
             self.sync_blocks(self.nodes[0:3])
 
         self.log.info("Usage can be over target because of high stale rate: %d" % calc_usage(self.prunedir))
@@ -187,15 +185,15 @@ def reorg_test(self):
         self.log.info("New best height: %d" % self.nodes[1].getblockcount())
 
         # Disconnect node1 and generate the new chain
-        disconnect_nodes(self.nodes[0], 1)
-        disconnect_nodes(self.nodes[1], 2)
+        self.disconnect_nodes(0, 1)
+        self.disconnect_nodes(1, 2)
 
         self.log.info("Generating new longer chain of 300 more blocks")
         self.nodes[1].generate(300)
 
         self.log.info("Reconnect nodes")
-        connect_nodes(self.nodes[0], 1)
-        connect_nodes(self.nodes[1], 2)
+        self.connect_nodes(0, 1)
+        self.connect_nodes(1, 2)
         self.sync_blocks(self.nodes[0:3], timeout=120)
 
         self.log.info("Verify height on node 2: %d" % self.nodes[2].getblockcount())
@@ -336,7 +334,7 @@ def wallet_test(self):
         # check that wallet loads successfully when restarting a pruned node after IBD.
         # this was reported to fail in #7494.
         self.log.info("Syncing node 5 to test wallet")
-        connect_nodes(self.nodes[0], 5)
+        self.connect_nodes(0, 5)
         nds = [self.nodes[0], self.nodes[5]]
         self.sync_blocks(nds, wait=5, timeout=300)
         self.restart_node(5, extra_args=["-prune=550"]) # restart to trigger rescan

test/functional/feature_segwit.py

@@ -22,7 +22,6 @@
     assert_equal,
     assert_is_hex_string,
     assert_raises_rpc_error,
-    connect_nodes,
     hex_str_to_bytes,
     try_rpc,
 )
@@ -78,7 +77,7 @@ def skip_test_if_missing_module(self):
 
     def setup_network(self):
         super().setup_network()
-        connect_nodes(self.nodes[0], 2)
+        self.connect_nodes(0, 2)
         self.sync_all()
 
     def success_mine(self, node, txid, sign, redeem_script=""):

test/functional/interface_zmq.py

@@ -11,7 +11,6 @@
 from test_framework.messages import CTransaction, hash256, FromHex
 from test_framework.util import (
     assert_equal,
-    connect_nodes,
     assert_raises_rpc_error,
 )
 from io import BytesIO
@@ -102,7 +101,7 @@ def test_basic(self):
         rawtx = subs[3]
 
         self.restart_node(0, ["-zmqpub%s=%s" % (sub.topic.decode(), address) for sub in [hashblock, hashtx, rawblock, rawtx]])
-        connect_nodes(self.nodes[0], 1)
+        self.connect_nodes(0, 1)
         for socket in sockets:
             socket.connect(address)
 
@@ -207,7 +206,7 @@ def test_reorg(self):
         connect_blocks = self.nodes[1].generatetoaddress(2, ADDRESS_BCRT1_P2WSH_OP_TRUE)
 
         # nodes[0] will reorg chain after connecting back nodes[1]
-        connect_nodes(self.nodes[0], 1)
+        self.connect_nodes(0, 1)
         self.sync_blocks() # tx in mempool valid but not advertised
 
         # Should receive nodes[1] tip
@@ -264,7 +263,7 @@ def test_sequence(self):
         self.nodes[1].generatetoaddress(2, ADDRESS_BCRT1_P2WSH_OP_TRUE)
 
         # nodes[0] will reorg chain after connecting back nodes[1]
-        connect_nodes(self.nodes[0], 1)
+        self.connect_nodes(0, 1)
 
         # Then we receive all block (dis)connect notifications for the 2 block reorg
         assert_equal((dc_block, "D", None), seq.receive_sequence())
@@ -406,7 +405,7 @@ def test_mempool_sync(self):
         seq = ZMQSubscriber(socket, b'sequence')
 
         self.restart_node(0, ['-zmqpub%s=%s' % (seq.topic.decode(), address)])
-        connect_nodes(self.nodes[0], 1)
+        self.connect_nodes(0, 1)
         socket.connect(address)
         # Relax so that the subscriber is ready before publishing zmq messages
         sleep(0.2)

test/functional/mempool_persist.py

@@ -45,8 +45,6 @@
     assert_equal,
     assert_greater_than_or_equal,
     assert_raises_rpc_error,
-    connect_nodes,
-    disconnect_nodes,
 )
 
 
@@ -83,11 +81,11 @@ def run_test(self):
         assert_greater_than_or_equal(tx_creation_time_higher, tx_creation_time)
 
         # disconnect nodes & make a txn that remains in the unbroadcast set.
-        disconnect_nodes(self.nodes[0], 1)
+        self.disconnect_nodes(0, 1)
         assert(len(self.nodes[0].getpeerinfo()) == 0)
         assert(len(self.nodes[0].p2ps) == 0)
         self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), Decimal("12"))
-        connect_nodes(self.nodes[0], 2)
+        self.connect_nodes(0, 2)
 
         self.log.debug("Stop-start the nodes. Verify that node0 has the transactions in its mempool and node1 does not. Verify that node2 calculates its balance correctly after loading wallet transactions.")
         self.stop_nodes()

test/functional/mempool_unbroadcast.py

@@ -11,9 +11,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
-    connect_nodes,
     create_confirmed_utxos,
-    disconnect_nodes,
 )
 
 MAX_INITIAL_BROADCAST_DELAY = 15 * 60 # 15 minutes in seconds
@@ -36,7 +34,7 @@ def test_broadcast(self):
         min_relay_fee = node.getnetworkinfo()["relayfee"]
         utxos = create_confirmed_utxos(min_relay_fee, node, 10)
 
-        disconnect_nodes(node, 1)
+        self.disconnect_nodes(0, 1)
 
         self.log.info("Generate transactions that only node 0 knows about")
 
@@ -70,7 +68,7 @@ def test_broadcast(self):
         self.restart_node(0)
 
         self.log.info("Reconnect nodes & check if they are sent to node 1")
-        connect_nodes(node, 1)
+        self.connect_nodes(0, 1)
 
         # fast forward into the future & ensure that the second node has the txns
         node.mockscheduler(MAX_INITIAL_BROADCAST_DELAY)
@@ -91,7 +89,7 @@ def test_broadcast(self):
         time.sleep(2) # allow sufficient time for possibility of broadcast
         assert_equal(len(conn.get_invs()), 0)
 
-        disconnect_nodes(node, 1)
+        self.disconnect_nodes(0, 1)
         node.disconnect_p2ps()
 
     def test_txn_removal(self):