refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

This PR replaces RecursiveMutex CTxMemPool::cs with Mutex CTxMemPool:cs.

All of the related code branches are covered by appropriate lock assertions to insure that the mutex locking policy has not been changed by accident

Related to #19303.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19826 (Pass mempool reference to chainstate constructor by MarcoFalke)
• #19806 (validation: UTXO snapshot activation by jamesob)
• #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
• #19753 (p2p: don't add AlreadyHave transactions to recentRejects by troygiorshev)
• #19652 (Add thread safety annotations to Mempool{Info}ToJSON() by hebasto)
• #19647 (Add thread safety annotations to CTxMemPool methods by hebasto)
• #19645 (Allow updating mempool-txn with cheaper witnesses by ariard)
• #19610 (p2p: refactor AlreadyHave(), CInv::type, INV/TX processing by jonatack)
• #19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
• #19544 (refactor: Add ParseBool to rpc/util by fjahr)
• #19498 (Tidy up ProcessOrphanTx by jnewbery)
• #19488 (Refactor mempool.dat to be extensible, and store missing info by luke-jr)
• #19478 (Remove CTxMempool::mapLinks data structure member by JeremyRubin)
• #19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)
• #19093 (RPC: Return transaction fee from testmempoolaccept by rajarshimaitra)
• #18191 (Change UpdateForDescendants to use Epochs by JeremyRubin)
• #18017 (txmempool: split epoch logic into class by ajtowns)
• #13990 (Allow fee estimation to work with lower fees by ajtowns)
• #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

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.

From IRC:

<sipa> eh, i agree - that change isn't a step in the right direction @sipa Which direction is right?

From IRC:

<jeremyrubin> I think it's basically getting rid of a recursive mutex for code that's still designed to take a recursive mutex <gwillen> it's better than a true recursive mutex because it's not possible to recurse by accident, you have to declare at call time which behavior you want (although better if you had to declare statically at compile time) <sipa> it looks like that <jeremyrubin> The correct refactor would be to make the code not do anything fancy with locks, or to just leave it <jeremyrubin> gwillen: I think the chances of a bug or error in custom logic is higher than a recursive mutex <jeremyrubin> accidental recursion seems unlikely... <jeremyrubin> and accidental recursion would be a bug lock or no <gwillen> (sorry I mean, accidental mutex recursion, that is, calling a function while holding a mutex, not expecting the callee to also lock it, resulting in the callee violating the caller's invariants) <gwillen> (this is the fundamental problem of recursive mutexes) <gwillen> (and I assume the motivation behind hebasto's refactor)

Correct, @gwillen :)

Searching for concept (N)ACKs before separating digestible chunks for reviewing into smaller pulls.

Currently this adds a lot of code complexity. Also, it adds mental complexity to write code that doesn't crash or deadlock/UB whereas the RecursiveMutex just works (TM).

Maybe there is a way to achieve the same without the added complexity? For example, always force the caller to take the lock for the right scope. This would also solve issues of non-atomic RPC responses or at make them more visible. Though, it makes caller code slightly more verbose.

Currently this adds a lot of code complexity. Also, it adds mental complexity to write code that doesn't crash or deadlock/UB whereas the RecursiveMutex just works (TM).

A function that locks RecursiveMutex there are no guarantees that protected invariants are held before locking. This adds mental complexity to read code.

Maybe there is a way to achieve the same without the added complexity? For example, always force the caller to take the lock for the right scope. This would also solve issues of non-atomic RPC responses or at make them more visible.

I assume these steps are much easier for non-recursive mutex, no?

@hebasto Do you have any thoughts on "function takes bool locked" versus "split function into _locked and _unlocked variants"?

In some of the cases here, I see that the latter would require some (maybe substantial) refactoring to avoid code duplication, and I assume that's why you didn't go that route. But that would not only eliminate conditional locking (which is scary), it would probably allow the use of RAII locks. I'm curious to hear your thoughts.

@gwillen

@hebasto Do you have any thoughts on "function takes bool locked" versus "split function into _locked and _unlocked variants"?

For new code I prefer clean "split function into _locked and _unlocked variants" like #19238 (review). Unfortunately, that is not the case in this PR.

In some of the cases here, I see that the latter would require some (maybe substantial) refactoring to avoid code duplication, and I assume that's why you didn't go that route.

You are correct. I've used the "function takes bool locked" approach in 72f7486b5ebe96762c5d5a68849c61e58c812ffd and 21787abedd7a0808c0175a0b1d795df97fd3b970 as a quick-and-dirty solution to fix broken tests.

But that would not only eliminate conditional locking (which is scary), it would probably allow the use of RAII locks. I'm curious to hear your thoughts.

Hmm... In 21787abedd7a0808c0175a0b1d795df97fd3b970 I've had to drop the RAII lock in favor of a pair ENTER_CRITICAL_SECTION() - LEAVE_CRITICAL_SECTION(). That is why I dont't like this approach.

Rebased 4e00526c689c4164d02d8ca76331f3ed5da7b13c -> c9e7d011d69bb1ef965945bf90d7441165430808 (pr19306.01 -> pr19306.02) due to the conflict with #19293.

Rebased c9e7d011d69bb1ef965945bf90d7441165430808 -> 3fc8fa23fc4ff2978c89bba46f08e746b6e4c154 (pr19306.02 -> pr19306.03) due to the conflicts with #18027 and #19198.

Rebased 3fc8fa23fc4ff2978c89bba46f08e746b6e4c154 -> ff3d969891b0687219906f18e66c5bb499915968 (pr19306.03 -> pr19306.04) due to the conflict with https://github.com/bitcoin-core/gui/pull/11.

Rebased ff3d969891b0687219906f18e66c5bb499915968 -> 511670449669116df5488cd4f807de620e55a7e3 (pr19306.04 -> pr19306.05) due to the conflict with #19331.

Rebased 511670449669116df5488cd4f807de620e55a7e3 -> 0c03cea32d3ab30a58e62bbe42af6ebef016ede4 (pr19306.05 -> pr19306.06) due to the conflicts with #19174 and #19474.

Rebased 0c03cea32d3ab30a58e62bbe42af6ebef016ede4 -> 656cba72f475603497e318ed3f01db4ab694b2af (pr19306.06 -> pr19306.07) due to the conflicts with #18044 and #18637.

Rebased 656cba72f475603497e318ed3f01db4ab694b2af -> e23248c51c092269a33cde7ad0ff70a815876396 (pr19306.07 -> pr19306.08) due to the conflicts with #18011, #19569, and #19604.

Some commits split out into #19647. So please start reviewing from #19647.

Rebased e23248c51c092269a33cde7ad0ff70a815876396 -> 95b4daa91ea4eecd3f345a20f285fb0528f5070d (pr19306.08 -> pr19306.09) due to the conflict with #19569.

Rebased 95b4daa91ea4eecd3f345a20f285fb0528f5070d -> 1faf43ac3b2cb2a116f501e56c2cd6fed903409c (pr19306.09 -> pr19306.10) due to the conflict with #19704.

Rebased 1faf43ac3b2cb2a116f501e56c2cd6fed903409c -> a887d73dcb05d59067635aff91baf85e0c7c7396 (pr19306.10 -> pr19306.12) due to the merge conflicts.