Merge #20079: p2p: Treat handshake misbehavior like unknown message

faaad1bbac46cfeb22654b4c59f0aac7a680c03a p2p: Ignore version msgs after initial version msg (MarcoFalke)
fad68afcff731153d1c83f7f56c91ecbb264b59a p2p: Ignore non-version msgs before version msg (MarcoFalke)

Pull request description:

  Handshake misbehaviour doesn't cost us more than any other unknown message, so it seems odd to treat it differently

ACKs for top commit:
  jnewbery:
    utACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a
  practicalswift:
    ACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a: patch looks correct

Tree-SHA512: 9f30c3b5c1f6604fd02cff878f10999956152419a3dd9825f8267cbdeff7d06787418b41c7fde8a00a5e557fe89204546e05d5689042dbf7b07fbb7eb95cddff

src/net_processing.cpp

@@ -3274,10 +3274,8 @@ void PeerManagerImpl::ProcessMessage(
     if (peer == nullptr) return;
 
     if (msg_type == NetMsgType::VERSION) {
-        // Each connection can only send one version message
+        if (pfrom.nVersion != 0) {
-        if (pfrom.nVersion != 0)
+            LogPrint(BCLog::NET, "redundant version message from peer=%d\n", pfrom.GetId());
-        {
-            Misbehaving(pfrom.GetId(), 1, "redundant version message");
             return;
         }
 
@@ -3507,7 +3505,7 @@ void PeerManagerImpl::ProcessMessage(
 
     if (pfrom.nVersion == 0) {
         // Must have a version message before anything else
-        Misbehaving(pfrom.GetId(), 1, "non-version message before version handshake");
+        LogPrint(BCLog::NET, "non-version message before version handshake. Message \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
         return;
     }
 

test/functional/p2p_invalid_messages.py

@@ -17,6 +17,7 @@ from test_framework.messages import (
     msg_headers,
     msg_inv,
     MSG_TX,
+    msg_version,
 )
 from test_framework.p2p import (
     P2PDataStore, P2PInterface
@@ -50,6 +51,7 @@ class InvalidMessagesTest(BitcoinTestFramework):
 
     def run_test(self):
         self.test_buffer()
+        self.test_duplicate_version_msg()
         self.test_magic_bytes()
         self.test_checksum()
         self.test_size()
@@ -78,6 +80,13 @@ class InvalidMessagesTest(BitcoinTestFramework):
         conn.sync_with_ping(timeout=1)
         self.nodes[0].disconnect_p2ps()
 
+    def test_duplicate_version_msg(self):
+        self.log.info("Test duplicate version message is ignored")
+        conn = self.nodes[0].add_p2p_connection(P2PDataStore())
+        with self.nodes[0].assert_debug_log(['redundant version message from peer']):
+            conn.send_and_ping(msg_version())
+        self.nodes[0].disconnect_p2ps()
+
     def test_magic_bytes(self):
         self.log.info("Test message with invalid magic bytes disconnects peer")
         conn = self.nodes[0].add_p2p_connection(P2PDataStore())

test/functional/p2p_leak.py

@@ -25,8 +25,6 @@ from test_framework.util import (
     assert_greater_than_or_equal,
 )
 
-DISCOURAGEMENT_THRESHOLD = 100
-
 
 class LazyPeer(P2PInterface):
     def __init__(self):
@@ -97,27 +95,16 @@ class P2PLeakTest(BitcoinTestFramework):
         self.setup_nodes()
 
     def run_test(self):
-        # Peer that never sends a version. We will send a bunch of messages
-        # from this peer anyway and verify eventual disconnection.
-        no_version_disconnect_peer = self.nodes[0].add_p2p_connection(
-            LazyPeer(), send_version=False, wait_for_verack=False)
-
         # Another peer that never sends a version, nor any other messages. It shouldn't receive anything from the node.
         no_version_idle_peer = self.nodes[0].add_p2p_connection(LazyPeer(), send_version=False, wait_for_verack=False)
 
         # Peer that sends a version but not a verack.
         no_verack_idle_peer = self.nodes[0].add_p2p_connection(NoVerackIdlePeer(), wait_for_verack=False)
 
-        # Send enough ping messages (any non-version message will do) prior to sending
-        # version to reach the peer discouragement threshold. This should get us disconnected.
-        for _ in range(DISCOURAGEMENT_THRESHOLD):
-            no_version_disconnect_peer.send_message(msg_ping())
-
         # Wait until we got the verack in response to the version. Though, don't wait for the node to receive the
         # verack, since we never sent one
         no_verack_idle_peer.wait_for_verack()
 
-        no_version_disconnect_peer.wait_until(lambda: no_version_disconnect_peer.ever_connected, check_connected=False)
         no_version_idle_peer.wait_until(lambda: no_version_idle_peer.ever_connected)
         no_verack_idle_peer.wait_until(lambda: no_verack_idle_peer.version_received)
 
@@ -127,13 +114,9 @@ class P2PLeakTest(BitcoinTestFramework):
         #Give the node enough time to possibly leak out a message
         time.sleep(5)
 
-        #This peer should have been banned
-        assert not no_version_disconnect_peer.is_connected
-
         self.nodes[0].disconnect_p2ps()
 
         # Make sure no unexpected messages came in
-        assert no_version_disconnect_peer.unexpected_msg == False
         assert no_version_idle_peer.unexpected_msg == False
         assert no_verack_idle_peer.unexpected_msg == False
 
@@ -152,7 +135,7 @@ class P2PLeakTest(BitcoinTestFramework):
         p2p_old_peer = self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False)
         old_version_msg = msg_version()
         old_version_msg.nVersion = 31799
-        with self.nodes[0].assert_debug_log(['peer=4 using obsolete version 31799; disconnecting']):
+        with self.nodes[0].assert_debug_log(['peer=3 using obsolete version 31799; disconnecting']):
             p2p_old_peer.send_message(old_version_msg)
             p2p_old_peer.wait_for_disconnect()
 

test/functional/p2p_timeouts.py

@@ -66,7 +66,9 @@ class TimeoutsTest(BitcoinTestFramework):
         assert no_version_node.is_connected
         assert no_send_node.is_connected
 
+        with self.nodes[0].assert_debug_log(['Unsupported message "ping" prior to verack from peer=0']):
             no_verack_node.send_message(msg_ping())
+        with self.nodes[0].assert_debug_log(['non-version message before version handshake. Message "ping" from peer=1']):
             no_version_node.send_message(msg_ping())
 
         self.mock_forward(1)