Fix node protection logic false positives

We could be reading multiple messages from a socket buffer at once _without actually processing them yet_ which means that `fSuccessfullyConnected` might not be switched to `true` at the time we already parsed `VERACK` message and started to parse the next one. This is basically a false positive and we drop a legit node as a result even though the order of messages sent by this node was completely fine. To fix this I partially reverted #2790 (where the issue was initially introduced) and moved the logic for tracking the first message into ProcessMessage instead.

Author: UdjinM6

src/net.cpp

@@ -768,24 +768,6 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete
         int handled;
         if (!msg.in_data) {
             handled = msg.readHeader(pch, nBytes);
-            if (msg.in_data && nTimeFirstMessageReceived == 0) {
-                if (fSuccessfullyConnected) {
-                    // First message after VERSION/VERACK.
-                    // We record the time when the header is fully received and not when the full message is received.
-                    // otherwise a peer might send us a very large message as first message after VERSION/VERACK and fill
-                    // up our memory with multiple parallel connections doing this.
-                    nTimeFirstMessageReceived = nTimeMicros;
-                    fFirstMessageIsMNAUTH = msg.hdr.GetCommand() == NetMsgType::MNAUTH;
-                } else {
-                    // We're still in the VERSION/VERACK handshake process, so any message received in this state is
-                    // expected to be very small. This protects against attackers filling up memory by sending oversized
-                    // VERSION messages while the incoming connection is still protected against eviction
-                    if (msg.hdr.nMessageSize > 1024) {
-                        LogPrint(BCLog::NET, "Oversized VERSION/VERACK message from peer=%i, disconnecting\n", GetId());
-                        return false;
-                    }
-                }
-            }
         } else {
             handled = msg.readData(pch, nBytes);
         }
@@ -1032,7 +1014,7 @@ bool CConnman::AttemptToEvictConnection()
             if (fMasternodeMode) {
                 // This handles eviction protected nodes. Nodes are always protected for a short time after the connection
                 // was accepted. This short time is meant for the VERSION/VERACK exchange and the possible MNAUTH that might
-                // follow when the incoming connection is from another masternode. When another message then MNAUTH
+                // follow when the incoming connection is from another masternode. When a message other than MNAUTH
                 // is received after VERSION/VERACK, the protection is lifted immediately.
                 bool isProtected = GetSystemTimeInSeconds() - node->nTimeConnected < INBOUND_EVICTION_PROTECTION_TIME;
                 if (node->nTimeFirstMessageReceived != 0 && !node->fFirstMessageIsMNAUTH) {

src/net_processing.cpp

@@ -1788,10 +1788,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
             }
             LogPrint(BCLog::NET, "Reject %s\n", SanitizeString(ss.str()));
         }
+        return true;
     }
 
-    else if (strCommand == NetMsgType::VERSION)
-    {
+    if (strCommand == NetMsgType::VERSION) {
         // Each connection can only send one version message
         if (pfrom->nVersion != 0)
         {
@@ -1961,9 +1961,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         return true;
     }
 
-
-    else if (pfrom->nVersion == 0)
-    {
+    if (pfrom->nVersion == 0) {
         // Must have a version message before anything else
         LOCK(cs_main);
         Misbehaving(pfrom->GetId(), 1);
@@ -2028,18 +2026,24 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         }
 
         pfrom->fSuccessfullyConnected = true;
+        return true;
     }
 
-    else if (!pfrom->fSuccessfullyConnected)
-    {
+    if (!pfrom->fSuccessfullyConnected) {
         // Must have a verack message before anything else
         LOCK(cs_main);
         Misbehaving(pfrom->GetId(), 1);
         return false;
     }
 
-    else if (strCommand == NetMsgType::ADDR)
-    {
+    if (pfrom->nTimeFirstMessageReceived == 0) {
+        // First message after VERSION/VERACK
+        pfrom->nTimeFirstMessageReceived = GetTimeMicros();
+        pfrom->fFirstMessageIsMNAUTH = strCommand == NetMsgType::MNAUTH;
+        // Note: do not break the flow here
+    }
+
+    if (strCommand == NetMsgType::ADDR) {
         std::vector<CAddress> vAddr;
         vRecv >> vAddr;