From d88c0d8440cf640ef4f2c7a40b8b8b31bfd38f23 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 18 Jan 2022 19:02:05 +0200 Subject: [PATCH 1/5] refactor: Get rid of `BanMan::BannedSetIsDirty()` --- src/banman.cpp | 8 +------- src/banman.h | 1 - 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 2a6e0e010fe..50dc0750e1f 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -53,7 +53,7 @@ void BanMan::DumpBanlist() { LOCK(m_cs_banned); SweepBanned(); - if (!BannedSetIsDirty()) return; + if (!m_is_dirty) return; banmap = m_banned; SetBannedSetDirty(false); } @@ -203,12 +203,6 @@ void BanMan::SweepBanned() } } -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 diff --git a/src/banman.h b/src/banman.h index 77b043f081b..7f3c74733ed 100644 --- a/src/banman.h +++ b/src/banman.h @@ -81,7 +81,6 @@ public: 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) From 46709c5f27bf6cbc8eba1298b04bd079da2cdded Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 18 Jan 2022 19:18:02 +0200 Subject: [PATCH 2/5] refactor: Get rid of `BanMan::SetBannedSetDirty()` --- src/banman.cpp | 11 +++-------- src/banman.h | 2 -- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 50dc0750e1f..5b2a1795433 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -55,12 +55,13 @@ void BanMan::DumpBanlist() SweepBanned(); if (!m_is_dirty) return; banmap = m_banned; - SetBannedSetDirty(false); + m_is_dirty = false; } int64_t n_start = GetTimeMillis(); if (!m_ban_db.Write(banmap)) { - SetBannedSetDirty(true); + LOCK(m_cs_banned); + m_is_dirty = true; } LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk %dms\n", banmap.size(), @@ -202,9 +203,3 @@ void BanMan::SweepBanned() m_client_interface->BannedListChanged(); } } - -void BanMan::SetBannedSetDirty(bool dirty) -{ - LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag - m_is_dirty = dirty; -} diff --git a/src/banman.h b/src/banman.h index 7f3c74733ed..7a032dfdd01 100644 --- a/src/banman.h +++ b/src/banman.h @@ -81,8 +81,6 @@ public: private: void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); - //!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); From 784c316f9cb664c9577cbfed1873bae573efd1b4 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 18 Jan 2022 03:26:37 -0300 Subject: [PATCH 3/5] scripted-diff: rename m_cs_banned -> m_banned_mutex -BEGIN VERIFY SCRIPT- s() { sed -i 's/m_cs_banned/m_banned_mutex/g' $1; } s src/banman.cpp s src/banman.h -END VERIFY SCRIPT- --- src/banman.cpp | 24 ++++++++++++------------ src/banman.h | 12 ++++++------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 5b2a1795433..3286ca8965d 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -27,7 +27,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); @@ -51,7 +51,7 @@ void BanMan::DumpBanlist() banmap_t banmap; { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); SweepBanned(); if (!m_is_dirty) return; banmap = m_banned; @@ -60,7 +60,7 @@ void BanMan::DumpBanlist() int64_t n_start = GetTimeMillis(); if (!m_ban_db.Write(banmap)) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_is_dirty = true; } @@ -71,7 +71,7 @@ void BanMan::DumpBanlist() void BanMan::ClearBanned() { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_banned.clear(); m_is_dirty = true; } @@ -81,14 +81,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 +103,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 +122,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 +139,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 +161,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 +172,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 +180,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; diff --git a/src/banman.h b/src/banman.h index 7a032dfdd01..e0377227444 100644 --- a/src/banman.h +++ b/src/banman.h @@ -80,17 +80,17 @@ public: void DumpBanlist(); private: - void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); + void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //!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; - banmap_t m_banned GUARDED_BY(m_cs_banned); - bool m_is_dirty GUARDED_BY(m_cs_banned){false}; + RecursiveMutex 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 From 0fb29087080a4e60d7c709ff5edf14e830ef3a69 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 18 Jan 2022 03:29:14 -0300 Subject: [PATCH 4/5] refactor: replace RecursiveMutex m_banned_mutex with Mutex --- src/banman.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/banman.h b/src/banman.h index e0377227444..c0e07b866dc 100644 --- a/src/banman.h +++ b/src/banman.h @@ -84,7 +84,7 @@ private: //!clean unused entries (if bantime has expired) void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex); - RecursiveMutex m_banned_mutex; + 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; From 37d150d8c5ffcb2bddcd99951a739e97571194c7 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 24 May 2022 10:27:30 +0200 Subject: [PATCH 5/5] refactor: Add more negative `!m_banned_mutex` thread safety annotations Could be verified with $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' $ make clean $ make 2>&1 | grep m_banned_mutex --- src/banman.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/banman.h b/src/banman.h index c0e07b866dc..9200f07aaf4 100644 --- a/src/banman.h +++ b/src/banman.h @@ -60,24 +60,24 @@ 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 CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false); - void Discourage(const CNetAddr& net_addr); - void ClearBanned(); + 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) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void Discourage(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + 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 CSubNet& sub_net); - void GetBanned(banmap_t& banmap); - void DumpBanlist(); + bool Unban(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + bool Unban(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void GetBanned(banmap_t& banmap) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void DumpBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); private: void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);