Merge #19816: test: Rename wait until helper to wait_until_helper

fa1cd9e test: Remove unused lock arg from BitcoinTestFramework.wait_until (MarcoFalke)
fad2794 test: Rename wait until helper to wait_until_helper (MarcoFalke)
facb41b test: Remove unused p2p_lock in VersionBitsWarningTest (MarcoFalke)

Pull request description:

  This avoids confusion with the `wait_until` member functions, which should be preferred because they take the appropriate locks and scale the timeout appropriately on their own.

ACKs for top commit:
  laanwj:
    Code review ACK fa1cd9e
  hebasto:
    ACK fa1cd9e, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 319d400085606a4c738e314824037f72998e6657d8622b363726842aba968744f23c56d27275dfe506b8cbbb6e97fc39ca1d325db05d4d67df0e8b35f2244d5c

Author: fanquake

test/functional/example_test.py

@@ -207,10 +207,11 @@ def run_test(self):
         self.log.info("Check that each block was received only once")
         # The network thread uses a global lock on data access to the P2PConnection objects when sending and receiving
         # messages. The test thread should acquire the global lock before accessing any P2PConnection data to avoid locking
-        # and synchronization issues. Note wait_until() acquires this global lock when testing the predicate.
+        # and synchronization issues. Note p2p.wait_until() acquires this global lock internally when testing the predicate.
         with p2p_lock:
             for block in self.nodes[2].p2p.block_receive_map.values():
                 assert_equal(block, 1)
 
+
 if __name__ == '__main__':
     ExampleTest().main()

test/functional/feature_versionbits_warning.py

@@ -12,7 +12,7 @@
 
 from test_framework.blocktools import create_block, create_coinbase
 from test_framework.messages import msg_block
-from test_framework.p2p import p2p_lock, P2PInterface
+from test_framework.p2p import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 
 VB_PERIOD = 144           # versionbits period length for regtest
@@ -90,7 +90,7 @@ def run_test(self):
 
         # Generating one block guarantees that we'll get out of IBD
         node.generatetoaddress(1, node_deterministic_address)
-        self.wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'], timeout=10, lock=p2p_lock)
+        self.wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'])
         # Generating one more block will be enough to generate an error.
         node.generatetoaddress(1, node_deterministic_address)
         # Check that get*info() shows the versionbits unknown rules warning

test/functional/p2p_leak.py

@@ -17,7 +17,7 @@
     msg_ping,
     msg_version,
 )
-from test_framework.p2p import p2p_lock, P2PInterface
+from test_framework.p2p import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -113,9 +113,9 @@ def run_test(self):
         # verack, since we never sent one
         no_verack_idle_peer.wait_for_verack()
 
-        self.wait_until(lambda: no_version_disconnect_peer.ever_connected, timeout=10, lock=p2p_lock)
-        self.wait_until(lambda: no_version_idle_peer.ever_connected, timeout=10, lock=p2p_lock)
-        self.wait_until(lambda: no_verack_idle_peer.version_received, timeout=10, lock=p2p_lock)
+        no_version_disconnect_peer.wait_until(lambda: no_version_disconnect_peer.ever_connected, check_connected=False)
+        no_version_idle_peer.wait_until(lambda: no_version_idle_peer.ever_connected)
+        no_verack_idle_peer.wait_until(lambda: no_verack_idle_peer.version_received)
 
         # Mine a block and make sure that it's not sent to the connected peers
         self.nodes[0].generate(nblocks=1)

test/functional/test_framework/p2p.py

@@ -69,7 +69,7 @@
     NODE_WITNESS,
     sha256,
 )
-from test_framework.util import wait_until
+from test_framework.util import wait_until_helper
 
 logger = logging.getLogger("TestFramework.p2p")
 
@@ -293,7 +293,7 @@ def __init__(self):
 
         # Track the most recent message of each type.
         # To wait for a message to be received, pop that message from
-        # this and use wait_until.
+        # this and use self.wait_until.
         self.last_message = {}
 
         # A count of the number of ping messages we've sent to the node
@@ -398,7 +398,7 @@ def test_function():
                 assert self.is_connected
             return test_function_in()
 
-        wait_until(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
+        wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
 
     def wait_for_disconnect(self, timeout=60):
         test_function = lambda: not self.is_connected
@@ -522,7 +522,7 @@ def run(self):
     def close(self, timeout=10):
         """Close the connections and network event loop."""
         self.network_event_loop.call_soon_threadsafe(self.network_event_loop.stop)
-        wait_until(lambda: not self.network_event_loop.is_running(), timeout=timeout)
+        wait_until_helper(lambda: not self.network_event_loop.is_running(), timeout=timeout)
         self.network_event_loop.close()
         self.join(timeout)
         # Safe to remove event loop.

test/functional/test_framework/test_framework.py

@@ -31,7 +31,7 @@
     disconnect_nodes,
     get_datadir_path,
     initialize_datadir,
-    wait_until,
+    wait_until_helper,
 )
 
 
@@ -603,8 +603,8 @@ def sync_all(self, nodes=None):
         self.sync_blocks(nodes)
         self.sync_mempools(nodes)
 
-    def wait_until(self, test_function, timeout=60, lock=None):
-        return wait_until(test_function, timeout=timeout, lock=lock, timeout_factor=self.options.timeout_factor)
+    def wait_until(self, test_function, timeout=60):
+        return wait_until_helper(test_function, timeout=timeout, timeout_factor=self.options.timeout_factor)
 
     # Private helper methods. These should not be accessed by the subclass test scripts.
 

test/functional/test_framework/test_node.py

@@ -31,7 +31,7 @@
     get_auth_cookie,
     get_rpc_proxy,
     rpc_url,
-    wait_until,
+    wait_until_helper,
     p2p_port,
     EncodeDecimal,
 )
@@ -231,7 +231,7 @@ def wait_for_rpc_connection(self):
                 if self.version_is_at_least(190000):
                     # getmempoolinfo.loaded is available since commit
                     # bb8ae2c (version 0.19.0)
-                    wait_until(lambda: rpc.getmempoolinfo()['loaded'])
+                    wait_until_helper(lambda: rpc.getmempoolinfo()['loaded'], timeout_factor=self.timeout_factor)
                     # Wait for the node to finish reindex, block import, and
                     # loading the mempool. Usually importing happens fast or
                     # even "immediate" when the node is started. However, there
@@ -359,7 +359,7 @@ def is_node_stopped(self):
         return True
 
     def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT):
-        wait_until(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
+        wait_until_helper(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
 
     @contextlib.contextmanager
     def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2):
@@ -560,7 +560,7 @@ def disconnect_p2ps(self):
         for p in self.p2ps:
             p.peer_disconnect()
         del self.p2ps[:]
-        wait_until(lambda: self.num_test_p2p_connections() == 0)
+        wait_until_helper(lambda: self.num_test_p2p_connections() == 0, timeout_factor=self.timeout_factor)
 
 
 class TestNodeCLIAttr:

test/functional/test_framework/util.py

@@ -226,14 +226,14 @@ def satoshi_round(amount):
     return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
 
 
-def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0):
+def wait_until_helper(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0):
     """Sleep until the predicate resolves to be True.
 
     Warning: Note that this method is not recommended to be used in tests as it is
-    not aware of the context of the test framework. Using `wait_until()` counterpart
-    from `BitcoinTestFramework` or `P2PInterface` class ensures an understandable
-    amount of timeout and a common shared timeout_factor. Furthermore, `wait_until()`
-    from `P2PInterface` class in `mininode.py` has a preset lock.
+    not aware of the context of the test framework. Using the `wait_until()` members
+    from `BitcoinTestFramework` or `P2PInterface` class ensures the timeout is
+    properly scaled. Furthermore, `wait_until()` from `P2PInterface` class in
+    `p2p.py` has a preset lock.
     """
     if attempts == float('inf') and timeout == float('inf'):
         timeout = 60
@@ -438,7 +438,7 @@ def get_peer_ids():
                 raise
 
     # wait to disconnect
-    wait_until(lambda: not get_peer_ids(), timeout=5)
+    wait_until_helper(lambda: not get_peer_ids(), timeout=5)
 
 
 def connect_nodes(from_connection, node_num):
@@ -449,8 +449,8 @@ def connect_nodes(from_connection, node_num):
     # See comments in net_processing:
     # * Must have a version message before anything else
     # * Must have a verack message before anything else
-    wait_until(lambda: all(peer['version'] != 0 for peer in from_connection.getpeerinfo()))
-    wait_until(lambda: all(peer['bytesrecv_per_msg'].pop('verack', 0) == 24 for peer in from_connection.getpeerinfo()))
+    wait_until_helper(lambda: all(peer['version'] != 0 for peer in from_connection.getpeerinfo()))
+    wait_until_helper(lambda: all(peer['bytesrecv_per_msg'].pop('verack', 0) == 24 for peer in from_connection.getpeerinfo()))
 
 
 # Transaction/Block functions