Add a new peer state tracking class to reduce cs_main contention.

TheBlueMatt · TheBlueMatt · commit e89822964e9e · 2019-06-12T12:52:58.000-04:00
CNodeState was added for validation-state-tracking, and thus,
logically, was protected by cs_main. However, as it has grown to
include non-validation state (taking state from CNode), and as
we've reduced cs_main usage for other unrelated things, CNodeState
is left with lots of cs_main locking in net_processing.

In order to ease transition to something new, this adds only a
dummy CPeerState which is held as a reference for the duration of
message processing.

Note that moving things is somewhat tricky pre validation-thread as
a consistent lockorder must be kept - we can't take a lock on the
new cs_peerstate in anything that's called directly from
validation.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -199,6 +199,17 @@ struct CBlockReject {
     uint256 hashBlock;
 };
 
+/**
+ * Maintain state about nodes, protected by our own lock. Historically we put all
+ * peer tracking state in CNodeState, however this results in significant cs_main
+ * contention. Thus, new state tracking should go here, and we should eventually
+ * move most (non-validation-specific) state here.
+ */
+struct CPeerState {
+    CPeerState() {}
+};
+
+
 /**
  * Maintain validation-specific state about nodes, protected by cs_main, instead
  * by CNode's own locks. This simplifies asynchronous operation, where
@@ -393,7 +404,20 @@ struct CNodeState {
 // Keeps track of the time (in microseconds) when transactions were requested last time
 limitedmap<uint256, int64_t> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
 
+/** Note that this must be locked BEFORE cs_main! */
+CCriticalSection cs_peerstate;
+
 /** Map maintaining per-node state. */
+static std::map<NodeId, CPeerState> mapPeerState GUARDED_BY(cs_peerstate);
+
+static CPeerState *PeerState(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_peerstate) LOCKS_EXCLUDED(cs_main) {
+    std::map<NodeId, CPeerState>::iterator it = mapPeerState.find(pnode);
+    if (it == mapPeerState.end())
+        return nullptr;
+    return &it->second;
+}
+
+/** Map maintaining new per-node state. */
 static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);
 
 static CNodeState *State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
@@ -771,12 +795,22 @@ void PeerLogicValidation::InitializeNode(CNode *pnode) {
         LOCK(cs_main);
         mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->fInbound, pnode->m_manual_connection));
     }
+    {
+        LOCK(cs_peerstate);
+        mapPeerState.emplace_hint(mapPeerState.end(), nodeid, CPeerState{});
+    }
+
     if(!pnode->fInbound)
         PushNodeVersion(pnode, connman, GetTime());
 }
 
 void PeerLogicValidation::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) {
     fUpdateConnectionTime = false;
+
+    LOCK(cs_peerstate);
+    CPeerState* peerstate = PeerState(nodeid);
+    assert(peerstate != nullptr);
+
     LOCK(cs_main);
     CNodeState *state = State(nodeid);
     assert(state != nullptr);
@@ -799,13 +833,15 @@ void PeerLogicValidation::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTim
     assert(g_outbound_peers_with_protect_from_disconnect >= 0);
 
     mapNodeState.erase(nodeid);
+    mapPeerState.erase(nodeid);
 
     if (mapNodeState.empty()) {
         // Do a consistency check after the last peer is removed.
         assert(mapBlocksInFlight.empty());
         assert(nPreferredDownload == 0);
         assert(nPeersWithValidatedDownloads == 0);
         assert(g_outbound_peers_with_protect_from_disconnect == 0);
+        assert(mapPeerState.empty());
     }
     LogPrint(BCLog::NET, "Cleared nodestate for peer=%d\n", nodeid);
 }
@@ -1843,7 +1879,7 @@ void static ProcessOrphanTx(CConnman* connman, std::set<uint256>& orphan_work_se
     }
 }
 
-bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman* connman, const std::atomic<bool>& interruptMsgProc, bool enable_bip61)
+bool static ProcessMessage(CNode* pfrom, CPeerState* peerstate, const std::string& strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman* connman, const std::atomic<bool>& interruptMsgProc, bool enable_bip61) EXCLUSIVE_LOCKS_REQUIRED(cs_peerstate)
 {
     LogPrint(BCLog::NET, "received: %s (%u bytes) peer=%d\n", SanitizeString(strCommand), vRecv.size(), pfrom->GetId());
     if (gArgs.IsArgSet("-dropmessagestest") && GetRand(gArgs.GetArg("-dropmessagestest", 0)) == 0)
@@ -2785,7 +2821,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         } // cs_main
 
         if (fProcessBLOCKTXN)
-            return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, nTimeReceived, chainparams, connman, interruptMsgProc, enable_bip61);
+            return ProcessMessage(pfrom, peerstate, NetMsgType::BLOCKTXN, blockTxnMsg, nTimeReceived, chainparams, connman, interruptMsgProc, enable_bip61);
 
         if (fRevertToHeaderProcessing) {
             // Headers received from HB compact block peers are permitted to be
@@ -3231,6 +3267,8 @@ bool PeerLogicValidation::SendRejectsAndCheckIfBanned(CNode* pnode, bool enable_
 bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgProc)
 {
     const CChainParams& chainparams = Params();
+    LOCK(cs_peerstate);
+    CPeerState* peerstate = PeerState(pfrom->GetId());
     //
     // Message format
     //  (4) message start
@@ -3313,7 +3351,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
     bool fRet = false;
     try
     {
-        fRet = ProcessMessage(pfrom, strCommand, vRecv, msg.nTime, chainparams, connman, interruptMsgProc, m_enable_bip61);
+        fRet = ProcessMessage(pfrom, peerstate, strCommand, vRecv, msg.nTime, chainparams, connman, interruptMsgProc, m_enable_bip61);
         if (interruptMsgProc)
             return false;
         if (!pfrom->vRecvGetData.empty())
@@ -3522,6 +3560,9 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         // If we get here, the outgoing message serialization version is set and can't change.
         const CNetMsgMaker msgMaker(pto->GetSendVersion());
 
+        LOCK(cs_peerstate);
+        CPeerState* peerstate = PeerState(pto->GetId());
+
         //
         // Message: ping
         //
