Merge bitcoin/bitcoin#29736: test: Extends wait_for_getheaders so a specific block hash can be checked

c4f857cc301d856f3c60acbe6271d3fe19441c7a test: Extends wait_for_getheaders so a specific block hash can be checked (Sergi Delgado Segura)

Pull request description:

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

  Previously, `wait_for_getheaders` would check whether a node had received **any** getheaders message. This implied that, if a test needed to check for a specific block hash within a headers message, it had to make sure that it was checking the desired message. This normally involved having to manually clear `last_message`. This method, apart from being too verbose, was error-prone, given an undesired `getheaders` would make tests pass.

  This adds the ability to check for a specific block_hash within the last `getheaders` message.

ACKs for top commit:
  achow101:
    ACK c4f857cc301d856f3c60acbe6271d3fe19441c7a
  BrandonOdiwuor:
    crACK c4f857cc301d856f3c60acbe6271d3fe19441c7a
  cbergqvist:
    ACK c4f857cc301d856f3c60acbe6271d3fe19441c7a
  stratospher:
    tested ACK c4f857c. went through all getheaders messages sent in the tests and checked that it's the one we want.

Tree-SHA512: afc9a31673344dfaaefcf692ec2ab65958c3d4c005f5f3af525e9960f0622d8246d5311e59aba06cfd5c9e0ef9eb90a7fc8e210f030bfbe67b897c061efdeed1

6 files changed, 28 insertions, 31 deletions

diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py
index da796d3f70..5f2dde8eac 100755
--- a/test/functional/mining_basic.py
+++ b/test/functional/mining_basic.py
@@ -308,7 +308,7 @@ class MiningTest(BitcoinTestFramework):
 
         # Should ask for the block from a p2p node, if they announce the header as well:
         peer = node.add_p2p_connection(P2PDataStore())
-        peer.wait_for_getheaders(timeout=5)  # Drop the first getheaders
+        peer.wait_for_getheaders(timeout=5, block_hash=block.hashPrevBlock)
         peer.send_blocks_and_test(blocks=[block], node=node)
         # Must be active now:
         assert chain_tip(block.hash, status='active', branchlen=0) in node.getchaintips()
diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 0950579580..9e314db110 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -387,7 +387,7 @@ class CompactBlocksTest(BitcoinTestFramework):
 
             if announce == "inv":
                 test_node.send_message(msg_inv([CInv(MSG_BLOCK, block.sha256)]))
-                test_node.wait_until(lambda: "getheaders" in test_node.last_message, timeout=30)
+                test_node.wait_for_getheaders(timeout=30)
                 test_node.send_header_for_blocks([block])
             else:
                 test_node.send_header_for_blocks([block])
diff --git a/test/functional/p2p_initial_headers_sync.py b/test/functional/p2p_initial_headers_sync.py
index e67c384da7..bc6e0fb355 100755
--- a/test/functional/p2p_initial_headers_sync.py
+++ b/test/functional/p2p_initial_headers_sync.py
@@ -38,9 +38,10 @@ class HeadersSyncTest(BitcoinTestFramework):
     def run_test(self):
         self.log.info("Adding a peer to node0")
         peer1 = self.nodes[0].add_p2p_connection(P2PInterface())
+        best_block_hash = int(self.nodes[0].getbestblockhash(), 16)
 
         # Wait for peer1 to receive a getheaders
-        peer1.wait_for_getheaders()
+        peer1.wait_for_getheaders(block_hash=best_block_hash)
         # An empty reply will clear the outstanding getheaders request,
         # allowing additional getheaders requests to be sent to this peer in
         # the future.
@@ -60,17 +61,12 @@ class HeadersSyncTest(BitcoinTestFramework):
             assert "getheaders" not in peer2.last_message
             assert "getheaders" not in peer3.last_message
 
-        with p2p_lock:
-            peer1.last_message.pop("getheaders", None)
-
         self.log.info("Have all peers announce a new block")
         self.announce_random_block(all_peers)
 
         self.log.info("Check that peer1 receives a getheaders in response")
-        peer1.wait_for_getheaders()
+        peer1.wait_for_getheaders(block_hash=best_block_hash)
         peer1.send_message(msg_headers()) # Send empty response, see above
-        with p2p_lock:
-            peer1.last_message.pop("getheaders", None)
 
         self.log.info("Check that exactly 1 of {peer2, peer3} received a getheaders in response")
         count = 0
@@ -80,7 +76,6 @@ class HeadersSyncTest(BitcoinTestFramework):
                 if "getheaders" in p.last_message:
                     count += 1
                     peer_receiving_getheaders = p
-                    p.last_message.pop("getheaders", None)
                     p.send_message(msg_headers()) # Send empty response, see above
 
         assert_equal(count, 1)
@@ -89,14 +84,14 @@ class HeadersSyncTest(BitcoinTestFramework):
         self.announce_random_block(all_peers)
 
         self.log.info("Check that peer1 receives a getheaders in response")
-        peer1.wait_for_getheaders()
+        peer1.wait_for_getheaders(block_hash=best_block_hash)
 
         self.log.info("Check that the remaining peer received a getheaders as well")
         expected_peer = peer2
         if peer2 == peer_receiving_getheaders:
             expected_peer = peer3
 
-        expected_peer.wait_for_getheaders()
+        expected_peer.wait_for_getheaders(block_hash=best_block_hash)
 
         self.log.info("Success!")
 
diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
index af47c6d9f0..213c748eda 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -191,14 +191,13 @@ class TestP2PConn(P2PInterface):
     def announce_block_and_wait_for_getdata(self, block, use_header, timeout=60):
         with p2p_lock:
             self.last_message.pop("getdata", None)
-            self.last_message.pop("getheaders", None)
         msg = msg_headers()
         msg.headers = [CBlockHeader(block)]
         if use_header:
             self.send_message(msg)
         else:
             self.send_message(msg_inv(inv=[CInv(MSG_BLOCK, block.sha256)]))
-            self.wait_for_getheaders(timeout=timeout)
+            self.wait_for_getheaders(block_hash=block.hashPrevBlock, timeout=timeout)
             self.send_message(msg)
         self.wait_for_getdata([block.sha256], timeout=timeout)
 
diff --git a/test/functional/p2p_sendheaders.py b/test/functional/p2p_sendheaders.py
index 508d6fe403..27a3aa8fb9 100755
--- a/test/functional/p2p_sendheaders.py
+++ b/test/functional/p2p_sendheaders.py
@@ -311,6 +311,7 @@ class SendHeadersTest(BitcoinTestFramework):
 
         # Now that we've synced headers, headers announcements should work
         tip = self.mine_blocks(1)
+        expected_hash = tip
         inv_node.check_last_inv_announcement(inv=[tip])
         test_node.check_last_headers_announcement(headers=[tip])
 
@@ -334,7 +335,10 @@ class SendHeadersTest(BitcoinTestFramework):
                 if j == 0:
                     # Announce via inv
                     test_node.send_block_inv(tip)
-                    test_node.wait_for_getheaders()
+                    if i == 0:
+                        test_node.wait_for_getheaders(block_hash=expected_hash)
+                    else:
+                        assert "getheaders" not in test_node.last_message
                     # Should have received a getheaders now
                     test_node.send_header_for_blocks(blocks)
                     # Test that duplicate inv's won't result in duplicate
@@ -521,6 +525,7 @@ class SendHeadersTest(BitcoinTestFramework):
         self.log.info("Part 5: Testing handling of unconnecting headers")
         # First we test that receipt of an unconnecting header doesn't prevent
         # chain sync.
+        expected_hash = tip
         for i in range(10):
             self.log.debug("Part 5.{}: starting...".format(i))
             test_node.last_message.pop("getdata", None)
@@ -533,15 +538,14 @@ class SendHeadersTest(BitcoinTestFramework):
                 block_time += 1
                 height += 1
             # Send the header of the second block -> this won't connect.
-            with p2p_lock:
-                test_node.last_message.pop("getheaders", None)
             test_node.send_header_for_blocks([blocks[1]])
-            test_node.wait_for_getheaders()
+            test_node.wait_for_getheaders(block_hash=expected_hash)
             test_node.send_header_for_blocks(blocks)
             test_node.wait_for_getdata([x.sha256 for x in blocks])
             [test_node.send_message(msg_block(x)) for x in blocks]
             test_node.sync_with_ping()
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), blocks[1].sha256)
+            expected_hash = blocks[1].sha256
 
         blocks = []
         # Now we test that if we repeatedly don't send connecting headers, we
@@ -556,13 +560,12 @@ class SendHeadersTest(BitcoinTestFramework):
 
         for i in range(1, MAX_NUM_UNCONNECTING_HEADERS_MSGS):
             # Send a header that doesn't connect, check that we get a getheaders.
-            with p2p_lock:
-                test_node.last_message.pop("getheaders", None)
             test_node.send_header_for_blocks([blocks[i]])
-            test_node.wait_for_getheaders()
+            test_node.wait_for_getheaders(block_hash=expected_hash)
 
         # Next header will connect, should re-set our count:
         test_node.send_header_for_blocks([blocks[0]])
+        expected_hash = blocks[0].sha256
 
         # Remove the first two entries (blocks[1] would connect):
         blocks = blocks[2:]
@@ -571,10 +574,8 @@ class SendHeadersTest(BitcoinTestFramework):
         # before we get disconnected.  Should be 5*MAX_NUM_UNCONNECTING_HEADERS_MSGS
         for i in range(5 * MAX_NUM_UNCONNECTING_HEADERS_MSGS - 1):
             # Send a header that doesn't connect, check that we get a getheaders.
-            with p2p_lock:
-                test_node.last_message.pop("getheaders", None)
             test_node.send_header_for_blocks([blocks[i % len(blocks)]])
-            test_node.wait_for_getheaders()
+            test_node.wait_for_getheaders(block_hash=expected_hash)
 
         # Eventually this stops working.
         test_node.send_header_for_blocks([blocks[-1]])
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index ce76008c46..00bd1e4017 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -644,15 +644,17 @@ class P2PInterface(P2PConnection):
 
         self.wait_until(test_function, timeout=timeout)
 
-    def wait_for_getheaders(self, *, timeout=60):
-        """Waits for a getheaders message.
+    def wait_for_getheaders(self, block_hash=None, *, timeout=60):
+        """Waits for a getheaders message containing a specific block hash.
 
-        Receiving any getheaders message will satisfy the predicate. the last_message["getheaders"]
-        value must be explicitly cleared before calling this method, or this will return
-        immediately with success. TODO: change this method to take a hash value and only
-        return true if the correct block header has been requested."""
+        If no block hash is provided, checks whether any getheaders message has been received by the node."""
         def test_function():
-            return self.last_message.get("getheaders")
+            last_getheaders = self.last_message.pop("getheaders", None)
+            if block_hash is None:
+                 return last_getheaders
+            if last_getheaders is None:
+                return False
+            return block_hash == last_getheaders.locator.vHave[0]
 
         self.wait_until(test_function, timeout=timeout)