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

4 files changed, 17 insertions, 25 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 200c286f5d..dacedef472 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2255,10 +2255,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
     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;
         }
 
@@ -2452,7 +2450,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 
     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;
     }
 
diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py
index db72a361d9..c0b3c2cb12 100755
--- a/test/functional/p2p_invalid_messages.py
+++ b/test/functional/p2p_invalid_messages.py
@@ -18,6 +18,7 @@ from test_framework.messages import (
     msg_inv,
     msg_ping,
     MSG_TX,
+    msg_version,
     ser_string,
 )
 from test_framework.p2p import (
@@ -60,6 +61,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()
@@ -92,6 +94,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())
diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py
index 4b32d60db0..ca8bf908a9 100755
--- a/test/functional/p2p_leak.py
+++ b/test/functional/p2p_leak.py
@@ -24,8 +24,6 @@ from test_framework.util import (
     assert_greater_than_or_equal,
 )
 
-DISCOURAGEMENT_THRESHOLD = 100
-
 
 class LazyPeer(P2PInterface):
     def __init__(self):
@@ -93,27 +91,16 @@ class P2PLeakTest(BitcoinTestFramework):
         self.num_nodes = 1
 
     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)
 
@@ -123,13 +110,9 @@ class P2PLeakTest(BitcoinTestFramework):
         #Give the node enough time to possibly leak out a message
         time.sleep(5)
 
-        # Expect this peer to be disconnected for misbehavior
-        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
 
@@ -148,7 +131,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()
 
diff --git a/test/functional/p2p_timeouts.py b/test/functional/p2p_timeouts.py
index ce12ce26ce..47832b04bf 100755
--- a/test/functional/p2p_timeouts.py
+++ b/test/functional/p2p_timeouts.py
@@ -57,8 +57,10 @@ class TimeoutsTest(BitcoinTestFramework):
         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)