test: Fix p2p_leak.py intermittent failure #22153

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202106_fix_p2p_leak changing 1 files +4 −2

mzumsande commented at 8:07 PM on June 4, 2021: member

Fixes #22085

Root cause: There was just 1 second between the wait (5 seconds) and the -peertimeout=4. Since ShouldRunInactivityChecks in net.cpp measures the timeout in seconds, its result can only change once per second, even though it is called more often. So in situations when the connection is established early in a given second like here (2021-05-27T12:28:04.001913Z ), the 1 second leeway was not be sufficient, leading to the intermittent failures.

Fix this by lowering the timeout by one second.

in test/functional/p2p_leak.py:137 in 4150a3e7d3 outdated

 133 | @@ -134,7 +134,7 @@ def run_test(self):
 134 |          self.nodes[0].generate(nblocks=1)
 135 |  
 136 |          # Give the node enough time to possibly leak out a message
 137 | -        time.sleep(5)
 138 | +        time.sleep(6)

MarcoFalke commented at 8:14 PM on June 4, 2021:

        time.sleep(peer_timeout + 2)

MarcoFalke approved
MarcoFalke commented at 8:15 PM on June 4, 2021: member

ACK. Nice find.

Alternative could be to set the peer timeout to 3 and leave the 5 secs.

nit: Could make peer_timeout a python constant
test: Fix p2p_leak.py intermittent failure by lowering timeout ca3a77068b
mzumsande force-pushed on Jun 4, 2021
mzumsande commented at 8:47 PM on June 4, 2021: member

I changed this to decreasing the timeout (was a little afraid that this could lead to other kinds of spurious bugs, but 3s should still be more than enough) and used a constant as suggested.
DrahtBot added the label Tests on Jun 4, 2021
ghost commented at 9:52 PM on June 4, 2021: none

Concept ACK

This test failed for one of my PR: #21157

https://cirrus-ci.com/task/4842445461520384?logs=ci#L3127

Not sure about the approach used in https://github.com/bitcoin/bitcoin/pull/22153/commits/ca3a77068b8c9c6107d078ea083f4ab7c0010548

I was reading few things suggested to pause in python until few other things have completed: https://blog.miguelgrinberg.com/post/how-to-make-python-wait. Maybe one of them can be used.
MarcoFalke commented at 6:36 AM on June 5, 2021: member

re-ACK ca3a77068b8c9c6107d078ea083f4ab7c0010548
MarcoFalke merged this on Jun 5, 2021
MarcoFalke closed this on Jun 5, 2021
MarcoFalke commented at 11:30 AM on June 5, 2021: member

@prayank23 Good idea. I think we can use assert_debug_log to speed up the test by 1 second and apply a stricter check
mzumsande deleted the branch on Jun 5, 2021
sidhujag referenced this in commit 97b22f2243 on Jun 6, 2021
gwillen referenced this in commit b9aa053c96 on Jun 1, 2022
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#22085 qa: Intermittent failure in p2p_leak.py