Merge #19252: test: wait for disconnect in disconnect_p2ps + bloomfilter test followups

9a40cfc558 [refactor] use waiting inside disconnect_p2ps (gzhao408)
aeb9fb414e [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408)
e81942d2e1 [test] logging and style followups for bloomfilter tests (gzhao408)

Pull request description:

  Followup to #19083 which adds bloomfilter-related tests.

  1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/19083#discussion_r437383989). And clean up any redundant `wait_until`s in the functional tests.
  2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](https://github.com/bitcoin/bitcoin/pull/19083#pullrequestreview-428955784)

ACKs for top commit:
  jonatack:
    Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc`
  MarcoFalke:
    ACK 9a40cfc558 🐂

Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87

test/functional/p2p_filter.py

@@ -124,11 +124,11 @@ class FilterTest(BitcoinTestFramework):
         self.log.info("Check that a node with bloom filters enabled services p2p mempool messages")
         filter_peer = P2PBloomFilter()
 
-        self.log.info("Create a tx relevant to the peer before connecting")
+        self.log.debug("Create a tx relevant to the peer before connecting")
         filter_address = self.nodes[0].decodescript(filter_peer.watch_script_pubkey)['addresses'][0]
         txid = self.nodes[0].sendtoaddress(filter_address, 90)
 
-        self.log.info("Send a mempool msg after connecting and check that the tx is received")
+        self.log.debug("Send a mempool msg after connecting and check that the tx is received")
         self.nodes[0].add_p2p_connection(filter_peer)
         filter_peer.send_and_ping(filter_peer.watch_filter_init)
         self.nodes[0].p2p.send_message(msg_mempool())
@@ -227,8 +227,8 @@ class FilterTest(BitcoinTestFramework):
         self.test_frelay_false(filter_peer_without_nrelay)
         self.test_filter(filter_peer_without_nrelay)
 
-        self.log.info('Test msg_mempool')
         self.test_msg_mempool()
 
+
 if __name__ == '__main__':
     FilterTest().main()

test/functional/p2p_leak.py

@@ -132,9 +132,6 @@ class P2PLeakTest(BitcoinTestFramework):
 
         self.nodes[0].disconnect_p2ps()
 
-        # Wait until all connections are closed
-        wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0)
-
         # Make sure no unexpected messages came in
         assert no_version_bannode.unexpected_msg == False
         assert no_version_idlenode.unexpected_msg == False

test/functional/p2p_nobloomfilter_messages.py

@@ -4,7 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test invalid p2p messages for nodes with bloom filters disabled.
 
-Test that, when bloom filters are not enabled, nodes are disconnected if:
+Test that, when bloom filters are not enabled, peers are disconnected if:
 1. They send a p2p mempool message
 2. They send a p2p filterload message
 3. They send a p2p filteradd message
@@ -17,31 +17,32 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
 
-class P2PNobloomfilterMessages(BitcoinTestFramework):
+class P2PNoBloomFilterMessages(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
         self.extra_args = [["-peerbloomfilters=0"]]
 
     def test_message_causes_disconnect(self, message):
-        # Add a p2p connection that sends a message and check that it disconnects
+        """Add a p2p connection that sends a message and check that it disconnects."""
         peer = self.nodes[0].add_p2p_connection(P2PInterface())
         peer.send_message(message)
         peer.wait_for_disconnect()
-        assert_equal(len(self.nodes[0].getpeerinfo()), 0)
+        assert_equal(self.nodes[0].getconnectioncount(), 0)
 
     def run_test(self):
-        self.log.info("Test that node is disconnected if it sends mempool message")
+        self.log.info("Test that peer is disconnected if it sends mempool message")
         self.test_message_causes_disconnect(msg_mempool())
 
-        self.log.info("Test that node is disconnected if it sends filterload message")
+        self.log.info("Test that peer is disconnected if it sends filterload message")
         self.test_message_causes_disconnect(msg_filterload())
 
-        self.log.info("Test that node is disconnected if it sends filteradd message")
+        self.log.info("Test that peer is disconnected if it sends filteradd message")
         self.test_message_causes_disconnect(msg_filteradd(data=b'\xcc'))
 
         self.log.info("Test that peer is disconnected if it sends a filterclear message")
         self.test_message_causes_disconnect(msg_filterclear())
 
+
 if __name__ == '__main__':
-    P2PNobloomfilterMessages().main()
+    P2PNoBloomFilterMessages().main()

test/functional/p2p_node_network_limited.py

@@ -83,7 +83,6 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
         assert_equal(node1.firstAddrnServices, expected_services)
 
         self.nodes[0].disconnect_p2ps()
-        node1.wait_for_disconnect()
 
         # connect unsynced node 2 with pruned NODE_NETWORK_LIMITED peer
         # because node 2 is in IBD and node 0 is a NODE_NETWORK_LIMITED peer, sync must not be possible

test/functional/test_framework/test_node.py

@@ -23,6 +23,7 @@ import sys
 
 from .authproxy import JSONRPCException
 from .descriptors import descsum_create
+from .messages import MY_SUBVERSION
 from .util import (
     MAX_NODES,
     append_config,
@@ -549,11 +550,16 @@ class TestNode():
         assert self.p2ps, self._node_msg("No p2p connection")
         return self.p2ps[0]
 
+    def num_connected_mininodes(self):
+        """Return number of test framework p2p connections to the node."""
+        return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION])
+
     def disconnect_p2ps(self):
         """Close all p2p connections to the node."""
         for p in self.p2ps:
             p.peer_disconnect()
         del self.p2ps[:]
+        wait_until(lambda: self.num_connected_mininodes() == 0)
 
 
 class TestNodeCLIAttr: