Merge bitcoin/bitcoin#25454: p2p: Avoid multiple getheaders messages in flight to the same peer

99f4785cad94657dcf349d00fdd6f1d44cac9bb0 Replace GetTime() with NodeClock in MaybeSendGetHeaders() (Suhas Daftuar)
abf5d16c24cb08b0451bdbd4d1de63a12930e8f5 Don't send getheaders message when another request is outstanding (Suhas Daftuar)
ffe87db247b19ffb8bfba329c5dd0be39ef5a53f Cleanup received_new_header calculation to use WITH_LOCK (Suhas Daftuar)
6d95cd3e7444ebaaabb64a76783ea3551530f1d7 Move peer state updates from headers message into separate function (Suhas Daftuar)
2b341db731793844f12944363186edea23eabdeb Move headers direct fetch to end of ProcessHeadersMessage (Suhas Daftuar)
29c45185223441943ab610e62937a118c7c3a5b2 Move headers-direct-fetch logic into own function (Suhas Daftuar)
bf8ea6df75749c27f753b562c4724b3f8d263ad4 Move additional headers fetching to own function (Suhas Daftuar)
9492e93bf9f4a841bf43ca4b593871c0863d5b63 Add helper function for checking header continuity (Suhas Daftuar)
7f2450871b3ea0b4d02d56bd2ca365fcc25cf90e Move handling of unconnecting headers into own function (Suhas Daftuar)

Pull request description:

  Change `getheaders` messages so that we wait up to 2 minutes for a response to a prior `getheaders` message before issuing a new one.

  Also change the handling of the `getheaders` message sent in response to a block INV, so that we no longer use the hashstop variable (including the hash stop will just mean that if our peer's headers chain is longer, then we won't learn
  it, so there's no benefit to using hashstop).

  Also, now respond to a `getheaders` during IBD with an empty headers message (rather than nothing) -- this better conforms to the intent of the new logic that it's better to not ignore a peer's `getheaders` message, even if you have nothing to give. This also avoids a lot of functional tests breaking.

  This PR also reworks the headers processing logic to make it more readable.

ACKs for top commit:
  ajtowns:
    ACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0 ; code review, check over new logic of when to send getheaders messages
  dergoegge:
    Code review ACK  99f4785cad94657dcf349d00fdd6f1d44cac9bb0
  mzumsande:
    Code Review ACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0
  sipa:
    utACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0
  w0xlt:
    tACK https://github.com/bitcoin/bitcoin/pull/25454/commits/99f4785cad94657dcf349d00fdd6f1d44cac9bb0 Good improvement in the code.

Tree-SHA512: b8a63f6f71ac83e292edc0200def7835ad8b06b2955dd34e3ea6fac85980fa6962efd31d689ef5ea121ff5477ec14aafa4bbe2d0db134c05f4a31a57a8ced365

2 files changed, 5 insertions, 1 deletions

diff --git a/test/functional/feature_minchainwork.py b/test/functional/feature_minchainwork.py
index fa10855a98..9d0ad5ef9d 100755
--- a/test/functional/feature_minchainwork.py
+++ b/test/functional/feature_minchainwork.py
@@ -82,7 +82,7 @@ class MinimumChainWorkTest(BitcoinTestFramework):
         msg.hashstop = 0
         peer.send_and_ping(msg)
         time.sleep(5)
-        assert "headers" not in peer.last_message
+        assert ("headers" not in peer.last_message or len(peer.last_message["headers"].headers) == 0)
 
         self.log.info("Generating one more block")
         self.generate(self.nodes[0], 1)
diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
index 952f1e5cc5..eedb7e6fa1 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -371,6 +371,10 @@ class SegWitTest(BitcoinTestFramework):
         block1 = self.build_next_block()
         block1.solve()
 
+        # Send an empty headers message, to clear out any prior getheaders
+        # messages that our peer may be waiting for us on.
+        self.test_node.send_message(msg_headers())
+
         self.test_node.announce_block_and_wait_for_getdata(block1, use_header=False)
         assert self.test_node.last_message["getdata"].inv[0].type == blocktype
         test_witness_block(self.nodes[0], self.test_node, block1, True)