Merge bitcoin#20816: net: Move RecordBytesSent() call out of cs_vSend…

… lock

378aedc [net] Add cs_vSend lock annotations (John Newbery)
6732545 [net] Move RecordBytesSent() call out of cs_vSend lock (John Newbery)

Pull request description:

  RecordBytesSent() does not require cs_vSend to be locked, so reduce the scope of cs_vSend.

  Also correctly annotate the CNode data members that are guarded by cs_vSend.

  This is a simpler alternative to bitcoin#19673.

ACKs for top commit:
  jnewbery:
    ok, reverting to commit 378aedc which has two ACKs already. Any style issues can be fixed up in future PRs.
  troygiorshev:
    ACK 378aedc
  theStack:
    re-ACK 378aedc
  MarcoFalke:
    review ACK 378aedc 🔌

Tree-SHA512: e9cd6c472b7e1479120c1bf2d1c640cf6d18c7d589a5f9b7dfc4875e5790adaab403a7a1b945a47e79e7249a614b8583270e4549f89b22e8a9edb2e4818b0d07

src/net.cpp

@@ -1399,11 +1399,16 @@ void CConnman::CalculateNumConnectionsChangedStats()
     mapSentBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0;
     auto vNodesCopy = CopyNodeVector(CConnman::FullyConnectedOnly);
     for (auto pnode : vNodesCopy) {
-        LOCK(pnode->cs_vRecv);
-        for (const mapMsgCmdSize::value_type &i : pnode->mapRecvBytesPerMsgCmd)
-            mapRecvBytesMsgStats[i.first] += i.second;
-        for (const mapMsgCmdSize::value_type &i : pnode->mapSendBytesPerMsgCmd)
-            mapSentBytesMsgStats[i.first] += i.second;
+        {
+            LOCK(pnode->cs_vRecv);
+            for (const mapMsgCmdSize::value_type &i : pnode->mapRecvBytesPerMsgCmd)
+                mapRecvBytesMsgStats[i.first] += i.second;
+        }
+        {
+            LOCK(pnode->cs_vSend);
+            for (const mapMsgCmdSize::value_type &i : pnode->mapSendBytesPerMsgCmd)
+                mapSentBytesMsgStats[i.first] += i.second;
+        }
         if(pnode->fClient)
             spvNodes++;
         else
@@ -1877,11 +1882,9 @@ void CConnman::SocketHandler()
             break;
         }
 
-        LOCK(pnode->cs_vSend);
-        size_t nBytes = SocketSendData(pnode);
-        if (nBytes) {
-            RecordBytesSent(nBytes);
-        }
+        // Send data
+        size_t bytes_sent = WITH_LOCK(pnode->cs_vSend, return SocketSendData(pnode));
+        if (bytes_sent) RecordBytesSent(bytes_sent);
     }
 
     ReleaseNodeVector(vErrorNodes);

src/net.h

@@ -1026,8 +1026,10 @@ class CNode
     NetPermissionFlags m_permissionFlags{ PF_NONE };
     std::atomic<ServiceFlags> nServices{NODE_NONE};
     SOCKET hSocket GUARDED_BY(cs_hSocket);
-    size_t nSendSize{0}; // total size of all vSendMsg entries
-    size_t nSendOffset{0}; // offset inside the first vSendMsg already sent
+    /** Total size of all vSendMsg entries */
+    size_t nSendSize GUARDED_BY(cs_vSend){0};
+    /** Offset inside the first vSendMsg already sent */
+    size_t nSendOffset GUARDED_BY(cs_vSend){0};
     uint64_t nSendBytes GUARDED_BY(cs_vSend){0};
     std::list<std::vector<unsigned char>> vSendMsg GUARDED_BY(cs_vSend);
     std::atomic<size_t> nSendMsgSize{0};
@@ -1119,7 +1121,7 @@ class CNode
     Network ConnectedThroughNetwork() const;
 
 protected:
-    mapMsgCmdSize mapSendBytesPerMsgCmd;
+    mapMsgCmdSize mapSendBytesPerMsgCmd GUARDED_BY(cs_vSend);
     mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv);
 
 public: