Merge bitcoin/bitcoin#24097: Replace RecursiveMutex m_cs_banned with Mutex, and rename it

37d150d8c5 refactor: Add more negative `!m_banned_mutex` thread safety annotations (Hennadii Stepanov)
0fb2908708 refactor: replace RecursiveMutex m_banned_mutex with Mutex (w0xlt)
784c316f9c scripted-diff: rename m_cs_banned -> m_banned_mutex (w0xlt)
46709c5f27 refactor: Get rid of `BanMan::SetBannedSetDirty()` (Hennadii Stepanov)
d88c0d8440 refactor: Get rid of `BanMan::BannedSetIsDirty()` (Hennadii Stepanov)

Pull request description:

  This PR is an alternative to bitcoin/bitcoin#24092. Last two commit have been cherry-picked from the latter.

ACKs for top commit:
  maflcko:
    ACK 37d150d8c5 🎾
  achow101:
    ACK 37d150d8c5
  theStack:
    Code-review ACK 37d150d8c5
  vasild:
    ACK 37d150d8c5

Tree-SHA512: 5e9d40101a09af6e0645a6ede67432ea68631a1b960f9e6af0ad07415ca7718a30fcc1aad5182d1d5265dc54c26aba2008fc9973840255c09adbab8fedf10075

src/banman.cpp

@@ -28,7 +28,7 @@ BanMan::~BanMan()
 
 void BanMan::LoadBanlist()
 {
-    LOCK(m_cs_banned);
+    LOCK(m_banned_mutex);
 
     if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
 
@@ -52,16 +52,17 @@ void BanMan::DumpBanlist()
 
     banmap_t banmap;
     {
-        LOCK(m_cs_banned);
+        LOCK(m_banned_mutex);
         SweepBanned();
-        if (!BannedSetIsDirty()) return;
+        if (!m_is_dirty) return;
         banmap = m_banned;
-        SetBannedSetDirty(false);
+        m_is_dirty = false;
     }
 
     const auto start{SteadyClock::now()};
     if (!m_ban_db.Write(banmap)) {
-        SetBannedSetDirty(true);
+        LOCK(m_banned_mutex);
+        m_is_dirty = true;
     }
 
     LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk  %dms\n", banmap.size(),
@@ -71,7 +72,7 @@ void BanMan::DumpBanlist()
 void BanMan::ClearBanned()
 {
     {
-        LOCK(m_cs_banned);
+        LOCK(m_banned_mutex);
         m_banned.clear();
         m_is_dirty = true;
     }
@@ -81,14 +82,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;
@@ -103,7 +104,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;
@@ -122,7 +123,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());
 }
 
@@ -139,7 +140,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;
@@ -161,7 +162,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;
     }
@@ -172,7 +173,7 @@ 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();
     banmap = m_banned; //create a thread safe copy
@@ -180,7 +181,7 @@ void BanMan::GetBanned(banmap_t& banmap)
 
 void BanMan::SweepBanned()
 {
-    AssertLockHeld(m_cs_banned);
+    AssertLockHeld(m_banned_mutex);
 
     int64_t now = GetTime();
     bool notify_ui = false;
@@ -203,15 +204,3 @@ void BanMan::SweepBanned()
         m_client_interface->BannedListChanged();
     }
 }
-
-bool BanMan::BannedSetIsDirty()
-{
-    LOCK(m_cs_banned);
-    return m_is_dirty;
-}
-
-void BanMan::SetBannedSetDirty(bool dirty)
-{
-    LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag
-    m_is_dirty = dirty;
-}

src/banman.h

@@ -60,40 +60,37 @@ class BanMan
 public:
     ~BanMan();
     BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
-    void Ban(const CNetAddr& net_addr, 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) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
-    void Ban(const CSubNet& sub_net, 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) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
-    void Discourage(const CNetAddr& net_addr);
+    void Discourage(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
-    void ClearBanned();
+    void ClearBanned() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
 
     //! Return whether net_addr is banned
-    bool IsBanned(const CNetAddr& net_addr);
+    bool IsBanned(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
 
     //! Return whether sub_net is exactly banned
-    bool IsBanned(const CSubNet& sub_net);
+    bool IsBanned(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
 
     //! Return whether net_addr is discouraged.
-    bool IsDiscouraged(const CNetAddr& net_addr);
+    bool IsDiscouraged(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
 
-    bool Unban(const CNetAddr& net_addr);
+    bool Unban(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
-    bool Unban(const CSubNet& sub_net);
+    bool Unban(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
-    void GetBanned(banmap_t& banmap);
+    void GetBanned(banmap_t& banmap) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
-    void DumpBanlist();
+    void DumpBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
 
 private:
-    void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned);
+    void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
-    bool BannedSetIsDirty();
-    //!set the "dirty" flag for the banlist
-    void SetBannedSetDirty(bool dirty = true);
     //!clean unused entries (if bantime has expired)
-    void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_cs_banned);
+    void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex);
 
-    RecursiveMutex m_cs_banned;
+    Mutex m_banned_mutex;
-    banmap_t m_banned GUARDED_BY(m_cs_banned);
+    banmap_t m_banned GUARDED_BY(m_banned_mutex);
-    bool m_is_dirty GUARDED_BY(m_cs_banned){false};
+    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