refactor: replace RecursiveMutex m_cs_banned with Mutex (and rename)

[w0xlt]

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.

[maflcko]

The second commit is UB. Can you explain why the third commit is safe to do?

[w0xlt]

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]

What does UB mean?

Undefined behavior. See https://en.cppreference.com/w/cpp/language/ub and https://bitcoincore.reviews/17639 for more.

[maflcko]

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]

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]

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.

[hebasto]

Please consider an alternative #24097.

@w0xlt I've taken your commits into #24097.

[DrahtBot]

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]

Closed in favor of #24097.