Skip to content

Commit 2ad5838

Browse files
sipasipa
committed
Clean up separated ban/discourage interface
1 parent b691f2d commit 2ad5838

File tree

13 files changed

+77
-113
lines changed

13 files changed

+77
-113
lines changed

‎src/addrdb.h

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -17,21 +17,13 @@ class CSubNet;
1717
class CAddrMan;
1818
class CDataStream;
1919

20-
typedef enum BanReason
21-
{
22-
BanReasonUnknown = 0,
23-
BanReasonNodeMisbehaving = 1,
24-
BanReasonManuallyAdded = 2
25-
} BanReason;
26-
2720
class CBanEntry
2821
{
2922
public:
3023
static const int CURRENT_VERSION=1;
3124
int nVersion;
3225
int64_t nCreateTime;
3326
int64_t nBanUntil;
34-
uint8_t banReason;
3527

3628
CBanEntry()
3729
{
@@ -44,31 +36,17 @@ class CBanEntry
4436
nCreateTime = nCreateTimeIn;
4537
}
4638

47-
explicit CBanEntry(int64_t n_create_time_in, BanReason ban_reason_in) : CBanEntry(n_create_time_in)
39+
SERIALIZE_METHODS(CBanEntry, obj)
4840
{
49-
banReason = ban_reason_in;
41+
uint8_t ban_reason = 2; //! For backward compatibility
42+
READWRITE(obj.nVersion, obj.nCreateTime, obj.nBanUntil, ban_reason);
5043
}
5144

52-
SERIALIZE_METHODS(CBanEntry, obj) { READWRITE(obj.nVersion, obj.nCreateTime, obj.nBanUntil, obj.banReason); }
53-
5445
void SetNull()
5546
{
5647
nVersion = CBanEntry::CURRENT_VERSION;
5748
nCreateTime = 0;
5849
nBanUntil = 0;
59-
banReason = BanReasonUnknown;
60-
}
61-
62-
std::string banReasonToString() const
63-
{
64-
switch (banReason) {
65-
case BanReasonNodeMisbehaving:
66-
return "node misbehaving";
67-
case BanReasonManuallyAdded:
68-
return "manually added";
69-
default:
70-
return "unknown";
71-
}
7250
}
7351
};
7452

‎src/banman.cpp

Lines changed: 16 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -68,30 +68,16 @@ void BanMan::ClearBanned()
6868
if (m_client_interface) m_client_interface->BannedListChanged();
6969
}
7070

71-
int BanMan::IsBannedLevel(CNetAddr net_addr)
71+
bool BanMan::IsDiscouraged(const CNetAddr& net_addr)
7272
{
73-
// Returns the most severe level of banning that applies to this address.
74-
// 0 - Not banned
75-
// 1 - Automatic misbehavior ban
76-
// 2 - Any other ban
77-
auto current_time = GetTime();
7873
LOCK(m_cs_banned);
79-
for (const auto& it : m_banned) {
80-
CSubNet sub_net = it.first;
81-
CBanEntry ban_entry = it.second;
82-
83-
if (current_time < ban_entry.nBanUntil && sub_net.Match(net_addr)) {
84-
return 2;
85-
}
86-
}
87-
return m_discouraged.contains(net_addr.GetAddrBytes()) ? 1 : 0;
74+
return m_discouraged.contains(net_addr.GetAddrBytes());
8875
}
8976

90-
bool BanMan::IsBanned(CNetAddr net_addr)
77+
bool BanMan::IsBanned(const CNetAddr& net_addr)
9178
{
9279
auto current_time = GetTime();
9380
LOCK(m_cs_banned);
94-
if (m_discouraged.contains(net_addr.GetAddrBytes())) return true;
9581
for (const auto& it : m_banned) {
9682
CSubNet sub_net = it.first;
9783
CBanEntry ban_entry = it.second;
@@ -103,7 +89,7 @@ bool BanMan::IsBanned(CNetAddr net_addr)
10389
return false;
10490
}
10591

106-
bool BanMan::IsBanned(CSubNet sub_net)
92+
bool BanMan::IsBanned(const CSubNet& sub_net)
10793
{
10894
auto current_time = GetTime();
10995
LOCK(m_cs_banned);
@@ -117,21 +103,21 @@ bool BanMan::IsBanned(CSubNet sub_net)
117103
return false;
118104
}
119105

120-
void BanMan::Ban(const CNetAddr& net_addr, const BanReason& ban_reason, int64_t ban_time_offset, bool since_unix_epoch)
106+
void BanMan::Ban(const CNetAddr& net_addr, int64_t ban_time_offset, bool since_unix_epoch)
121107
{
122-
if (ban_reason == BanReasonNodeMisbehaving) {
123-
LOCK(m_cs_banned);
124-
m_discouraged.insert(net_addr.GetAddrBytes());
125-
return;
126-
}
127108
CSubNet sub_net(net_addr);
128-
Ban(sub_net, ban_reason, ban_time_offset, since_unix_epoch);
109+
Ban(sub_net, ban_time_offset, since_unix_epoch);
129110
}
130111

131-
void BanMan::Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset, bool since_unix_epoch)
112+
void BanMan::Discourage(const CNetAddr& net_addr)
132113
{
133-
assert(ban_reason == BanReasonManuallyAdded);
134-
CBanEntry ban_entry(GetTime(), ban_reason);
114+
LOCK(m_cs_banned);
115+
m_discouraged.insert(net_addr.GetAddrBytes());
116+
}
117+
118+
void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_unix_epoch)
119+
{
120+
CBanEntry ban_entry(GetTime());
135121

136122
int64_t normalized_ban_time_offset = ban_time_offset;
137123
bool normalized_since_unix_epoch = since_unix_epoch;
@@ -151,8 +137,8 @@ void BanMan::Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ba
151137
}
152138
if (m_client_interface) m_client_interface->BannedListChanged();
153139

154-
//store banlist to disk immediately if user requested ban
155-
if (ban_reason == BanReasonManuallyAdded) DumpBanlist();
140+
//store banlist to disk immediately
141+
DumpBanlist();
156142
}
157143

158144
bool BanMan::Unban(const CNetAddr& net_addr)

‎src/banman.h

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,20 @@ class BanMan
5959
public:
6060
~BanMan();
6161
BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
62-
void Ban(const CNetAddr& net_addr, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
63-
void Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
62+
void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
63+
void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
64+
void Discourage(const CNetAddr& net_addr);
6465
void ClearBanned();
65-
int IsBannedLevel(CNetAddr net_addr);
66-
bool IsBanned(CNetAddr net_addr);
67-
bool IsBanned(CSubNet sub_net);
66+
67+
//! Return whether net_addr is banned
68+
bool IsBanned(const CNetAddr& net_addr);
69+
70+
//! Return whether sub_net is exactly banned
71+
bool IsBanned(const CSubNet& sub_net);
72+
73+
//! Return whether net_addr is discouraged.
74+
bool IsDiscouraged(const CNetAddr& net_addr);
75+
6876
bool Unban(const CNetAddr& net_addr);
6977
bool Unban(const CSubNet& sub_net);
7078
void GetBanned(banmap_t& banmap);

‎src/interfaces/node.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,10 +146,10 @@ class NodeImpl : public Node
146146
}
147147
return false;
148148
}
149-
bool ban(const CNetAddr& net_addr, BanReason reason, int64_t ban_time_offset) override
149+
bool ban(const CNetAddr& net_addr, int64_t ban_time_offset) override
150150
{
151151
if (m_context.banman) {
152-
m_context.banman->Ban(net_addr, reason, ban_time_offset);
152+
m_context.banman->Ban(net_addr, ban_time_offset);
153153
return true;
154154
}
155155
return false;

‎src/interfaces/node.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ class Node
122122
virtual bool getBanned(banmap_t& banmap) = 0;
123123

124124
//! Ban node.
125-
virtual bool ban(const CNetAddr& net_addr, BanReason reason, int64_t ban_time_offset) = 0;
125+
virtual bool ban(const CNetAddr& net_addr, int64_t ban_time_offset) = 0;
126126

127127
//! Unban node.
128128
virtual bool unban(const CSubNet& ip) = 0;

‎src/net.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,18 +1010,18 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
10101010
// on all platforms. Set it again here just to be sure.
10111011
SetSocketNoDelay(hSocket);
10121012

1013-
int bannedlevel = m_banman ? m_banman->IsBannedLevel(addr) : 0;
1014-
10151013
// Don't accept connections from banned peers.
1016-
if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && bannedlevel == 2)
1014+
bool banned = m_banman->IsBanned(addr);
1015+
if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && banned)
10171016
{
10181017
LogPrint(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToString());
10191018
CloseSocket(hSocket);
10201019
return;
10211020
}
10221021

10231022
// Only accept connections from discouraged peers if our inbound slots aren't (almost) full.
1024-
if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && nInbound + 1 >= nMaxInbound && bannedlevel >= 1)
1023+
bool discouraged = m_banman->IsDiscouraged(addr);
1024+
if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && nInbound + 1 >= nMaxInbound && discouraged)
10251025
{
10261026
LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToString());
10271027
CloseSocket(hSocket);
@@ -1051,7 +1051,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
10511051
pnode->m_permissionFlags = permissionFlags;
10521052
// If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility)
10531053
pnode->m_legacyWhitelisted = legacyWhitelisted;
1054-
pnode->m_prefer_evict = bannedlevel > 0;
1054+
pnode->m_prefer_evict = discouraged;
10551055
m_msgproc->InitializeNode(pnode);
10561056

10571057
LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString());
@@ -2052,10 +2052,10 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
20522052
return;
20532053
}
20542054
if (!pszDest) {
2055-
if (IsLocal(addrConnect) ||
2056-
FindNode(static_cast<CNetAddr>(addrConnect)) || (m_banman && m_banman->IsBanned(addrConnect)) ||
2057-
FindNode(addrConnect.ToStringIPPort()))
2055+
bool banned_or_discouraged = m_banman && (m_banman->IsDiscouraged(addrConnect) || m_banman->IsBanned(addrConnect));
2056+
if (IsLocal(addrConnect) || FindNode(static_cast<CNetAddr>(addrConnect)) || banned_or_discouraged || FindNode(addrConnect.ToStringIPPort())) {
20582057
return;
2058+
}
20592059
} else if (FindNode(std::string(pszDest)))
20602060
return;
20612061

‎src/net_permissions.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ enum NetPermissionFlags
2424
// Always relay transactions from this peer, even if already in mempool
2525
// Keep parameter interaction: forcerelay implies relay
2626
PF_FORCERELAY = (1U << 2) | PF_RELAY,
27-
// Can't be banned for misbehavior
27+
// Can't be banned/disconnected/discouraged for misbehavior
2828
PF_NOBAN = (1U << 4),
2929
// Can query the mempool
3030
PF_MEMPOOL = (1U << 5),

‎src/net_processing.cpp

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,8 @@ struct CNodeState {
252252
bool fCurrentlyConnected;
253253
//! Accumulated misbehaviour score for this peer.
254254
int nMisbehavior;
255-
//! Whether this peer should be disconnected and banned (unless whitelisted).
256-
bool fShouldBan;
255+
//! Whether this peer should be disconnected and marked as discouraged (unless whitelisted with noban).
256+
bool m_should_discourage;
257257
//! String name of this peer (debugging/logging purposes).
258258
const std::string name;
259259
//! The best known block we know this peer has announced.
@@ -404,7 +404,7 @@ struct CNodeState {
404404
{
405405
fCurrentlyConnected = false;
406406
nMisbehavior = 0;
407-
fShouldBan = false;
407+
m_should_discourage = false;
408408
pindexBestKnownBlock = nullptr;
409409
hashLastUnknownBlock.SetNull();
410410
pindexLastCommonBlock = nullptr;
@@ -1036,7 +1036,7 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV
10361036
if (state->nMisbehavior >= banscore && state->nMisbehavior - howmuch < banscore)
10371037
{
10381038
LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed);
1039-
state->fShouldBan = true;
1039+
state->m_should_discourage = true;
10401040
} else
10411041
LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d)%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed);
10421042
}
@@ -2476,7 +2476,8 @@ void ProcessMessage(
24762476
if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
24772477
addr.nTime = nNow - 5 * 24 * 60 * 60;
24782478
pfrom.AddAddressKnown(addr);
2479-
if (banman->IsBanned(addr)) continue; // Do not process banned addresses beyond remembering we received them
2479+
if (banman->IsDiscouraged(addr)) continue; // Do not process banned/discouraged addresses beyond remembering we received them
2480+
if (banman->IsBanned(addr)) continue;
24802481
bool fReachable = IsReachable(addr);
24812482
if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
24822483
{
@@ -3329,7 +3330,7 @@ void ProcessMessage(
33293330
std::vector<CAddress> vAddr = connman->GetAddresses();
33303331
FastRandomContext insecure_rand;
33313332
for (const CAddress &addr : vAddr) {
3332-
if (!banman->IsBanned(addr)) {
3333+
if (!banman->IsDiscouraged(addr) && !banman->IsBanned(addr)) {
33333334
pfrom.PushAddress(addr, insecure_rand);
33343335
}
33353336
}
@@ -3564,26 +3565,26 @@ void ProcessMessage(
35643565
return;
35653566
}
35663567

3567-
bool PeerLogicValidation::CheckIfBanned(CNode& pnode)
3568+
bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
35683569
{
35693570
AssertLockHeld(cs_main);
35703571
CNodeState &state = *State(pnode.GetId());
35713572

3572-
if (state.fShouldBan) {
3573-
state.fShouldBan = false;
3574-
if (pnode.HasPermission(PF_NOBAN))
3573+
if (state.m_should_discourage) {
3574+
state.m_should_discourage = false;
3575+
if (pnode.HasPermission(PF_NOBAN)) {
35753576
LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode.addr.ToString());
3576-
else if (pnode.m_manual_connection)
3577+
} else if (pnode.m_manual_connection) {
35773578
LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode.addr.ToString());
3578-
else if (pnode.addr.IsLocal()) {
3579+
} else if (pnode.addr.IsLocal()) {
35793580
// Disconnect but don't discourage this local node
35803581
LogPrintf("Warning: disconnecting but not discouraging local peer %s!\n", pnode.addr.ToString());
35813582
pnode.fDisconnect = true;
35823583
} else {
3583-
// Disconnect and ban all nodes sharing the address
3584+
// Disconnect and discourage all nodes sharing the address
35843585
LogPrintf("Disconnecting and discouraging peer %s!\n", pnode.addr.ToString());
35853586
if (m_banman) {
3586-
m_banman->Ban(pnode.addr, BanReasonNodeMisbehaving);
3587+
m_banman->Discourage(pnode.addr);
35873588
}
35883589
connman->DisconnectNode(pnode.addr);
35893590
}
@@ -3683,7 +3684,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
36833684
}
36843685

36853686
LOCK(cs_main);
3686-
CheckIfBanned(*pfrom);
3687+
MaybeDiscourageAndDisconnect(*pfrom);
36873688

36883689
return fMoreWork;
36893690
}
@@ -3886,7 +3887,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
38863887
if (!lockMain)
38873888
return true;
38883889

3889-
if (CheckIfBanned(*pto)) return true;
3890+
if (MaybeDiscourageAndDisconnect(*pto)) return true;
38903891

38913892
CNodeState &state = *State(pto->GetId());
38923893

‎src/net_processing.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI
3131
ChainstateManager& m_chainman;
3232
CTxMemPool& m_mempool;
3333

34-
bool CheckIfBanned(CNode& pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
34+
bool MaybeDiscourageAndDisconnect(CNode& pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
3535

3636
public:
3737
PeerLogicValidation(CConnman* connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool);

‎src/qt/rpcconsole.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1218,7 +1218,7 @@ void RPCConsole::banSelectedNode(int bantime)
12181218
// Find possible nodes, ban it and clear the selected node
12191219
const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
12201220
if (stats) {
1221-
m_node.ban(stats->nodeStats.addr, BanReasonManuallyAdded, bantime);
1221+
m_node.ban(stats->nodeStats.addr, bantime);
12221222
m_node.disconnectByAddress(stats->nodeStats.addr);
12231223
}
12241224
}

0 commit comments

Comments
 (0)