Merge #18873: test: Fix intermittent sync_blocks failures

fa3f9a05660687bf4146e089050e944a1d6cbe3c test: Fix intermittent sync_blocks failures (MarcoFalke)

Pull request description:

  Fixes #18872
  Fixes #18737
  Fixes #18801

  See docstring for motivation and description

ACKs for top commit:
  laanwj:
    Code review ACK fa3f9a05660687bf4146e089050e944a1d6cbe3c

Tree-SHA512: acd52d386a6849f7ff1cb1a51a439dc3c76e0e3a4dd8d22030df0ebb09b44497c61c56331ff65724b695d82d86b0ebb24608f9e637008e5dacb7676b0c448889

test/functional/feature_loadblock.py

@@ -16,10 +16,8 @@ import sys
 import tempfile
 import urllib
 
-from test_framework.test_framework import (
-    BitcoinTestFramework,
-)
-from test_framework.util import assert_equal, wait_until
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
 
 
 class LoadblockTest(BitcoinTestFramework):
@@ -75,7 +73,7 @@ class LoadblockTest(BitcoinTestFramework):
         self.log.info("Restart second, unsynced node with bootstrap file")
         self.stop_node(1)
         self.start_node(1, ["-loadblock=" + bootstrap_file])
-        wait_until(lambda: self.nodes[1].getblockcount() == 100)
+        assert_equal(self.nodes[1].getblockcount(), 100)  # start_node is blocking on all block files being imported
 
         assert_equal(self.nodes[1].getblockchaininfo()['blocks'], 100)
         assert_equal(self.nodes[0].getbestblockhash(), self.nodes[1].getbestblockhash())

test/functional/feature_reindex.py

@@ -10,10 +10,10 @@
 """
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import wait_until
+from test_framework.util import assert_equal
+
 
 class ReindexTest(BitcoinTestFramework):
-
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
@@ -24,7 +24,7 @@ class ReindexTest(BitcoinTestFramework):
         self.stop_nodes()
         extra_args = [["-reindex-chainstate" if justchainstate else "-reindex"]]
         self.start_nodes(extra_args)
-        wait_until(lambda: self.nodes[0].getblockcount() == blockcount)
+        assert_equal(self.nodes[0].getblockcount(), blockcount)  # start_node is blocking on reindex
         self.log.info("Success")
 
     def run_test(self):

test/functional/mempool_persist.py

@@ -40,7 +40,7 @@ import os
 
 from test_framework.test_framework import BitcoinTestFramework
 # from test_framework.mininode import P2PTxInvStore
-from test_framework.util import assert_equal, assert_raises_rpc_error, connect_nodes, disconnect_nodes, wait_until
+from test_framework.util import assert_equal, assert_raises_rpc_error, connect_nodes, disconnect_nodes
 
 
 class MempoolPersistTest(BitcoinTestFramework):
@@ -88,8 +88,8 @@ class MempoolPersistTest(BitcoinTestFramework):
         self.start_node(1, extra_args=["-persistmempool=0"])
         self.start_node(0)
         self.start_node(2)
-        wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"], timeout=1)
-        wait_until(lambda: self.nodes[2].getmempoolinfo()["loaded"], timeout=1)
+        assert self.nodes[0].getmempoolinfo()["loaded"]  # start_node is blocking on the mempool being loaded
+        assert self.nodes[2].getmempoolinfo()["loaded"]
         assert_equal(len(self.nodes[0].getrawmempool()), 6)
         assert_equal(len(self.nodes[2].getrawmempool()), 5)
         # The others have loaded their mempool. If node_1 loaded anything, we'd probably notice by now:
@@ -107,13 +107,13 @@ class MempoolPersistTest(BitcoinTestFramework):
         self.log.debug("Stop-start node0 with -persistmempool=0. Verify that it doesn't load its mempool.dat file.")
         self.stop_nodes()
         self.start_node(0, extra_args=["-persistmempool=0", "-disablewallet"])
-        wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"])
+        assert self.nodes[0].getmempoolinfo()["loaded"]
         assert_equal(len(self.nodes[0].getrawmempool()), 0)
 
         self.log.debug("Stop-start node0. Verify that it has the transactions in its mempool.")
         self.stop_nodes()
         self.start_node(0)
-        wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"])
+        assert self.nodes[0].getmempoolinfo()["loaded"]
         assert_equal(len(self.nodes[0].getrawmempool()), 6)
 
         mempooldat0 = os.path.join(self.nodes[0].datadir, self.chain, 'mempool.dat')
@@ -127,7 +127,7 @@ class MempoolPersistTest(BitcoinTestFramework):
         os.rename(mempooldat0, mempooldat1)
         self.stop_nodes()
         self.start_node(1, extra_args=[])
-        wait_until(lambda: self.nodes[1].getmempoolinfo()["loaded"])
+        assert self.nodes[1].getmempoolinfo()["loaded"]
         assert_equal(len(self.nodes[1].getrawmempool()), 6)
 
         self.log.debug("Prevent dashd from writing mempool.dat to disk. Verify that `savemempool` fails")

test/functional/test_framework/test_node.py

@@ -236,6 +236,24 @@ class TestNode():
                 rpc = get_rpc_proxy(rpc_url(self.datadir, self.index, self.chain, self.rpchost), self.index, timeout=self.rpc_timeout, coveragedir=self.coverage_dir)
                 rpc.getblockcount()
                 # If the call to getblockcount() succeeds then the RPC connection is up
+                wait_until(lambda: rpc.getmempoolinfo()['loaded'])
+                # 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
+                # is no guarantee and sometimes ThreadImport might finish
+                # later. This is going to cause intermittent test failures,
+                # because generally the tests assume the node is fully
+                # ready after being started.
+                #
+                # For example, the node will reject block messages from p2p
+                # when it is still importing with the error "Unexpected
+                # block message received"
+                #
+                # The wait is done here to make tests as robust as possible
+                # and prevent racy tests and intermittent failures as much
+                # as possible. Some tests might not need this, but the
+                # overhead is trivial, and the added gurantees are worth
+                # the minimal performance cost.
                 self.log.debug("RPC successfully started")
                 if self.use_cli:
                     return
@@ -269,6 +287,9 @@ class TestNode():
             wallet_path = "wallet/{}".format(urllib.parse.quote(wallet_name))
             return self.rpc / wallet_path
 
+    def version_is_at_least(self, ver):
+        return self.version is None or self.version >= ver
+
     def stop_node(self, expected_stderr='', wait=0):
         """Stop the node."""
         if not self.running:

test/functional/wallet_abandonconflict.py

@@ -18,7 +18,6 @@ from test_framework.util import (
     assert_raises_rpc_error,
     connect_nodes,
     disconnect_nodes,
-    wait_until,
 )
 
 
@@ -98,7 +97,7 @@ class AbandonConflictTest(BitcoinTestFramework):
         # TODO: redo with eviction
         self.stop_node(0)
         self.start_node(0, extra_args=["-minrelaytxfee=0.0001"])
-        wait_until(lambda: self.nodes[0].getmempoolinfo()['loaded'])
+        assert self.nodes[0].getmempoolinfo()['loaded']
 
         # Verify txs no longer in either node's mempool
         assert_equal(len(self.nodes[0].getrawmempool()), 0)
@@ -134,7 +133,7 @@ class AbandonConflictTest(BitcoinTestFramework):
         # Verify that even with a low min relay fee, the tx is not reaccepted from wallet on startup once abandoned
         self.stop_node(0)
         self.start_node(0, extra_args=["-minrelaytxfee=0.00001"])
-        wait_until(lambda: self.nodes[0].getmempoolinfo()['loaded'])
+        assert self.nodes[0].getmempoolinfo()['loaded']
 
         assert_equal(len(self.nodes[0].getrawmempool()), 0)
         assert_equal(self.nodes[0].getbalance(), balance)
@@ -156,7 +155,7 @@ class AbandonConflictTest(BitcoinTestFramework):
         # Remove using high relay fee again
         self.stop_node(0)
         self.start_node(0, extra_args=["-minrelaytxfee=0.0001"])
-        wait_until(lambda: self.nodes[0].getmempoolinfo()['loaded'])
+        assert self.nodes[0].getmempoolinfo()['loaded']
         assert_equal(len(self.nodes[0].getrawmempool()), 0)
         newbalance = self.nodes[0].getbalance()
         assert_equal(newbalance, balance - Decimal("24.9996"))

test/functional/wallet_basic.py

@@ -4,7 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the wallet."""
 from decimal import Decimal
-import time
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -455,12 +454,8 @@ class WalletTest(BitcoinTestFramework):
         self.stop_node(0)
         self.start_node(0, extra_args=["-walletrejectlongchains", "-limitancestorcount=" + str(2 * chainlimit)])
 
-        # wait for loadmempool
-        timeout = 10
-        while (timeout > 0 and len(self.nodes[0].getrawmempool()) < chainlimit * 2):
-            time.sleep(0.5)
-            timeout -= 0.5
-        assert_equal(len(self.nodes[0].getrawmempool()), chainlimit * 2)
+        # wait until the wallet has submitted all transactions to the mempool
+        wait_until(lambda: len(self.nodes[0].getrawmempool()) == chainlimit * 2)
 
         # Prevent potential race condition when calling wallet RPCs right after restart
         self.nodes[0].syncwithvalidationinterfacequeue()