Merge #19877: [test] clarify rpc_net & p2p_disconnect_ban functional …

…tests

47ff509 [test] Clarify setup of node topology. (Amiti Uttarwar)
0672522 [move-only, test]: Match test order with run order (Amiti Uttarwar)

Pull request description:

  small improvements to clarify logic in the functional tests
  1. have test logic in `rpc_net.py` match run order of the test
  2. remove `connect_nodes` calls that are redundant with the automatic test setup executed by the test framework

  Noticed when I was trying to debug a test for #19725. Small changes but imo very helpful, because they initially confused me.

ACKs for top commit:
  laanwj:
    ACK 47ff509

Tree-SHA512: 2843da2c0b4f06b2600b3adb97900a62be7bb2228770abd67d86f2a65c58079af22c7c20957474a98c17da85f40a958a6f05cb8198aa0c56a58adc1c31100492

test/functional/p2p_disconnect_ban.py

@@ -18,8 +18,11 @@ def set_test_params(self):
 
     def run_test(self):
         self.log.info("Connect nodes both way")
+        # By default, the test framework sets up an addnode connection from
+        # node 1 --> node0. By connecting node0 --> node 1, we're left with
+        # the two nodes being connected both ways.
+        # Topology will look like: node0 <--> node1
         self.connect_nodes(0, 1)
-        self.connect_nodes(1, 0)
 
         self.log.info("Test setban and listbanned RPCs")
 

test/functional/rpc_net.py

@@ -51,9 +51,12 @@ def set_test_params(self):
     def run_test(self):
         # Get out of IBD for the minfeefilter and getpeerinfo tests.
         self.nodes[0].generate(101)
-        # Connect nodes both ways.
+
+        # By default, the test framework sets up an addnode connection from
+        # node 1 --> node0. By connecting node0 --> node 1, we're left with
+        # the two nodes being connected both ways.
+        # Topology will look like: node0 <--> node1
         self.connect_nodes(0, 1)
-        self.connect_nodes(1, 0)
         self.sync_all()
 
         self.test_connection_count()
@@ -69,6 +72,36 @@ def test_connection_count(self):
         # After using `connect_nodes` to connect nodes 0 and 1 to each other.
         assert_equal(self.nodes[0].getconnectioncount(), 2)
 
+    def test_getpeerinfo(self):
+        self.log.info("Test getpeerinfo")
+        # Create a few getpeerinfo last_block/last_transaction values.
+        if self.is_wallet_compiled():
+            self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 1)
+        self.nodes[1].generate(1)
+        self.sync_all()
+        time_now = int(time.time())
+        peer_info = [x.getpeerinfo() for x in self.nodes]
+        # Verify last_block and last_transaction keys/values.
+        for node, peer, field in product(range(self.num_nodes), range(2), ['last_block', 'last_transaction']):
+            assert field in peer_info[node][peer].keys()
+            if peer_info[node][peer][field] != 0:
+                assert_approx(peer_info[node][peer][field], time_now, vspan=60)
+        # check both sides of bidirectional connection between nodes
+        # the address bound to on one side will be the source address for the other node
+        assert_equal(peer_info[0][0]['addrbind'], peer_info[1][0]['addr'])
+        assert_equal(peer_info[1][0]['addrbind'], peer_info[0][0]['addr'])
+        assert_equal(peer_info[0][0]['minfeefilter'], Decimal("0.00000500"))
+        assert_equal(peer_info[1][0]['minfeefilter'], Decimal("0.00001000"))
+        # check the `servicesnames` field
+        for info in peer_info:
+            assert_net_servicesnames(int(info[0]["services"], 0x10), info[0]["servicesnames"])
+
+        assert_equal(peer_info[0][0]['connection_type'], 'inbound')
+        assert_equal(peer_info[0][1]['connection_type'], 'manual')
+
+        assert_equal(peer_info[1][0]['connection_type'], 'manual')
+        assert_equal(peer_info[1][1]['connection_type'], 'inbound')
+
     def test_getnettotals(self):
         self.log.info("Test getnettotals")
         # getnettotals totalbytesrecv and totalbytessent should be
@@ -151,36 +184,6 @@ def test_getaddednodeinfo(self):
         # check that a non-existent node returns an error
         assert_raises_rpc_error(-24, "Node has not been added", self.nodes[0].getaddednodeinfo, '1.1.1.1')
 
-    def test_getpeerinfo(self):
-        self.log.info("Test getpeerinfo")
-        # Create a few getpeerinfo last_block/last_transaction values.
-        if self.is_wallet_compiled():
-            self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 1)
-        self.nodes[1].generate(1)
-        self.sync_all()
-        time_now = int(time.time())
-        peer_info = [x.getpeerinfo() for x in self.nodes]
-        # Verify last_block and last_transaction keys/values.
-        for node, peer, field in product(range(self.num_nodes), range(2), ['last_block', 'last_transaction']):
-            assert field in peer_info[node][peer].keys()
-            if peer_info[node][peer][field] != 0:
-                assert_approx(peer_info[node][peer][field], time_now, vspan=60)
-        # check both sides of bidirectional connection between nodes
-        # the address bound to on one side will be the source address for the other node
-        assert_equal(peer_info[0][0]['addrbind'], peer_info[1][0]['addr'])
-        assert_equal(peer_info[1][0]['addrbind'], peer_info[0][0]['addr'])
-        assert_equal(peer_info[0][0]['minfeefilter'], Decimal("0.00000500"))
-        assert_equal(peer_info[1][0]['minfeefilter'], Decimal("0.00001000"))
-        # check the `servicesnames` field
-        for info in peer_info:
-            assert_net_servicesnames(int(info[0]["services"], 0x10), info[0]["servicesnames"])
-
-        assert_equal(peer_info[0][0]['connection_type'], 'inbound')
-        assert_equal(peer_info[0][1]['connection_type'], 'manual')
-
-        assert_equal(peer_info[1][0]['connection_type'], 'manual')
-        assert_equal(peer_info[1][1]['connection_type'], 'inbound')
-
     def test_service_flags(self):
         self.log.info("Test service flags")
         self.nodes[0].add_p2p_connection(P2PInterface(), services=(1 << 4) | (1 << 63))