RecursiveMutex to Mutex transition

recursiveMx.md

RecursiveMutex to Mutex transition

What are we talking about?

using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>;

typedef AnnotatedMixin<std::mutex> Mutex;

1. Do we want to move away from RecursiveMutex (in general)?

Yes.

Pros:

• src/sync.h#L120:

 * TODO: We should move away from using the recursive lock by default.

• Pieter Wuille:

<sipa> recursive mutexes are evil
<sipa> there is probably a place for them, like goto
<sipa> but almost always, they just lead to badly designed abstractions
<sipa> a clear design should have code that operates outside the critical section, and code that operates inside
<sipa> but not code that works in both

• Anthony Williams (C++ Concurrency in Action, 2019, 3.3.3 Recursive locking):

Most of the time, if you think you want a recursive mutex, you probably need to change your design instead.

Cons: None.

The tracking issue: #19303.

2. Whether to switch the CTxMemPool:cs to non-recursive mutex?

Pros: see #1.

Cons:

• Marco Falke:

... invasive refactors are hard to prioritize due to the conflicts they cause with non-refactoring changes.

• Anthony Towns:

... if removing RecursiveMutex makes the code/reasoning more complex, it's not a good idea.

• Marco Falke:

the RecursiveMutex just works (TM).

Note 1. src/txmempool.h#L566-L569:

     * To provide these guarantees, it is necessary to lock both `cs_main` and
     * `mempool.cs` whenever adding transactions to the mempool and whenever
     * changing the chain tip. It's necessary to keep both mutexes locked until
     * the mempool is consistent with the new chain tip and fully populated.

Note 2. The goal is achievable, see: #19306.

3. Which way to switch the CTxMemPool:cs to non-recursive mutex?

If #2 gets project consensus, there are two approaches to untie some tangled knots in non-trivaial cases:

1. João Barbosa (promag):

... move the lock up the stack if it prevents unnecessary unlocks/locks.

2. Vasil Dimov:

... two functions could be used - one that locks and another that does not...

If doing big refactors around CTxMemPool::cs, we may consider making it private.

There is this from Clang documentation, which sounds perfectly reasonable to me:

Good software engineering practice dictates that mutexes should be private members, because the locking mechanism used by a thread-safe class is part of its internal implementation.