[test] Fix sync issue in disconnect_p2ps #20522
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MY_SUBVERSION is defined in messages.py as a byte string, but here we were comparing this value to the value returned by the RPC. Convert to ensure the types match.
Contributor
Author
Contributor
Author
|
the CI failure is a
|
maflcko
reviewed
Nov 28, 2020
| def num_test_p2p_connections(self): | ||
| """Return number of test framework p2p connections to the node.""" | ||
| return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION]) | ||
| return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION.decode("utf-8")]) |
Member
There was a problem hiding this comment.
It might be better to leave this as-is and call encode where it is needed. Doing so will prevent similar issues in the future. Missing an encode, however, can't result in a hard-to-debug issue because it is detected immediately as a type error (either by mypy or at runtime)
jonatack
reviewed
Nov 28, 2020
Member
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Concept ACK. A passing/failing test would be good to demonstrate that this works and document the case (and possibly added to the test framework tests).
Contributor
|
ACK 3ebde21
I agree. However, I think this fix is good enough for now and unblocks #19315, so we should merge it. I have a branch here: https://github.com/jnewbery/bitcoin/tree/pr20522.1 which changes MY_SUBVERSION to be a string, and does a lot more cleaning up of the |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Nov 29, 2020
3ebde21 [test] Fix wait condition in disconnect_p2ps (Amiti Uttarwar) Pull request description: bitcoin#19315 currently has a [test failure](https://cirrus-ci.com/task/4545582645641216) because of a race. `disconnect_p2ps` is intended to have a `wait_until` clause that prevents this race, but the conditional doesn't match since its comparing two different object types. `MY_SUBVERSION` is defined in messages.py as a byte string, but is compared to the value returned by the RPC. This PR simply converts types to ensure they match, which should prevent the race from occurring. HUGE PROPS TO jnewbery for discovering the issue 🔎 ACKs for top commit: jnewbery: ACK 3ebde21 glozow: Code review ACK bitcoin@3ebde21 Tree-SHA512: ca096b80a3e4d757a645f38846d6dc89d6a3d35c3435513a72d278e305faddd4aff9e75a767941b51b2abbf59c82679bac1e9a0140d6f285efe3053e51bcc2a8
laanwj
added a commit
that referenced
this pull request
Feb 18, 2021
9f21ed4 [test] Check user agent string from test framework connections (John Newbery) 9ce4c3c [test] Add P2P_SERVICES to p2p.py (John Newbery) 0105426 [test] Move MY_RELAY to p2p.py (John Newbery) 9b4054c [test] Move MY_SUBVERSION to p2p.py (John Newbery) 7e158a6 [test] Move MY_VERSION to p2p.py (John Newbery) 6523111 [test] Move MIN_VERSION_SUPPORTED to p2p.py (John Newbery) Pull request description: The messages.py module should contain code and helpers for [de]serializing p2p messages. Specific usage of those messages should be in p2p.py. This PR moves test framework specific constants to p2p.py. It also changes the SUBVERSION constant to be a string instead of a bytes object. That means that it needs to be explicitly converted to a bytes object to serialize into a version message. Failing to do so would cause an easy-to-spot bug. This should avoid silent failures like the one solved in #20522. ACKs for top commit: laanwj: Code review ACK 9f21ed4 Tree-SHA512: 41d46575ac0ec36ad074d6c6a5b9cef50b05eeb8ddd8ed0a8f0d0c4617cc7b8baa6580af5b83a668230ce1ac27bf0e56914d0361a48b1b05fd75e2e60350eeaf
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jan 19, 2022
Summary: ``` MY_SUBVERSION is defined in messages.py as a byte string, but here we were comparing this value to the value returned by the RPC. Convert to ensure the types match. ``` Backport of [[bitcoin/bitcoin#20522 | core#20522]]. This is the fix to the occasional failures in p2p_add_connections that can be seen on CI. Test Plan: ``` for i in {1..100} do ./test/functional/test_runner.py p2p_add_connections done ``` Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D10846
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#19315 currently has a test failure because of a race.
disconnect_p2psis intended to have await_untilclause that prevents this race, but the conditional doesn't match since its comparing two different object types.MY_SUBVERSIONis defined in messages.py as a byte string, but is compared to the value returned by the RPC. This PR simply converts types to ensure they match, which should prevent the race from occurring.HUGE PROPS TO jnewbery for discovering the issue 🔎