test: headers sync timeout #32677

1. 
   stringintech commented at 6:50 pm on June 3, 2025: contributor

   When reviewing PR #32051 and considering which functional tests might need to be adapted/extended accordingly, I noticed there appears to be limited functional test coverage for header sync timeouts and thought it might be helpful to add one.

   This test attempts to cover two scenarios:

   1. Normal peer timeout behavior: When a peer fails to respond to initial getheaders requests within the timeout period, it should be disconnected and the node should attempt to sync headers from the next available peer.

   2. Noban peer behavior: When a peer with noban privileges times out, it should remain connected while the node still attempts to sync headers from other peers.

2. 
   DrahtBot commented at 6:50 pm on June 3, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32677.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	yuvicc, mzumsande

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Jun 3, 2025
4. stringintech force-pushed on Jun 3, 2025
5. DrahtBot added the label CI failed on Jun 3, 2025
6. DrahtBot removed the label CI failed on Jun 3, 2025
7. 
   yuvicc commented at 4:53 am on June 6, 2025: contributor

   Concept ACK

   Great observation and thanks for adding. I was thinking if we could add this timeout test in p2p_initial_headers_sync.py instead of creating another one.

8. 
   stringintech commented at 7:28 pm on June 7, 2025: contributor

   I was thinking if we could add this timeout test in p2p_initial_headers_sync.py instead of creating another one. @yuvicc Actually, I did not consider that. However, now that I am thinking about it, the two sub-tests in the new test file share some context that is not used by the test file you mentioned. So, I think it is cleaner to keep them separate, especially if it will be extended to test the addnode peers as well (like here). But I will be thinking more about it while waiting for other feedback.

9. in test/functional/p2p_initial_headers_sync_timeout.py:83 in 16a62fa881 outdated

   78+                expected_msgs=["initial getheaders (0) to peer=0"]):
   79+            self.peer1 = self.node.add_p2p_connection(P2PInterface())
   80+        self.peer1.wait_for_getheaders()
   81+
   82+        self.log.info("Add outbound peer2")
   83+        with self.node.wait_for_new_peer():
   

   ---

   ---

   

   mzumsande commented at 1:43 am on June 9, 2025:

   this seems unnecessary, add_outbound_p2p_connection already has a built-in wait for the verack.
10. in test/functional/p2p_initial_headers_sync_timeout.py:94 in 16a62fa881 outdated

    89+        assert_equal(self.node.num_test_p2p_connections(), 2)
    90+
    91+    def trigger_timeout(self):
    92+        self.log.info("Trigger the headers download timeout by advancing "
    93+                      "mock time")
    94+        best_header_time = self.node.getblockchaininfo()['time']
    

    ---

    ---

    

    mzumsande commented at 1:51 am on June 9, 2025:

    This doesn’t return the timestamp of the best header, but the time of the tip (which is still at genesis, after all headers up to minchainwork have been downloaded but no blocks connected). As far as I know (didn’t check) we don’t expose m_chainman.m_best_header->Time() via RPC, so to test the dynamic part in a functional test we would have to check the actual timestamps of the headers we sent the node, instead of using getblockchaininfo.

    ---

    

    stringintech commented at 9:39 pm on June 11, 2025:

    I used getblockchaininfo()['time'] because we hadn’t sent the node any headers yet, so I relied on the assumption that the best header should still be at genesis with the genesis block time (which I should have commented better). But I agree with your suggestion as it is more direct and I have applied it - I appreciate your further feedback on it.
11. 
    mzumsande commented at 2:07 am on June 9, 2025: contributor

    Concept ACK. Didn’t read the above, but I also thought it might be better to add the additional tests to p2p_initial_headers_sync.py - after all, this is not a particularly long test.
12. test: headers sync timeout bff80f59bd
13. stringintech force-pushed on Jun 11, 2025
14. 
    stringintech commented at 9:45 pm on June 11, 2025: contributor

    16a62fa to bff80f5 - Addressed feedback by @mzumsande and @yuvicc; Thanks!

Contributors
stringintech DrahtBot yuvicc mzumsande

Labels
Tests