test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py #30761

1. 
   maflcko commented at 10:05 am on August 30, 2024: member

   Similar to #30705:

   The goal of this test case is to check that the sync works at all, not to check any timeout.

   On extremely slow hardware (for example qemu virtual hardware), downloading the 4110 BLOCKS_TO_MINE may take longer than the block download timeout.

   Fix it by pinning the time using mocktime temporarily, and advance it immediately after the sync.

2. test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py fa247e6e8c
3. 
   DrahtBot commented at 10:05 am on August 30, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	tdb3

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

4. DrahtBot added the label Tests on Aug 30, 2024
5. 
   maflcko commented at 10:20 am on August 30, 2024: member

   Example: https://drahtbot.space/temp_scratch/p2p_headers_sync_with_minchainwork_257.tar.zstd , which contains a [msghand] [net_processing.cpp:6260] [SendMessages] Timeout downloading block 275fa5456f30065b7a557e56c1232e6754f98eec1fbb65b6a539e15cd3134945 from peer=1, disconnecting.
6. in test/functional/p2p_headers_sync_with_minchainwork.py:56 in fa247e6e8c

   49@@ -48,6 +50,10 @@ def reconnect_all(self):
   50         self.connect_nodes(0, 2)
   51         self.connect_nodes(0, 3)
   52 
   53+    def mocktime_all(self, time):
   54+        for n in self.nodes:
   55+            n.setmocktime(time)
   56+
   

   ---

   ---

   

   tdb3 commented at 7:08 pm on August 31, 2024:

   Setting mock time for all nodes seems like it could potentially be useful in multiple functional tests. Maybe it could be added to BitcoinTestFramework rather than live in this functional test?

   ---

   

   maflcko commented at 6:25 am on September 2, 2024:

   Possibly. I guess, if another test needs it, it should be trivial to move?

   Edit: To extend on that,

   • I is unclear whether another test will need this, so moving to the framework seems too early.
   • The framework already has methods on the test node to deal with mocktime, so adding more will increase the complexity and bloat even more.

   ---

   

   tdb3 commented at 2:05 pm on September 2, 2024:

   That’s reasonable. I don’t feel super strongly about it.
7. 
   tdb3 commented at 7:09 pm on August 31, 2024: contributor

   Concept ACK Left a question/suggestion.
8. 
   stratospher commented at 12:09 pm on September 2, 2024: contributor

   ACK fa247e6. Checked the timeout downloading block logs before/after using setmocktime.
9. tdb3 approved
10. 
    tdb3 commented at 2:05 pm on September 2, 2024: contributor

    ACK fa247e6e8c7fddf9e3461c3e2e6f5fade0fe64cf
11. fanquake merged this on Sep 2, 2024
12. fanquake closed this on Sep 2, 2024

    ---

13. maflcko deleted the branch on Sep 2, 2024
14. 
    marcofleon commented at 2:17 pm on September 2, 2024: contributor

    Code review ACK fa247e6e8c7fddf9e3461c3e2e6f5fade0fe64cf. Post merge

Contributors
maflcko DrahtBot tdb3 stratospher marcofleon

Labels
Tests