refactor: replace RecursiveMutex `m_cs_banned` with Mutex (and rename) #24092

pull w0xlt wants to merge 3 commits into bitcoin:master from w0xlt:refactor_replace_recursive_mutex_m_cs_banned changing 2 files +50 −33

w0xlt commented at 7:27 AM on January 18, 2022: contributor

This PR is related to #19303 and gets rid of the RecursiveMutex m_cs_banned.

The last commit prevents double lock for m_banned_mutex by changing BanMan::GetBanned() to lock it only after BanMan::SweepBanned() releases it.

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-

431722c491

refactor: replace RecursiveMutex m_banned_mutex with Mutex b43ad37dc4
fanquake added the label Refactoring on Jan 18, 2022
MarcoFalke commented at 7:34 AM on January 18, 2022: member

The second commit is UB. Can you explain why the third commit is safe to do?
w0xlt commented at 9:39 AM on January 18, 2022: contributor
What does UB mean? The second commit follows the same format adopted in cac8366 .

Can you explain why the third commit is safe to do?

The third commit is necessary to avoid double lock error for m_banned_mutex. BanMan::SweepBanned() is only called internally in src/banman.cpp.
1. It is called in BanMan::BanMan but this function does not acquire lock, so there is no need to change it.
2. It is called in BanMan::DumpBanlist and this is the same case as above.
3. BanMan::GetBanned acquired lock for mutex before calling BanMan::SweepBanned so a double lock occurred. With RecursiveMutex, it wasn´t a problem. But using mutex, the lock acquisiton needs to be moved after BanMan::SweepBanned is completed and its lock released.
jonatack commented at 9:57 AM on January 18, 2022: member

What does UB mean?

Undefined behavior. See https://en.cppreference.com/w/cpp/language/ub and https://bitcoincore.reviews/17639 for more.
MarcoFalke commented at 10:18 AM on January 18, 2022: member

The third commit is necessary

I understand that it is necessary to avoid the UB. I was asking why it is safe to release the lock in between.
shaavan commented at 11:35 AM on January 18, 2022: contributor
I was asking why it is safe to release the lock in between.

The way I understand the risk behind releasing a lock in between functions is, say:
1. SweepBanned is called.
2. LOCK is set up, m_banned is updated, LOCK is released.
3. GetBanned sets up LOCK; the value of m_banned is copied to banmap.
However, say another function acquires lock between steps 2 and 3 and updates the value of m_banned somehow, then we will get a false value return for banmap in step 3.

I shall take another look at the codebase and see if this scenario is possible.
hebasto commented at 12:07 PM on January 18, 2022: member

I'd suggest to apply Clang Thread Safety Analysis (TSA) annotations. In particular, void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex);.

Also BanMan::SweepBanned could have two versions: the first one which requires the lock is hold, and the second one which requires the lock is not hold.

prevent double lock for 'm_banned_mutex'

`BanMan::SweepBanned()` is only called internally in `src/banman.cpp`.
`BanMan::GetBanned()` locks 'm_banned_mutex' and then calls `BanMan::SweepBanned()`
without releasing the lock.
This commit adds a new funtion `SweepBanned(bool isLockHeld)` to handle this.

fd6c51b04d

w0xlt force-pushed on Jan 18, 2022
hebasto commented at 5:31 PM on January 18, 2022: member

Please consider an alternative #24097. @w0xlt I've taken your commits into #24097.
DrahtBot commented at 6:02 PM on January 18, 2022: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24097 (Replace RecursiveMutex m_cs_banned with Mutex, and rename it by hebasto)
- #19825 (rpc: simpler setban and new ban manipulation commands by dhruv)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
w0xlt force-pushed on Jan 18, 2022
w0xlt commented at 9:30 PM on January 18, 2022: contributor

Closed in favor of #24097.
w0xlt closed this on Jan 18, 2022
DrahtBot locked this on Jan 18, 2023

Contributors

Labels

Linked (view graph)

#19303 Replace all of the RecursiveMutex instances with the Mutex ones

refactor: replace RecursiveMutex `m_cs_banned` with Mutex (and rename) #24092

Conflicts