Ignore unknown messages before VERACK

sdaftuar commented at 7:55 pm on August 14, 2020: member

This allows for feature negotiation to take place with messages between VERSION and VERACK in the future, without requiring additional software changes to specifically ignore messages for features that are unimplemented by our software.

DrahtBot added the label P2P on Aug 14, 2020

DrahtBot commented at 8:26 pm on August 20, 2020: member

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

Conflicts

No conflicts as of last run.

sdaftuar marked this as ready for review on Aug 24, 2020

sdaftuar commented at 1:49 pm on August 24, 2020: member

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.

fanquake commented at 7:46 am on August 25, 2020: member

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

laanwj commented at 7:10 am on August 27, 2020: member

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)

in src/net_processing.cpp:2556 in c4fb703c71 outdated

2552@@ -2553,9 +2553,7 @@ void ProcessMessage(
2553     }
2554 
2555     if (!pfrom.fSuccessfullyConnected) {
2556-        // Must have a verack message before anything else
2557-        LOCK(cs_main);
2558-        Misbehaving(pfrom.GetId(), 1, "non-verack message before version handshake");
2559+        LogPrint(BCLog::NET, "Unknown command \"%s\" prior to verack from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());

jnewbery commented at 11:52 am on August 28, 2020:

Couple of suggestions:

we generally refer to “message types” rather than “commands” these days.
the message type may be known, just not supported prior to a verack (eg an “addr” message)

Suggested wording: “Unsupported message "%s" prior to … "

sdaftuar commented at 9:29 pm on August 28, 2020:

Shed painted.

jnewbery commented at 11:54 am on August 28, 2020: member

Concept ACK. We already tolerate up to 99 unsupported messages prior to a verack, so in almost all cases this is not a behavior change. Explicitly dropping the message and continuing rather than incrementing the misbehaving score seems like an improvement.

DrahtBot added the label Needs rebase on Aug 28, 2020

Ignore unknown messages before VERACK 675e55e013

sdaftuar force-pushed on Aug 28, 2020

DrahtBot removed the label Needs rebase on Aug 28, 2020

practicalswift commented at 1:03 am on August 29, 2020: contributor

Concept ACK

sdaftuar commented at 11:23 pm on September 25, 2020: member

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.

sipa commented at 1:20 am on September 26, 2020: member

utACK 675e55e01392971aa56bda56cb09498b466d0902

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.

practicalswift commented at 11:52 am on September 26, 2020: contributor

ACK 675e55e01392971aa56bda56cb09498b466d0902: patch looks correct

MarcoFalke commented at 7:49 pm on September 26, 2020: member

ACK 675e55e01392971aa56bda56cb09498b466d0902

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:

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

MarcoFalke commented at 7:54 pm on September 26, 2020: member

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.

hebasto approved

hebasto commented at 5:28 pm on October 3, 2020: member

Approach ACK 675e55e01392971aa56bda56cb09498b466d0902. I agree that this change will make easier the introducing of new protocol features without version coordination burden, and without risk of network partition.

The only suggestion is to ignore unknown messages, i.e., when receiving a message that must go after verack, the node should keep current (unchanged) behavior.

MarcoFalke commented at 7:26 am on October 4, 2020: member

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.

hebasto commented at 7:31 am on October 4, 2020: member

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?

MarcoFalke commented at 7:49 am on October 4, 2020: member

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.

hebasto approved

hebasto commented at 10:27 am on October 4, 2020: member

ACK 675e55e01392971aa56bda56cb09498b466d0902, the offender peer will be eventually disconnected due to the timeout.

MarcoFalke merged this on Oct 4, 2020

MarcoFalke closed this on Oct 4, 2020

sidhujag referenced this in commit 6b99c7e784 on Oct 4, 2020

MarcoFalke referenced this in commit f7653eb5ae on Mar 5, 2021

sidhujag referenced this in commit 40a78b38ba on Mar 5, 2021

DrahtBot locked this on Feb 15, 2022

Ignore unknown messages before VERACK #19723

Conflicts