Replace automatic bans with discouragement filter

Summary:
This patch improves performance and resource usage around IP
addresses that are banned for misbehavior. They're already not
actually banned, 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 better reflect reality;
they're not banned, just discouraged.

Contains release notes and several interface improvements by
John Newbery.

Partial backport of Core [[bitcoin/bitcoin#19219 | PR19219]]
bitcoin/bitcoin@b6a3448

Test Plan: `ninja check check-functional`

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6506

doc/release-notes.md

@@ -100,3 +100,20 @@ When a ZMQ PUB socket reaches its high water mark for a subscriber, then
 additional messages to the subscriber are dropped until the number of
 queued messages again falls below the high water mark value.
 
+Change in automatic banning
+---------------------------
+
+Automatic banning of peers for bad behavior has been slightly altered:
+
+- automatic bans will no longer time out automatically after 24 hours.
+  Depending on traffic from other peers, automatic bans may time out at an
+  indeterminate time.
+- automatic bans will no longer be persisted over restarts. Only manual bans
+  will be persisted.
+- automatic bans will no longer be returned by the `listbanned` RPC.
+- automatic bans can no longer be lifted with the `setban remove` RPC command.
+  If you need to remove an automatic ban, you can clear all bans (including
+  manual bans) with the `clearbanned` RPC, or stop-start to clear automatic bans.
+- automatic bans 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.

src/banman.cpp

@@ -70,6 +70,7 @@ void BanMan::DumpBanlist() {
 void BanMan::ClearBanned() {
     {
         LOCK(m_cs_banned);
+        m_discouraged.reset();
         m_banned.clear();
         m_is_dirty = true;
     }
@@ -85,26 +86,25 @@ int BanMan::IsBannedLevel(CNetAddr net_addr) {
     // 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) {
         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 2;
         }
     }
-    return level;
+    return m_discouraged.contains(net_addr.GetAddrBytes()) ? 1 : 0;
 }
 
 bool BanMan::IsBanned(CNetAddr net_addr) {
     auto current_time = GetTime();
     LOCK(m_cs_banned);
+    if (m_discouraged.contains(net_addr.GetAddrBytes())) {
+        return true;
+    }
     for (const auto &it : m_banned) {
         CSubNet sub_net = it.first;
         CBanEntry ban_entry = it.second;
@@ -131,12 +131,18 @@ bool BanMan::IsBanned(CSubNet sub_net) {
 
 void BanMan::Ban(const CNetAddr &net_addr, const BanReason &ban_reason,
                  int64_t ban_time_offset, bool since_unix_epoch) {
+    if (ban_reason == BanReasonNodeMisbehaving) {
+        LOCK(m_cs_banned);
+        m_discouraged.insert(net_addr.GetAddrBytes());
+        return;
+    }
     CSubNet sub_net(net_addr);
     Ban(sub_net, ban_reason, 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) {
+    assert(ban_reason == BanReasonManuallyAdded);
     CBanEntry ban_entry(GetTime(), ban_reason);
 
     int64_t normalized_ban_time_offset = ban_time_offset;

src/banman.h

@@ -6,6 +6,7 @@
 #define BITCOIN_BANMAN_H
 
 #include <addrdb.h>
+#include <bloom.h>
 #include <fs.h>
 #include <sync.h>
 
@@ -67,7 +68,18 @@ class BanMan {
     CClientUIInterface *m_client_interface = nullptr;
     CBanDB m_ban_db;
     const int64_t m_default_ban_time;
-};
 
+    /**
+     *  Individual addresses that of peers that misbehave (see Misbehaving() in
+     *  net_processing) are not banned but discouraged: we still allow incoming
+     *  connections from them, but prefer them for eviction. Similar to banned
+     *  IPs, we never make outgoing connections to them, and do not rumour them
+     *  to other peers.
+     *
+     *  This is implemented as a bloom filter. We can (probabilistically) test
+     *  for membership, but can't list or unban automatically banned peers.
+     */
+    CRollingBloomFilter m_discouraged GUARDED_BY(m_cs_banned){50000, 0.000001};
+};
 
 #endif // BITCOIN_BANMAN_H

src/net_processing.cpp

@@ -3903,11 +3903,14 @@ bool PeerLogicValidation::SendRejectsAndCheckIfBanned(CNode *pnode,
                       pnode->addr.ToString());
         } else if (pnode->addr.IsLocal()) {
             // Disconnect but don't ban _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
+            LogPrintf("Disconnecting and discouraging peer %s!\n",
+                      pnode->addr.ToString());
             if (m_banman) {
                 m_banman->Ban(pnode->addr, BanReasonNodeMisbehaving);
             }

src/netaddress.h

@@ -100,6 +100,9 @@ class CNetAddr {
     uint64_t GetHash() const;
     bool GetInAddr(struct in_addr *pipv4Addr) const;
     std::vector<uint8_t> GetGroup() const;
+    std::vector<uint8_t> GetAddrBytes() const {
+        return {std::begin(ip), std::end(ip)};
+    }
     int GetReachabilityFrom(const CNetAddr *paddrPartner = nullptr) const;
 
     explicit CNetAddr(const struct in6_addr &pipv6Addr,

src/rpc/net.cpp

@@ -664,8 +664,8 @@ static UniValue getnetworkinfo(const Config &config,
 static UniValue setban(const Config &config, const JSONRPCRequest &request) {
     const RPCHelpMan help{
         "setban",
-        "\nAttempts to add or remove an IP/Subnet from the "
-        "banned list.\n",
+        "\nAttempts to add or remove an IP/Subnet from the banned list.\n"
+        "Peers that are automatically banned cannot be unbanned.\n",
         {
             {"subnet", RPCArg::Type::STR, RPCArg::Optional::NO,
              "The IP/Subnet (see getpeerinfo for nodes IP) with an optional "
@@ -725,8 +725,9 @@ static UniValue setban(const Config &config, const JSONRPCRequest &request) {
     }
 
     if (strCommand == "add") {
-        if (isSubnet ? g_rpc_node->banman->IsBanned(subNet)
-                     : g_rpc_node->banman->IsBanned(netAddr)) {
+        if ((isSubnet && g_rpc_node->banman->IsBanned(subNet)) ||
+            (!isSubnet && g_rpc_node->banman->IsBannedLevel(netAddr) ==
+                              BanReasonManuallyAdded)) {
             throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED,
                                "Error: IP/Subnet already banned");
         }
@@ -760,7 +761,7 @@ static UniValue setban(const Config &config, const JSONRPCRequest &request) {
                        : g_rpc_node->banman->Unban(netAddr))) {
             throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET,
                                "Error: Unban failed. Requested address/subnet "
-                               "was not previously banned.");
+                               "was not previously manually banned.");
         }
     }
     return NullUniValue;
@@ -771,7 +772,7 @@ static UniValue listbanned(const Config &config,
     if (request.fHelp || request.params.size() != 0) {
         throw std::runtime_error(RPCHelpMan{
             "listbanned",
-            "\nList all banned IPs/Subnets.\n",
+            "\nList all manually banned IPs/Subnets.\n",
             {},
             RPCResults{},
             RPCExamples{HelpExampleCli("listbanned", "") +
@@ -795,7 +796,6 @@ static UniValue listbanned(const Config &config,
         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

@@ -382,12 +382,6 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) {
     }
     BOOST_CHECK(banman->IsBanned(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(config, dummyNode.GetId(), dummy);
 }