Merge bitcoin/bitcoin#29353: test: p2p: adhere to typical VERSION message protocol flow

c340503 test: p2p: adhere to typical VERSION message protocol flow (Sebastian Falbesoner)
7ddfc28 test: p2p: process post-v2-handshake data immediately (Sebastian Falbesoner)
b198b9c test: p2p: introduce helper for sending prepared VERSION message (Sebastian Falbesoner)

Pull request description:

  This PR addresses a quirk in the test framework's p2p implementation regarding the version handshake protocol:

  Currently, the VERSION message is sent immediately after an inbound connection (i.e. TestNode outbound connection) is made. This doesn't follow the usual protocol flow where the initiator sends a version first, the responder processes that and only then responds with its own version message. Change that accordingly by only sending immediate VERSION message for outbound connections (or after v2 handshake for v2 connections, respectively), and sending out VERSION message as response for incoming VERSION messages (i.e. in the function `on_version`) for inbound connections.

  I first stumbled upon this issue through reading comment https://mirror.b10c.me/bitcoin-bitcoin/24748/#discussion_r1465420112 (see last paragraph) and recently again in the course of working on a v2-followup for #29279, where this causes issues for TestNode outbound connections that disconnect *before* sending out their own version message.

  Note that these changes lead to slightly more code in some functional tests that override the `on_version` method, as the version reply has to be sent explicitly now, but I think is less confusing and reflects better what is actually happening.

ACKs for top commit:
  epiccurious:
    utACK c340503
  stratospher:
    tested ACK c340503. very useful to have since we'd want real node behaviour!
  mzumsande:
    ACK c340503
  sr-gi:
    tACK bitcoin/bitcoin@c340503

Tree-SHA512: 63eac287d3e1c87a01852bfd9f0530363354bbb642280298673b9c8817056356373adf348955c4e92af95c7c6efa8cc515cee2892e9f077bfbe1bce8e97ad082

Author: glozow

test/functional/p2p_add_connections.py

@@ -17,7 +17,7 @@ def on_version(self, message):
         # message is received from the test framework. Don't send any responses
         # to the node's version message since the connection will already be
         # closed.
-        pass
+        self.send_version()
 
 class P2PAddConnections(BitcoinTestFramework):
     def set_test_params(self):

test/functional/p2p_addr_relay.py

@@ -75,6 +75,7 @@ def addr_received(self):
         return self.num_ipv4_received != 0
 
     def on_version(self, message):
+        self.send_version()
         self.send_message(msg_verack())
         if (self.send_getaddr):
             self.send_message(msg_getaddr())

test/functional/p2p_sendtxrcncl.py

@@ -29,6 +29,7 @@ def on_version(self, message):
         # Avoid sending verack in response to version.
         # When calling add_p2p_connection, wait_for_verack=False must be set (see
         # comment in add_p2p_connection).
+        self.send_version()
         if message.nVersion >= 70016 and self.wtxidrelay:
             self.send_message(msg_wtxidrelay())
 
@@ -43,7 +44,8 @@ def on_sendtxrcncl(self, message):
 
 class P2PFeelerReceiver(SendTxrcnclReceiver):
     def on_version(self, message):
-        pass  # feeler connections can not send any message other than their own version
+        # feeler connections can not send any message other than their own version
+        self.send_version()
 
 
 class PeerTrackMsgOrder(P2PInterface):

test/functional/test_framework/p2p.py

@@ -224,11 +224,10 @@ def connection_made(self, transport):
         if self.supports_v2_p2p and self.v2_state.initiating and not self.v2_state.tried_v2_handshake:
             send_handshake_bytes = self.v2_state.initiate_v2_handshake()
             self.send_raw_message(send_handshake_bytes)
-        # if v2 connection, send `on_connection_send_msg` after initial v2 handshake.
-        # if reconnection situation, send `on_connection_send_msg` after version message is received in `on_version()`.
-        if self.on_connection_send_msg and not self.supports_v2_p2p and not self.reconnect:
-            self.send_message(self.on_connection_send_msg)
-            self.on_connection_send_msg = None  # Never used again
+        # for v1 outbound connections, send version message immediately after opening
+        # (for v2 outbound connections, send it after the initial v2 handshake)
+        if self.p2p_connected_to_node and not self.supports_v2_p2p:
+            self.send_version()
         self.on_open()
 
     def connection_lost(self, exc):
@@ -284,9 +283,13 @@ def v2_handshake(self):
         if not is_mac_auth:
             raise ValueError("invalid v2 mac tag in handshake authentication")
         self.recvbuf = self.recvbuf[length:]
-        if self.v2_state.tried_v2_handshake and self.on_connection_send_msg:
-            self.send_message(self.on_connection_send_msg)
-            self.on_connection_send_msg = None
+        if self.v2_state.tried_v2_handshake:
+            # for v2 outbound connections, send version message immediately after v2 handshake
+            if self.p2p_connected_to_node:
+                self.send_version()
+            # process post-v2-handshake data immediately, if available
+            if len(self.recvbuf) > 0:
+                self._on_data()
 
     # Socket read methods
 
@@ -557,11 +560,10 @@ def on_verack(self, message):
 
     def on_version(self, message):
         assert message.nVersion >= MIN_P2P_VERSION_SUPPORTED, "Version {} received. Test framework only supports versions greater than {}".format(message.nVersion, MIN_P2P_VERSION_SUPPORTED)
-        # reconnection using v1 P2P has happened since version message can be processed, previously unsent version message is sent using v1 P2P here
-        if self.reconnect:
-            if self.on_connection_send_msg:
-                self.send_message(self.on_connection_send_msg)
-                self.on_connection_send_msg = None
+        # for inbound connections, reply to version with own version message
+        # (could be due to v1 reconnect after a failed v2 handshake)
+        if not self.p2p_connected_to_node:
+            self.send_version()
             self.reconnect = False
         if message.nVersion >= 70016 and self.wtxidrelay:
             self.send_message(msg_wtxidrelay())
@@ -676,6 +678,11 @@ def test_function():
 
     # Message sending helper functions
 
+    def send_version(self):
+        if self.on_connection_send_msg:
+            self.send_message(self.on_connection_send_msg)
+            self.on_connection_send_msg = None  # Never used again
+
     def send_and_ping(self, message, timeout=60):
         self.send_message(message)
         self.sync_with_ping(timeout=timeout)