test: Avoid race after connect_nodes #22817

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2108-testRaceConnect changing 1 files +13 −12

MarcoFalke commented at 8:19 AM on August 27, 2021: member

Wait until the connection is fully established on both sides (verack). Fixes #22714
test: Avoid race after connect_nodes fa04f26aa7
fanquake added the label Tests on Aug 27, 2021
in test/functional/test_framework/test_framework.py:574 in fa04f26aa7
```
 581 | +        # See comments in net_processing:
 582 | +        # * Must have a version message before anything else
 583 | +        # * Must have a verack message before anything else
 584 | +        wait_until_helper(lambda: all(peer['version'] != 0 for peer in from_connection.getpeerinfo()))
 585 | +        wait_until_helper(lambda: all(peer['version'] != 0 for peer in to_connection.getpeerinfo()))
 586 | +        wait_until_helper(lambda: all(peer['bytesrecv_per_msg'].pop('verack', 0) == 24 for peer in from_connection.getpeerinfo()))
```
laanwj commented at 2:23 PM on August 27, 2021:

Any reason to use pop here instead of get? (I don't think so, there's no reason to mutate the structure) Also: >= 24 might be more robust; it's clearly wrong if more than one verack gets sent, but it shouldn't hang forever in that case.

MarcoFalke commented at 7:22 AM on August 28, 2021:

This is the existing code, so I didn't touch it. I forgot to mention to review with --ignore-all-space.

I think it is fine for tests to fail when two veracks are sent.

kiminuo commented at 6:23 PM on September 24, 2021:

This is the existing code, so I didn't touch it.

I was thinking that .get(key, default) did not exist in older Python versions but even Python 2.7 supports it. However, the change comes from this PR #18866 :-)

Still I think .pop('verack', 0) ->.get('verack', 0) should work just fine as proposed by @laanwj and it seems more natural.
kiminuo commented at 6:34 PM on September 24, 2021: contributor

utACK fa04f26aa77d2cf746db4a8a43068b7c5c9cd02b

The change looks good me.
MarcoFalke merged this on Sep 25, 2021
MarcoFalke closed this on Sep 25, 2021
MarcoFalke deleted the branch on Sep 25, 2021
sidhujag referenced this in commit 55937c70c9 on Sep 25, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#22714 test: wait_until() failure in p2p_permissions.py