test: Avoid race after connect_nodes#22817
Conversation
| # * Must have a verack message before anything else | ||
| wait_until_helper(lambda: all(peer['version'] != 0 for peer in from_connection.getpeerinfo())) | ||
| wait_until_helper(lambda: all(peer['version'] != 0 for peer in to_connection.getpeerinfo())) | ||
| wait_until_helper(lambda: all(peer['bytesrecv_per_msg'].pop('verack', 0) == 24 for peer in from_connection.getpeerinfo())) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
utACK fa04f26 The change looks good me. |
fa04f26 test: Avoid race after connect_nodes (MarcoFalke) Pull request description: Wait until the connection is fully established on both sides (verack). Fixes bitcoin#22714 ACKs for top commit: kiminuo: utACK fa04f26 Tree-SHA512: bc2c44b44b688086ff84046924cf5251dd625584e93ce8fa17de27023855b32f3bb55109b846abbcec775e2836c7f3c5a81d6b4aff7c4ac065b9aefa044c1883
4 participants
Wait until the connection is fully established on both sides (verack). Fixes #22714