merge bitcoin#19607: Add Peer struct for per-peer data in net processing

src/coinjoin/client.cpp

@@ -54,7 +54,6 @@ void CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataStream&
     vRecv >> dsq;
 
     if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 100);
         return;
     }
@@ -64,7 +63,6 @@ void CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataStream&
         if (auto dmn = mnList.GetValidMN(dsq.m_protxHash)) {
             dsq.masternodeOutpoint = dmn->collateralOutpoint;
         } else {
-            LOCK(cs_main);
             Misbehaving(peer.GetId(), 10);
             return;
         }
@@ -100,7 +98,6 @@ void CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataStream&
     }
 
     if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 10);
         return;
     }

src/coinjoin/server.cpp

@@ -113,7 +113,6 @@ void CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv)
     vRecv >> dsq;
 
     if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 100);
         return;
     }
@@ -123,7 +122,6 @@ void CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv)
         if (auto dmn = mnList.GetValidMN(dsq.m_protxHash)) {
             dsq.masternodeOutpoint = dmn->collateralOutpoint;
         } else {
-            LOCK(cs_main);
             Misbehaving(peer.GetId(), 10);
             return;
         }
@@ -159,7 +157,6 @@ void CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv)
     }
 
     if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 10);
         return;
     }

src/evo/mnauth.cpp

@@ -70,26 +70,22 @@ void CMNAuth::ProcessMessage(CNode& peer, CConnman& connman, std::string_view ms
 
     // only one MNAUTH allowed
     if (!peer.GetVerifiedProRegTxHash().IsNull()) {
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 100, "duplicate mnauth");
         return;
     }
 
     if ((~peer.nServices) & (NODE_NETWORK | NODE_BLOOM)) {
         // either NODE_NETWORK or NODE_BLOOM bit is missing in node's services
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 100, "mnauth from a node with invalid services");
         return;
     }
 
     if (mnauth.proRegTxHash.IsNull()) {
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 100, "empty mnauth proRegTxHash");
         return;
     }
 
     if (!mnauth.sig.IsValid()) {
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 100, "invalid mnauth signature");
         return;
     }
@@ -97,7 +93,6 @@ void CMNAuth::ProcessMessage(CNode& peer, CConnman& connman, std::string_view ms
     const auto mnList = deterministicMNManager->GetListAtChainTip();
     const auto dmn = mnList.GetMN(mnauth.proRegTxHash);
     if (!dmn) {
-        LOCK(cs_main);
         // in case node was unlucky and not up to date, just let it be connected as a regular node, which gives it
         // a chance to get up-to-date and thus realize that it's not a MN anymore. We still give it a
         // low DoS score.
@@ -122,7 +117,6 @@ void CMNAuth::ProcessMessage(CNode& peer, CConnman& connman, std::string_view ms
     LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, peer.nVersion, peer.GetId());
 
     if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator.Get(), signHash)) {
-        LOCK(cs_main);
         // Same as above, MN seems to not know its fate yet, so give it a chance to update. If this is a
         // malicious node (DoSing us), it'll get banned soon.
         Misbehaving(peer.GetId(), 10, "mnauth signature verification failed");

src/governance/governance.cpp

@@ -219,7 +219,6 @@ void CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, std::str
         } else {
             LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- Rejected vote, error = %s\n", exception.what());
             if ((exception.GetNodePenalty() != 0) && ::masternodeSync->IsSynced()) {
-                LOCK(cs_main);
                 Misbehaving(peer.GetId(), exception.GetNodePenalty());
             }
             return;
@@ -625,7 +624,6 @@ void CGovernanceManager::SyncObjects(CNode& peer, CConnman& connman) const
     if (netfulfilledman.HasFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC)) {
         // Asking for the whole list multiple times in a short period of time is no good
         LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- peer already asked me for the list\n", __func__);
-        LOCK(cs_main);
         Misbehaving(peer.GetId(), 20);
         return;
     }

src/llmq/blockprocessor.cpp

@@ -55,14 +55,14 @@ void CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_view m
 
     if (qc.IsNull()) {
         LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- null commitment from peer=%d\n", __func__, peer.GetId());
-        WITH_LOCK(cs_main, Misbehaving(peer.GetId(), 100));
+        Misbehaving(peer.GetId(), 100);
         return;
     }
 
     if (!Params().HasLLMQ(qc.llmqType)) {
         LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- invalid commitment type %d from peer=%d\n", __func__,
                  ToUnderlying(qc.llmqType), peer.GetId());
-        WITH_LOCK(cs_main, Misbehaving(peer.GetId(), 100));
+        Misbehaving(peer.GetId(), 100);
         return;
     }
     auto type = qc.llmqType;
@@ -125,7 +125,7 @@ void CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_view m
         LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- commitment for quorum %s:%d is not valid quorumIndex[%d] nversion[%d], peer=%d\n",
                  __func__, qc.quorumHash.ToString(),
                  ToUnderlying(qc.llmqType), qc.quorumIndex, qc.nVersion, peer.GetId());
-        WITH_LOCK(cs_main, Misbehaving(peer.GetId(), 100));
+        Misbehaving(peer.GetId(), 100);
         return;
     }
 

src/llmq/chainlocks.cpp

@@ -125,7 +125,6 @@ void CChainLocksHandler::ProcessNewChainLock(const NodeId from, const llmq::CCha
     if (!llmq::CSigningManager::VerifyRecoveredSig(Params().GetConsensus().llmqTypeChainLocks, *llmq::quorumManager, clsig.getHeight(), requestId, clsig.getBlockHash(), clsig.getSig())) {
         LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- invalid CLSIG (%s), peer=%d\n", __func__, clsig.ToString(), from);
         if (from != -1) {
-            LOCK(cs_main);
             Misbehaving(from, 10);
         }
         return;

src/llmq/dkgsessionhandler.cpp

@@ -429,7 +429,6 @@ bool ProcessPendingMessageBatch(CDKGSession& session, CDKGPendingMessages& pendi
         if (!p.second) {
             LogPrint(BCLog::LLMQ_DKG, "%s -- failed to deserialize message, peer=%d\n", __func__, nodeId);
             {
-                LOCK(cs_main);
                 Misbehaving(nodeId, 100);
             }
             continue;
@@ -439,7 +438,6 @@ bool ProcessPendingMessageBatch(CDKGSession& session, CDKGPendingMessages& pendi
             if (ban) {
                 LogPrint(BCLog::LLMQ_DKG, "%s -- banning node due to failed preverification, peer=%d\n", __func__, nodeId);
                 {
-                    LOCK(cs_main);
                     Misbehaving(nodeId, 100);
                 }
             }
@@ -470,7 +468,6 @@ bool ProcessPendingMessageBatch(CDKGSession& session, CDKGPendingMessages& pendi
         session.ReceiveMessage(*p.second, ban);
         if (ban) {
             LogPrint(BCLog::LLMQ_DKG, "%s -- banning node after ReceiveMessage failed, peer=%d\n", __func__, nodeId);
-            LOCK(cs_main);
             Misbehaving(nodeId, 100);
             badNodes.emplace(nodeId);
         }

src/llmq/dkgsessionmgr.cpp

@@ -185,7 +185,6 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor
     }
 
     if (vRecv.empty()) {
-        LOCK(cs_main);
         Misbehaving(pfrom.GetId(), 100);
         return;
     }
@@ -198,7 +197,6 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor
     vRecv.Rewind(sizeof(uint8_t));
 
     if (!Params().HasLLMQ(llmqType)) {
-        LOCK(cs_main);
         LogPrintf("CDKGSessionManager -- invalid llmqType [%d]\n", ToUnderlying(llmqType));
         Misbehaving(pfrom.GetId(), 100);
         return;
@@ -219,7 +217,6 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor
     if (quorumIndex == -1) {
         CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(cs_main, return LookupBlockIndex(quorumHash));
         if (pQuorumBaseBlockIndex == nullptr) {
-            LOCK(cs_main);
             LogPrintf("CDKGSessionManager -- unknown quorumHash %s\n", quorumHash.ToString());
             // NOTE: do not insta-ban for this, we might be lagging behind
             Misbehaving(pfrom.GetId(), 10);
@@ -227,7 +224,6 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor
         }
 
         if (!utils::IsQuorumTypeEnabled(llmqType, quorum_manager, pQuorumBaseBlockIndex->pprev)) {
-            LOCK(cs_main);
             LogPrintf("CDKGSessionManager -- llmqType [%d] quorums aren't active\n", ToUnderlying(llmqType));
             Misbehaving(pfrom.GetId(), 100);
             return;
@@ -239,14 +235,12 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor
                 llmqParams.signingActiveQuorumCount - 1 : 0;
 
         if (quorumIndex > quorumIndexMax) {
-            LOCK(cs_main);
             LogPrintf("CDKGSessionManager -- invalid quorumHash %s\n", quorumHash.ToString());
             Misbehaving(pfrom.GetId(), 100);
             return;
         }
 
         if (!dkgSessionHandlers.count(std::make_pair(llmqType, quorumIndex))) {
-            LOCK(cs_main);
             LogPrintf("CDKGSessionManager -- no session handlers for quorumIndex [%d]\n", quorumIndex);
             Misbehaving(pfrom.GetId(), 100);
             return;

src/llmq/instantsend.cpp

@@ -788,7 +788,6 @@ void CInstantSendManager::ProcessMessageInstantSendLock(const CNode& pfrom, cons
     }
 
     if (!islock->TriviallyValid()) {
-        LOCK(cs_main);
         Misbehaving(pfrom.GetId(), 100);
         return;
     }
@@ -798,14 +797,14 @@ void CInstantSendManager::ProcessMessageInstantSendLock(const CNode& pfrom, cons
         const auto blockIndex = WITH_LOCK(cs_main, return LookupBlockIndex(islock->cycleHash));
         if (blockIndex == nullptr) {
             // Maybe we don't have the block yet or maybe some peer spams invalid values for cycleHash
-            WITH_LOCK(cs_main, Misbehaving(pfrom.GetId(), 1));
+            Misbehaving(pfrom.GetId(), 1);
             return;
         }
 
         // Deterministic islocks MUST use rotation based llmq
         auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend;
         if (blockIndex->nHeight % GetLLMQParams(llmqType).dkgInterval != 0) {
-            WITH_LOCK(cs_main, Misbehaving(pfrom.GetId(), 100));
+            Misbehaving(pfrom.GetId(), 100);
             return;
         }
     }

src/llmq/quorums.cpp

@@ -611,7 +611,6 @@ void CQuorumManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, C
     auto errorHandler = [&](const std::string& strError, int nScore = 10) {
         LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: %s, from peer=%d\n", strFunc, msg_type, strError, pfrom.GetId());
         if (nScore > 0) {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), nScore);
         }
     };

src/llmq/signing.cpp

@@ -577,7 +577,6 @@ void CSigningManager::ProcessMessageRecoveredSig(const CNode& pfrom, const std::
     bool ban = false;
     if (!PreVerifyRecoveredSig(qman, *recoveredSig, ban)) {
         if (ban) {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 100);
         }
         return;
@@ -752,7 +751,6 @@ bool CSigningManager::ProcessPendingRecoveredSigs()
 
         if (batchVerifier.badSources.count(nodeId)) {
             LogPrint(BCLog::LLMQ, "CSigningManager::%s -- invalid recSig from other node, banning peer=%d\n", __func__, nodeId);
-            LOCK(cs_main);
             Misbehaving(nodeId, 100);
             continue;
         }

src/llmq/signing_shares.cpp

@@ -1402,7 +1402,6 @@ void CSigSharesManager::BanNode(NodeId nodeId)
     }
 
     {
-        LOCK(cs_main);
         Misbehaving(nodeId, 100);
     }
 

src/net_processing.cpp

@@ -268,12 +268,6 @@ struct CNodeState {
     const CService address;
     //! Whether we have a fully established connection.
     bool fCurrentlyConnected;
-    //! Accumulated misbehaviour score for this peer.
-    int nMisbehavior;
-    //! Whether this peer should be disconnected and marked as discouraged (unless it has the noban permission).
-    bool m_should_discourage;
-    //! String name of this peer (debugging/logging purposes).
-    const std::string name;
     //! The best known block we know this peer has announced.
     const CBlockIndex *pindexBestKnownBlock;
     //! The hash of the last unknown block this peer has announced.
@@ -410,13 +404,10 @@ struct CNodeState {
     //! Whether this peer is a manual connection
     bool m_is_manual_connection;
 
-    CNodeState(CAddress addrIn, std::string addrNameIn, bool is_inbound, bool is_manual) :
+    CNodeState(CAddress addrIn, bool is_inbound, bool is_manual) :
-        address(addrIn), name(std::move(addrNameIn)), m_is_inbound(is_inbound),
+        address(addrIn), m_is_inbound(is_inbound), m_is_manual_connection(is_manual)
-        m_is_manual_connection (is_manual)
     {
         fCurrentlyConnected = false;
-        nMisbehavior = 0;
-        m_should_discourage = false;
         pindexBestKnownBlock = nullptr;
         hashLastUnknownBlock.SetNull();
         pindexLastCommonBlock = nullptr;
@@ -453,6 +444,50 @@ static CNodeState *State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
     return &it->second;
 }
 
+/**
+ * Data structure for an individual peer. This struct is not protected by
+ * cs_main since it does not contain validation-critical data.
+ *
+ * Memory is owned by shared pointers and this object is destructed when
+ * the refcount drops to zero.
+ *
+ * TODO: move most members from CNodeState to this structure.
+ * TODO: move remaining application-layer data members from CNode to this structure.
+ */
+struct Peer {
+    /** Same id as the CNode object for this peer */
+    const NodeId m_id{0};
+
+    /** Protects misbehavior data members */
+    Mutex m_misbehavior_mutex;
+    /** Accumulated misbehavior score for this peer */
+    int nMisbehavior GUARDED_BY(m_misbehavior_mutex){0};
+    /** Whether this peer should be disconnected and marked as discouraged (unless it has the noban permission). */
+    bool m_should_discourage GUARDED_BY(m_misbehavior_mutex){false};
+
+    Peer(NodeId id) : m_id(id) {}
+};
+
+using PeerRef = std::shared_ptr<Peer>;
+
+/**
+ * Map of all Peer objects, keyed by peer id. This map is protected
+ * by the global g_peer_mutex. Once a shared pointer reference is
+ * taken, the lock may be released. Individual fields are protected by
+ * their own locks.
+ */
+Mutex g_peer_mutex;
+static std::map<NodeId, PeerRef> g_peer_map GUARDED_BY(g_peer_mutex);
+
+/** Get a shared pointer to the Peer object.
+ *  May return nullptr if the Peer object can't be found. */
+static PeerRef GetPeerRef(NodeId id)
+{
+    LOCK(g_peer_mutex);
+    auto it = g_peer_map.find(id);
+    return it != g_peer_map.end() ? it->second : nullptr;
+}
+
 static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     nPreferredDownload -= state->fPreferredDownload;
@@ -906,13 +941,17 @@ static bool IsOutboundDisconnectionCandidate(const CNode& node)
 
 void PeerLogicValidation::InitializeNode(CNode *pnode) {
     CAddress addr = pnode->addr;
-    std::string addrName = pnode->GetAddrName();
     NodeId nodeid = pnode->GetId();
     {
         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));
+        mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, pnode->fInbound, pnode->m_manual_connection));
     }
-    if(!pnode->fInbound)
+    {
+        PeerRef peer = std::make_shared<Peer>(nodeid);
+        LOCK(g_peer_mutex);
+        g_peer_map.emplace_hint(g_peer_map.end(), nodeid, std::move(peer));
+    }
+    if (!pnode->fInbound)
         PushNodeVersion(*pnode, m_connman, GetTime());
 }
 
@@ -933,13 +972,21 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio
     NodeId nodeid = node.GetId();
     fUpdateConnectionTime = false;
     LOCK(cs_main);
+    int misbehavior{0};
+    {
+        PeerRef peer = GetPeerRef(nodeid);
+        assert(peer != nullptr);
+        misbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->nMisbehavior);
+        LOCK(g_peer_mutex);
+        g_peer_map.erase(nodeid);
+    }
     CNodeState *state = State(nodeid);
     assert(state != nullptr);
 
     if (state->fSyncStarted)
         nSyncStarted--;
 
-    if (state->nMisbehavior == 0 && state->fCurrentlyConnected && !node.m_block_relay_only_peer) {
+    if (misbehavior == 0 && state->fCurrentlyConnected && !node.m_block_relay_only_peer) {
         // Note: we avoid changing visible addrman state for block-relay-only peers
         fUpdateConnectionTime = true;
     }
@@ -967,17 +1014,23 @@ void PeerLogicValidation::FinalizeNode(const CNode& node, bool& fUpdateConnectio
 }
 
 bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) {
-    LOCK(cs_main);
+    {
-    CNodeState *state = State(nodeid);
+        LOCK(cs_main);
-    if (state == nullptr)
+        CNodeState* state = State(nodeid);
-        return false;
+        if (state == nullptr)
-    stats.nMisbehavior = state->nMisbehavior;
+            return false;
-    stats.nSyncHeight = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1;
+        stats.nSyncHeight = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1;
-    stats.nCommonHeight = state->pindexLastCommonBlock ? state->pindexLastCommonBlock->nHeight : -1;
+        stats.nCommonHeight = state->pindexLastCommonBlock ? state->pindexLastCommonBlock->nHeight : -1;
-    for (const QueuedBlock& queue : state->vBlocksInFlight) {
+        for (const QueuedBlock& queue : state->vBlocksInFlight) {
-        if (queue.pindex)
+            if (queue.pindex)
-            stats.vHeightInFlight.push_back(queue.pindex->nHeight);
+                stats.vHeightInFlight.push_back(queue.pindex->nHeight);
+        }
     }
+
+    PeerRef peer = GetPeerRef(nodeid);
+    if (peer == nullptr) return false;
+    stats.nMisbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->nMisbehavior);
+
     return true;
 }
 
@@ -1127,33 +1180,35 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set<uin
 /**
  * Increment peer's misbehavior score. If the new value surpasses banscore (specified on startup or by default), mark node to be discouraged, meaning the peer might be disconnected & added to the discouragement filter.
  */
-void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message)
 {
     assert(howmuch > 0);
 
-    CNodeState* const state = State(pnode);
+    PeerRef peer = GetPeerRef(pnode);
-    if (state == nullptr) return;
+    if (peer == nullptr) return;
 
-    state->nMisbehavior += howmuch;
+    LOCK(peer->m_misbehavior_mutex);
+    peer->nMisbehavior += howmuch;
     const int banscore = gArgs.GetArg("-banscore", DEFAULT_BANSCORE_THRESHOLD);
     const std::string message_prefixed = message.empty() ? "" : (": " + message);
-    if (state->nMisbehavior >= banscore && state->nMisbehavior - howmuch < banscore)
+    if (peer->nMisbehavior >= banscore && peer->nMisbehavior - howmuch < banscore)
     {
-        LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", pnode, state->nMisbehavior - howmuch, state->nMisbehavior, message_prefixed);
+        LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", pnode, peer->nMisbehavior - howmuch, peer->nMisbehavior, message_prefixed);
-        state->m_should_discourage = true;
+        peer->m_should_discourage = true;
         statsClient.inc("misbehavior.banned", 1.0f);
     } else {
-        LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s\n", pnode, state->nMisbehavior - howmuch, state->nMisbehavior, message_prefixed);
+        LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s\n", pnode, peer->nMisbehavior - howmuch, peer->nMisbehavior, message_prefixed);
         statsClient.count("misbehavior.amount", howmuch, 1.0);
     }
 }
 
 bool IsBanned(NodeId pnode)
 {
-    CNodeState *state = State(pnode);
+    PeerRef peer = GetPeerRef(pnode);
-    if (state == nullptr)
+    if (peer == nullptr)
         return false;
-    if (state->m_should_discourage) {
+    LOCK(peer->m_misbehavior_mutex);
+    if (peer->m_should_discourage) {
         return true;
     }
     return false;
@@ -1178,7 +1233,6 @@ static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool v
     case ValidationInvalidReason::CONSENSUS:
     case ValidationInvalidReason::BLOCK_MUTATED:
         if (!via_compact_block) {
-            LOCK(cs_main);
             Misbehaving(nodeid, 100, message);
             return true;
         }
@@ -1202,21 +1256,14 @@ static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool v
     case ValidationInvalidReason::BLOCK_INVALID_HEADER:
     case ValidationInvalidReason::BLOCK_CHECKPOINT:
     case ValidationInvalidReason::BLOCK_INVALID_PREV:
-        {
+        Misbehaving(nodeid, 100, message);
-            LOCK(cs_main);
-            Misbehaving(nodeid, 100, message);
-        }
         return true;
     // Conflicting (but not necessarily invalid) data or different policy:
     case ValidationInvalidReason::BLOCK_MISSING_PREV:
     case ValidationInvalidReason::BLOCK_CHAINLOCK:
     case ValidationInvalidReason::TX_BAD_SPECIAL:
     case ValidationInvalidReason::TX_CONFLICT_LOCK:
-        {
+        Misbehaving(nodeid, 10, message);
-            // TODO: Handle this much more gracefully (10 DoS points is super arbitrary)
-            LOCK(cs_main);
-            Misbehaving(nodeid, 10, message);
-        }
         return true;
     case ValidationInvalidReason::RECENT_CONSENSUS_CHANGE:
     case ValidationInvalidReason::BLOCK_TIME_FUTURE:
@@ -2000,7 +2047,6 @@ inline void static SendBlockTransactions(const CBlock& block, const BlockTransac
     BlockTransactions resp(req);
     for (size_t i = 0; i < req.indexes.size(); i++) {
         if (req.indexes[i] >= block.vtx.size()) {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 100, "getblocktxn with out-of-bounds tx indices");
             return;
         }
@@ -2572,7 +2618,6 @@ void PeerLogicValidation::ProcessMessage(
               (msg_type == NetMsgType::FILTERLOAD ||
                msg_type == NetMsgType::FILTERADD))
     {
-        LOCK(cs_main);
         Misbehaving(pfrom.GetId(), 100);
         return;
     }
@@ -2581,7 +2626,6 @@ void PeerLogicValidation::ProcessMessage(
         // Each connection can only send one version message
         if (pfrom.nVersion != 0)
         {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 1, "redundant version message");
             return;
         }
@@ -2670,7 +2714,6 @@ void PeerLogicValidation::ProcessMessage(
 
         if (Params().NetworkIDString() == CBaseChainParams::DEVNET) {
             if (cleanSubVer.find(strprintf("devnet.%s", gArgs.GetDevNetName())) == std::string::npos) {
-                LOCK(cs_main);
                 LogPrintf("connected to wrong devnet. Reported version is %s, expected devnet name is %s\n", cleanSubVer, gArgs.GetDevNetName());
                 if (!pfrom.fInbound)
                     Misbehaving(pfrom.GetId(), 100); // don't try to connect again
@@ -2768,7 +2811,6 @@ void PeerLogicValidation::ProcessMessage(
 
     if (pfrom.nVersion == 0) {
         // Must have a version message before anything else
-        LOCK(cs_main);
         Misbehaving(pfrom.GetId(), 1, "non-version message before version handshake");
         return;
     }
@@ -2843,7 +2885,6 @@ void PeerLogicValidation::ProcessMessage(
 
     if (!pfrom.fSuccessfullyConnected) {
         // Must have a verack message before anything else
-        LOCK(cs_main);
         Misbehaving(pfrom.GetId(), 1, "non-verack message before version handshake");
         return;
     }
@@ -2886,7 +2927,6 @@ void PeerLogicValidation::ProcessMessage(
         }
         if (vAddr.size() > 1000)
         {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size()));
             return;
         }
@@ -2975,7 +3015,6 @@ void PeerLogicValidation::ProcessMessage(
         vRecv >> vInv;
         if (vInv.size() > MAX_INV_SZ)
         {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 20, strprintf("message inv size() = %u", vInv.size()));
             return;
         }
@@ -3066,7 +3105,6 @@ void PeerLogicValidation::ProcessMessage(
         vRecv >> vInv;
         if (vInv.size() > MAX_INV_SZ)
         {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 20, strprintf("message getdata size() = %u", vInv.size()));
             return;
         }
@@ -3750,7 +3788,6 @@ void PeerLogicValidation::ProcessMessage(
         // Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks.
         unsigned int nCount = ReadCompactSize(vRecv);
         if (nCount > MAX_HEADERS_RESULTS) {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 20, strprintf("headers message size = %u", nCount));
             return;
         }
@@ -3952,7 +3989,6 @@ void PeerLogicValidation::ProcessMessage(
         if (!filter.IsWithinSizeConstraints())
         {
             // There is no excuse for sending a too-large filter
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 100, "too-large bloom filter");
         }
         else if (!pfrom.m_block_relay_only_peer)
@@ -3982,7 +4018,6 @@ void PeerLogicValidation::ProcessMessage(
             }
         }
         if (bad) {
-            LOCK(cs_main);
             Misbehaving(pfrom.GetId(), 100, "bad filteradd message");
         }
         return;
@@ -4035,7 +4070,6 @@ void PeerLogicValidation::ProcessMessage(
 
     if (msg_type == NetMsgType::MNLISTDIFF) {
         // we have never requested this
-        LOCK(cs_main);
         Misbehaving(pfrom.GetId(), 100, strprintf("received not-requested mnlistdiff. peer=%d", pfrom.GetId()));
         return;
     }
@@ -4059,7 +4093,6 @@ void PeerLogicValidation::ProcessMessage(
 
     if (msg_type == NetMsgType::QUORUMROTATIONINFO) {
         // we have never requested this
-        LOCK(cs_main);
         Misbehaving(pfrom.GetId(), 100, strprintf("received not-requested quorumrotationinfo. peer=%d", pfrom.GetId()));
         return;
     }
@@ -4136,15 +4169,17 @@ void PeerLogicValidation::ProcessMessage(
 bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
 {
     const NodeId peer_id{pnode.GetId()};
+    PeerRef peer = GetPeerRef(peer_id);
+    if (peer == nullptr) return false;
+
     {
-        LOCK(cs_main);
+        LOCK(peer->m_misbehavior_mutex);
-        CNodeState& state = *State(peer_id);
 
         // There's nothing to do if the m_should_discourage flag isn't set
-        if (!state.m_should_discourage) return false;
+        if (!peer->m_should_discourage) return false;
 
-        state.m_should_discourage = false;
+        peer->m_should_discourage = false;
-    }  // cs_main
+    } // peer.m_misbehavior_mutex
 
     if (pnode.HasPermission(PF_NOBAN)) {
         // We never disconnect or discourage peers for bad behavior if they have the NOBAN permission flag

src/net_processing.h

@@ -104,7 +104,7 @@ bool IsBanned(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 // Upstream moved this into net_processing.cpp (13417), however since we use Misbehaving in a number of dash specific
 // files such as mnauth.cpp and governance.cpp it makes sense to keep it in the header
 /** Increase a node's misbehavior score. */
-void Misbehaving(NodeId nodeid, int howmuch, const std::string& message="") EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message="");
 
 void EraseObjectRequest(NodeId nodeId, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 void RequestObject(NodeId nodeId, const CInv& inv, std::chrono::microseconds current_time, bool fForce=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

src/spork.cpp

@@ -130,7 +130,6 @@ void CSporkManager::ProcessSpork(const CNode& peer, CConnman& connman, CDataStre
     }
 
     if (spork.nTimeSigned > GetAdjustedTime() + 2 * 60 * 60) {
-        LOCK(cs_main);
         LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: too far into the future\n");
         Misbehaving(peer.GetId(), 100);
         return;
@@ -139,7 +138,6 @@ void CSporkManager::ProcessSpork(const CNode& peer, CConnman& connman, CDataStre
     auto opt_keyIDSigner = spork.GetSignerKeyID();
 
     if (opt_keyIDSigner == std::nullopt || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(*opt_keyIDSigner))) {
-        LOCK(cs_main);
         LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n");
         Misbehaving(peer.GetId(), 100);
         return;

src/test/denialofservice_tests.cpp

@@ -240,10 +240,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
     peerLogic->InitializeNode(&dummyNode1);
     dummyNode1.nVersion = 1;
     dummyNode1.fSuccessfullyConnected = true;
-    {
+    Misbehaving(dummyNode1.GetId(), 100); // Should get banned
-        LOCK(cs_main);
-        Misbehaving(dummyNode1.GetId(), 100); // Should get banned
-    }
     {
         LOCK(dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
@@ -257,20 +254,14 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
     peerLogic->InitializeNode(&dummyNode2);
     dummyNode2.nVersion = 1;
     dummyNode2.fSuccessfullyConnected = true;
-    {
+    Misbehaving(dummyNode2.GetId(), 50);
-        LOCK(cs_main);
-        Misbehaving(dummyNode2.GetId(), 50);
-    }
     {
         LOCK(dummyNode2.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
     }
     BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not banned yet...
     BOOST_CHECK(banman->IsDiscouraged(addr1));  // ... but 1 still should be
-    {
+    Misbehaving(dummyNode2.GetId(), 50);
-        LOCK(cs_main);
-        Misbehaving(dummyNode2.GetId(), 50);
-    }
     {
         LOCK(dummyNode2.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
@@ -299,7 +290,6 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
     dummyNode1.nVersion = 1;
     dummyNode1.fSuccessfullyConnected = true;
     {
-        LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 100);
     }
     {
@@ -308,7 +298,6 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
     }
     BOOST_CHECK(!banman->IsDiscouraged(addr1));
     {
-        LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 10);
     }
     {
@@ -317,7 +306,6 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
     }
     BOOST_CHECK(!banman->IsDiscouraged(addr1));
     {
-        LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 1);
     }
     {
@@ -350,10 +338,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     dummyNode.nVersion = 1;
     dummyNode.fSuccessfullyConnected = true;
 
-    {
+    Misbehaving(dummyNode.GetId(), 100);
-        LOCK(cs_main);
-        Misbehaving(dummyNode.GetId(), 100);
-    }
     {
         LOCK(dummyNode.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode));