Skip to content

Commit

Permalink
net_processing: remove Misbehavior score and increments
Browse files Browse the repository at this point in the history 
This is now all unused.
  • Loading branch information
@sipa
sipa committed Mar 12, 2024
1 parent 3cbf1f3 commit e7ccaa0
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 51 deletions.
2 changes: 1 addition & 1 deletion src/banman.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class CSubNet;
// disk on shutdown and reloaded on startup. Banning can be used to
// prevent connections with spy nodes or other griefers.
//
// 2. Discouragement. If a peer misbehaves enough (see Misbehaving() in
// 2. Discouragement. If a peer misbehaves (see Misbehaving() in
// net_processing.cpp), we'll mark that address as discouraged. We still allow
// incoming connections from them, but they're preferred for eviction when
// we receive new incoming connections. We never make outgoing connections to
Expand Down
68 changes: 25 additions & 43 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,6 @@ struct Peer {

/** 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 NetPermissionFlags::NoBan permission). */
bool m_should_discourage GUARDED_BY(m_misbehavior_mutex){false};

Expand Down Expand Up @@ -516,7 +514,7 @@ class PeerManagerImpl final : public PeerManager
m_best_height = height;
m_best_block_time = time;
};
void UnitTestMisbehaving(NodeId peer_id, int howmuch) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), howmuch, ""); };
void UnitTestMisbehaving(NodeId peer_id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), ""); };
void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv,
const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
Expand All @@ -541,11 +539,9 @@ class PeerManagerImpl final : public PeerManager
* May return an empty shared_ptr if the Peer object can't be found. */
PeerRef RemovePeer(NodeId id) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);

/**
* 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(Peer& peer, int howmuch, const std::string& message);
/** Mark a peer as misbehaving, which will cause it to be disconnected and its
* address discouraged. */
void Misbehaving(Peer& peer, const std::string& message);

/**
* Potentially mark a node discouraged based on the contents of a BlockValidationState object
Expand Down Expand Up @@ -1621,7 +1617,6 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
void PeerManagerImpl::FinalizeNode(const CNode& node)
{
NodeId nodeid = node.GetId();
int misbehavior{0};
{
LOCK(cs_main);
{
Expand All @@ -1632,7 +1627,6 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
// destructed.
PeerRef peer = RemovePeer(nodeid);
assert(peer != nullptr);
misbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->m_misbehavior_score);
m_wtxid_relay_peers -= peer->m_wtxid_relay;
assert(m_wtxid_relay_peers >= 0);
}
Expand Down Expand Up @@ -1675,7 +1669,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
assert(m_orphanage.Size() == 0);
}
} // cs_main
if (node.fSuccessfullyConnected && misbehavior == 0 &&
if (node.fSuccessfullyConnected &&
!node.IsBlockOnlyConn() && !node.IsInboundConn()) {
// Only change visible addrman state for full outbound peers. We don't
// call Connected() for feeler connections since they don't have
Expand Down Expand Up @@ -1787,25 +1781,13 @@ void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % m_opts.max_extra_txs;
}

void PeerManagerImpl::Misbehaving(Peer& peer, int howmuch, const std::string& message)
void PeerManagerImpl::Misbehaving(Peer& peer, const std::string& message)
{
assert(howmuch > 0);

LOCK(peer.m_misbehavior_mutex);
const int score_before{peer.m_misbehavior_score};
peer.m_misbehavior_score += howmuch;
const int score_now{peer.m_misbehavior_score};

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

if (score_now >= DISCOURAGEMENT_THRESHOLD && score_before < DISCOURAGEMENT_THRESHOLD) {
warning = " DISCOURAGE THRESHOLD EXCEEDED";
peer.m_should_discourage = true;
}

LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d)%s%s\n",
peer.m_id, score_before, score_now, warning, message_prefixed);
peer.m_should_discourage = true;
LogPrint(BCLog::NET, "Misbehaving: peer=%d%s\n", peer.m_id, message_prefixed);
}

bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state,
Expand All @@ -1823,7 +1805,7 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
case BlockValidationResult::BLOCK_CONSENSUS:
case BlockValidationResult::BLOCK_MUTATED:
if (!via_compact_block) {
if (peer) Misbehaving(*peer, 100, message);
if (peer) Misbehaving(*peer, message);
return true;
}
break;
Expand All @@ -1838,19 +1820,19 @@ bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
// Discourage outbound (but not inbound) peers if on an invalid chain.
// Exempt HB compact block peers. Manual connections are always protected from discouragement.
if (!via_compact_block && !node_state->m_is_inbound) {
if (peer) Misbehaving(*peer, 100, message);
if (peer) Misbehaving(*peer, message);
return true;
}
break;
}
case BlockValidationResult::BLOCK_INVALID_HEADER:
case BlockValidationResult::BLOCK_CHECKPOINT:
case BlockValidationResult::BLOCK_INVALID_PREV:
if (peer) Misbehaving(*peer, 100, message);
if (peer) Misbehaving(*peer, message);
return true;
// Conflicting (but not necessarily invalid) data or different policy:
case BlockValidationResult::BLOCK_MISSING_PREV:
if (peer) Misbehaving(*peer, 100, message);
if (peer) Misbehaving(*peer, message);
return true;
case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE:
case BlockValidationResult::BLOCK_TIME_FUTURE:
Expand All @@ -1870,7 +1852,7 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat
break;
// The node is providing invalid data:
case TxValidationResult::TX_CONSENSUS:
if (peer) Misbehaving(*peer, 100, "");
if (peer) Misbehaving(*peer, "");
return true;
// Conflicting (but not necessarily invalid) data or different policy:
case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE:
Expand Down Expand Up @@ -2530,7 +2512,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
BlockTransactions resp(req);
for (size_t i = 0; i < req.indexes.size(); i++) {
if (req.indexes[i] >= block.vtx.size()) {
Misbehaving(peer, 100, "getblocktxn with out-of-bounds tx indices");
Misbehaving(peer, "getblocktxn with out-of-bounds tx indices");
return;
}
resp.txn[i] = block.vtx[req.indexes[i]];
Expand All @@ -2543,13 +2525,13 @@ bool PeerManagerImpl::CheckHeadersPoW(const std::vector<CBlockHeader>& headers,
{
// Do these headers have proof-of-work matching what's claimed?
if (!HasValidProofOfWork(headers, consensusParams)) {
Misbehaving(peer, 100, "header with invalid proof of work");
Misbehaving(peer, "header with invalid proof of work");
return false;
}

// Are these headers connected to each other?
if (!CheckHeadersAreContinuous(headers)) {
Misbehaving(peer, 100, "non-continuous headers sequence");
Misbehaving(peer, "non-continuous headers sequence");
return false;
}
return true;
Expand Down Expand Up @@ -3329,7 +3311,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn);
if (status == READ_STATUS_INVALID) {
RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect
Misbehaving(peer, 100, "invalid compact block/non-matching block transactions");
Misbehaving(peer, "invalid compact block/non-matching block transactions");
return;
} else if (status == READ_STATUS_FAILED) {
if (first_in_flight) {
Expand Down Expand Up @@ -3813,7 +3795,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,

if (vAddr.size() > MAX_ADDR_TO_SEND)
{
Misbehaving(*peer, 100, strprintf("%s message size = %u", msg_type, vAddr.size()));
Misbehaving(*peer, strprintf("%s message size = %u", msg_type, vAddr.size()));
return;
}

Expand Down Expand Up @@ -3895,7 +3877,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
vRecv >> vInv;
if (vInv.size() > MAX_INV_SZ)
{
Misbehaving(*peer, 100, strprintf("inv message size = %u", vInv.size()));
Misbehaving(*peer, strprintf("inv message size = %u", vInv.size()));
return;
}

Expand Down Expand Up @@ -3987,7 +3969,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
vRecv >> vInv;
if (vInv.size() > MAX_INV_SZ)
{
Misbehaving(*peer, 100, strprintf("getdata message size = %u", vInv.size()));
Misbehaving(*peer, strprintf("getdata message size = %u", vInv.size()));
return;
}

Expand Down Expand Up @@ -4533,7 +4515,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact);
if (status == READ_STATUS_INVALID) {
RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect
Misbehaving(*peer, 100, "invalid compact block");
Misbehaving(*peer, "invalid compact block");
return;
} else if (status == READ_STATUS_FAILED) {
if (first_in_flight) {
Expand Down Expand Up @@ -4678,7 +4660,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
// 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) {
Misbehaving(*peer, 100, strprintf("headers message size = %u", nCount));
Misbehaving(*peer, strprintf("headers message size = %u", nCount));
return;
}
headers.resize(nCount);
Expand Down Expand Up @@ -4725,7 +4707,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
if (prev_block && IsBlockMutated(/*block=*/*pblock,
/*check_witness_root=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT))) {
LogDebug(BCLog::NET, "Received mutated block from peer=%d\n", peer->m_id);
Misbehaving(*peer, 100, "mutated block");
Misbehaving(*peer, "mutated block");
WITH_LOCK(cs_main, RemoveBlockRequest(pblock->GetHash(), peer->m_id));
return;
}
Expand Down Expand Up @@ -4906,7 +4888,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
if (!filter.IsWithinSizeConstraints())
{
// There is no excuse for sending a too-large filter
Misbehaving(*peer, 100, "too-large bloom filter");
Misbehaving(*peer, "too-large bloom filter");
} else if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) {
{
LOCK(tx_relay->m_bloom_filter_mutex);
Expand Down Expand Up @@ -4942,7 +4924,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
}
}
if (bad) {
Misbehaving(*peer, 100, "bad filteradd message");
Misbehaving(*peer, "bad filteradd message");
}
return;
}
Expand Down
4 changes: 1 addition & 3 deletions src/net_processing.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ static const uint32_t DEFAULT_MAX_ORPHAN_TRANSACTIONS{100};
static const uint32_t DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN{100};
static const bool DEFAULT_PEERBLOOMFILTERS = false;
static const bool DEFAULT_PEERBLOCKFILTERS = false;
/** Threshold for marking a node to be discouraged, e.g. disconnected and added to the discouragement filter. */
static const int DISCOURAGEMENT_THRESHOLD{100};
/** Maximum number of outstanding CMPCTBLOCK requests for the same block. */
static const unsigned int MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3;

Expand Down Expand Up @@ -96,7 +94,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
virtual void SetBestBlock(int height, std::chrono::seconds time) = 0;

/* Public for unit testing. */
virtual void UnitTestMisbehaving(NodeId peer_id, int howmuch) = 0;
virtual void UnitTestMisbehaving(NodeId peer_id) = 0;

/**
* Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound.
Expand Down
8 changes: 4 additions & 4 deletions src/test/denialofservice_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
peerLogic->InitializeNode(*nodes[0], NODE_NETWORK);
nodes[0]->fSuccessfullyConnected = true;
connman->AddTestNode(*nodes[0]);
peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
peerLogic->UnitTestMisbehaving(nodes[0]->GetId()); // Should be discouraged
BOOST_CHECK(peerLogic->SendMessages(nodes[0]));

BOOST_CHECK(banman->IsDiscouraged(addr[0]));
Expand All @@ -359,7 +359,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
// [1] is not discouraged/disconnected yet.
BOOST_CHECK(!banman->IsDiscouraged(addr[1]));
BOOST_CHECK(!nodes[1]->fDisconnect);
peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD); // [1] reaches discouragement threshold
peerLogic->UnitTestMisbehaving(nodes[1]->GetId());
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
// Expect both [0] and [1] to be discouraged/disconnected now.
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
Expand All @@ -382,7 +382,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
peerLogic->InitializeNode(*nodes[2], NODE_NETWORK);
nodes[2]->fSuccessfullyConnected = true;
connman->AddTestNode(*nodes[2]);
peerLogic->UnitTestMisbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD);
peerLogic->UnitTestMisbehaving(nodes[2]->GetId());
BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
BOOST_CHECK(banman->IsDiscouraged(addr[1]));
Expand Down Expand Up @@ -424,7 +424,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
peerLogic->InitializeNode(dummyNode, NODE_NETWORK);
dummyNode.fSuccessfullyConnected = true;

peerLogic->UnitTestMisbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD);
peerLogic->UnitTestMisbehaving(dummyNode.GetId());
BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
BOOST_CHECK(banman->IsDiscouraged(addr));

Expand Down

0 comments on commit e7ccaa0

Please sign in to comment.