#19315 currently has a test failure 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 🔎
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.
the CI failure is a FileNotFoundError from feature_config_args, so seems unrelated
543@@ -544,7 +544,7 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
544
545 def num_test_p2p_connections(self):
546 """Return number of test framework p2p connections to the node."""
547- return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION])
548+ return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION.decode("utf-8")])
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)
ACK 3ebde2143aa98af213872b98b474d904e55056f7
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.
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 msg_version handing by the test framework. I’ll open a PR for that after this is merged.