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

Conflicts

No conflicts as of last run.

I've withdrawn my BIP proposal relating to this change, but I think it's worth considering making this change anyway.

My thought is that if other software on the network deploys changes similar to BIP 339 in the future (where a new message is included between version and verack) that we don't want to support in our project, then it's easier to ignore all such messages than it is to add support for specifically ignoring each such message as it is proposed.

If we were to overlook someone's optional p2p protocol change and then bumped our protocol version at a later date in support of some other change, we would risk needlessly disconnecting other peers.

Note that if a peer never sends a verack, we still disconnect them after 60 seconds, so I think the only downside from this change is that we'd tolerate 60 seconds worth of garbage from a peer before they disconnect. Given that a peer can waste our bandwidth anyway by sending the verack and spamming us with (eg) bogus transactions, this bandwidth issue seems like an orthogonal concern. If we want to address the bandwidth issue somehow, I think we could do so for both pre-verack and post-verack messages at a later time.

Concept ACK - I am almost up to date with the mailing list discussion.

I think there's a kind of elegance in doing the negotiation at that stage. It allows the peers to commit to certain features before the P2P connection goes into normal use. Current signaling messages can be sent at any point which can causes unnessecary complication because the switch can happen later, at any time.
(and this without extending the VERSION message, which is already a mess of legacy concerns and whose changing requires centralized coordination)

Concept ACK

Pinging reviewers -- the code change here is pretty simple, so if there are concept ACKs on this I hope we can get actual ACKs as well? I think it would make logical sense to deploy this in 0.21 (at the same time we are deploying wtxid-relay), though there is no code-dependency that requires it.

utACK 675e55e

I think it makes sense to ignore unknown messages before VERACK. It's the obvious place to negotiate optional features, and if there is no problem having them after VERACK, I don't see why there should be punishment for it before that point.

ACK 675e55e: patch looks correct

ACK 675e55e

It would be nice if the tests were updated at the same time:

• Fix a small incorrect comment
• Add a test that peers are actually disconnected due to timeout if they never finish the handshake

You may use this diff freely:

diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py
index 4b32d60db0..68e2bd4a8b 100755
--- a/test/functional/p2p_leak.py
+++ b/test/functional/p2p_leak.py
@@ -5,7 +5,7 @@
 """Test message sending before handshake completion.
 
 A node should never send anything other than VERSION/VERACK until it's
-received a VERACK.
+received a VERACK, except for feature negotiation of wtxidrelay.
 
 This test connects to a node and sends it a few messages, trying to entice it
 into sending us something it shouldn't."""
@@ -91,6 +91,7 @@ class P2PVersionStore(P2PInterface):
 class P2PLeakTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
+        self.extra_args = [['-peertimeout=4']]
 
     def run_test(self):
         # Peer that never sends a version. We will send a bunch of messages
@@ -125,6 +126,9 @@ class P2PLeakTest(BitcoinTestFramework):
 
         # Expect this peer to be disconnected for misbehavior
         assert not no_version_disconnect_peer.is_connected
+        # Expect peers to be disconnected due to timeout
+        assert not no_version_idle_peer.is_connected
+        assert not no_verack_idle_peer.is_connected
 
         self.nodes[0].disconnect_p2ps()
 

Note that the test I suggest passes on master, as the patch in this pull is not a behaviour change for peers that send less than 100 messages prior to finishing the handshake. So I am happy to create a separate pull, but I think the changes here are a nice excuse to shove in the test as well.

when receiving a message that must go after verack, the node should keep current (unchanged) behavior

Can you explain one benefit of keeping the behavior exactly the same for those messages? I can only see downsides.

when receiving a message that must go after verack, the node should keep current (unchanged) behavior

Can you explain one benefit of keeping the behavior exactly the same for those messages? I can only see downsides.

If a peer sends known messages in wrong order it obviously is broken or violates protocol intentionally. The downside I see is more complicated code. What others downsides?

Yes, if a peer sends us 99 non-verack messages before the version handshake is complete, it is obviously broken. Though, no version of Bitcoin Core with default settings would disconnect it for that. Nor would this pull request change that. I don't see the point of the additional code complexity to keep a theoretical behaviour that isn't observed in practise and confusing/arbitrary anyway.