Merge #16898: test: Remove connect_nodes_bi

fadfd844de8c53034a97dfa6f771ffe9f523fba2 test: Remove unused connect_nodes_bi (MarcoFalke)
fa3b9ee8b2280af4bcbcfffff275aaf8dd125929 scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke)
faaee1e39a91b3f603881655d3980c29af09852b test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke)
1111bb91f517838e5b9f778bf6b5a9c8d561e857 test: Reformat python imports to aid scripted diff (MarcoFalke)

Pull request description:

  By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side).

  This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times.

  Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests.

  Historic background:

  `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.

ACKs for top commit:
  laanwj:
    ACK fadfd844de8c53034a97dfa6f771ffe9f523fba2
  jonasschnelli:
    utACK fadfd844de8c53034a97dfa6f771ffe9f523fba2 - more of less a cleanup PR.
  promag:
    Tested ACK fadfd844de8c53034a97dfa6f771ffe9f523fba2, ran extended tests.

Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

2 files changed, 13 insertions, 7 deletions

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 9aff08fdc7..780aa5fe03 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -25,7 +25,7 @@ from .util import (
     PortSeed,
     assert_equal,
     check_json_precision,
-    connect_nodes_bi,
+    connect_nodes,
     disconnect_nodes,
     get_datadir_path,
     initialize_datadir,
@@ -281,8 +281,18 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         # Connect the nodes as a "chain".  This allows us
         # to split the network between nodes 1 and 2 to get
         # two halves that can work on competing chains.
+        #
+        # Topology looks like this:
+        # node0 <-- node1 <-- node2 <-- node3
+        #
+        # If all nodes are in IBD (clean chain from genesis), node0 is assumed to be the source of blocks (miner). To
+        # ensure block propagation, all nodes will establish outgoing connections toward node0.
+        # See fPreferredDownload in net_processing.
+        #
+        # If further outbound connections are needed, they can be added at the beginning of the test with e.g.
+        # connect_nodes(self.nodes[1], 2)
         for i in range(self.num_nodes - 1):
-            connect_nodes_bi(self.nodes, i, i + 1)
+            connect_nodes(self.nodes[i + 1], i)
         self.sync_all()
 
     def setup_nodes(self):
@@ -423,7 +433,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         """
         Join the (previously split) network halves together.
         """
-        connect_nodes_bi(self.nodes, 1, 2)
+        connect_nodes(self.nodes[1], 2)
         self.sync_all()
 
     def sync_blocks(self, nodes=None, **kwargs):
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index f9f5fe553e..3175872b00 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -377,10 +377,6 @@ def connect_nodes(from_connection, node_num):
     # with transaction relaying
     wait_until(lambda:  all(peer['version'] != 0 for peer in from_connection.getpeerinfo()))
 
-def connect_nodes_bi(nodes, a, b):
-    connect_nodes(nodes[a], b)
-    connect_nodes(nodes[b], a)
-
 def sync_blocks(rpc_connections, *, wait=1, timeout=60):
     """
     Wait until everybody has the same tip.