Fixes #29508.
Make sure that v2 handshake is complete before comparing getpeerinfo outputs so that transport_protocol_type isn't stuck at 'detecting'.
This is done by adding a wait_until statement till transport_protocol_type = v2 so that bitcoind waits until the v2 handshake is complete. (on the python side, this is ensured by default since wait_for_handshake = True inside add_p2p_connection())
Make sure that v2 handshake is complete before comparing getpeerinfo
outputs so that `transport_protocol_type` isn't stuck at 'detecting'.
- on the python side, this is ensured by default
`wait_for_handshake = True` inside `add_p2p_connection()`.
- on the c++ side, add a wait_until statement till
`transport_protocol_type = v2` so that v2 handshake is complete.
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
One way to test this patch would be using this diff. The intermittent failure happens when v2 handshake is complete on the python side but not on bitcoind side. That is - bitcoind is in SendState::READY (meaning bitcoind has sent garbage terminator + version packet to our python P2PInterface and wait_for_v2_handshake=True) and RecvState paused in something like RecvState::GARB_GARBTERM.
this diff delays the garbage terminator being sent from the python side using sleep so that bitcoind can be in SendState::READY before RecvState::GARB_GARBTERM.
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index dc04696114..b546bad4cf 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -270,10 +270,10 @@ class P2PConnection(asyncio.Protocol):
# and sends response after deriving shared ECDH secret using received ellswift bytes
length, response = self.v2_state.complete_handshake(BytesIO(self.recvbuf))
self.recvbuf = self.recvbuf[length:]
- if response:
- self.send_raw_message(response)
- else:
- return # only after response is sent can `authenticate_handshake()` be done
# `self.v2_state.peer` is instantiated only after shared ECDH secret/BIP324 derived keys and ciphers
# is derived in `complete_handshake()`.
@@ -283,6 +283,9 @@ class P2PConnection(asyncio.Protocol):
if not is_mac_auth:
raise ValueError("invalid v2 mac tag in handshake authentication")
self.recvbuf = self.recvbuf[length:]
+ if response:
+ import time; time.sleep(5)
+ self.send_raw_message(response)
if self.v2_state.tried_v2_handshake:
# for v2 outbound connections, send version message immediately after v2 handshake
if self.p2p_connected_to_node:
ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d
Using your patch I was able to reproduce the failure, and using this PR the test would pass.
I might actually be useful to have a (deterministically random) delay in test_framework/p2p.py where you put it. 0.1 seconds already does the trick for me.
112 | @@ -113,6 +113,8 @@ def test_getpeerinfo(self): 113 | self.nodes[0].setmocktime(no_version_peer_conntime) 114 | with self.nodes[0].wait_for_new_peer(): 115 | no_version_peer = self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False) 116 | + if self.options.v2transport: 117 | + self.wait_until(lambda: self.nodes[0].getpeerinfo()[no_version_peer_id]["transport_protocol_type"] == "v2")
Does it never go in "detecting" for v1?
Edit: I guess it does, and in this case, this would be more robust as waiting for the value to become one of "v1" or "v2" or waiting for it to not be "detecting", regardless of self.options.v2transport
Not in this test, because the test framework always sets the -v2transport bitcoind arg explicitly, see here. If we just run python3 rpc_net.py, V1Transport::GetInfo() is called, and that can never be "detecting". If it wasn't like this, the value here would always be "detecting" instead of "v1", because with send_version=False ,the node cannot actually know whether it's v1 having received no data at all - it just assumes it because it doesn't know about v2.
Concept ACK.
ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d
Code Review ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d
ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d
Milestone
27.0
