test: fix intermittent failures in p2p_timeouts.py #23812

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202112_p2p_timeout_fix changing 1 files +7 −4

mzumsande commented at 10:17 AM on December 18, 2021: member

Fixes #23800 by making sure that all peers are connected (i.e. m_connected is set) before the mocktime is bumped. We can't wait for verack here, but we can wait for a debug log entry ("Added connection peer=2") instead.

In the failed CI runs (e.g. https://cirrus-ci.com/task/5600553806856192?logs=ci#L7469) different peers were added at different mocktimes.
in test/functional/p2p_timeouts.py:56 in 1f3140ffa9 outdated
```
  55 | +        # Setup the p2p connections, making sure the connections are established before the mocktime is bumped
  56 | +        with self.nodes[0].assert_debug_log(['Added connection peer=0']):
  57 | +            no_verack_node = self.nodes[0].add_p2p_connection(TestP2PConn(), wait_for_verack=False)
  58 | +        with self.nodes[0].assert_debug_log(['Added connection peer=1']):
  59 | +            no_version_node = self.nodes[0].add_p2p_connection(TestP2PConn(), send_version=False, wait_for_verack=False)
  60 | +        with self.nodes[0].assert_debug_log(['Added connection peer=2']):
```
MarcoFalke commented at 10:25 AM on December 18, 2021:

would an alternative be to connect no_verack_node last?

mzumsande commented at 11:00 AM on December 18, 2021:

Yes, I actually thought about that alternative, but relying on the order seemed a bit ugly to me too (and it would require changing later assert_debug_log because the peer ids would change). But I don't have a strong preference, would you prefer that solution?

MarcoFalke commented at 12:19 PM on December 18, 2021:

Or yet another alternative would be to append a full dummy connection for the sole purpose of syncing (wouldn't change the ids).

Personally I think any solution is fine.

mzumsande commented at 4:28 PM on December 18, 2021:

ok, then I'll keep it as is for now.
fanquake added the label Tests on Dec 18, 2021
mzumsande renamed this:
~~test: fix intermittent failures in p2p_timeout.py~~
test: fix intermittent failures in p2p_timeouts.py
on Dec 18, 2021

test: fix intermittent timeouts in p2p_timeouts.py

by checking that all nodes are added before the mocktime is bumped.
Fixes #23800

0a1b6fa5a1

mzumsande force-pushed on Dec 18, 2021
MarcoFalke approved
MarcoFalke commented at 10:45 AM on December 18, 2021: member

Excellent find and apologies for missing this.

Generally I am not a fan of using the debug log to sync two threads, so I was wondering about an alternative solution.

Though, this seems good to go as-is.
brunoerg commented at 2:18 AM on December 19, 2021: member

Concept ACK
theStack approved
theStack commented at 1:02 PM on December 19, 2021: member

Concept and approach ACK 0a1b6fa5a18f3efb2ac3e28a23a4fd5e1cf9eaf0
naumenkogs commented at 7:25 AM on December 20, 2021: member

ACK 0a1b6fa5a18f3efb2ac3e28a23a4fd5e1cf9eaf0
MarcoFalke merged this on Dec 20, 2021
MarcoFalke closed this on Dec 20, 2021
sidhujag referenced this in commit 3296e07e35 on Dec 20, 2021
DrahtBot locked this on Dec 20, 2022
mzumsande deleted the branch on Jun 23, 2023

Contributors

Labels

Linked (view graph)

#23800 ci, error: p2p_timeouts.py failed