merge bitcoin#19219: Replace automatic bans with discouragement filter

kwvg · Jun 11, 2022 · 57575e5 · 57575e5
1 parent 72cfd58
commit 57575e5
Show file tree

Hide file tree

Showing 14 changed files with 151 additions and 134 deletions.
diff --git a/doc/release-notes-19219.md b/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.
diff --git a/src/addrdb.h b/src/addrdb.h
@@ -17,21 +17,13 @@ class CSubNet;
 class CAddrMan;
 class CDataStream;
 
-typedef enum BanReason
-{
-    BanReasonUnknown          = 0,
-    BanReasonNodeMisbehaving  = 1,
-    BanReasonManuallyAdded    = 2
-} BanReason;
-
 class CBanEntry
 {
 public:
     static const int CURRENT_VERSION=1;
     int nVersion;
     int64_t nCreateTime;
     int64_t nBanUntil;
-    uint8_t banReason;
 
     CBanEntry()
     {
@@ -44,31 +36,17 @@ class CBanEntry
         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";
-        }
     }
 };
 

diff --git a/src/banman.cpp b/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) {
-        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;
+    return m_discouraged.contains(net_addr.GetAddrBytes());
 }
 
-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::Discourage(const CNetAddr& net_addr)
+{
+    LOCK(m_cs_banned);
+    m_discouraged.insert(net_addr.GetAddrBytes());
 }
 
-void BanMan::Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset, bool since_unix_epoch)
+void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_unix_epoch)
 {
-    CBanEntry ban_entry(GetTime(), ban_reason);
+    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
-    if (ban_reason == BanReasonManuallyAdded) DumpBanlist();
+    //store banlist to disk immediately
+    DumpBanlist();
 }
 
 bool BanMan::Unban(const CNetAddr& net_addr)

diff --git a/src/banman.h b/src/banman.h
@@ -9,6 +9,7 @@
 #include <memory>
 
 #include <addrdb.h>
+#include <bloom.h>
 #include <fs.h>
 #include <net_types.h> // For banmap_t
 #include <sync.h>
@@ -20,32 +21,55 @@ class CClientUIInterface;
 class CNetAddr;
 class CSubNet;
 
-// Denial-of-service detection/prevention
-// The idea is to detect peers that are behaving
-// badly and disconnect/ban 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
-// node that it will forward on that will make that
-// node's peers drop it. If there is, an attacker
-// can isolate a node and/or try to split the network.
-// Dropping a node for sending stuff that is invalid
-// now but might be valid in a later version is also
-// dangerous, because it can cause a network split
-// between nodes running old code and nodes running
-// new code.
+// Banman manages two related but distinct concepts:
+//
+// 1. Banning. This is configured manually by the user, through the setban RPC.
+// If an address or subnet is banned, we never accept incoming connections from
+// it and never create outgoing connections to it. We won't gossip its address
+// to other peers in addr messages. Banned addresses and subnets are stored to
+// banlist.dat 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
+// 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
+// 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 CSubNet& sub_net, 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, 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);
-    bool IsBanned(CSubNet sub_net);
+
+    //! Return whether net_addr is banned
+    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);
@@ -65,6 +89,7 @@ class BanMan
     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
diff --git a/src/init.cpp b/src/init.cpp
@@ -544,8 +544,8 @@ void SetupServerArgs(NodeContext& node)
     argsman.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);
     argsman.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);
     argsman.AddArg("-allowprivatenet", strprintf("Allow RFC1918 addresses to be relayed and connected to (default: %u)", DEFAULT_ALLOWPRIVATENET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.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);
     argsman.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);
     argsman.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp
@@ -257,10 +257,10 @@ class NodeImpl : public Node
         }
         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;

diff --git a/src/interfaces/node.h b/src/interfaces/node.h
@@ -195,7 +195,7 @@ class Node
     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;

diff --git a/src/net.cpp b/src/net.cpp
@@ -1087,17 +1087,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, but if our inbound slots aren't almost full, accept
-    // if the only banning reason was an automatic misbehavior ban.
-    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0))
+    // Don't accept connections from banned peers.
+    bool banned = m_banman->IsBanned(addr);
+    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && banned)
     {
         LogPrint(BCLog::NET, "%s (banned)\n", strDropped);
         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;
+    }
+
     // Evict connections until we are below nMaxInbound. In case eviction protection resulted in nodes to not be evicted
     // before, they might get evicted in batches now (after the protection timeout).
     // We don't evict verified MN connections and also don't take them into account when checking limits. We can do this
@@ -1134,7 +1141,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);
 
     if (fLogIPs) {
@@ -2506,8 +2513,8 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
         return;
     }
     if (!pszDest) {
-        // banned or exact match?
-        if ((m_banman && m_banman->IsBanned(addrConnect)) || FindNode(addrConnect.ToStringIPPort()))
+        // banned, discouraged or exact match?
+        if ((m_banman && (m_banman->IsDiscouraged(addrConnect) || m_banman->IsBanned(addrConnect))) || FindNode(addrConnect.ToStringIPPort()))
             return;
         // local and not a connection to itself?
         bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort();

diff --git a/src/net_permissions.h b/src/net_permissions.h
@@ -21,7 +21,7 @@ enum NetPermissionFlags
     // Always relay transactions from this peer, even if already in mempool or rejected from policy
     // 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),