net: fix a few races. Credit @TheBlueMatt

These are (afaik) all long-standing races or concurrent accesses. Going
forward, we can clean these up so that they're not all individual atomic
accesses.

- Reintroduce cs_vRecv to guard receive-specific vars
- Lock vRecv/vSend for CNodeStats
- Make some vars atomic.
- Only set the connection time in CNode's constructor so that it doesn't change

zcash: cherry picked from commit 321d0fc6b6624c65508f8b9059418cb936f0bbbe
zcash: https://github.com/bitcoin/bitcoin/pull/9708

src/main.cpp

@@ -6482,7 +6482,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string
                     if (pingUsecTime > 0) {
                         // Successful ping time measurement, replace previous
                         pfrom->nPingUsecTime = pingUsecTime;
-                        pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime, pingUsecTime);
+                        pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime.load(), pingUsecTime);
                     } else {
                         // This should never happen
                         sProblem = "Timing mishap";

src/net.cpp

@@ -653,8 +653,14 @@ void CNode::copyStats(CNodeStats &stats)
     stats.cleanSubVer = cleanSubVer;
     stats.fInbound = fInbound;
     stats.nStartingHeight = nStartingHeight;
-    stats.nSendBytes = nSendBytes;
-    stats.nRecvBytes = nRecvBytes;
+    {
+        LOCK(cs_vSend);
+        stats.nSendBytes = nSendBytes;
+    }
+    {
+        LOCK(cs_vRecv);
+        stats.nRecvBytes = nRecvBytes;
+    }
     stats.fWhitelisted = fWhitelisted;
 
     // It is common for nodes with good ping times to suddenly become lagged,
@@ -792,7 +798,10 @@ void SocketSendData(CNode *pnode)
         }
         if (nBytes > 0) {
             pnode->nLastSend = GetTime();
-            pnode->nSendBytes += nBytes;
+            {
+                LOCK(pnode->cs_vSend);
+                pnode->nSendBytes += nBytes;
+            }
             pnode->nSendOffset += nBytes;
             pnode->RecordBytesSent(nBytes);
             if (pnode->nSendOffset == data.size()) {
@@ -1278,7 +1287,10 @@ void ThreadSocketHandler()
                             if (!pnode->ReceiveMsgBytes(pchBuf, nBytes))
                                 pnode->CloseSocketDisconnect();
                             pnode->nLastRecv = GetTime();
-                            pnode->nRecvBytes += nBytes;
+                            {
+                                LOCK(pnode->cs_vRecv);
+                                pnode->nRecvBytes += nBytes;
+                            }
                             pnode->RecordBytesRecv(nBytes);
                         }
                         else if (nBytes == 0)

src/net.h

@@ -266,6 +266,7 @@ public:
     std::deque<CSerializeData> vSendMsg;
     CCriticalSection cs_vSend;
     CCriticalSection cs_hSocket;
+    CCriticalSection cs_vRecv;
 
     std::deque<CInv> vRecvGetData;
     std::deque<CNetMessage> vRecvMsg;
@@ -273,10 +274,10 @@ public:
     uint64_t nRecvBytes;
     int nRecvVersion;
 
-    int64_t nLastSend;
-    int64_t nLastRecv;
+    std::atomic<int64_t> nLastSend;
+    std::atomic<int64_t> nLastRecv;
     int64_t nTimeConnected;
-    int64_t nTimeOffset;
+    std::atomic<int64_t> nTimeOffset;
     const CAddress addr;
     std::string addrName;
     CService addrLocal;
@@ -302,8 +303,8 @@ public:
     CSemaphoreGrant grantOutbound;
     CCriticalSection cs_filter;
     CBloomFilter* pfilter;
-    int nRefCount;
     NodeId id;
+    std::atomic<int> nRefCount;
 
     const uint64_t nKeyedNetGroup;
 
@@ -346,15 +347,15 @@ public:
 
     // Ping time measurement:
     // The pong reply we're expecting, or 0 if no pong expected.
-    uint64_t nPingNonceSent;
+    std::atomic<uint64_t> nPingNonceSent;
     // Time (in usec) the last ping was sent, or 0 if no ping was ever sent.
-    int64_t nPingUsecStart;
+    std::atomic<int64_t> nPingUsecStart;
     // Last measured round-trip time.
-    int64_t nPingUsecTime;
+    std::atomic<int64_t> nPingUsecTime;
     // Best measured round-trip time.
-    int64_t nMinPingUsecTime;
+    std::atomic<int64_t> nMinPingUsecTime;
     // Whether a ping is requested.
-    bool fPingQueued;
+    std::atomic<bool> fPingQueued;
 
     CNode(SOCKET hSocketIn, const CAddress &addrIn, const std::string &addrNameIn = "", bool fInboundIn = false);
     ~CNode();