Merge bitcoin/bitcoin#28589: test: assumeutxo func test race fixes

7e40032260 tests: assumeutxo: accept final height from either chainstate (James O'Beirne)
5bd2010f02 test: assumeutxo: avoid race in functional test (James O'Beirne)
7005a01c19 test: add wait_for_connect to BitcoinTestFramework.connect_nodes (James O'Beirne)

Pull request description:

  Fixes https://github.com/bitcoin/bitcoin/issues/28585.

  Fixes a few races within the assumeutxo tests:
  - In general, `-stopatheight` can't be used with `connect_nodes` safely because the latter performs blocking assertions that are racy with the stopatheight triggering.
  - Now that the snapshot chainstate is listed as `normal` after background validation, accept the final height from either chainstate.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 7e40032260
  fjahr:
    Code review ACK 7e40032260
  achow101:
    ACK 7e40032260
  ryanofsky:
    Code review ACK 7e40032260

Tree-SHA512: 8cbd2a0ca8643f94baa0ae3561dcf68c3519d5ba851c6049e1768f28cae6434f47ffc28d404bf38ed11030ce3f00aae0a8be3f6d563e6ae6680d83c928a173d8

test/functional/feature_assumeutxo.py

@@ -142,7 +142,10 @@ class AssumeutxoTest(BitcoinTestFramework):
             f"-stopatheight={PAUSE_HEIGHT}", *self.extra_args[1]])
 
         # Finally connect the nodes and let them sync.
-        self.connect_nodes(0, 1)
+        #
+        # Set `wait_for_connect=False` to avoid a race between performing connection
+        # assertions and the -stopatheight tripping.
+        self.connect_nodes(0, 1, wait_for_connect=False)
 
         n1.wait_until_stopped(timeout=5)
 
@@ -156,7 +159,15 @@ class AssumeutxoTest(BitcoinTestFramework):
         self.connect_nodes(0, 1)
 
         self.log.info(f"Ensuring snapshot chain syncs to tip. ({FINAL_HEIGHT})")
-        wait_until_helper(lambda: n1.getchainstates()['snapshot']['blocks'] == FINAL_HEIGHT)
+
+        def check_for_final_height():
+            chainstates = n1.getchainstates()
+            # The background validation may have completed before we run our first
+            # check, so accept a final blockheight from either chainstate type.
+            cs = chainstates.get('snapshot') or chainstates.get('normal')
+            return cs['blocks'] == FINAL_HEIGHT
+
+        wait_until_helper(check_for_final_height)
         self.sync_blocks(nodes=(n0, n1))
 
         self.log.info("Ensuring background validation completes")

test/functional/test_framework/test_framework.py

@@ -586,7 +586,14 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
     def wait_for_node_exit(self, i, timeout):
         self.nodes[i].process.wait(timeout)
 
-    def connect_nodes(self, a, b, *, peer_advertises_v2=None):
+    def connect_nodes(self, a, b, *, peer_advertises_v2=None, wait_for_connect: bool = True):
+        """
+        Kwargs:
+            wait_for_connect: if True, block until the nodes are verified as connected. You might
+                want to disable this when using -stopatheight with one of the connected nodes,
+                since there will be a race between the actual connection and performing
+                the assertions before one node shuts down.
+        """
         from_connection = self.nodes[a]
         to_connection = self.nodes[b]
         from_num_peers = 1 + len(from_connection.getpeerinfo())
@@ -603,6 +610,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             # compatibility with older clients
             from_connection.addnode(ip_port, "onetry")
 
+        if not wait_for_connect:
+            return
+
         # poll until version handshake complete to avoid race conditions
         # with transaction relaying
         # See comments in net_processing: