test: p2p: adhere to typical VERSION message protocol flow

theStack commented at 0:05 am on January 31, 2024: contributor

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.

DrahtBot commented at 0:05 am on January 31, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, epiccurious, stratospher, sr-gi

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Jan 31, 2024

theStack force-pushed on Jan 31, 2024

theStack commented at 2:40 pm on January 31, 2024: contributor

Squashed the last two commits, as the second to last commit failed without the last bugfix commit (“test: p2p: process post-v2-handshake data immediately”); this should hopefully make the “test each commit” CI target happy.

mzumsande commented at 4:12 pm on January 31, 2024: contributor

Concept ACK, I think the test framework should replicate the bitcoind logic (that, as a fun fact, has been in place since the patch #101 by Hal Finney)

test: p2p: introduce helper for sending prepared VERSION message

This deduplicates code for sending out the VERSION message
(if available and not sent yet), currently used at three
different places:

1) in the `connection_made` asyncio callback
   (for v1 connections that are not v2 reconnects)
2) at the end of `v2_handshake`, if the v2 handshake succeeded
3) in the `on_version` callback, if a reconnection with v1 happens

b198b9c2ce

test: p2p: process post-v2-handshake data immediately

In the course of executing the asyncio data reception callback during a
v2 handshake, it's possible that the receive buffer already contains
data for after the handshake (usually a VERSION message for inbound
connections).
If we don't process that data immediately, we would do so after the next
message is received, but with the adapted protocol flow introduced in
the next commit, there is no next message, as the TestNode wouldn't
continue until we send back our own version in `on_version`. Fix this by
calling `self._on_data` immediately if there's data left in the receive
buffer after a completed v2 handshake.

7ddfc28309

test: p2p: adhere to typical VERSION message protocol flow

The test framework's p2p implementation currently sends out it's VERSION
message 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, and the responders 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 messages as response for incoming VERSION
messages (i.e. in the function `on_version`) for inbound connections.

Note that some of the overruled `on_version` methods in functional tests
needed to be changed to send the version explicitly.

c340503b67

in test/functional/p2p_add_connections.py:20 in 06deeb75bb outdated

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

sr-gi commented at 7:13 pm on January 31, 2024:

This is really just calling super().send_version. We could get rid of the whole class given no other method is being overwritten (or leave the alias if we want it for readability, but get rid of the method)

in test/functional/p2p_sendtxrcncl.py:48 in 06deeb75bb outdated

43@@ -43,7 +44,8 @@ def on_sendtxrcncl(self, message):
44 
45 class P2PFeelerReceiver(SendTxrcnclReceiver):
46     def on_version(self, message):
47-        pass  # feeler connections can not send any message other than their own version
48+        # feeler connections can not send any message other than their own version
49+        self.send_version()

sr-gi commented at 7:14 pm on January 31, 2024:

Same as for p2p_add_connections::P2PFeelerReceiver

theStack force-pushed on Feb 1, 2024

theStack commented at 12:57 pm on February 1, 2024: contributor

Brought back the commit “test: p2p: process post-v2-handshake data immediately” again and put it in before the one changing the version message flow. This leads to a cleaner history compared to the squashed commit (containing two long descriptions), while still passing the checks at each commit. Kudos to @stratospher for the idea.

mzumsande commented at 11:53 pm on February 1, 2024: contributor

ACK c340503b67585b631909584f1acaa327f5af25d3

I ran the functional test suite both with and without --v2transport on this branch and both runs succeeded.

epiccurious commented at 1:02 am on February 2, 2024: none

utACK c340503b67585b631909584f1acaa327f5af25d3

stratospher commented at 6:19 am on February 2, 2024: contributor

tested ACK c340503b67585b631909584f1acaa327f5af25d3. very useful to have since we’d want real node behaviour!

there is alsop2p_leak.py, p2p_timeouts.py, p2p_tx_privacy.py where on_version() is used with add_p2p_connection()/inbound to TestNode connections which aren’t affected by this. I guess we should think about it only if we end up doing outbound to TestNode connections there.

sr-gi commented at 8:28 pm on February 5, 2024: member

tACK https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3

As a side note, I found the newly added comments regarding inbounds and outbounds to be a bit confusing. This is due to them being written from the POV of the Python node instead of the TestNode’s, which is the logic used by add_p2p_connection and add_outbound_p2p_connection

glozow merged this on Feb 6, 2024

glozow closed this on Feb 6, 2024

theStack deleted the branch on Feb 6, 2024

theStack commented at 6:26 pm on February 6, 2024: contributor

As a side note, I found the newly added comments regarding inbounds and outbounds to be a bit confusing. This is due to them being written from the POV of the Python node instead of the TestNode’s, which is the logic used by add_p2p_connection and add_outbound_p2p_connection

I agree that the inbound/outbound naming is currently not ideal w.r.t. mixed node perspectives and should be improved. Will look into a potential follow-up PR addressing this (https://github.com/bitcoin/bitcoin/pull/29353#discussion_r1473340868 and #29353 (review) can also be tackled in that one then). Another idea I had recently was to improve the naming of the p2p_connected_to_node boolean, as it can easily interpreted as “are we connected to the node?” rather than “in which direction was the connection initiated?”.

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

Code Coverage

Reviews