Merge bitcoin/bitcoin#32823: test: Fix wait_for_getheaders() call in test_outbound_eviction_blocks_relay_only()

ec004cd test: Use rehash() in outbound eviction block-relay (pablomartin4btc)
26598ed test: Clarify roles in outbound eviction comments (pablomartin4btc)

Pull request description:

  This change avoids relying on `tip_header.hash`, which is `None` when the header is deserialized from hex during `CBlockHeader()` construction.
  Instead, `tip_header.rehash()` explicitly computes the hash, making the test behavior more robust.

  Using the explicit `rehash()` avoids depending on `wait_for_getheaders()` falling back to any received message, thus making the test more deterministic.

  This is a follow-up to #32742.

  Also, as noted in a previous review [comment](bitcoin/bitcoin#32742 (review)), "_the hash field is wrong either way, simply due to being the wrong type (it is an optional hex string), as opposed to an optional int_".

  ---

  The first commit intention is to improve clarity around the tests purpose, helping reviewers follow what's being verified and why. What started as a small comment during review of #32742 led me reviewing and try to improve most relevant tests comments for consistency and correctness.

ACKs for top commit:
  achow101:
    ACK ec004cd
  theStack:
    lgtm ACK ec004cd #️⃣
  yuvicc:
    ACK ec004cd
  danielabrozzoni:
    ACK ec004cd

Tree-SHA512: 6a14dedfdc425cd806f63443b3b9f79df69a7717452739f5d7fef1b2bdba23402670d63cf1d6b66c9f1a6b460d4d4a6f185426d0a4982fa95115a234cd6baef7

Author: achow101

test/functional/p2p_outbound_eviction.py

@@ -55,7 +55,7 @@ def test_outbound_eviction_unprotected(self):
         self.log.info("Test that the peer gets evicted")
         peer.wait_for_disconnect()
 
-        self.log.info("Create an outbound connection and send header but never catch up")
+        self.log.info("Create an outbound connection and send header but the peer never catches up")
         # Mimic a node that just falls behind for long enough
         # This should also apply for a node doing IBD that does not catch up in time
         # Connect a peer and make it send us headers ending in our tip's parent
@@ -75,15 +75,15 @@ def test_outbound_eviction_unprotected(self):
         self.log.info("Create an outbound connection and keep lagging behind, but not too much")
         # Test that if the peer never catches up with our current tip, but it does with the
         # expected work that we set when setting the timer (that is, our tip at the time)
-        # we do not disconnect the peer
+        # the node does not disconnect the peer
         peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="outbound-full-relay")
 
         self.log.info("Mine a block so our peer starts lagging")
         prev_prev_hash = tip_header.hashPrevBlock
         best_block_hash = self.generateblock(node, output="raw(42)", transactions=[])["hash"]
         peer.sync_with_ping()
 
-        self.log.info("Keep catching up with the old tip and check that we are not evicted")
+        self.log.info("The peer keeps catching up with the old tip; check that the node does not evict the peer")
         for i in range(10):
             # Generate an additional block so the peers is 2 blocks behind
             prev_header = from_hex(CBlockHeader(), node.getblockheader(best_block_hash, False))
@@ -96,21 +96,21 @@ def test_outbound_eviction_unprotected(self):
             node.setmocktime(cur_mock_time)
             peer.sync_with_ping()
 
-            # Wait until we get out last call (by receiving a getheaders)
+            # Make the peer wait until it gets node's last call (by receiving a getheaders)
             peer.wait_for_getheaders(block_hash=prev_prev_hash)
 
-            # Send a header with the previous tip (so we go back to 1 block behind)
+            # The peer sends a header with the previous tip (so the peer goes back to 1 block behind)
             peer.send_and_ping(msg_headers([prev_header]))
             prev_prev_hash = tip_header.hashPrevBlock
 
         self.log.info("Create an outbound connection and take some time to catch up, but do it in time")
         # Check that if the peer manages to catch up within time, the timeouts are removed (and the peer is not disconnected)
-        # We are reusing the peer from the previous case which already sent us a valid (but old) block and whose timer is ticking
+        # We are reusing the peer from the previous case which already sent the node a valid (but old) block and whose timer is ticking
 
-        # Send an updated headers message matching our tip
+        # Make the peer send an updated headers message matching our tip
         peer.send_and_ping(msg_headers([from_hex(CBlockHeader(), node.getblockheader(best_block_hash, False))]))
 
-        # Wait for long enough for the timeouts to have triggered and check that we are still connected
+        # Wait for long enough for the timeouts to have triggered and check that the peer is still connected
         cur_mock_time += (CHAIN_SYNC_TIMEOUT + 1)
         node.setmocktime(cur_mock_time)
         peer.sync_with_ping()
@@ -123,8 +123,10 @@ def test_outbound_eviction_unprotected(self):
 
     def test_outbound_eviction_protected(self):
         # This tests the eviction logic for **protected** outbound peers (that is, PeerManagerImpl::ConsiderEviction)
-        # Outbound connections are flagged as protected as long as they have sent us a connecting block with at least as
-        # much work as our current tip and we have enough empty protected_peers slots.
+        # Outbound connections are flagged as protected if:
+        #   - The peer sends a connecting block with at least as much work as our current tip.
+        #   - There are still available slots in the node's protected_peers list.
+        # This test ensures that such protected outbound peers are not disconnected even after chain sync and headers timeouts.
         node = self.nodes[0]
         cur_mock_time = node.mocktime
         tip_header = from_hex(CBlockHeader(), node.getblockheader(node.getbestblockhash(), False))
@@ -145,7 +147,7 @@ def test_outbound_eviction_protected(self):
         peer.wait_for_getheaders(block_hash=tip_header.hashPrevBlock)
         cur_mock_time += (HEADERS_RESPONSE_TIME + 1)
         node.setmocktime(cur_mock_time)
-        self.log.info("Test that the node does not get evicted")
+        self.log.info("Test that the peer does not get evicted")
         peer.sync_with_ping()
 
         node.disconnect_p2ps()
@@ -205,7 +207,7 @@ def test_outbound_eviction_mixed(self):
 
         cur_mock_time += (HEADERS_RESPONSE_TIME + 1)
         node.setmocktime(cur_mock_time)
-        self.log.info("Check how none of the honest nor protected peers was evicted but all the misbehaving unprotected were")
+        self.log.info("Check that none of the honest or protected peers were evicted, but all misbehaving unprotected peers were")
         for peer in protected_peers + honest_unprotected_peers:
             peer.sync_with_ping()
         for peer in misbehaving_unprotected_peers:
@@ -233,7 +235,7 @@ def test_outbound_eviction_blocks_relay_only(self):
         cur_mock_time += (CHAIN_SYNC_TIMEOUT + 1)
         node.setmocktime(cur_mock_time)
         peer.sync_with_ping()
-        peer.wait_for_getheaders(block_hash=tip_header.hash)
+        peer.wait_for_getheaders(block_hash=tip_header.rehash())
         cur_mock_time += (HEADERS_RESPONSE_TIME + 1)
         node.setmocktime(cur_mock_time)
         self.log.info("Test that the peer gets evicted")