-
Notifications
You must be signed in to change notification settings - Fork 717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Tests] Actually wait for nodes to connect #2948
[Tests] Actually wait for nodes to connect #2948
Conversation
… failures fae153b test: Fix verack race to avoid intermittent test failures (MarcoFalke) Pull request description: Fixes bitcoin#18832 ACKs for top commit: laanwj: ACK fae153b Tree-SHA512: 071de8c8e2b2787c9433c7460e18b9a54beaf471a52ce848c5ac7263fc2a40f5b976d4f558ecc494fd0fa07284b7c98d29267cade58f80ab74fe9a7d18d94298
… instead of index + node
faee330 test: Fail if connect_nodes fails (MacroFake) Pull request description: Currently, `connect_nodes` will return silently when the connection is disconnected while connecting. This is confusing, so fix it. Can be tested by reverting the signet test change and observing the failure when running the test. ACKs for top commit: laanwj: Tested ACK faee330 Tree-SHA512: 641ca8adcb9f5ff33239b143573bddc0dfde41dbd103751ee870f1572ca2469f6a0d4bab6693102454cd3e270ef8251d87fbfac48f6d8adac70d2d6bbffaae56
…s.py fa1bf4e test: Fix intermittent timeout in p2p_permissions.py (MarcoFalke) Pull request description: The sync is based on `bytesrecv_per_msg["verack"]`. However, the bytes are counted before processing the message, so they are not sufficient to ensure the connection is fully up. ACKs for top commit: mzumsande: ACK fa1bf4e aureleoules: ACK fa1bf4e Tree-SHA512: eb1ed537032c76a449b1ed5e42ff062e9b8b3c7e11fde2a5b8183ae0d6fbe31dba39e2c758836160cd8157d9ac5cc1f5d1916415861b8d711b7370c88f5e9790
6629d1d test: improve robustness of connect_nodes() (furszy) Pull request description: Decoupled from bitcoin#27837 because this can help other too, found it investigating a CI failure https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200. The `connect_nodes` function in the test framework relies on a stable number of peer connections to verify that the new connection between the nodes is successfully established. This approach is fragile, as any of the peers involved in the process can drop, lose, or create a connection at any step, causing subsequent `wait_until` checks to stall indefinitely even when the peers in question were connected successfully. This commit improves the situation by using the nodes' subversion and the connection direction (inbound/outbound) to identify the exact peer connection and perform the checks exclusively on it. ACKs for top commit: stratospher: reACK 6629d1d. achow101: ACK 6629d1d maflcko: utACK 6629d1d AngusP: re-ACK 6629d1d Tree-SHA512: 5f345c0ce49ea81b643e97c5cffd133e182838752c27592fcdeac14ad10919fb4b7ff38e289e42a7c3c638a170bd0d0b7a9cd493898997a2082a7b7ceef4aeeb
0000276 test: Remove redundant verack check (MarcoFalke) Pull request description: Currently the sync in `connect_nodes` mentions the `version` and `verack` message types, but only checks the `verack`. Neither check is required, as the `pong` check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn't add value. Also clarify in the comments that the goal is to check the flag `fSuccessfullyConnected` indirectly. ACKs for top commit: furszy: utACK 0000276 brunoerg: ACK 0000276 tdb3: ACK 0000276 Tree-SHA512: f9ddcb1436d2f70da462a8dd470ecfc90a534dd6507c23877ef7626e7c02326c077001a42ad0171a87fba5c5275d1970d8c5e5d82c56c8412de944856fdfd6db
819b87a
to
6fe4635
Compare
6fe4635
to
5f8c06e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 5f8c06e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 5f8c06e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 5f8c06e
Looks like a much better improvement, will rebase the json-rpc pr after merging this!
This PR solves the failures of
wallet_listtransactions.py
, like the one of https://github.com/PIVX-Project/PIVX/actions/runs/11705208154/job/32601252220?pr=2947.They happen due to mempool sync timeout:
The issue is that the connection between nodes is established after the transaction is sent, as we can see from the logs:
Hence the newly connected node will never receive the transaction and the mempool will never be synced.
This bug is fixed by ensuring that
connect_nodes
actually wait for the connection to be established. As a consequence of those checks we cannot anymore connect nodes in parallel inconnect_nodes_clique
(which will make tests run slightly slower)