Replace RecursiveMutex m_cs_banned with Mutex, and rename it

hebasto commented at 5:30 pm on January 18, 2022: member

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

hebasto marked this as a draft on Jan 18, 2022

hebasto force-pushed on Jan 18, 2022

DrahtBot commented at 5:58 pm on January 18, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, vasild, maflcko, achow101
Concept ACK	shaavan
Stale ACK	w0xlt, promag

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

hebasto marked this as ready for review on Jan 18, 2022

w0xlt approved

w0xlt commented at 10:39 pm on January 18, 2022: contributor

tACK 39e101f

In addition to the refactoring, this PR:

. Adds lock acquisition in BanMan::DumpBanlist before calling SweepBanned().

. Replaces BanMan::SetBannedSetDirty() with WITH_LOCK(m_cs_banned, m_is_dirty = false); (write operation) or simply m_is_dirty (non-blocking read operation).

. SweepBanned() no longer locks the mutex, just checks that it is held. This function is now called with WITH_LOCK(m_banned_mutex, SweepBanned());.

. BanMan::DumpBanlist called BanMan::SweepBanned() twice (directly and via BanMan::GetBanned). Now it gets rid of BanMan::GetBanned and handles banmap_t banmap within the lock scope.

shaavan approved

shaavan commented at 8:55 am on January 19, 2022: contributor

Code Review ACK 39e101f4a257240f098432d6fa77b322924c0109

I prefer that the SweepBanned function, instead of locking m_cs_banned by itself, now asserts it being locked by its caller function. This prevents any need for double locking, and hence m_cs_banned could be safely converted from RecursiveMutex to Mutex.

The renaming of m_cs_banned to m_banned_mutex is an improvement, and I agree with it.

The conversion of m_banned_mutex from RecursiveMutex to Mutex is very cleanly implemented and is clutter-free.

promag commented at 9:49 am on January 24, 2022: contributor

Code review ACK 39e101f4a257240f098432d6fa77b322924c0109.

There is a behavior change where m_client_interface->BannedListChanged() is now called with the lock held. The only place that connects to that signal is in ClientModel where updateBanlist is called asynchronously, so I’d say this change is deadlock-free.

hebasto commented at 10:01 am on January 24, 2022: member

@promag

There is a behavior change where m_client_interface->BannedListChanged() is now called with the lock held.

The same behavior is on the master branch: https://github.com/bitcoin/bitcoin/blob/d0bf9bb6a539f151ec92725d20a2b6c22cb095a5/src/banman.cpp#L160-L166 isn’t it?

promag commented at 10:08 am on January 24, 2022: contributor

@hebasto right! Only checked that from DumpBanlist the lock isn’t held, but from GetBanned it is. So this change makes it consistent 🎉

in src/banman.cpp:43 in 3dae951930 outdated

38@@ -39,9 +39,11 @@ BanMan::~BanMan()
39 
40 void BanMan::DumpBanlist()
41 {
42-    SweepBanned(); // clean unused entries (if bantime has expired)
43-
44-    if (!BannedSetIsDirty()) return;
45+    {
46+        LOCK(m_cs_banned);

maflcko commented at 5:38 pm on January 25, 2022:

first commit: if there are any races possible, shouldn’t this lock be held for the whole duration of the function?

hebasto commented at 9:33 am on January 26, 2022:

Yes. But another dedicated mutex should be used for that. No need to hold the m_banned_mutex during disk operations: m_ban_db.Write(banmap) and LogPrint(BCLog::NET, ...).

Do you prefer to add it into this PR?

maflcko commented at 9:36 am on January 26, 2022:

Is it that common to modify (write) the banmap to disk that this requires a separate mutex?

hebasto commented at 9:45 am on January 26, 2022:

Is it that common to modify (write) the banmap to disk that this requires a separate mutex?

I’m not sure if I understand your Q correctly. But if you mean that m_ban_db.Write(banmap) modifies m_banned, it is not the case because a local copy banmap is used.

And isn’t it a suboptimal design when callers of BanMan public functions are waiting for a relatively and unpredictably long I/O operation?

maflcko commented at 9:48 am on January 26, 2022:

Setting the dirty flag to false can only be done after the map has been written, so I don’t see how a second mutex can solve this

hebasto commented at 10:41 am on January 26, 2022:

Case A – current implementation (both master and this PR)

Nothing prevents from m_ban_db.Write(banmap) being called concurrently from different threads.

Case B – `BanMan::DumpBanlist()` acquires `m_cs_banned`/`m_banned_mutex` for the whole its duration

Performance degradation is possible. E.g., caller of BanMan::IsDiscouraged() must wait for BanMan::DumpBanlist() finished in another thread.

Case C – `BanMan::DumpBanlist()` acquires a dedicated `m_dump_mutex` for the whole its duration

No concurrent m_ban_db.Write(banmap) calls. No performance degradation.

Setting the dirty flag to false can only be done after the map has been written

How about the following patch:

 0--- a/src/banman.cpp
 1+++ b/src/banman.cpp
 2@@ -45,11 +45,12 @@ void BanMan::DumpBanlist()
 3         SweepBanned();
 4         if (!m_is_dirty) return;
 5         banmap = m_banned;
 6+        m_is_dirty = false;
 7     }
 8 
 9     int64_t n_start = GetTimeMillis();
10-    if (m_ban_db.Write(banmap)) {
11-        WITH_LOCK(m_banned_mutex, m_is_dirty = false);
12+    if (!m_ban_db.Write(banmap)) {
13+        WITH_LOCK(m_banned_mutex, m_is_dirty = true);
14     }
15 
16     LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk  %dms\n", banmap.size(),

?

maflcko commented at 11:04 am on January 26, 2022:

How about the following patch:

Can you explain the patch?

hebasto commented at 11:19 am on January 26, 2022:

Currently, when thread A performs m_ban_db.Write(banmap) thread B can set m_is_dirty = true which will be overwritten by thread A just after successful finish of m_ban_db.Write(banmap) call. This causes inconsistency between the actual m_banned state and the m_is_dirty flag.

With the suggested patch it is guaranteed that every time we access m_is_dirty its value is consistent with the actual m_banned state.

maflcko commented at 11:48 am on January 26, 2022:

But what is the point of the dirty flag when it will effectively be always true?

hebasto commented at 11:58 am on January 26, 2022:

It’s being set to false in BanMan::DumpBanlist(), it stays false when m_ban_db.Write(banmap)) is successful. And it stays false if other member functions do no change m_banned.

It allows to skip m_ban_db.Write(banmap)) for unchanged m_banned.

maflcko commented at 12:22 pm on January 26, 2022:

So you are suggesting this patch combined with a dedicated dump mutex?

hebasto commented at 12:26 pm on January 26, 2022:

Yes, but in another PR to keep this one focused on the transition form RecursiveMutex to Mutex.

maflcko commented at 12:30 pm on January 26, 2022:

Strictly speaking, fixing the logical race is a bugfix, so it might be better to do it first to allow easier cherry-picking to older commits. Would you mind going with the fix first?

hebasto commented at 12:31 pm on January 26, 2022:

Sure!

hebasto commented at 12:32 pm on January 26, 2022:

Is it that common to modify (write) the banmap to disk that this requires a separate mutex?

Please check out DumpMempool implementation.

hebasto commented at 11:34 am on January 31, 2022:

Would you mind going with the fix first?

Done in #24168.

maflcko added the label Refactoring on Jan 25, 2022

hebasto commented at 3:35 pm on January 26, 2022: member

@MarcoFalke

Strictly speaking, fixing the logical race is a bugfix, so it might be better to do it first to allow easier cherry-picking to older commits. Would you mind going with the fix first?

I split bugfixes into #24168.

Fellow reviewers, please review #24168 first.

Converting this to a draft for now.

hebasto marked this as a draft on Jan 26, 2022

maflcko referenced this in commit eacc0e87f8 on Jan 31, 2022

DrahtBot added the label Needs rebase on Jan 31, 2022

hebasto force-pushed on Jan 31, 2022

hebasto marked this as ready for review on Jan 31, 2022

hebasto commented at 11:33 am on January 31, 2022: member

Rebased 39e101f4a257240f098432d6fa77b322924c0109 -> 0ad0288dcb462e4156ba8f619eebe2fa321ce7af (pr24097.02 -> pr24097.03) on top of the merged #24168.

DrahtBot removed the label Needs rebase on Jan 31, 2022

in src/banman.cpp:64 in 0ad0288dcb outdated

57 
58     int64_t n_start = GetTimeMillis();
59     if (!m_ban_db.Write(banmap)) {
60-        SetBannedSetDirty(true);
61+        LOCK(m_banned_mutex);
62+        m_is_dirty = true;

shaavan commented at 11:09 am on February 1, 2022:

Doubt: Why did you prefer this way of writing code over using WITH_LOCK, which you discussed with @MarcoFalke in the earlier comment.

 0--- a/src/banman.cpp
 1+++ b/src/banman.cpp
 2@@ -45,11 +45,12 @@ void BanMan::DumpBanlist()
 3         SweepBanned();
 4         if (!m_is_dirty) return;
 5         banmap = m_banned;
 6+        m_is_dirty = false;
 7     }
 8 
 9     int64_t n_start = GetTimeMillis();
10-    if (m_ban_db.Write(banmap)) {
11-        WITH_LOCK(m_banned_mutex, m_is_dirty = false);
12+    if (!m_ban_db.Write(banmap)) {
13+        WITH_LOCK(m_banned_mutex, m_is_dirty = true);
14     }
15 
16     LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk  %dms\n", banmap.size(),

hebasto commented at 12:00 pm on February 2, 2022:

With m_is_dirty = false; a few lines above I think it’s more readable, no?

shaavan commented at 9:20 am on February 3, 2022:

Makes sense. So it was an aesthetic choice, which increases code readability, and both of these don’t have any functional difference. Am I correct in my analysis?

hebasto commented at 9:21 am on February 3, 2022:

Yes, you are. See https://github.com/bitcoin/bitcoin/blob/c194293883fbb656779102309b2cb3e60889feff/src/sync.h#L270

shaavan commented at 10:18 am on February 3, 2022:

Got it. Thanks, @hebasto!

in src/banman.cpp:184 in 0ad0288dcb outdated

174 }
175 
176 void BanMan::SweepBanned()
177 {
178+    AssertLockHeld(m_banned_mutex);
179+

shaavan commented at 11:09 am on February 1, 2022:

nit: This extra line can be removed.

hebasto commented at 12:03 pm on February 2, 2022:

Of course, it’s only a personal opinion, but for me it is not an “extra” line. It is a line which separates assertions from the function logic that helps to make the presence of assertions clearer.

shaavan commented at 9:23 am on February 3, 2022:

I agree with you. However, I think it is better to follow a code uniformity. And some places have a line between, and some don’t. This breaks the code uniformity. But these are just my 2 cents, and I am okay with whatever you think would be the optimal choice in this matter.

shaavan commented at 11:10 am on February 1, 2022: contributor

Concept ACK

I had a nit suggestion and a doubt that I would like to discuss.

sidhujag referenced this in commit b7b7bf20e3 on Feb 1, 2022

hebasto marked this as a draft on May 16, 2022

hebasto commented at 8:20 pm on May 16, 2022: member

Converted to draft for now. Please review #25149 first.

DrahtBot added the label Needs rebase on May 24, 2022

refactor: Get rid of `BanMan::BannedSetIsDirty()` d88c0d8440

refactor: Get rid of `BanMan::SetBannedSetDirty()` 46709c5f27

hebasto marked this as ready for review on May 24, 2022

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-

784c316f9c

refactor: replace RecursiveMutex m_banned_mutex with Mutex 0fb2908708

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

37d150d8c5

hebasto force-pushed on May 24, 2022

hebasto commented at 8:31 am on May 24, 2022: member

Converted to draft for now. Please review #25149 first.

Rebased on top of the merged #25149.

cc @w0xlt @theStack @ajtowns

DrahtBot removed the label Needs rebase on May 24, 2022

theStack commented at 2:46 pm on May 24, 2022: contributor

Concept ACK

theStack approved

theStack commented at 3:29 pm on August 19, 2022: contributor

Code-review ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7

achow101 requested review from jamesob on Apr 25, 2023

DrahtBot added the label CI failed on May 31, 2023

DrahtBot removed the label CI failed on May 31, 2023

achow101 requested review from vasild on Sep 20, 2023

achow101 requested review from maflcko on Sep 20, 2023

in src/banman.h:64 in 37d150d8c5

59@@ -60,40 +60,37 @@ class BanMan
60 public:
61     ~BanMan();
62     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();
67+    void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);
68+    void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex);

vasild commented at 1:44 pm on October 10, 2023:

I find it strange that there is a requirement to not hold the mutex when calling the constructor because at the time the constructor is called the mutex does not exist yet (it is a member variable).

I checked that if I remove these annotations, then indeed -Wthread-safety-negative produces a warning due to the LOCK() inside the constructor. The LOCK() is not needed either because the constructor will not be called concurrently with another method (or constructor). Removing the LOCK() of course causes warnings that touching m_banned requires the mutex :( I guess this is some weird limitation in the thread safety analysis.

ajtowns commented at 0:38 am on October 11, 2023:

The constructor is BanMan, the annotated functions are Ban ?

vasild commented at 11:30 am on October 11, 2023:

Right! :facepalm:

vasild approved

vasild commented at 1:45 pm on October 10, 2023: contributor

ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7

DrahtBot requested review from shaavan on Oct 10, 2023

DrahtBot requested review from promag on Oct 10, 2023

DrahtBot requested review from w0xlt on Oct 10, 2023

DrahtBot removed review request from shaavan on Oct 10, 2023

DrahtBot removed review request from w0xlt on Oct 10, 2023

DrahtBot requested review from shaavan on Oct 10, 2023

DrahtBot requested review from w0xlt on Oct 10, 2023

DrahtBot removed review request from shaavan on Oct 11, 2023

DrahtBot removed review request from w0xlt on Oct 11, 2023

DrahtBot requested review from shaavan on Oct 11, 2023

DrahtBot requested review from w0xlt on Oct 11, 2023

DrahtBot removed review request from shaavan on Oct 11, 2023

DrahtBot removed review request from w0xlt on Oct 11, 2023

DrahtBot requested review from shaavan on Oct 11, 2023

DrahtBot requested review from w0xlt on Oct 11, 2023

hebasto commented at 11:14 am on October 24, 2023: member

@maflcko Mind having a look at this PR please?

DrahtBot removed review request from shaavan on Oct 24, 2023

DrahtBot removed review request from w0xlt on Oct 24, 2023

DrahtBot requested review from w0xlt on Oct 24, 2023

DrahtBot requested review from shaavan on Oct 24, 2023

maflcko commented at 8:53 am on October 25, 2023: member

ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 🎾

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 🎾
36hI0SRliStMofz4F+1OOUsdVFjJFjIicGUMlRpqAROJJGilc1hwLblhiZg1bxrodqlBVD+O8wLbCbdwCzCQPCA==

DrahtBot removed review request from maflcko on Oct 25, 2023

DrahtBot removed review request from w0xlt on Oct 25, 2023

DrahtBot removed review request from shaavan on Oct 25, 2023

DrahtBot requested review from w0xlt on Oct 25, 2023

DrahtBot requested review from shaavan on Oct 25, 2023

hebasto commented at 2:46 pm on November 2, 2023: member

@fanquake @achow101

Is there anything else to do here?

DrahtBot removed review request from w0xlt on Nov 2, 2023

DrahtBot removed review request from shaavan on Nov 2, 2023

DrahtBot requested review from w0xlt on Nov 2, 2023

DrahtBot requested review from shaavan on Nov 2, 2023

achow101 commented at 5:50 pm on November 2, 2023: member

ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7

DrahtBot removed review request from w0xlt on Nov 2, 2023

DrahtBot removed review request from shaavan on Nov 2, 2023

DrahtBot requested review from w0xlt on Nov 2, 2023

DrahtBot requested review from shaavan on Nov 2, 2023

achow101 merged this on Nov 2, 2023

achow101 closed this on Nov 2, 2023

hebasto deleted the branch on Nov 2, 2023

PastaPastaPasta referenced this in commit eb92c28790 on Jan 12, 2024

PastaPastaPasta referenced this in commit 69e42928b4 on Jan 12, 2024

PastaPastaPasta referenced this in commit 50287e2403 on Jan 14, 2024

bitcoin locked this on Nov 1, 2024

Replace RecursiveMutex m_cs_banned with Mutex, and rename it #24097

Code Coverage

Reviews

Conflicts

Case A – current implementation (both master and this PR)

Case B – BanMan::DumpBanlist() acquires m_cs_banned/m_banned_mutex for the whole its duration

Case C – BanMan::DumpBanlist() acquires a dedicated m_dump_mutex for the whole its duration

Case B – `BanMan::DumpBanlist()` acquires `m_cs_banned`/`m_banned_mutex` for the whole its duration

Case C – `BanMan::DumpBanlist()` acquires a dedicated `m_dump_mutex` for the whole its duration