Merge bitcoin#20079: p2p: Treat handshake misbehavior like unknown me…

…ssage

faaad1b p2p: Ignore version msgs after initial version msg (MarcoFalke)
fad68af 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 faaad1b
  practicalswift:
    ACK faaad1b: patch looks correct

Tree-SHA512: 9f30c3b5c1f6604fd02cff878f10999956152419a3dd9825f8267cbdeff7d06787418b41c7fde8a00a5e557fe89204546e05d5689042dbf7b07fbb7eb95cddff

src/net_processing.cpp

@@ -2871,10 +2871,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)
-        {
-            Misbehaving(pfrom.GetId(), 1, "redundant version message");
+        if (pfrom.nVersion != 0) {
+            LogPrint(BCLog::NET, "redundant version message from peer=%d\n", pfrom.GetId());
             return;
         }
 
@@ -3085,7 +3083,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 @@
     msg_headers,
     msg_inv,
     MSG_TX,
+    msg_version,
 )
 from test_framework.p2p import (
     P2PDataStore, P2PInterface
@@ -50,6 +51,7 @@ def set_test_params(self):
 
     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 @@ def test_buffer(self):
         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

@@ -156,7 +156,7 @@ def run_test(self):
         old_version_msg = msg_version()
         old_version_msg.nVersion = 31799
         self.wait_until(lambda: p2p_old_node.is_connected)
-        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_node.send_message(old_version_msg)
             p2p_old_node.wait_for_disconnect()
 

test/functional/p2p_timeouts.py

@@ -57,8 +57,10 @@ def run_test(self):
         assert no_version_node.is_connected
         assert no_send_node.is_connected
 
-        no_verack_node.send_message(msg_ping())
-        no_version_node.send_message(msg_ping())
+        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())
 
         sleep(1)