Skip to content

Commit fa8aa0a

Browse files
author
MacroFake
committed
Pass Peer& to Misbehaving()
`Misbehaving` has several coding related issues (ignoring the conceptual issues here for now): * It is public, but it is not supposed to be called from outside of net_processing. Fix that by making it private and creating a public `UnitTestMisbehaving` method for unit testing only. * It doesn't do anything if a `nullptr` is passed. It would be less confusing to just skip the call instead. Fix that by passing `Peer&` to `Misbehaving()`. * It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be propagated. This is harmless, but verbose. Fix it by removing the no longer needed call to `GetPeerRef` and the no longer needed lock annotations.
1 parent 3ba6dd6 commit fa8aa0a

File tree

3 files changed

+45
-45
lines changed

3 files changed

+45
-45
lines changed

‎src/net_processing.cpp‎

Lines changed: 38 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -492,7 +492,7 @@ class PeerManagerImpl final : public PeerManager
492492
void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
493493
void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
494494
void SetBestHeight(int height) override { m_best_height = height; };
495-
void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
495+
void UnitTestMisbehaving(NodeId peer_id, int howmuch) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), howmuch, ""); };
496496
void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
497497
const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override
498498
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex);
@@ -516,6 +516,12 @@ class PeerManagerImpl final : public PeerManager
516516
* May return an empty shared_ptr if the Peer object can't be found. */
517517
PeerRef RemovePeer(NodeId id) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
518518

519+
/**
520+
* Increment peer's misbehavior score. If the new value >= DISCOURAGEMENT_THRESHOLD, mark the node
521+
* to be discouraged, meaning the peer might be disconnected and added to the discouragement filter.
522+
*/
523+
void Misbehaving(Peer& peer, int howmuch, const std::string& message);
524+
519525
/**
520526
* Potentially mark a node discouraged based on the contents of a BlockValidationState object
521527
*
@@ -549,13 +555,12 @@ class PeerManagerImpl final : public PeerManager
549555
void ProcessOrphanTx(std::set<uint256>& orphan_work_set) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans)
550556
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
551557
/** Process a single headers message from a peer. */
552-
void ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
558+
void ProcessHeadersMessage(CNode& pfrom, Peer& peer,
553559
const std::vector<CBlockHeader>& headers,
554560
bool via_compact_block)
555561
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
556562

557-
void SendBlockTransactions(CNode& pfrom, const CBlock& block, const BlockTransactionsRequest& req)
558-
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
563+
void SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req);
559564

560565
/** Register with TxRequestTracker that an INV has been received from a
561566
* peer. The announcement parameters are decided in PeerManager and then
@@ -1441,41 +1446,39 @@ void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
14411446
vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % max_extra_txn;
14421447
}
14431448

1444-
void PeerManagerImpl::Misbehaving(const NodeId pnode, const int howmuch, const std::string& message)
1449+
void PeerManagerImpl::Misbehaving(Peer& peer, int howmuch, const std::string& message)
14451450
{
14461451
assert(howmuch > 0);
14471452

1448-
PeerRef peer = GetPeerRef(pnode);
1449-
if (peer == nullptr) return;
1450-
1451-
LOCK(peer->m_misbehavior_mutex);
1452-
const int score_before{peer->m_misbehavior_score};
1453-
peer->m_misbehavior_score += howmuch;
1454-
const int score_now{peer->m_misbehavior_score};
1453+
LOCK(peer.m_misbehavior_mutex);
1454+
const int score_before{peer.m_misbehavior_score};
1455+
peer.m_misbehavior_score += howmuch;
1456+
const int score_now{peer.m_misbehavior_score};
14551457

14561458
const std::string message_prefixed = message.empty() ? "" : (": " + message);
14571459
std::string warning;
14581460

14591461
if (score_now >= DISCOURAGEMENT_THRESHOLD && score_before < DISCOURAGEMENT_THRESHOLD) {
14601462
warning = " DISCOURAGE THRESHOLD EXCEEDED";
1461-
peer->m_should_discourage = true;
1463+
peer.m_should_discourage = true;
14621464
}
14631465

14641466
LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s%s\n",
1465-
pnode, score_before, score_now, warning, message_prefixed);
1467+
peer.m_id, score_before, score_now, warning, message_prefixed);
14661468
}
14671469

14681470
bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state,
14691471
bool via_compact_block, const std::string& message)
14701472
{
1473+
PeerRef peer{GetPeerRef(nodeid)};
14711474
switch (state.GetResult()) {
14721475
case BlockValidationResult::BLOCK_RESULT_UNSET:
14731476
break;
14741477
// The node is providing invalid data:
14751478
case BlockValidationResult::BLOCK_CONSENSUS:
14761479
case BlockValidationResult::BLOCK_MUTATED:
14771480
if (!via_compact_block) {
1478-
Misbehaving(nodeid, 100, message);
1481+
if (peer) Misbehaving(*peer, 100, message);
14791482
return true;
14801483
}
14811484
break;
@@ -1490,20 +1493,20 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
14901493
// Discourage outbound (but not inbound) peers if on an invalid chain.
14911494
// Exempt HB compact block peers. Manual connections are always protected from discouragement.
14921495
if (!via_compact_block && !node_state->m_is_inbound) {
1493-
Misbehaving(nodeid, 100, message);
1496+
if (peer) Misbehaving(*peer, 100, message);
14941497
return true;
14951498
}
14961499
break;
14971500
}
14981501
case BlockValidationResult::BLOCK_INVALID_HEADER:
14991502
case BlockValidationResult::BLOCK_CHECKPOINT:
15001503
case BlockValidationResult::BLOCK_INVALID_PREV:
1501-
Misbehaving(nodeid, 100, message);
1504+
if (peer) Misbehaving(*peer, 100, message);
15021505
return true;
15031506
// Conflicting (but not necessarily invalid) data or different policy:
15041507
case BlockValidationResult::BLOCK_MISSING_PREV:
15051508
// TODO: Handle this much more gracefully (10 DoS points is super arbitrary)
1506-
Misbehaving(nodeid, 10, message);
1509+
if (peer) Misbehaving(*peer, 10, message);
15071510
return true;
15081511
case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE:
15091512
case BlockValidationResult::BLOCK_TIME_FUTURE:
@@ -1517,12 +1520,13 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
15171520

15181521
bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, const std::string& message)
15191522
{
1523+
PeerRef peer{GetPeerRef(nodeid)};
15201524
switch (state.GetResult()) {
15211525
case TxValidationResult::TX_RESULT_UNSET:
15221526
break;
15231527
// The node is providing invalid data:
15241528
case TxValidationResult::TX_CONSENSUS:
1525-
Misbehaving(nodeid, 100, message);
1529+
if (peer) Misbehaving(*peer, 100, message);
15261530
return true;
15271531
// Conflicting (but not necessarily invalid) data or different policy:
15281532
case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE:
@@ -2172,12 +2176,12 @@ uint32_t PeerManagerImpl::GetFetchFlags(const CNode& pfrom) const EXCLUSIVE_LOCK
21722176
return nFetchFlags;
21732177
}
21742178

2175-
void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, const CBlock& block, const BlockTransactionsRequest& req)
2179+
void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req)
21762180
{
21772181
BlockTransactions resp(req);
21782182
for (size_t i = 0; i < req.indexes.size(); i++) {
21792183
if (req.indexes[i] >= block.vtx.size()) {
2180-
Misbehaving(pfrom.GetId(), 100, "getblocktxn with out-of-bounds tx indices");
2184+
Misbehaving(peer, 100, "getblocktxn with out-of-bounds tx indices");
21812185
return;
21822186
}
21832187
resp.txn[i] = block.vtx[req.indexes[i]];
@@ -2187,7 +2191,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, const CBlock& block, c
21872191
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCKTXN, resp));
21882192
}
21892193

2190-
void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
2194+
void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
21912195
const std::vector<CBlockHeader>& headers,
21922196
bool via_compact_block)
21932197
{
@@ -2227,15 +2231,15 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
22272231
UpdateBlockAvailability(pfrom.GetId(), headers.back().GetHash());
22282232

22292233
if (nodestate->nUnconnectingHeaders % MAX_UNCONNECTING_HEADERS == 0) {
2230-
Misbehaving(pfrom.GetId(), 20, strprintf("%d non-connecting headers", nodestate->nUnconnectingHeaders));
2234+
Misbehaving(peer, 20, strprintf("%d non-connecting headers", nodestate->nUnconnectingHeaders));
22312235
}
22322236
return;
22332237
}
22342238

22352239
uint256 hashLastBlock;
22362240
for (const CBlockHeader& header : headers) {
22372241
if (!hashLastBlock.IsNull() && header.hashPrevBlock != hashLastBlock) {
2238-
Misbehaving(pfrom.GetId(), 20, "non-continuous headers sequence");
2242+
Misbehaving(peer, 20, "non-continuous headers sequence");
22392243
return;
22402244
}
22412245
hashLastBlock = header.GetHash();
@@ -2998,7 +3002,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
29983002

29993003
if (vAddr.size() > MAX_ADDR_TO_SEND)
30003004
{
3001-
Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size()));
3005+
Misbehaving(*peer, 20, strprintf("%s message size = %u", msg_type, vAddr.size()));
30023006
return;
30033007
}
30043008

@@ -3079,7 +3083,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
30793083
vRecv >> vInv;
30803084
if (vInv.size() > MAX_INV_SZ)
30813085
{
3082-
Misbehaving(pfrom.GetId(), 20, strprintf("inv message size = %u", vInv.size()));
3086+
Misbehaving(*peer, 20, strprintf("inv message size = %u", vInv.size()));
30833087
return;
30843088
}
30853089

@@ -3154,7 +3158,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
31543158
vRecv >> vInv;
31553159
if (vInv.size() > MAX_INV_SZ)
31563160
{
3157-
Misbehaving(pfrom.GetId(), 20, strprintf("getdata message size = %u", vInv.size()));
3161+
Misbehaving(*peer, 20, strprintf("getdata message size = %u", vInv.size()));
31583162
return;
31593163
}
31603164

@@ -3252,7 +3256,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
32523256
// Unlock m_most_recent_block_mutex to avoid cs_main lock inversion
32533257
}
32543258
if (recent_block) {
3255-
SendBlockTransactions(pfrom, *recent_block, req);
3259+
SendBlockTransactions(pfrom, *peer, *recent_block, req);
32563260
return;
32573261
}
32583262

@@ -3270,7 +3274,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
32703274
bool ret = ReadBlockFromDisk(block, pindex, m_chainparams.GetConsensus());
32713275
assert(ret);
32723276

3273-
SendBlockTransactions(pfrom, block, req);
3277+
SendBlockTransactions(pfrom, *peer, block, req);
32743278
return;
32753279
}
32763280
}
@@ -3678,7 +3682,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
36783682
ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact);
36793683
if (status == READ_STATUS_INVALID) {
36803684
RemoveBlockRequest(pindex->GetBlockHash()); // Reset in-flight state in case Misbehaving does not result in a disconnect
3681-
Misbehaving(pfrom.GetId(), 100, "invalid compact block");
3685+
Misbehaving(*peer, 100, "invalid compact block");
36823686
return;
36833687
} else if (status == READ_STATUS_FAILED) {
36843688
// Duplicate txindexes, the block is now in-flight, so just request it
@@ -3805,7 +3809,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
38053809
ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn);
38063810
if (status == READ_STATUS_INVALID) {
38073811
RemoveBlockRequest(resp.blockhash); // Reset in-flight state in case Misbehaving does not result in a disconnect
3808-
Misbehaving(pfrom.GetId(), 100, "invalid compact block/non-matching block transactions");
3812+
Misbehaving(*peer, 100, "invalid compact block/non-matching block transactions");
38093813
return;
38103814
} else if (status == READ_STATUS_FAILED) {
38113815
// Might have collided, fall back to getdata now :(
@@ -3866,7 +3870,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
38663870
// Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks.
38673871
unsigned int nCount = ReadCompactSize(vRecv);
38683872
if (nCount > MAX_HEADERS_RESULTS) {
3869-
Misbehaving(pfrom.GetId(), 20, strprintf("headers message size = %u", nCount));
3873+
Misbehaving(*peer, 20, strprintf("headers message size = %u", nCount));
38703874
return;
38713875
}
38723876
headers.resize(nCount);
@@ -4060,7 +4064,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
40604064
if (!filter.IsWithinSizeConstraints())
40614065
{
40624066
// There is no excuse for sending a too-large filter
4063-
Misbehaving(pfrom.GetId(), 100, "too-large bloom filter");
4067+
Misbehaving(*peer, 100, "too-large bloom filter");
40644068
} else if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) {
40654069
{
40664070
LOCK(tx_relay->m_bloom_filter_mutex);
@@ -4095,7 +4099,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
40954099
}
40964100
}
40974101
if (bad) {
4098-
Misbehaving(pfrom.GetId(), 100, "bad filteradd message");
4102+
Misbehaving(*peer, 100, "bad filteradd message");
40994103
}
41004104
return;
41014105
}

‎src/net_processing.h‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,12 +71,8 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
7171
/** Set the best height */
7272
virtual void SetBestHeight(int height) = 0;
7373

74-
/**
75-
* Increment peer's misbehavior score. If the new value >= DISCOURAGEMENT_THRESHOLD, mark the node
76-
* to be discouraged, meaning the peer might be disconnected and added to the discouragement filter.
77-
* Public for unit testing.
78-
*/
79-
virtual void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) = 0;
74+
/* Public for unit testing. */
75+
virtual void UnitTestMisbehaving(NodeId peer_id, int howmuch) = 0;
8076

8177
/**
8278
* Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound.

‎src/test/denialofservice_tests.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
305305
peerLogic->InitializeNode(nodes[0]);
306306
nodes[0]->fSuccessfullyConnected = true;
307307
connman->AddTestNode(*nodes[0]);
308-
peerLogic->Misbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD, /*message=*/""); // Should be discouraged
308+
peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
309309
{
310310
LOCK(nodes[0]->cs_sendProcessing);
311311
BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
@@ -328,7 +328,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
328328
peerLogic->InitializeNode(nodes[1]);
329329
nodes[1]->fSuccessfullyConnected = true;
330330
connman->AddTestNode(*nodes[1]);
331-
peerLogic->Misbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1, /*message=*/"");
331+
peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1);
332332
{
333333
LOCK(nodes[1]->cs_sendProcessing);
334334
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
@@ -339,7 +339,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
339339
// [1] is not discouraged/disconnected yet.
340340
BOOST_CHECK(!banman->IsDiscouraged(addr[1]));
341341
BOOST_CHECK(!nodes[1]->fDisconnect);
342-
peerLogic->Misbehaving(nodes[1]->GetId(), 1, /*message=*/""); // [1] reaches discouragement threshold
342+
peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), 1); // [1] reaches discouragement threshold
343343
{
344344
LOCK(nodes[1]->cs_sendProcessing);
345345
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
@@ -366,7 +366,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
366366
peerLogic->InitializeNode(nodes[2]);
367367
nodes[2]->fSuccessfullyConnected = true;
368368
connman->AddTestNode(*nodes[2]);
369-
peerLogic->Misbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD, /*message=*/"");
369+
peerLogic->UnitTestMisbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD);
370370
{
371371
LOCK(nodes[2]->cs_sendProcessing);
372372
BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
@@ -411,7 +411,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
411411
peerLogic->InitializeNode(&dummyNode);
412412
dummyNode.fSuccessfullyConnected = true;
413413

414-
peerLogic->Misbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD, /*message=*/"");
414+
peerLogic->UnitTestMisbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD);
415415
{
416416
LOCK(dummyNode.cs_sendProcessing);
417417
BOOST_CHECK(peerLogic->SendMessages(&dummyNode));

0 commit comments

Comments
 (0)