Merge #19219: Replace automatic bans with discouragement filter

2ad58381ff Clean up separated ban/discourage interface (Pieter Wuille)
b691f2df5f Replace automatic bans with discouragement filter (Pieter Wuille)

Pull request description:

  This patch improves performance and resource usage around IP addresses that are banned for misbehavior. They're already not actually banned since #14929, as connections from them are still allowed, but they are preferred for eviction if the inbound connection slots are full.

  Stop treating these like manually banned IP ranges, and instead just keep them in a rolling Bloom filter of misbehaving nodes, which isn't persisted to disk or exposed through the ban framework. The effect remains the same: preferred for eviction, avoided for outgoing connections, and not relayed to other peers.

  Also change the name of this mechanism to "discouraged" to better reflect reality.

ACKs for top commit:
  naumenkogs:
    utACK 2ad58381ff
  amitiuttarwar:
    code review ACK 2ad58381ff
  jonatack:
    ACK 2ad5838 per changes since last review `git range-diff 3276c14 1f7e0ca 2ad5838`
  jnewbery:
    Code review ACK 2ad58381ff

Tree-SHA512: 5dedef401d9cbfa026812651303e6286223563dbeed7a10766ed536ac9e3f29ed4bd0df29cc6deadceeb35cbe9f066346add14ef0833958ca9f93d123fe7aab5

doc/release-notes-19219.md

@@ -0,0 +1,23 @@
+#### Changes regarding misbehaving peers
+
+Peers that misbehave (e.g. send us invalid blocks) are now referred to as
+discouraged nodes in log output, as they're not (and weren't) strictly banned:
+incoming connections are still allowed from them, but they're preferred for
+eviction.
+
+Furthermore, a few additional changes are introduced to how discouraged
+addresses are treated:
+
+- Discouraging an address does not time out automatically after 24 hours
+  (or the `-bantime` setting). Depending on traffic from other peers,
+  discouragement may time out at an indeterminate time.
+
+- Discouragement is not persisted over restarts.
+
+- There is no method to list discouraged addresses. They are not returned by
+  the `listbanned` RPC. That RPC also no longer reports the `ban_reason`
+  field, as `"manually added"` is the only remaining option.
+
+- Discouragement cannot be removed with the `setban remove` RPC command.
+  If you need to remove a discouragement, you can remove all discouragements by
+  stop-starting your node.

src/addrdb.h

@@ -17,13 +17,6 @@ class CSubNet;
 class CAddrMan;
 class CDataStream;
 
-typedef enum BanReason
-{
-    BanReasonUnknown          = 0,
-    BanReasonNodeMisbehaving  = 1,
-    BanReasonManuallyAdded    = 2
-} BanReason;
-
 class CBanEntry
 {
 public:
@@ -31,7 +24,6 @@ public:
     int nVersion;
     int64_t nCreateTime;
     int64_t nBanUntil;
-    uint8_t banReason;
 
     CBanEntry()
     {
@@ -44,31 +36,17 @@ public:
         nCreateTime = nCreateTimeIn;
     }
 
-    explicit CBanEntry(int64_t n_create_time_in, BanReason ban_reason_in) : CBanEntry(n_create_time_in)
+    SERIALIZE_METHODS(CBanEntry, obj)
     {
-        banReason = ban_reason_in;
+        uint8_t ban_reason = 2; //! For backward compatibility
+        READWRITE(obj.nVersion, obj.nCreateTime, obj.nBanUntil, ban_reason);
     }
 
-    SERIALIZE_METHODS(CBanEntry, obj) { READWRITE(obj.nVersion, obj.nCreateTime, obj.nBanUntil, obj.banReason); }
-
     void SetNull()
     {
         nVersion = CBanEntry::CURRENT_VERSION;
         nCreateTime = 0;
         nBanUntil = 0;
-        banReason = BanReasonUnknown;
-    }
-
-    std::string banReasonToString() const
-    {
-        switch (banReason) {
-        case BanReasonNodeMisbehaving:
-            return "node misbehaving";
-        case BanReasonManuallyAdded:
-            return "manually added";
-        default:
-            return "unknown";
-        }
     }
 };
 

src/banman.cpp

@@ -68,28 +68,13 @@ void BanMan::ClearBanned()
     if (m_client_interface) m_client_interface->BannedListChanged();
 }
 
-int BanMan::IsBannedLevel(CNetAddr net_addr)
+bool BanMan::IsDiscouraged(const CNetAddr& net_addr)
 {
-    // Returns the most severe level of banning that applies to this address.
-    // 0 - Not banned
-    // 1 - Automatic misbehavior ban
-    // 2 - Any other ban
-    int level = 0;
-    auto current_time = GetTime();
     LOCK(m_cs_banned);
-    for (const auto& it : m_banned) {
+    return m_discouraged.contains(net_addr.GetAddrBytes());
-        CSubNet sub_net = it.first;
-        CBanEntry ban_entry = it.second;
-
-        if (current_time < ban_entry.nBanUntil && sub_net.Match(net_addr)) {
-            if (ban_entry.banReason != BanReasonNodeMisbehaving) return 2;
-            level = 1;
-        }
-    }
-    return level;
 }
 
-bool BanMan::IsBanned(CNetAddr net_addr)
+bool BanMan::IsBanned(const CNetAddr& net_addr)
 {
     auto current_time = GetTime();
     LOCK(m_cs_banned);
@@ -104,7 +89,7 @@ bool BanMan::IsBanned(CNetAddr net_addr)
     return false;
 }
 
-bool BanMan::IsBanned(CSubNet sub_net)
+bool BanMan::IsBanned(const CSubNet& sub_net)
 {
     auto current_time = GetTime();
     LOCK(m_cs_banned);
@@ -118,15 +103,21 @@ bool BanMan::IsBanned(CSubNet sub_net)
     return false;
 }
 
-void BanMan::Ban(const CNetAddr& net_addr, const BanReason& ban_reason, int64_t ban_time_offset, bool since_unix_epoch)
+void BanMan::Ban(const CNetAddr& net_addr, int64_t ban_time_offset, bool since_unix_epoch)
 {
     CSubNet sub_net(net_addr);
-    Ban(sub_net, ban_reason, ban_time_offset, since_unix_epoch);
+    Ban(sub_net, ban_time_offset, since_unix_epoch);
 }
 
-void BanMan::Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset, bool since_unix_epoch)
+void BanMan::Discourage(const CNetAddr& net_addr)
 {
-    CBanEntry ban_entry(GetTime(), ban_reason);
+    LOCK(m_cs_banned);
+    m_discouraged.insert(net_addr.GetAddrBytes());
+}
+
+void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_unix_epoch)
+{
+    CBanEntry ban_entry(GetTime());
 
     int64_t normalized_ban_time_offset = ban_time_offset;
     bool normalized_since_unix_epoch = since_unix_epoch;
@@ -146,8 +137,8 @@ void BanMan::Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ba
     }
     if (m_client_interface) m_client_interface->BannedListChanged();
 
-    //store banlist to disk immediately if user requested ban
+    //store banlist to disk immediately
-    if (ban_reason == BanReasonManuallyAdded) DumpBanlist();
+    DumpBanlist();
 }
 
 bool BanMan::Unban(const CNetAddr& net_addr)

src/banman.h

@@ -6,6 +6,7 @@
 #define BITCOIN_BANMAN_H
 
 #include <addrdb.h>
+#include <bloom.h>
 #include <fs.h>
 #include <net_types.h> // For banmap_t
 #include <sync.h>
@@ -23,32 +24,55 @@ class CClientUIInterface;
 class CNetAddr;
 class CSubNet;
 
-// Denial-of-service detection/prevention
+// Banman manages two related but distinct concepts:
-// The idea is to detect peers that are behaving
+//
-// badly and disconnect/ban them, but do it in a
+// 1. Banning. This is configured manually by the user, through the setban RPC.
-// one-coding-mistake-won't-shatter-the-entire-network
+// If an address or subnet is banned, we never accept incoming connections from
-// way.
+// it and never create outgoing connections to it. We won't gossip its address
-// IMPORTANT:  There should be nothing I can give a
+// to other peers in addr messages. Banned addresses and subnets are stored to
-// node that it will forward on that will make that
+// banlist.dat on shutdown and reloaded on startup. Banning can be used to
-// node's peers drop it. If there is, an attacker
+// prevent connections with spy nodes or other griefers.
-// can isolate a node and/or try to split the network.
+//
-// Dropping a node for sending stuff that is invalid
+// 2. Discouragement. If a peer misbehaves enough (see Misbehaving() in
-// now but might be valid in a later version is also
+// net_processing.cpp), we'll mark that address as discouraged. We still allow
-// dangerous, because it can cause a network split
+// incoming connections from them, but they're preferred for eviction when
-// between nodes running old code and nodes running
+// we receive new incoming connections. We never make outgoing connections to
-// new code.
+// them, and do not gossip their address to other peers. This is implemented as
+// a bloom filter. We can (probabilistically) test for membership, but can't
+// list all discouraged addresses or unmark them as discouraged. Discouragement
+// can prevent our limited connection slots being used up by incompatible
+// or broken peers.
+//
+// Neither banning nor discouragement are protections against denial-of-service
+// attacks, since if an attacker has a way to waste our resources and we
+// disconnect from them and ban that address, it's trivial for them to
+// reconnect from another IP address.
+//
+// Attempting to automatically disconnect or ban any class of peer carries the
+// risk of splitting the network. For example, if we banned/disconnected for a
+// transaction that fails a policy check and a future version changes the
+// policy check so the transaction is accepted, then that transaction could
+// cause the network to split between old nodes and new nodes.
 
 class BanMan
 {
 public:
     ~BanMan();
     BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
-    void Ban(const CNetAddr& net_addr, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
+    void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
-    void Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
+    void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
+    void Discourage(const CNetAddr& net_addr);
     void ClearBanned();
-    int IsBannedLevel(CNetAddr net_addr);
+
-    bool IsBanned(CNetAddr net_addr);
+    //! Return whether net_addr is banned
-    bool IsBanned(CSubNet sub_net);
+    bool IsBanned(const CNetAddr& net_addr);
+
+    //! Return whether sub_net is exactly banned
+    bool IsBanned(const CSubNet& sub_net);
+
+    //! Return whether net_addr is discouraged.
+    bool IsDiscouraged(const CNetAddr& net_addr);
+
     bool Unban(const CNetAddr& net_addr);
     bool Unban(const CSubNet& sub_net);
     void GetBanned(banmap_t& banmap);
@@ -68,6 +92,7 @@ private:
     CClientUIInterface* m_client_interface = nullptr;
     CBanDB m_ban_db;
     const int64_t m_default_ban_time;
+    CRollingBloomFilter m_discouraged GUARDED_BY(m_cs_banned) {50000, 0.000001};
 };
 
 #endif

src/init.cpp

@@ -431,8 +431,8 @@ void SetupServerArgs(NodeContext& node)
 
     gArgs.AddArg("-addnode=<ip>", "Add a node to connect to and attempt to keep the connection open (see the `addnode` RPC command help for more info). This option can be specified multiple times to add multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by the net-specific datadir location.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    gArgs.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-bind=<addr>", "Bind to given address and always listen on it. Use [host]:port notation for IPv6", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

src/interfaces/node.cpp

@@ -146,10 +146,10 @@ public:
         }
         return false;
     }
-    bool ban(const CNetAddr& net_addr, BanReason reason, int64_t ban_time_offset) override
+    bool ban(const CNetAddr& net_addr, int64_t ban_time_offset) override
     {
         if (m_context.banman) {
-            m_context.banman->Ban(net_addr, reason, ban_time_offset);
+            m_context.banman->Ban(net_addr, ban_time_offset);
             return true;
         }
         return false;

src/interfaces/node.h

@@ -122,7 +122,7 @@ public:
     virtual bool getBanned(banmap_t& banmap) = 0;
 
     //! Ban node.
-    virtual bool ban(const CNetAddr& net_addr, BanReason reason, int64_t ban_time_offset) = 0;
+    virtual bool ban(const CNetAddr& net_addr, int64_t ban_time_offset) = 0;
 
     //! Unban node.
     virtual bool unban(const CSubNet& ip) = 0;

src/net.cpp

@@ -1010,17 +1010,24 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
     // on all platforms.  Set it again here just to be sure.
     SetSocketNoDelay(hSocket);
 
-    int bannedlevel = m_banman ? m_banman->IsBannedLevel(addr) : 0;
+    // Don't accept connections from banned peers.
-
+    bool banned = m_banman->IsBanned(addr);
-    // Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept
+    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && banned)
-    // if the only banning reason was an automatic misbehavior ban.
-    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0))
     {
         LogPrint(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToString());
         CloseSocket(hSocket);
         return;
     }
 
+    // Only accept connections from discouraged peers if our inbound slots aren't (almost) full.
+    bool discouraged = m_banman->IsDiscouraged(addr);
+    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && nInbound + 1 >= nMaxInbound && discouraged)
+    {
+        LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToString());
+        CloseSocket(hSocket);
+        return;
+    }
+
     if (nInbound >= nMaxInbound)
     {
         if (!AttemptToEvictConnection()) {
@@ -1044,7 +1051,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
     pnode->m_permissionFlags = permissionFlags;
     // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility)
     pnode->m_legacyWhitelisted = legacyWhitelisted;
-    pnode->m_prefer_evict = bannedlevel > 0;
+    pnode->m_prefer_evict = discouraged;
     m_msgproc->InitializeNode(pnode);
 
     LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString());
@@ -2045,10 +2052,10 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
         return;
     }
     if (!pszDest) {
-        if (IsLocal(addrConnect) ||
+        bool banned_or_discouraged = m_banman && (m_banman->IsDiscouraged(addrConnect) || m_banman->IsBanned(addrConnect));
-            FindNode(static_cast<CNetAddr>(addrConnect)) || (m_banman && m_banman->IsBanned(addrConnect)) ||
+        if (IsLocal(addrConnect) || FindNode(static_cast<CNetAddr>(addrConnect)) || banned_or_discouraged || FindNode(addrConnect.ToStringIPPort())) {
-            FindNode(addrConnect.ToStringIPPort()))
             return;
+        }
     } else if (FindNode(std::string(pszDest)))
         return;
 

src/net_permissions.h

@@ -24,7 +24,7 @@ enum NetPermissionFlags
     // Always relay transactions from this peer, even if already in mempool
     // Keep parameter interaction: forcerelay implies relay
     PF_FORCERELAY = (1U << 2) | PF_RELAY,
-    // Can't be banned for misbehavior
+    // Can't be banned/disconnected/discouraged for misbehavior
     PF_NOBAN = (1U << 4),
     // Can query the mempool
     PF_MEMPOOL = (1U << 5),

src/net_processing.cpp

@@ -249,8 +249,8 @@ 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 marked as discouraged (unless whitelisted with noban).
-    bool fShouldBan;
+    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.
@@ -401,7 +401,7 @@ struct CNodeState {
     {
         fCurrentlyConnected = false;
         nMisbehavior = 0;
-        fShouldBan = false;
+        m_should_discourage = false;
         pindexBestKnownBlock = nullptr;
         hashLastUnknownBlock.SetNull();
         pindexLastCommonBlock = nullptr;
@@ -1016,7 +1016,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
 }
 
 /**
- * Mark a misbehaving peer to be banned depending upon the value of `-banscore`.
+ * Increment peer's misbehavior score. If the new value surpasses banscore (specified on startup or by default), mark node to be discouraged, meaning the peer might be disconnected & added to the discouragement filter.
  */
 void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
@@ -1032,14 +1032,14 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV
     std::string message_prefixed = message.empty() ? "" : (": " + message);
     if (state->nMisbehavior >= banscore && state->nMisbehavior - howmuch < banscore)
     {
-        LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d) BAN THRESHOLD EXCEEDED%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed);
+        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);
-        state->fShouldBan = true;
+        state->m_should_discourage = true;
     } else
         LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d)%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed);
 }
 
 /**
- * Potentially ban a node based on the contents of a BlockValidationState object
+ * Potentially mark a node discouraged based on the contents of a BlockValidationState object
  *
  * @param[in] via_compact_block this bool is passed in because net_processing should
  * punish peers differently depending on whether the data was provided in a compact
@@ -1069,7 +1069,7 @@ static bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& s
                 break;
             }
 
-            // Ban outbound (but not inbound) peers if on an invalid chain.
+            // Discourage outbound (but not inbound) peers if on an invalid chain.
             // Exempt HB compact block peers and manual connections.
             if (!via_compact_block && !node_state->m_is_inbound && !node_state->m_is_manual_connection) {
                 Misbehaving(nodeid, 100, message);
@@ -1104,7 +1104,7 @@ static bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& s
 }
 
 /**
- * Potentially ban a node based on the contents of a TxValidationState object
+ * Potentially disconnect and discourage a node based on the contents of a TxValidationState object
  *
  * @return Returns true if the peer was punished (probably disconnected)
  */
@@ -1336,7 +1336,7 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
 }
 
 /**
- * Handle invalid block rejection and consequent peer banning, maintain which
+ * Handle invalid block rejection and consequent peer discouragement, maintain which
  * peers announce compact blocks.
  */
 void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidationState& state) {
@@ -2473,7 +2473,8 @@ void ProcessMessage(
             if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
                 addr.nTime = nNow - 5 * 24 * 60 * 60;
             pfrom.AddAddressKnown(addr);
-            if (banman->IsBanned(addr)) continue; // Do not process banned addresses beyond remembering we received them
+            if (banman->IsDiscouraged(addr)) continue; // Do not process banned/discouraged addresses beyond remembering we received them
+            if (banman->IsBanned(addr)) continue;
             bool fReachable = IsReachable(addr);
             if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
             {
@@ -3115,7 +3116,7 @@ void ProcessMessage(
             // relayed before full validation (see BIP 152), so we don't want to disconnect
             // the peer if the header turns out to be for an invalid block.
             // Note that if a peer tries to build on an invalid chain, that
-            // will be detected and the peer will be banned.
+            // will be detected and the peer will be disconnected/discouraged.
             return ProcessHeadersMessage(pfrom, connman, chainman, mempool, {cmpctblock.header}, chainparams, /*via_compact_block=*/true);
         }
 
@@ -3201,7 +3202,7 @@ void ProcessMessage(
                 // 3. the block is otherwise invalid (eg invalid coinbase,
                 //    block is too big, too many legacy sigops, etc).
                 // So if CheckBlock failed, #3 is the only possibility.
-                // Under BIP 152, we don't DoS-ban unless proof of work is
+                // Under BIP 152, we don't discourage the peer unless proof of work is
                 // invalid (we don't require all the stateless checks to have
                 // been run).  This is handled below, so just treat this as
                 // though the block was successfully read, and rely on the
@@ -3326,7 +3327,7 @@ void ProcessMessage(
         std::vector<CAddress> vAddr = connman->GetAddresses();
         FastRandomContext insecure_rand;
         for (const CAddress &addr : vAddr) {
-            if (!banman->IsBanned(addr)) {
+            if (!banman->IsDiscouraged(addr) && !banman->IsBanned(addr)) {
                 pfrom.PushAddress(addr, insecure_rand);
             }
         }
@@ -3561,25 +3562,26 @@ void ProcessMessage(
     return;
 }
 
-bool PeerLogicValidation::CheckIfBanned(CNode& pnode)
+bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
 {
     AssertLockHeld(cs_main);
     CNodeState &state = *State(pnode.GetId());
 
-    if (state.fShouldBan) {
+    if (state.m_should_discourage) {
-        state.fShouldBan = false;
+        state.m_should_discourage = false;
-        if (pnode.HasPermission(PF_NOBAN))
+        if (pnode.HasPermission(PF_NOBAN)) {
             LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode.addr.ToString());
-        else if (pnode.m_manual_connection)
+        } else if (pnode.m_manual_connection) {
             LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode.addr.ToString());
-        else if (pnode.addr.IsLocal()) {
+        } else if (pnode.addr.IsLocal()) {
-            // Disconnect but don't ban _this_ local node
+            // Disconnect but don't discourage this local node
-            LogPrintf("Warning: disconnecting but not banning local peer %s!\n", pnode.addr.ToString());
+            LogPrintf("Warning: disconnecting but not discouraging local peer %s!\n", pnode.addr.ToString());
             pnode.fDisconnect = true;
         } else {
-            // Disconnect and ban all nodes sharing the address
+            // Disconnect and discourage all nodes sharing the address
+            LogPrintf("Disconnecting and discouraging peer %s!\n", pnode.addr.ToString());
             if (m_banman) {
-                m_banman->Ban(pnode.addr, BanReasonNodeMisbehaving);
+                m_banman->Discourage(pnode.addr);
             }
             connman->DisconnectNode(pnode.addr);
         }
@@ -3679,7 +3681,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
     }
 
     LOCK(cs_main);
-    CheckIfBanned(*pfrom);
+    MaybeDiscourageAndDisconnect(*pfrom);
 
     return fMoreWork;
 }
@@ -3882,7 +3884,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         if (!lockMain)
             return true;
 
-        if (CheckIfBanned(*pto)) return true;
+        if (MaybeDiscourageAndDisconnect(*pto)) return true;
 
         CNodeState &state = *State(pto->GetId());
 

src/net_processing.h

@@ -31,7 +31,7 @@ private:
     ChainstateManager& m_chainman;
     CTxMemPool& m_mempool;
 
-    bool CheckIfBanned(CNode& pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool MaybeDiscourageAndDisconnect(CNode& pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
 public:
     PeerLogicValidation(CConnman* connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool);

src/netaddress.h

@@ -90,6 +90,7 @@ class CNetAddr
         uint32_t GetMappedAS(const std::vector<bool> &asmap) const;
 
         std::vector<unsigned char> GetGroup(const std::vector<bool> &asmap) const;
+        std::vector<unsigned char> GetAddrBytes() const { return {std::begin(ip), std::end(ip)}; }
         int GetReachabilityFrom(const CNetAddr *paddrPartner = nullptr) const;
 
         explicit CNetAddr(const struct in6_addr& pipv6Addr, const uint32_t scope = 0);

src/qt/rpcconsole.cpp

@@ -1218,7 +1218,7 @@ void RPCConsole::banSelectedNode(int bantime)
         // Find possible nodes, ban it and clear the selected node
         const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
         if (stats) {
-            m_node.ban(stats->nodeStats.addr, BanReasonManuallyAdded, bantime);
+            m_node.ban(stats->nodeStats.addr, bantime);
             m_node.disconnectByAddress(stats->nodeStats.addr);
         }
     }

src/rpc/net.cpp

@@ -614,12 +614,12 @@ static UniValue setban(const JSONRPCRequest& request)
             absolute = true;
 
         if (isSubnet) {
-            node.banman->Ban(subNet, BanReasonManuallyAdded, banTime, absolute);
+            node.banman->Ban(subNet, banTime, absolute);
             if (node.connman) {
                 node.connman->DisconnectNode(subNet);
             }
         } else {
-            node.banman->Ban(netAddr, BanReasonManuallyAdded, banTime, absolute);
+            node.banman->Ban(netAddr, banTime, absolute);
             if (node.connman) {
                 node.connman->DisconnectNode(netAddr);
             }
@@ -628,7 +628,7 @@ static UniValue setban(const JSONRPCRequest& request)
     else if(strCommand == "remove")
     {
         if (!( isSubnet ? node.banman->Unban(subNet) : node.banman->Unban(netAddr) )) {
-            throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Unban failed. Requested address/subnet was not previously banned.");
+            throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Unban failed. Requested address/subnet was not previously manually banned.");
         }
     }
     return NullUniValue;
@@ -637,7 +637,7 @@ static UniValue setban(const JSONRPCRequest& request)
 static UniValue listbanned(const JSONRPCRequest& request)
 {
             RPCHelpMan{"listbanned",
-                "\nList all banned IPs/Subnets.\n",
+                "\nList all manually banned IPs/Subnets.\n",
                 {},
         RPCResult{RPCResult::Type::ARR, "", "",
             {
@@ -646,7 +646,6 @@ static UniValue listbanned(const JSONRPCRequest& request)
                         {RPCResult::Type::STR, "address", ""},
                         {RPCResult::Type::NUM_TIME, "banned_until", ""},
                         {RPCResult::Type::NUM_TIME, "ban_created", ""},
-                        {RPCResult::Type::STR, "ban_reason", ""},
                     }},
             }},
                 RPCExamples{
@@ -671,7 +670,6 @@ static UniValue listbanned(const JSONRPCRequest& request)
         rec.pushKV("address", entry.first.ToString());
         rec.pushKV("banned_until", banEntry.nBanUntil);
         rec.pushKV("ban_created", banEntry.nCreateTime);
-        rec.pushKV("ban_reason", banEntry.banReasonToString());
 
         bannedAddresses.push_back(rec);
     }

src/test/denialofservice_tests.cpp

@@ -238,8 +238,8 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
     }
-    BOOST_CHECK(banman->IsBanned(addr1));
+    BOOST_CHECK(banman->IsDiscouraged(addr1));
-    BOOST_CHECK(!banman->IsBanned(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned
+    BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned
 
     CAddress addr2(ip(0xa0b0c002), NODE_NONE);
     CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", true);
@@ -255,8 +255,8 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         LOCK2(cs_main, dummyNode2.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
     }
-    BOOST_CHECK(!banman->IsBanned(addr2)); // 2 not banned yet...
+    BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not banned yet...
-    BOOST_CHECK(banman->IsBanned(addr1));  // ... but 1 still should be
+    BOOST_CHECK(banman->IsDiscouraged(addr1));  // ... but 1 still should be
     {
         LOCK(cs_main);
         Misbehaving(dummyNode2.GetId(), 50);
@@ -265,7 +265,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         LOCK2(cs_main, dummyNode2.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
     }
-    BOOST_CHECK(banman->IsBanned(addr2));
+    BOOST_CHECK(banman->IsDiscouraged(addr2));
 
     bool dummy;
     peerLogic->FinalizeNode(dummyNode1.GetId(), dummy);
@@ -294,7 +294,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
     }
-    BOOST_CHECK(!banman->IsBanned(addr1));
+    BOOST_CHECK(!banman->IsDiscouraged(addr1));
     {
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 10);
@@ -303,7 +303,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
     }
-    BOOST_CHECK(!banman->IsBanned(addr1));
+    BOOST_CHECK(!banman->IsDiscouraged(addr1));
     {
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 1);
@@ -312,7 +312,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
     }
-    BOOST_CHECK(banman->IsBanned(addr1));
+    BOOST_CHECK(banman->IsDiscouraged(addr1));
     gArgs.ForceSetArg("-banscore", ToString(DEFAULT_BANSCORE_THRESHOLD));
 
     bool dummy;
@@ -344,13 +344,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
         LOCK2(cs_main, dummyNode.cs_sendProcessing);
         BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
     }
-    BOOST_CHECK(banman->IsBanned(addr));
+    BOOST_CHECK(banman->IsDiscouraged(addr));
-
-    SetMockTime(nStartTime+60*60);
-    BOOST_CHECK(banman->IsBanned(addr));
-
-    SetMockTime(nStartTime+60*60*24+1);
-    BOOST_CHECK(!banman->IsBanned(addr));
 
     bool dummy;
     peerLogic->FinalizeNode(dummyNode.GetId(), dummy);

src/test/fuzz/addrdb.cpp

@@ -18,18 +18,11 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 
     const CBanEntry ban_entry = [&] {
-        switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 3)) {
+        switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 2)) {
         case 0:
             return CBanEntry{fuzzed_data_provider.ConsumeIntegral<int64_t>()};
             break;
-        case 1:
+        case 1: {
-            return CBanEntry{fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.PickValueInArray<BanReason>({
-                                                                                  BanReason::BanReasonUnknown,
-                                                                                  BanReason::BanReasonNodeMisbehaving,
-                                                                                  BanReason::BanReasonManuallyAdded,
-                                                                              })};
-            break;
-        case 2: {
             const std::optional<CBanEntry> ban_entry = ConsumeDeserializable<CBanEntry>(fuzzed_data_provider);
             if (ban_entry) {
                 return *ban_entry;
@@ -39,5 +32,4 @@ void test_one_input(const std::vector<uint8_t>& buffer)
         }
         return CBanEntry{};
     }();
-    assert(!ban_entry.banReasonToString().empty());
 }