diff --git a/src/avalanche/processor.cpp b/src/avalanche/processor.cpp --- a/src/avalanche/processor.cpp +++ b/src/avalanche/processor.cpp @@ -295,7 +295,6 @@ auto w = queries.getWriteView(); auto it = w->find(std::make_tuple(nodeid, response.getRound())); if (it == w.end()) { - LOCK(cs_main); Misbehaving(nodeid, 2, "unexpcted-ava-response"); return false; } @@ -308,14 +307,12 @@ const std::vector &votes = response.GetVotes(); size_t size = invs.size(); if (votes.size() != size) { - LOCK(cs_main); Misbehaving(nodeid, 100, "invalid-ava-response-size"); return false; } for (size_t i = 0; i < size; i++) { if (invs[i].hash != votes[i].GetHash()) { - LOCK(cs_main); Misbehaving(nodeid, 100, "invalid-ava-response-content"); return false; } diff --git a/src/net_processing.h b/src/net_processing.h --- a/src/net_processing.h +++ b/src/net_processing.h @@ -132,7 +132,7 @@ }; struct CNodeStateStats { - int nMisbehavior = 0; + int m_misbehavior_score = 0; int nSyncHeight = -1; int nCommonHeight = -1; std::vector vHeightInFlight; @@ -146,8 +146,7 @@ * might be disconnected and added to the discouragement filter. */ void Misbehaving(const NodeId nodeid, const int howmuch, - const std::string &message = "") - EXCLUSIVE_LOCKS_REQUIRED(cs_main); + const std::string &message = ""); /** Relay transaction to every node */ void RelayTransaction(const TxId &txid, const CConnman &connman); diff --git a/src/net_processing.cpp b/src/net_processing.cpp --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -285,13 +285,6 @@ 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 whitelisted with noban). - 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. @@ -445,13 +438,10 @@ //! Whether this peer is a manual connection bool m_is_manual_connection; - CNodeState(CAddress addrIn, std::string addrNameIn, bool is_inbound, - bool is_manual) - : address(addrIn), name(std::move(addrNameIn)), - m_is_inbound(is_inbound), m_is_manual_connection(is_manual) { + CNodeState(CAddress addrIn, bool is_inbound, bool is_manual) + : address(addrIn), m_is_inbound(is_inbound), + m_is_manual_connection(is_manual) { fCurrentlyConnected = false; - nMisbehavior = 0; - m_should_discourage = false; pindexBestKnownBlock = nullptr; hashLastUnknownBlock = BlockHash(); pindexLastCommonBlock = nullptr; @@ -490,6 +480,53 @@ 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 m_misbehavior_score 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; + +/** + * 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 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; @@ -952,10 +989,14 @@ mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, - std::move(addrName), pnode->IsInboundConn(), pnode->IsManualConn())); } + { + PeerRef peer = std::make_shared(nodeid); + LOCK(g_peer_mutex); + g_peer_map.emplace_hint(g_peer_map.end(), nodeid, std::move(peer)); + } if (!pnode->IsInboundConn()) { PushNodeVersion(config, *pnode, m_connman, GetTime()); } @@ -965,6 +1006,15 @@ bool &fUpdateConnectionTime) { fUpdateConnectionTime = false; LOCK(cs_main); + int misbehavior{0}; + { + PeerRef peer = GetPeerRef(nodeid); + assert(peer != nullptr); + misbehavior = WITH_LOCK(peer->m_misbehavior_mutex, + return peer->m_misbehavior_score); + LOCK(g_peer_mutex); + g_peer_map.erase(nodeid); + } CNodeState *state = State(nodeid); assert(state != nullptr); @@ -972,7 +1022,7 @@ nSyncStarted--; } - if (state->nMisbehavior == 0 && state->fCurrentlyConnected) { + if (misbehavior == 0 && state->fCurrentlyConnected) { fUpdateConnectionTime = true; } @@ -1000,22 +1050,32 @@ } bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) { - LOCK(cs_main); - CNodeState *state = State(nodeid); - if (state == nullptr) { - return false; - } - stats.nMisbehavior = state->nMisbehavior; - stats.nSyncHeight = - state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1; - stats.nCommonHeight = state->pindexLastCommonBlock - ? state->pindexLastCommonBlock->nHeight - : -1; - for (const QueuedBlock &queue : state->vBlocksInFlight) { - if (queue.pindex) { - stats.vHeightInFlight.push_back(queue.pindex->nHeight); + { + LOCK(cs_main); + CNodeState *state = State(nodeid); + if (state == nullptr) { + return false; + } + stats.nSyncHeight = state->pindexBestKnownBlock + ? state->pindexBestKnownBlock->nHeight + : -1; + stats.nCommonHeight = state->pindexLastCommonBlock + ? state->pindexLastCommonBlock->nHeight + : -1; + for (const QueuedBlock &queue : state->vBlocksInFlight) { + if (queue.pindex) { + stats.vHeightInFlight.push_back(queue.pindex->nHeight); + } } } + + PeerRef peer = GetPeerRef(nodeid); + if (peer == nullptr) { + return false; + } + stats.m_misbehavior_score = + WITH_LOCK(peer->m_misbehavior_mutex, return peer->m_misbehavior_score); + return true; } @@ -1173,36 +1233,35 @@ */ void Misbehaving(const NodeId pnode, const int howmuch, const std::string &message) { - AssertLockHeld(cs_main); - assert(howmuch > 0); - CNodeState *const state = State(pnode); - if (state == nullptr) { + PeerRef peer = GetPeerRef(pnode); + if (peer == nullptr) { return; } - state->nMisbehavior += howmuch; + LOCK(peer->m_misbehavior_mutex); + + peer->m_misbehavior_score += howmuch; const std::string message_prefixed = message.empty() ? "" : (": " + message); - if (state->nMisbehavior >= DISCOURAGEMENT_THRESHOLD && - state->nMisbehavior - howmuch < DISCOURAGEMENT_THRESHOLD) { + if (peer->m_misbehavior_score >= DISCOURAGEMENT_THRESHOLD && + peer->m_misbehavior_score - howmuch < DISCOURAGEMENT_THRESHOLD) { LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) BAN THRESHOLD EXCEEDED%s\n", - pnode, state->nMisbehavior - howmuch, state->nMisbehavior, - message_prefixed); - state->m_should_discourage = true; + pnode, peer->m_misbehavior_score - howmuch, + peer->m_misbehavior_score, message_prefixed); + peer->m_should_discourage = true; } else { LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s\n", pnode, - state->nMisbehavior - howmuch, state->nMisbehavior, + peer->m_misbehavior_score - howmuch, peer->m_misbehavior_score, message_prefixed); } } // overloaded variant of above to operate on CNode*s static void Misbehaving(const CNode &node, int howmuch, - const std::string &message) - EXCLUSIVE_LOCKS_REQUIRED(cs_main) { + const std::string &message) { Misbehaving(node.GetId(), howmuch, message); } @@ -1236,7 +1295,6 @@ case BlockValidationResult::BLOCK_CONSENSUS: case BlockValidationResult::BLOCK_MUTATED: if (!via_compact_block) { - LOCK(cs_main); Misbehaving(nodeid, 100, message); return true; } @@ -1259,26 +1317,20 @@ } case BlockValidationResult::BLOCK_INVALID_HEADER: case BlockValidationResult::BLOCK_CHECKPOINT: - case BlockValidationResult::BLOCK_INVALID_PREV: { - LOCK(cs_main); + case BlockValidationResult::BLOCK_INVALID_PREV: Misbehaving(nodeid, 100, message); - } return true; - case BlockValidationResult::BLOCK_FINALIZATION: { + case BlockValidationResult::BLOCK_FINALIZATION: // TODO: Use the state object to report this is probably not the // best idea. This is effectively unreachable, unless there is a bug // somewhere. - LOCK(cs_main); Misbehaving(nodeid, 20, message); - } return true; // Conflicting (but not necessarily invalid) data or different policy: - case BlockValidationResult::BLOCK_MISSING_PREV: { + case BlockValidationResult::BLOCK_MISSING_PREV: // TODO: Handle this much more gracefully (10 DoS points is super // arbitrary) - LOCK(cs_main); Misbehaving(nodeid, 10, message); - } return true; case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE: case BlockValidationResult::BLOCK_TIME_FUTURE: @@ -1303,11 +1355,9 @@ case TxValidationResult::TX_RESULT_UNSET: break; // The node is providing invalid data: - case TxValidationResult::TX_CONSENSUS: { - LOCK(cs_main); + case TxValidationResult::TX_CONSENSUS: Misbehaving(nodeid, 100, message); return true; - } // Conflicting (but not necessarily invalid) data or different policy: case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE: case TxValidationResult::TX_NOT_STANDARD: @@ -1962,7 +2012,6 @@ BlockTransactions resp(req); for (size_t i = 0; i < req.indices.size(); i++) { if (req.indices[i] >= block.vtx.size()) { - LOCK(cs_main); Misbehaving(pfrom, 100, "getblocktxn with out-of-bounds tx indices"); return; @@ -2538,7 +2587,6 @@ (msg_type == NetMsgType::FILTERLOAD || msg_type == NetMsgType::FILTERADD)) { if (pfrom.nVersion >= NO_BLOOM_VERSION) { - LOCK(cs_main); Misbehaving(pfrom, 100, "no-bloom-version"); } else { pfrom.fDisconnect = true; @@ -2549,7 +2597,6 @@ if (msg_type == NetMsgType::VERSION) { // Each connection can only send one version message if (pfrom.nVersion != 0) { - LOCK(cs_main); Misbehaving(pfrom, 1, "redundant version message"); return; } @@ -2707,7 +2754,6 @@ pfrom.nTimeOffset = nTimeOffset; AddTimeData(pfrom.addr, nTimeOffset); } else { - LOCK(cs_main); Misbehaving(pfrom, 20, "Ignoring invalid timestamp in version message"); } @@ -2721,7 +2767,6 @@ if (pfrom.nVersion == 0) { // Must have a version message before anything else - LOCK(cs_main); Misbehaving(pfrom, 10, "non-version message before version handshake"); return; } @@ -2772,7 +2817,6 @@ if (!pfrom.fSuccessfullyConnected) { // Must have a verack message before anything else - LOCK(cs_main); Misbehaving(pfrom, 10, "non-verack message before version handshake"); return; } @@ -2785,7 +2829,6 @@ return; } if (vAddr.size() > 1000) { - LOCK(cs_main); Misbehaving(pfrom, 20, strprintf("oversized-addr: message addr size() = %u", vAddr.size())); @@ -2874,7 +2917,6 @@ std::vector vInv; vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - LOCK(cs_main); Misbehaving(pfrom, 20, strprintf("oversized-inv: message inv size() = %u", vInv.size())); @@ -2948,7 +2990,6 @@ std::vector vInv; vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - LOCK(cs_main); Misbehaving(pfrom, 20, strprintf("too-many-inv: message getdata size() = %u", vInv.size())); @@ -3726,7 +3767,6 @@ // deserializing 2000 full blocks. unsigned int nCount = ReadCompactSize(vRecv); if (nCount > MAX_HEADERS_RESULTS) { - LOCK(cs_main); Misbehaving(pfrom, 20, strprintf("too-many-headers: headers message size = %u", nCount)); @@ -3812,7 +3852,6 @@ unsigned int nCount = ReadCompactSize(vRecv); if (nCount > AVALANCHE_MAX_ELEMENT_POLL) { - LOCK(cs_main); Misbehaving( pfrom, 20, strprintf("too-many-ava-poll: poll message size = %u", nCount)); @@ -3914,7 +3953,6 @@ vRecv >> sig; return n.pubkey.VerifySchnorr(verifier.GetHash(), sig); })) { - LOCK(cs_main); Misbehaving(pfrom, 100, "invalid-ava-response-signature"); return; } @@ -4122,7 +4160,6 @@ if (!filter.IsWithinSizeConstraints()) { // There is no excuse for sending a too-large filter - LOCK(cs_main); Misbehaving(pfrom, 100, "too-large bloom filter"); } else if (pfrom.m_tx_relay != nullptr) { LOCK(pfrom.m_tx_relay->cs_filter); @@ -4152,7 +4189,6 @@ } } if (bad) { - LOCK(cs_main); // The structure of this code doesn't really allow for a good error // code. We'll go generic. Misbehaving(pfrom, 100, "bad filteradd message"); @@ -4245,17 +4281,20 @@ */ bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode &pnode) { const NodeId peer_id{pnode.GetId()}; + PeerRef peer = GetPeerRef(peer_id); + if (peer == nullptr) { + return false; + } { - LOCK(cs_main); - CNodeState &state = *State(peer_id); + LOCK(peer->m_misbehavior_mutex); // There's nothing to do if the m_should_discourage flag isn't set - if (!state.m_should_discourage) { + if (!peer->m_should_discourage) { return false; } - state.m_should_discourage = false; - } // cs_main + peer->m_should_discourage = false; + } // peer.m_misbehavior_mutex if (pnode.HasPermission(PF_NOBAN)) { // We never disconnect or discourage peers for bad behavior if they have diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -245,7 +245,7 @@ if (fStateStats) { if (IsDeprecatedRPCEnabled(gArgs, "banscore")) { // banscore is deprecated in v0.22.11 for removal in v0.23 - obj.pushKV("banscore", statestats.nMisbehavior); + obj.pushKV("banscore", statestats.m_misbehavior_score); } obj.pushKV("synced_headers", statestats.nSyncHeight); obj.pushKV("synced_blocks", statestats.nCommonHeight); diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -254,11 +254,8 @@ peerLogic->InitializeNode(config, &dummyNode1); dummyNode1.nVersion = 1; dummyNode1.fSuccessfullyConnected = true; - { - LOCK(cs_main); - // Should be discouraged - Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD); - } + // Should be discouraged + Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD); { LOCK2(cs_main, dummyNode1.cs_sendProcessing); BOOST_CHECK( @@ -275,10 +272,7 @@ peerLogic->InitializeNode(config, &dummyNode2); dummyNode2.nVersion = 1; dummyNode2.fSuccessfullyConnected = true; - { - LOCK(cs_main); - Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1); - } + Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1); { LOCK2(cs_main, dummyNode2.cs_sendProcessing); BOOST_CHECK( @@ -288,11 +282,8 @@ BOOST_CHECK(!banman->IsDiscouraged(addr2)); // ... but 1 still should be BOOST_CHECK(banman->IsDiscouraged(addr1)); - { - LOCK(cs_main); - // 2 reaches discouragement threshold - Misbehaving(dummyNode2.GetId(), 1); - } + // 2 reaches discouragement threshold + Misbehaving(dummyNode2.GetId(), 1); { LOCK2(cs_main, dummyNode2.cs_sendProcessing); BOOST_CHECK( @@ -331,10 +322,7 @@ dummyNode.nVersion = 1; dummyNode.fSuccessfullyConnected = true; - { - LOCK(cs_main); - Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD); - } + Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD); { LOCK2(cs_main, dummyNode.cs_sendProcessing); BOOST_CHECK(