Skip to content

Net: bound automatic peer discouragement #1937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
navidR wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Net: bound automatic peer discouragement #1937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,8 +483,8 @@ std::string HelpMessage(HelpMessageMode mode)

strUsage += HelpMessageGroup(_("Connection options:"));
strUsage += HelpMessageOpt("-addnode=<ip>", _("Add a node to connect to and attempt to keep the connection open"));
strUsage += HelpMessageOpt("-banscore=<n>", strprintf(_("Threshold for disconnecting misbehaving peers (default: %u)"), DEFAULT_BANSCORE_THRESHOLD));
strUsage += HelpMessageOpt("-bantime=<n>", strprintf(_("Number of seconds to keep misbehaving peers from reconnecting (default: %u)"), DEFAULT_MISBEHAVING_BANTIME));
strUsage += HelpMessageOpt("-banscore=<n>", strprintf(_("Threshold for disconnecting and discouraging misbehaving peers (default: %u)"), DEFAULT_BANSCORE_THRESHOLD));
strUsage += HelpMessageOpt("-bantime=<n>", strprintf(_("Default duration (in seconds) of manually configured bans (default: %u)"), DEFAULT_MISBEHAVING_BANTIME));
strUsage += HelpMessageOpt("-bind=<addr>", _("Bind to given address and always listen on it. Use [host]:port notation for IPv6"));
strUsage += HelpMessageOpt("-connect=<ip>", _("Connect only to the specified node(s); -noconnect or -connect=0 alone to disable automatic connections"));
strUsage += HelpMessageOpt("-discover", _("Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)"));
Expand Down
80 changes: 72 additions & 8 deletions src/net.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,13 @@ void CConnman::ClearBanned()
clientInterface->BannedListChanged();
}

bool CConnman::IsBanned(CNetAddr ip)
bool CConnman::IsDiscouraged(const CNetAddr& ip)
{
LOCK(cs_setBanned);
return setDiscouraged.contains(ip.GetAddrBytes());
}

bool CConnman::IsBanned(const CNetAddr& ip)
{
bool fResult = false;
{
Expand All @@ -518,7 +524,7 @@ bool CConnman::IsBanned(CNetAddr ip)
return fResult;
}

bool CConnman::IsBanned(CSubNet subnet)
bool CConnman::IsBanned(const CSubNet& subnet)
{
bool fResult = false;
{
Expand All @@ -539,6 +545,12 @@ void CConnman::Ban(const CNetAddr& addr, const BanReason &banReason, int64_t ban
Ban(subNet, banReason, bantimeoffset, sinceUnixEpoch);
}

void CConnman::Discourage(const CNetAddr& addr)
{
LOCK(cs_setBanned);
setDiscouraged.insert(addr.GetAddrBytes());
}

void CConnman::Ban(const CSubNet& subNet, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
CBanEntry banEntry(GetTime());
banEntry.banReason = banReason;
Expand Down Expand Up @@ -973,6 +985,7 @@ struct NodeEvictionCandidate
bool fBloomFilter;
CAddress addr;
uint64_t nKeyedNetGroup;
bool fPreferEvict;
};

static bool ReverseCompareNodeMinPingTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
Expand Down Expand Up @@ -1048,9 +1061,10 @@ bool CConnman::AttemptToEvictConnection()
}

NodeEvictionCandidate candidate = {node->id, node->nTimeConnected, node->nMinPingUsecTime,
node->nLastBlockTime, node->nLastTXTime,
(node->nServices & nRelevantServices) == nRelevantServices,
node->fRelayTxes, node->pfilter != NULL, node->addr, node->nKeyedNetGroup};
node->nLastBlockTime, node->nLastTXTime,
(node->nServices & nRelevantServices) == nRelevantServices,
node->fRelayTxes, node->pfilter != NULL, node->addr, node->nKeyedNetGroup,
node->fPreferEvict};
vEvictionCandidates.push_back(candidate);
}
}
Expand Down Expand Up @@ -1094,6 +1108,16 @@ bool CConnman::AttemptToEvictConnection()

if (vEvictionCandidates.empty()) return false;

// If any remaining peers are preferred for eviction, consider only them.
if (std::any_of(vEvictionCandidates.begin(), vEvictionCandidates.end(), [](const NodeEvictionCandidate& node) {
return node.fPreferEvict;
})) {
vEvictionCandidates.erase(std::remove_if(vEvictionCandidates.begin(), vEvictionCandidates.end(), [](const NodeEvictionCandidate& node) {
return !node.fPreferEvict;
}),
vEvictionCandidates.end());
}

// Identify the network group with the most connections and youngest member.
// (vEvictionCandidates is already sorted by reverse connect time)
uint64_t naMostConnections;
Expand Down Expand Up @@ -1255,8 +1279,15 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
return;
}

if (nInbound - nVerifiedInboundMasternodes >= nMaxInbound)
{
bool discouraged = IsDiscouraged(addr);
int nCountedInbound = nInbound - nVerifiedInboundMasternodes;
if (discouraged && !whitelisted && nCountedInbound + 1 >= nMaxInbound) {
LogPrintf("connection from %s dropped (discouraged)\n", addr.ToString());
CloseSocket(hSocket);
return;
}

if (nCountedInbound >= nMaxInbound) {
if (!AttemptToEvictConnection()) {
// No connection to evict, disconnect the new connection
LogPrint("net", "failed to find an eviction candidate - connection dropped (full)\n");
Expand All @@ -1271,6 +1302,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addr, CalculateKeyedNetGroup(addr), nonce, "", true, hListenSocket.is_onion_listener);
pnode->AddRef();
pnode->fWhitelisted = whitelisted;
pnode->fPreferEvict = discouraged;
GetNodeSignals().InitializeNode(pnode, *this);

LogPrint("net", "connection from %s accepted%s\n", addr.ToString(),
Expand Down Expand Up @@ -2333,7 +2365,7 @@ bool CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
bool fAllowLocal = fMasternodeMode;
if (!pszDest) {
// banned or exact match?
if (IsBanned(addrConnect) || FindNode(addrConnect.ToStringIPPort()))
if (IsBanned(addrConnect) || IsDiscouraged(addrConnect) || FindNode(addrConnect.ToStringIPPort()))
return false;
// local and not a connection to itself?
if (!fAllowLocal && IsLocal(addrConnect))
Expand Down Expand Up @@ -3272,6 +3304,37 @@ bool CConnman::DisconnectNode(const std::string& strNode)
}
return false;
}

bool CConnman::DisconnectNode(const CSubNet& subnet)
{
bool disconnected = false;
LOCK(cs_vNodes);
for (CNode* pnode : vNodes) {
if (subnet.Match(pnode->addr)) {
pnode->fDisconnect = true;
disconnected = true;
}
}
return disconnected;
}

bool CConnman::DisconnectNode(const CNetAddr& addr)
{
if (!addr.IsValid()) {
return false;
}

bool disconnected = false;
LOCK(cs_vNodes);
for (CNode* pnode : vNodes) {
if (static_cast<const CNetAddr&>(pnode->addr) == addr) {
pnode->fDisconnect = true;
disconnected = true;
}
}
return disconnected;
}

bool CConnman::DisconnectNode(NodeId id)
{
LOCK(cs_vNodes);
Expand Down Expand Up @@ -3499,6 +3562,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
nLastWarningTime = 0;
strSubVer = "";
fWhitelisted = false;
fPreferEvict = false;
fOneShot = false;
fAddnode = false;
fClient = false; // set by version message
Expand Down
15 changes: 12 additions & 3 deletions src/net.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ class CConnman

// Denial-of-service detection/prevention
// The idea is to detect peers that are behaving
// badly and disconnect/ban them, but do it in a
// badly and disconnect/discourage them, but do it in a
// one-coding-mistake-won't-shatter-the-entire-network
// way.
// IMPORTANT: There should be nothing I can give a
Expand All @@ -348,11 +348,16 @@ class CConnman
// dangerous, because it can cause a network split
// between nodes running old code and nodes running
// new code.
// Manual bans are persisted and reject connections. Automatic
// discouragement is bounded, avoids outbound connections, and makes
// inbound peers preferred for eviction.
void Ban(const CNetAddr& netAddr, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
void Ban(const CSubNet& subNet, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
void Discourage(const CNetAddr& netAddr);
void ClearBanned(); // needed for unit testing
bool IsBanned(CNetAddr ip);
bool IsBanned(CSubNet subnet);
bool IsBanned(const CNetAddr& ip);
bool IsBanned(const CSubNet& subnet);
bool IsDiscouraged(const CNetAddr& ip);
bool Unban(const CNetAddr &ip);
bool Unban(const CSubNet &ip);
void GetBanned(banmap_t &banmap);
Expand All @@ -376,6 +381,8 @@ class CConnman
size_t GetNodeCount(NumConnections num);
void GetNodeStats(std::vector<CNodeStats>& vstats);
bool DisconnectNode(const std::string& node);
bool DisconnectNode(const CSubNet& subnet);
bool DisconnectNode(const CNetAddr& addr);
bool DisconnectNode(NodeId id);

unsigned int GetSendBufferSize() const;
Expand Down Expand Up @@ -506,6 +513,7 @@ class CConnman
mutable CCriticalSection cs_vhListenSocket;
std::atomic<bool> fNetworkActive;
banmap_t setBanned;
CRollingBloomFilter setDiscouraged{50000, 0.000001};

@codeant-ai codeant-ai Bot Aug 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: setDiscouraged is never reset by ClearBanned, so repeated tests and users clearing bans retain automatic discouragement for previously recorded addresses. [state/lifecycle]

Assessment: 🟠 Major · 🔁 Occurrence: Sometimes

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** src/net.h
**Line:** 516:516
**Comment:**
	*State Lifecycle: `setDiscouraged` is never reset by `ClearBanned`, so repeated tests and users clearing bans retain automatic discouragement for previously recorded addresses.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

CCriticalSection cs_setBanned;
bool setBannedIsDirty;
bool fAddressesInitialized;
Expand Down Expand Up @@ -772,6 +780,7 @@ class CNode
std::string strSubVer, cleanSubVer;
CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer
bool fWhitelisted; // This peer can bypass DoS banning.
bool fPreferEvict; // This peer is preferred for eviction.
bool fFeeler; // If true this node is being used as a short lived feeler.
bool fOneShot;
bool fAddnode;
Expand Down
15 changes: 8 additions & 7 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ struct CNodeState {
bool fCurrentlyConnected;
//! Accumulated misbehaviour score for this peer.
int nMisbehavior;
//! Whether this peer should be disconnected and banned (unless whitelisted).
//! Whether this peer should be disconnected and discouraged (unless whitelisted).
bool fShouldBan;
//! String name of this peer (debugging/logging purposes).
const std::string name;
Expand Down Expand Up @@ -744,7 +744,7 @@ void Misbehaving(NodeId pnode, int howmuch)
int banscore = GetArg("-banscore", DEFAULT_BANSCORE_THRESHOLD);
if (state->nMisbehavior >= banscore && state->nMisbehavior - howmuch < banscore)
{
LogPrintf("%s: %s peer=%d (%d -> %d) BAN THRESHOLD EXCEEDED\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior);
LogPrintf("%s: %s peer=%d (%d -> %d) DISCOURAGEMENT THRESHOLD EXCEEDED\n", __func__, state->name, pnode, state->nMisbehavior - howmuch, state->nMisbehavior);
state->fShouldBan = true;
} else
LogPrintf("%s: %s peer=%d (%d -> %d)\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior);
Expand Down Expand Up @@ -3177,11 +3177,12 @@ static bool SendRejectsAndCheckIfBanned(CNode* pnode, CConnman& connman)
llmq::quorumSigSharesManager->MarkNodeBanned(pnode->GetId());
}
pnode->fDisconnect = true;
if (pnode->addr.IsLocal())
LogPrintf("Warning: not banning local peer %s!\n", pnode->addr.ToString());
else
{
connman.Ban(pnode->addr, BanReasonNodeMisbehaving);
if (pnode->addr.IsLocal()) {
LogPrintf("Warning: not discouraging local peer %s!\n", pnode->addr.ToString());
} else {
LogPrintf("Disconnecting and discouraging peer %s!\n", pnode->addr.ToString());
connman.Discourage(pnode->addr);

@codeant-ai codeant-ai Bot Aug 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: For an invalid peer address, this records the invalid address byte key; unresolved peers share that key, so one offender discourages every invalid address. [logic error]

Assessment: 🟠 Major · 🔁 Occurrence: Rarely

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** src/net_processing.cpp
**Line:** 3184:3184
**Comment:**
	*Logic Error: For an invalid peer address, this records the invalid address byte key; unresolved peers share that key, so one offender discourages every invalid address.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

connman.DisconnectNode(pnode->addr);
}
}
return true;
Expand Down
Loading
Loading