refactor: replace RecursiveMutex m_cs_banned with Mutex (and rename)

src/banman.cpp

@@ -19,7 +19,7 @@ BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t
 
     int64_t n_start = GetTimeMillis();
     if (m_ban_db.Read(m_banned)) {
-        SweepBanned(); // sweep out unused entries
+        SweepBanned(false); // sweep out unused entries
 
         LogPrint(BCLog::NET, "Loaded %d banned node addresses/subnets  %dms\n", m_banned.size(),
                  GetTimeMillis() - n_start);
@@ -39,7 +39,7 @@ BanMan::~BanMan()
 
 void BanMan::DumpBanlist()
 {
-    SweepBanned(); // clean unused entries (if bantime has expired)
+    SweepBanned(false); // clean unused entries (if bantime has expired)
 
     if (!BannedSetIsDirty()) return;
 
@@ -58,7 +58,7 @@ void BanMan::DumpBanlist()
 void BanMan::ClearBanned()
 {
     {
-        LOCK(m_cs_banned);
+        LOCK(m_banned_mutex);
         m_banned.clear();
         m_is_dirty = true;
     }
@@ -68,14 +68,14 @@ void BanMan::ClearBanned()
 
 bool BanMan::IsDiscouraged(const CNetAddr& net_addr)
 {
-    LOCK(m_cs_banned);
+    LOCK(m_banned_mutex);
     return m_discouraged.contains(net_addr.GetAddrBytes());
 }
 
 bool BanMan::IsBanned(const CNetAddr& net_addr)
 {
     auto current_time = GetTime();
-    LOCK(m_cs_banned);
+    LOCK(m_banned_mutex);
     for (const auto& it : m_banned) {
         CSubNet sub_net = it.first;
         CBanEntry ban_entry = it.second;
@@ -90,7 +90,7 @@ bool BanMan::IsBanned(const CNetAddr& net_addr)
 bool BanMan::IsBanned(const CSubNet& sub_net)
 {
     auto current_time = GetTime();
-    LOCK(m_cs_banned);
+    LOCK(m_banned_mutex);
     banmap_t::iterator i = m_banned.find(sub_net);
     if (i != m_banned.end()) {
         CBanEntry ban_entry = (*i).second;
@@ -109,7 +109,7 @@ void BanMan::Ban(const CNetAddr& net_addr, int64_t ban_time_offset, bool since_u
 
 void BanMan::Discourage(const CNetAddr& net_addr)
 {
-    LOCK(m_cs_banned);
+    LOCK(m_banned_mutex);
     m_discouraged.insert(net_addr.GetAddrBytes());
 }
 
@@ -126,7 +126,7 @@ void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_uni
     ban_entry.nBanUntil = (normalized_since_unix_epoch ? 0 : GetTime()) + normalized_ban_time_offset;
 
     {
-        LOCK(m_cs_banned);
+        LOCK(m_banned_mutex);
         if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) {
             m_banned[sub_net] = ban_entry;
             m_is_dirty = true;
@@ -148,7 +148,7 @@ bool BanMan::Unban(const CNetAddr& net_addr)
 bool BanMan::Unban(const CSubNet& sub_net)
 {
     {
-        LOCK(m_cs_banned);
+        LOCK(m_banned_mutex);
         if (m_banned.erase(sub_net) == 0) return false;
         m_is_dirty = true;
     }
@@ -159,31 +159,47 @@ bool BanMan::Unban(const CSubNet& sub_net)
 
 void BanMan::GetBanned(banmap_t& banmap)
 {
-    LOCK(m_cs_banned);
+    LOCK(m_banned_mutex);
     // Sweep the banlist so expired bans are not returned
-    SweepBanned();
+    SweepBanned(true);
     banmap = m_banned; //create a thread safe copy
 }
 
-void BanMan::SweepBanned()
+bool BanMan::SweepBanned()
 {
+    AssertLockHeld(m_banned_mutex);
+
     int64_t now = GetTime();
     bool notify_ui = false;
-    {
-        LOCK(m_cs_banned);
-        banmap_t::iterator it = m_banned.begin();
-        while (it != m_banned.end()) {
-            CSubNet sub_net = (*it).first;
-            CBanEntry ban_entry = (*it).second;
-            if (!sub_net.IsValid() || now > ban_entry.nBanUntil) {
-                m_banned.erase(it++);
-                m_is_dirty = true;
-                notify_ui = true;
-                LogPrint(BCLog::NET, "Removed banned node address/subnet: %s\n", sub_net.ToString());
-            } else
-                ++it;
-        }
+
+    banmap_t::iterator it = m_banned.begin();
+    while (it != m_banned.end()) {
+        CSubNet sub_net = (*it).first;
+        CBanEntry ban_entry = (*it).second;
+        if (!sub_net.IsValid() || now > ban_entry.nBanUntil) {
+            m_banned.erase(it++);
+            m_is_dirty = true;
+            notify_ui = true;
+            LogPrint(BCLog::NET, "Removed banned node address/subnet: %s\n", sub_net.ToString());
+        } else
+            ++it;
     }
+
+    return notify_ui;
+}
+
+void BanMan::SweepBanned(bool isLockHeld)
+{
+    bool notify_ui = false;
+
+    if (!isLockHeld) {
+        AssertLockNotHeld(m_banned_mutex);
+        LOCK(m_banned_mutex);
+        notify_ui = SweepBanned();
+    } else {
+        notify_ui = SweepBanned();
+    }
+
     // update UI
     if (notify_ui && m_client_interface) {
         m_client_interface->BannedListChanged();
@@ -192,12 +208,12 @@ void BanMan::SweepBanned()
 
 bool BanMan::BannedSetIsDirty()
 {
-    LOCK(m_cs_banned);
+    LOCK(m_banned_mutex);
     return m_is_dirty;
 }
 
 void BanMan::SetBannedSetDirty(bool dirty)
 {
-    LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag
+    LOCK(m_banned_mutex); //reuse m_banned lock for the m_is_dirty flag
     m_is_dirty = dirty;
 }

src/banman.h

@@ -84,15 +84,16 @@ class BanMan
     //!set the "dirty" flag for the banlist
     void SetBannedSetDirty(bool dirty = true);
     //!clean unused entries (if bantime has expired)
-    void SweepBanned();
+    bool SweepBanned();
+    void SweepBanned(bool isLockHeld) EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex);
 
-    RecursiveMutex m_cs_banned;
-    banmap_t m_banned GUARDED_BY(m_cs_banned);
-    bool m_is_dirty GUARDED_BY(m_cs_banned){false};
+    Mutex m_banned_mutex;
+    banmap_t m_banned GUARDED_BY(m_banned_mutex);
+    bool m_is_dirty GUARDED_BY(m_banned_mutex){false};
     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};
+    CRollingBloomFilter m_discouraged GUARDED_BY(m_banned_mutex) {50000, 0.000001};
 };
 
 #endif // BITCOIN_BANMAN_H