diff options
author | Wladimir J. van der Laan <laanwj@protonmail.com> | 2020-08-28 20:20:32 +0200 |
---|---|---|
committer | Wladimir J. van der Laan <laanwj@protonmail.com> | 2020-08-28 20:29:16 +0200 |
commit | 1cf73fb8eba3b89a30d5e943deaec5052a8b21b7 (patch) | |
tree | 4007579f71ee11455afd63e5424f54a19c0c4fbf /src/net_processing.cpp | |
parent | ca30d34cf94b7797272ef1920ca4b48716e7f999 (diff) | |
parent | 8e35bf59062b3a823182588e0bf809b3367c2be0 (diff) | |
download | bitcoin-1cf73fb8eba3b89a30d5e943deaec5052a8b21b7.tar.xz |
Merge #19607: [p2p] Add Peer struct for per-peer data in net processing
8e35bf59062b3a823182588e0bf809b3367c2be0 scripted-diff: rename misbehavior members (John Newbery)
1f96d2e673a78220eebf3bbd15b121c51c4cd97b [net processing] Move misbehavior tracking state to Peer (John Newbery)
7cd4159ac834432dadd60a5e8ee817f3cadbee55 [net processing] Add Peer (John Newbery)
aba03359a6e62a376ae44914f609f82a1556fc89 [net processing] Remove CNodeState.name (John Newbery)
Pull request description:
We currently have two structures for per-peer data:
- `CNode` in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory).
- `CNodeState` in net processing, which contains p2p application layer data, but requires cs_main to be locked for access.
This PR adds a third struct `Peer`, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from `CNode` should be moved to `Peer`, and any data that doesn't strictly require cs_main should be moved from `CNodeState` to `Peer` (probably all of `CNodeState` eventually).
`Peer` objects are stored as shared pointers in a net processing global map `g_peer_map`, which is protected by `g_peer_mutex`. To use a `Peer` object, `g_peer_mutex` is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of `Peer` are protected by different mutexes that guard related data. The lifetime of the `Peer` object is managed by the shared_ptr refcount.
This PR adds the `Peer` object and moves the misbehaving data from `CNodeState` to `Peer`. This allows us to immediately remove 15 `LOCK(cs_main)` instances.
For more motivation see #19398
ACKs for top commit:
laanwj:
Code review ACK 8e35bf59062b3a823182588e0bf809b3367c2be0
troygiorshev:
reACK 8e35bf59062b3a823182588e0bf809b3367c2be0 via `git range-diff master 9510938 8e35bf5`
theuni:
ACK 8e35bf59062b3a823182588e0bf809b3367c2be0.
jonatack:
ACK 8e35bf59062b3a823182588e0bf809b3367c2be0 keeping in mind Cory's comment (https://github.com/bitcoin/bitcoin/pull/19607#discussion_r470173964) for the follow-up
Tree-SHA512: ad84a92b78fb34c9f43813ca3dfbc7282c887d55300ea2ce0994d134da3e0c7dbc44d54380e00b13bb75a57c28857ac3236bea9135467075d78026767a19e4b1
Diffstat (limited to 'src/net_processing.cpp')
-rw-r--r-- | src/net_processing.cpp | 160 |
1 files changed, 98 insertions, 62 deletions
diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 60bdfbe9f5..adad428413 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -278,12 +278,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. @@ -432,13 +426,10 @@ struct CNodeState { //! Whether this peer relays txs via wtxid bool m_wtxid_relay{false}; - 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.SetNull(); pindexLastCommonBlock = nullptr; @@ -476,6 +467,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 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<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; @@ -841,7 +876,12 @@ void PeerLogicValidation::InitializeNode(CNode *pnode) { 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->IsInboundConn(), pnode->IsManualConn())); + mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, pnode->IsInboundConn(), pnode->IsManualConn())); + } + { + 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->IsInboundConn()) PushNodeVersion(*pnode, m_connman, GetTime()); @@ -870,13 +910,21 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const void PeerLogicValidation::FinalizeNode(NodeId nodeid, 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); if (state->fSyncStarted) nSyncStarted--; - if (state->nMisbehavior == 0 && state->fCurrentlyConnected) { + if (misbehavior == 0 && state->fCurrentlyConnected) { fUpdateConnectionTime = true; } @@ -906,17 +954,23 @@ void PeerLogicValidation::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTim } 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; } @@ -1060,21 +1114,21 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) * Increment peer's misbehavior score. If the new value >= DISCOURAGEMENT_THRESHOLD, mark the node * to be discouraged, meaning the peer might be disconnected and 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); - if (state == nullptr) return; + 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) - { - LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", pnode, state->nMisbehavior - howmuch, state->nMisbehavior, message_prefixed); - state->m_should_discourage = true; + if (peer->m_misbehavior_score >= DISCOURAGEMENT_THRESHOLD && peer->m_misbehavior_score - howmuch < DISCOURAGEMENT_THRESHOLD) { + LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", 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, message_prefixed); + LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s\n", pnode, peer->m_misbehavior_score - howmuch, peer->m_misbehavior_score, message_prefixed); } } @@ -1096,7 +1150,6 @@ static bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& s case BlockValidationResult::BLOCK_CONSENSUS: case BlockValidationResult::BLOCK_MUTATED: if (!via_compact_block) { - LOCK(cs_main); Misbehaving(nodeid, 100, message); return true; } @@ -1120,18 +1173,12 @@ static bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& s case BlockValidationResult::BLOCK_INVALID_HEADER: case BlockValidationResult::BLOCK_CHECKPOINT: case BlockValidationResult::BLOCK_INVALID_PREV: - { - LOCK(cs_main); - Misbehaving(nodeid, 100, message); - } + Misbehaving(nodeid, 100, message); return true; // Conflicting (but not necessarily invalid) data or different policy: case BlockValidationResult::BLOCK_MISSING_PREV: - { - // TODO: Handle this much more gracefully (10 DoS points is super arbitrary) - LOCK(cs_main); - Misbehaving(nodeid, 10, message); - } + // TODO: Handle this much more gracefully (10 DoS points is super arbitrary) + Misbehaving(nodeid, 10, message); return true; case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE: case BlockValidationResult::BLOCK_TIME_FUTURE: @@ -1155,11 +1202,8 @@ static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, break; // The node is providing invalid data: case TxValidationResult::TX_CONSENSUS: - { - LOCK(cs_main); - Misbehaving(nodeid, 100, message); - return true; - } + Misbehaving(nodeid, 100, message); + return true; // Conflicting (but not necessarily invalid) data or different policy: case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE: case TxValidationResult::TX_INPUTS_NOT_STANDARD: @@ -1806,7 +1850,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; } @@ -2318,7 +2361,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // Each connection can only send one version message if (pfrom.nVersion != 0) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 1, "redundant version message"); return; } @@ -2478,7 +2520,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty 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; } @@ -2545,7 +2586,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty 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; } @@ -2559,7 +2599,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } if (vAddr.size() > MAX_ADDR_TO_SEND) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 20, strprintf("addr message size = %u", vAddr.size())); return; } @@ -2638,7 +2677,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 20, strprintf("inv message size = %u", vInv.size())); return; } @@ -2714,7 +2752,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 20, strprintf("getdata message size = %u", vInv.size())); return; } @@ -3439,7 +3476,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // 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; } @@ -3641,7 +3677,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty 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_tx_relay != nullptr) @@ -3675,7 +3710,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } } if (bad) { - LOCK(cs_main); Misbehaving(pfrom.GetId(), 100, "bad filteradd message"); } return; @@ -3761,15 +3795,17 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty 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) return false; + 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 the NOBAN permission flag |