Merge bitcoin/bitcoin#25149: refactor: Add thread safety annotation to BanMan::SweepBanned()

ab75388 refactor: Remove redundant scope in `BanMan::SweepBanned()` (Hennadii Stepanov)
52c0b3e refactor: Add thread safety annotation to `BanMan::SweepBanned()` (Hennadii Stepanov)
3919059 refactor: Move code from ctor into private `BanMan::LoadBanlist()` (Hennadii Stepanov)

Pull request description:

  This PR adds a proper thread safety annotation to `BanMan::SweepBanned()`.

  Also a simple refactoring applied.

ACKs for top commit:
  ajtowns:
    ACK ab75388
  w0xlt:
    ACK bitcoin/bitcoin@ab75388
  theStack:
    Code-review ACK ab75388

Tree-SHA512: 8699079c308454f3ffe72be2e77f0935214156bd509f9338b1104f8d128bfdd02ee06ee8c1c99b2eefdf317a00edd555d52990caaeb1ed4540eedc1e3c9d1faf

Author: MacroFake

src/banman.cpp

@@ -16,6 +16,19 @@
 BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time)
     : m_client_interface(client_interface), m_ban_db(std::move(ban_file)), m_default_ban_time(default_ban_time)
 {
+    LoadBanlist();
+    DumpBanlist();
+}
+
+BanMan::~BanMan()
+{
+    DumpBanlist();
+}
+
+void BanMan::LoadBanlist()
+{
+    LOCK(m_cs_banned);
+
     if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
 
     int64_t n_start = GetTimeMillis();
@@ -29,13 +42,6 @@ BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t
         m_banned = {};
         m_is_dirty = true;
     }
-
-    DumpBanlist();
-}
-
-BanMan::~BanMan()
-{
-    DumpBanlist();
 }
 
 void BanMan::DumpBanlist()
@@ -173,23 +179,24 @@ void BanMan::GetBanned(banmap_t& banmap)
 
 void BanMan::SweepBanned()
 {
+    AssertLockHeld(m_cs_banned);
+
     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;
         }
     }
+
     // update UI
     if (notify_ui && m_client_interface) {
         m_client_interface->BannedListChanged();

src/banman.h

@@ -80,11 +80,12 @@ class BanMan
     void DumpBanlist();
 
 private:
+    void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned);
     bool BannedSetIsDirty();
     //!set the "dirty" flag for the banlist
     void SetBannedSetDirty(bool dirty = true);
     //!clean unused entries (if bantime has expired)
-    void SweepBanned();
+    void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_cs_banned);
 
     RecursiveMutex m_cs_banned;
     banmap_t m_banned GUARDED_BY(m_cs_banned);