net: remove cs_vRecvMsg

Fuzzbawls · Fuzzbawls · commit f30f10e69915 · 2020-09-02T00:42:01.000-07:00
vRecvMsg is now only touched by the socket handler thread.

The accounting vars (nRecvBytes/nLastRecv/mapRecvBytesPerMsgCmd) are
also only used by the socket handler thread, with the exception of
queries from rpc/gui. These accesses are not threadsafe, but they never
were. This needs to be addressed separately.

Also, update comment describing data flow
diff --git a/src/main.cpp b/src/main.cpp
@@ -6099,7 +6099,6 @@ int ActiveProtocol()
     return MIN_PEER_PROTO_VERSION_BEFORE_ENFORCEMENT;
 }
 
-// requires LOCK(cs_vRecvMsg)
 bool ProcessMessages(CNode* pfrom, CConnman& connman, std::atomic<bool>& interruptMsgProc)
 {
     // Message format
diff --git a/src/net.cpp b/src/net.cpp
@@ -693,7 +693,6 @@ void CNode::copyStats(CNodeStats& stats)
 }
 #undef X
 
-// requires LOCK(cs_vRecvMsg)
 bool CNode::ReceiveMsgBytes(const char* pch, unsigned int nBytes, bool& complete)
 {
     complete = false;
@@ -1086,12 +1085,9 @@ void CConnman::ThreadSocketHandler()
                     {
                         TRY_LOCK(pnode->cs_vSend, lockSend);
                         if (lockSend) {
-                            TRY_LOCK(pnode->cs_vRecvMsg, lockRecv);
-                            if (lockRecv) {
                                 TRY_LOCK(pnode->cs_inventory, lockInv);
                                 if (lockInv)
                                     fDelete = true;
-                            }
                         }
                     }
                     if (fDelete) {
@@ -1149,15 +1145,10 @@ void CConnman::ThreadSocketHandler()
                 //   write buffer in this case before receiving more. This avoids
                 //   needlessly queueing received data, if the remote peer is not themselves
                 //   receiving data. This means properly utilizing TCP flow control signalling.
-                // * Otherwise, if there is no (complete) message in the receive buffer,
-                //   or there is space left in the buffer, select() for receiving data.
-                // * (if neither of the above applies, there is certainly one message
-                //   in the receiver buffer ready to be processed).
-                // Together, that means that at least one of the following is always possible,
-                // so we don't deadlock:
-                // * We send some data.
-                // * We wait for data to be received (and disconnect after timeout).
-                // * We process a message in the buffer (message handler thread).
+                // * Otherwise, if there is space left in the receive buffer, select() for
+                //   receiving data.
+                // * Hand off all complete messages to the processor, to be handled without
+                //   blocking here.
                 {
                     LOCK(pnode->cs_vSend);
                     if (!pnode->vSendMsg.empty()) {
@@ -1166,8 +1157,7 @@ void CConnman::ThreadSocketHandler()
                     }
                 }
                 {
-                    TRY_LOCK(pnode->cs_vRecvMsg, lockRecv);
-                    if (lockRecv && !pnode->fPauseRecv)
+                    if (!pnode->fPauseRecv)
                         FD_SET(pnode->hSocket, &fdsetRecv);
                 }
             }
@@ -1220,8 +1210,7 @@ void CConnman::ThreadSocketHandler()
             if (pnode->hSocket == INVALID_SOCKET)
                 continue;
             if (FD_ISSET(pnode->hSocket, &fdsetRecv) || FD_ISSET(pnode->hSocket, &fdsetError)) {
-                TRY_LOCK(pnode->cs_vRecvMsg, lockRecv);
-                if (lockRecv) {
+                {
                     {
                         // typical socket buffer is 8K-64K
                         char pchBuf[0x10000];
@@ -1807,13 +1796,8 @@ void CConnman::ThreadMessageHandler()
                 continue;
 
             // Receive messages
-            {
-                TRY_LOCK(pnode->cs_vRecvMsg, lockRecv);
-                if (lockRecv) {
-                    bool fMoreNodeWork = GetNodeSignals().ProcessMessages(pnode, *this, flagInterruptMsgProc);
-                    fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
-                }
-            }
+            bool fMoreNodeWork = GetNodeSignals().ProcessMessages(pnode, *this, flagInterruptMsgProc);
+            fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
             if (flagInterruptMsgProc)
                 return;
 
diff --git a/src/net.h b/src/net.h
@@ -583,8 +583,6 @@ class CNode
     RecursiveMutex cs_sendProcessing;
 
     std::deque<CInv> vRecvGetData;
-    std::list<CNetMessage> vRecvMsg;
-    RecursiveMutex cs_vRecvMsg;
     uint64_t nRecvBytes;
     std::atomic<int> nRecvVersion;
 
@@ -674,6 +672,7 @@ class CNode
     const ServiceFlags nLocalServices;
     const int nMyStartingHeight;
     int nSendVersion;
+    std::list<CNetMessage> vRecvMsg;  // Used only by SocketHandler thread
 public:
     NodeId GetId() const
     {
@@ -694,7 +693,6 @@ class CNode
         return nRefCount;
     }
 
-    // requires LOCK(cs_vRecvMsg)
     unsigned int GetTotalRecvSize()
     {
         unsigned int total = 0;
@@ -703,7 +701,6 @@ class CNode
         return total;
     }
 
-    // requires LOCK(cs_vRecvMsg)
     bool ReceiveMsgBytes(const char* pch, unsigned int nBytes, bool& complete);
 
     void SetRecvVersion(int nVersionIn)

Original file line number	Diff line number	Diff line change
`@@ -6099,7 +6099,6 @@ int ActiveProtocol()`
`6099`	`6099`	`return MIN_PEER_PROTO_VERSION_BEFORE_ENFORCEMENT;`
`6100`	`6100`	`}`
`6101`	`6101`
`6102`		`-// requires LOCK(cs_vRecvMsg)`
`6103`	`6102`	`bool ProcessMessages(CNode* pfrom, CConnman& connman, std::atomic<bool>& interruptMsgProc)`
`6104`	`6103`	`{`
`6105`	`6104`	`// Message format`