Skip to content

Commit 0857f29

Browse files
achow101achow101
committed
Merge bitcoin/bitcoin#24097: Replace RecursiveMutex m_cs_banned with Mutex, and rename it
37d150d refactor: Add more negative `!m_banned_mutex` thread safety annotations (Hennadii Stepanov) 0fb2908 refactor: replace RecursiveMutex m_banned_mutex with Mutex (w0xlt) 784c316 scripted-diff: rename m_cs_banned -> m_banned_mutex (w0xlt) 46709c5 refactor: Get rid of `BanMan::SetBannedSetDirty()` (Hennadii Stepanov) d88c0d8 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 37d150d 🎾 achow101: ACK 37d150d theStack: Code-review ACK 37d150d vasild: ACK 37d150d Tree-SHA512: 5e9d40101a09af6e0645a6ede67432ea68631a1b960f9e6af0ad07415ca7718a30fcc1aad5182d1d5265dc54c26aba2008fc9973840255c09adbab8fedf10075
2 parents 2e9454a + 37d150d commit 0857f29

File tree

2 files changed

+32
-46
lines changed

2 files changed

+32
-46
lines changed

‎src/banman.cpp‎

Lines changed: 15 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ BanMan::~BanMan()
2828

2929
void BanMan::LoadBanlist()
3030
{
31-
LOCK(m_cs_banned);
31+
LOCK(m_banned_mutex);
3232

3333
if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
3434

@@ -52,16 +52,17 @@ void BanMan::DumpBanlist()
5252

5353
banmap_t banmap;
5454
{
55-
LOCK(m_cs_banned);
55+
LOCK(m_banned_mutex);
5656
SweepBanned();
57-
if (!BannedSetIsDirty()) return;
57+
if (!m_is_dirty) return;
5858
banmap = m_banned;
59-
SetBannedSetDirty(false);
59+
m_is_dirty = false;
6060
}
6161

6262
const auto start{SteadyClock::now()};
6363
if (!m_ban_db.Write(banmap)) {
64-
SetBannedSetDirty(true);
64+
LOCK(m_banned_mutex);
65+
m_is_dirty = true;
6566
}
6667

6768
LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk %dms\n", banmap.size(),
@@ -71,7 +72,7 @@ void BanMan::DumpBanlist()
7172
void BanMan::ClearBanned()
7273
{
7374
{
74-
LOCK(m_cs_banned);
75+
LOCK(m_banned_mutex);
7576
m_banned.clear();
7677
m_is_dirty = true;
7778
}
@@ -81,14 +82,14 @@ void BanMan::ClearBanned()
8182

8283
bool BanMan::IsDiscouraged(const CNetAddr& net_addr)
8384
{
84-
LOCK(m_cs_banned);
85+
LOCK(m_banned_mutex);
8586
return m_discouraged.contains(net_addr.GetAddrBytes());
8687
}
8788

8889
bool BanMan::IsBanned(const CNetAddr& net_addr)
8990
{
9091
auto current_time = GetTime();
91-
LOCK(m_cs_banned);
92+
LOCK(m_banned_mutex);
9293
for (const auto& it : m_banned) {
9394
CSubNet sub_net = it.first;
9495
CBanEntry ban_entry = it.second;
@@ -103,7 +104,7 @@ bool BanMan::IsBanned(const CNetAddr& net_addr)
103104
bool BanMan::IsBanned(const CSubNet& sub_net)
104105
{
105106
auto current_time = GetTime();
106-
LOCK(m_cs_banned);
107+
LOCK(m_banned_mutex);
107108
banmap_t::iterator i = m_banned.find(sub_net);
108109
if (i != m_banned.end()) {
109110
CBanEntry ban_entry = (*i).second;
@@ -122,7 +123,7 @@ void BanMan::Ban(const CNetAddr& net_addr, int64_t ban_time_offset, bool since_u
122123

123124
void BanMan::Discourage(const CNetAddr& net_addr)
124125
{
125-
LOCK(m_cs_banned);
126+
LOCK(m_banned_mutex);
126127
m_discouraged.insert(net_addr.GetAddrBytes());
127128
}
128129

@@ -139,7 +140,7 @@ void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_uni
139140
ban_entry.nBanUntil = (normalized_since_unix_epoch ? 0 : GetTime()) + normalized_ban_time_offset;
140141

141142
{
142-
LOCK(m_cs_banned);
143+
LOCK(m_banned_mutex);
143144
if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) {
144145
m_banned[sub_net] = ban_entry;
145146
m_is_dirty = true;
@@ -161,7 +162,7 @@ bool BanMan::Unban(const CNetAddr& net_addr)
161162
bool BanMan::Unban(const CSubNet& sub_net)
162163
{
163164
{
164-
LOCK(m_cs_banned);
165+
LOCK(m_banned_mutex);
165166
if (m_banned.erase(sub_net) == 0) return false;
166167
m_is_dirty = true;
167168
}
@@ -172,15 +173,15 @@ bool BanMan::Unban(const CSubNet& sub_net)
172173

173174
void BanMan::GetBanned(banmap_t& banmap)
174175
{
175-
LOCK(m_cs_banned);
176+
LOCK(m_banned_mutex);
176177
// Sweep the banlist so expired bans are not returned
177178
SweepBanned();
178179
banmap = m_banned; //create a thread safe copy
179180
}
180181

181182
void BanMan::SweepBanned()
182183
{
183-
AssertLockHeld(m_cs_banned);
184+
AssertLockHeld(m_banned_mutex);
184185

185186
int64_t now = GetTime();
186187
bool notify_ui = false;
@@ -203,15 +204,3 @@ void BanMan::SweepBanned()
203204
m_client_interface->BannedListChanged();
204205
}
205206
}
206-
207-
bool BanMan::BannedSetIsDirty()
208-
{
209-
LOCK(m_cs_banned);
210-
return m_is_dirty;
211-
}
212-
213-
void BanMan::SetBannedSetDirty(bool dirty)
214-
{
215-
LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag
216-
m_is_dirty = dirty;
217-
}

‎src/banman.h‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -60,40 +60,37 @@ class BanMan
6060
public:
6161
~BanMan();
6262
BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
63-
void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
64-
void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
65-
void Discourage(const CNetAddr& net_addr);
66-
void ClearBanned();
63+
void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
64+
void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
65+
void Discourage(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
66+
void ClearBanned() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
6767

6868
//! Return whether net_addr is banned
69-
bool IsBanned(const CNetAddr& net_addr);
69+
bool IsBanned(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
7070

7171
//! Return whether sub_net is exactly banned
72-
bool IsBanned(const CSubNet& sub_net);
72+
bool IsBanned(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
7373

7474
//! Return whether net_addr is discouraged.
75-
bool IsDiscouraged(const CNetAddr& net_addr);
75+
bool IsDiscouraged(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
7676

77-
bool Unban(const CNetAddr& net_addr);
78-
bool Unban(const CSubNet& sub_net);
79-
void GetBanned(banmap_t& banmap);
80-
void DumpBanlist();
77+
bool Unban(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
78+
bool Unban(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
79+
void GetBanned(banmap_t& banmap) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
80+
void DumpBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
8181

8282
private:
83-
void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned);
84-
bool BannedSetIsDirty();
85-
//!set the "dirty" flag for the banlist
86-
void SetBannedSetDirty(bool dirty = true);
83+
void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
8784
//!clean unused entries (if bantime has expired)
88-
void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_cs_banned);
85+
void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex);
8986

90-
RecursiveMutex m_cs_banned;
91-
banmap_t m_banned GUARDED_BY(m_cs_banned);
92-
bool m_is_dirty GUARDED_BY(m_cs_banned){false};
87+
Mutex m_banned_mutex;
88+
banmap_t m_banned GUARDED_BY(m_banned_mutex);
89+
bool m_is_dirty GUARDED_BY(m_banned_mutex){false};
9390
CClientUIInterface* m_client_interface = nullptr;
9491
CBanDB m_ban_db;
9592
const int64_t m_default_ban_time;
96-
CRollingBloomFilter m_discouraged GUARDED_BY(m_cs_banned) {50000, 0.000001};
93+
CRollingBloomFilter m_discouraged GUARDED_BY(m_banned_mutex) {50000, 0.000001};
9794
};
9895

9996
#endif // BITCOIN_BANMAN_H

0 commit comments

Comments
 (0)