test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py #30761

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2408-test-p2p changing 1 files +9 −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.
test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py fa247e6e8c
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.
DrahtBot added the label Tests on Aug 30, 2024
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.
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.
tdb3 commented at 7:09 PM on August 31, 2024: contributor

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

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

ACK fa247e6e8c7fddf9e3461c3e2e6f5fade0fe64cf
fanquake merged this on Sep 2, 2024
fanquake closed this on Sep 2, 2024
maflcko deleted the branch on Sep 2, 2024
marcofleon commented at 2:17 PM on September 2, 2024: contributor

Code review ACK fa247e6e8c7fddf9e3461c3e2e6f5fade0fe64cf. Post merge
fanquake referenced this in commit 342baabaff on Sep 3, 2024
achow101 referenced this in commit fa46088440 on Sep 5, 2024
bitcoin locked this on Sep 2, 2025

Type	Reviewers
Concept ACK	tdb3

Contributors

Labels

Linked (view graph)

#30705 test: Avoid intermittent block download timeout in p2p_ibd_stalling #30762 [28.x] rc backports

test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py #30761

Code Coverage

Reviews