phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-25 10:43:19 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge #19607: [p2p] Add Peer struct for per-peer data in net processing

Browse source 
8e35bf5906 scripted-diff: rename misbehavior members (John Newbery)
1f96d2e673 [net processing] Move misbehavior tracking state to Peer (John Newbery)
7cd4159ac8 [net processing] Add Peer (John Newbery)
aba03359a6 [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 8e35bf5906
  troygiorshev:
    reACK 8e35bf5906 via `git range-diff master 9510938 8e35bf5`
  theuni:
    ACK 8e35bf5906.
  jonatack:
    ACK 8e35bf5906 keeping in mind Cory's comment (https://github.com/bitcoin/bitcoin/pull/19607#discussion_r470173964) for the follow-up

Tree-SHA512: ad84a92b78fb34c9f43813ca3dfbc7282c887d55300ea2ce0994d134da3e0c7dbc44d54380e00b13bb75a57c28857ac3236bea9135467075d78026767a19e4b1
This commit is contained in:
Wladimir J. van der Laan 2020-08-28 20:20:32 +02:00
commit 1cf73fb8eb
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
4 changed files with 104 additions and 80 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

160
src/net_processing.cpp
View file

@ -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

2
src/net_processing.h
View file

@ -97,7 +97,7 @@ private:
};
struct CNodeStateStats {
int nMisbehavior = 0;
int m_misbehavior_score = 0;
int nSyncHeight = -1;
int nCommonHeight = -1;
std::vector<int> vHeightInFlight;

2
src/rpc/net.cpp
View file

@ -197,7 +197,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
if (fStateStats) {
if (IsDeprecatedRPCEnabled("banscore")) {
// banscore is deprecated in v0.21 for removal in v0.22
obj.pushKV("banscore", statestats.nMisbehavior);
obj.pushKV("banscore", statestats.m_misbehavior_score);
}
obj.pushKV("synced_headers", statestats.nSyncHeight);
obj.pushKV("synced_blocks", statestats.nCommonHeight);

20
src/test/denialofservice_tests.cpp
View file

@ -232,10 +232,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
peerLogic->InitializeNode(&dummyNode1);
dummyNode1.nVersion = 1;
dummyNode1.fSuccessfullyConnected = true;
{
LOCK(cs_main);
Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
}
Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
{
LOCK(dummyNode1.cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
@ -249,20 +246,14 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
peerLogic->InitializeNode(&dummyNode2);
dummyNode2.nVersion = 1;
dummyNode2.fSuccessfullyConnected = true;
{
LOCK(cs_main);
Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1);
}
Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1);
{
LOCK(dummyNode2.cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
}
BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not discouraged yet...
BOOST_CHECK(banman->IsDiscouraged(addr1)); // ... but 1 still should be
{
LOCK(cs_main);
Misbehaving(dummyNode2.GetId(), 1); // 2 reaches discouragement threshold
}
Misbehaving(dummyNode2.GetId(), 1); // 2 reaches discouragement threshold
{
LOCK(dummyNode2.cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
@ -292,10 +283,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
dummyNode.nVersion = 1;
dummyNode.fSuccessfullyConnected = true;
{
LOCK(cs_main);
Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD);
}
Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD);
{
LOCK(dummyNode.cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(&dummyNode));