Avoid locking CTxMemPool::cs recursively in some cases #19872

This is another step (after #19854) to transit CTxMemPool::cs from RecursiveMutex to Mutex.

Split out from #19306. Thread safety annotations, lock assertions, and required explicit locking added. No behavior change.

Please note that now, since #19668 has been merged, it is safe to apply AssertLockHeld() macros as they do not swallow compile time Thread Safety Analysis warnings.

cc @ajtowns @jnewbery @promag @vasild

Updated c22c4d2320f288f57fd04515d29b72d6342ea11a -> e79bc0f275977fafcb98a3065256222639a46328 (pr19872.01 -> pr19872.02, diff):

• fixed CentOS 32-bit build on Travis CI

<!--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:

• #19848 (Remove mempool global from interfaces by MarcoFalke)
• #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
• #19753 (p2p: don't add AlreadyHave transactions to recentRejects by troygiorshev)
• #19645 (Allow updating mempool-txn with cheaper witnesses by ariard)
• #19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
• #19556 (Remove mempool global by MarcoFalke)
• #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)
• #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.

Rebased e79bc0f275977fafcb98a3065256222639a46328 -> 2a3e7cb01f4004310e0853d182e08574698db111 (pr19872.02 -> pr19872.03) due to the conflict with #19848.

Rebased 2a3e7cb01f4004310e0853d182e08574698db111 -> f594b9a4a2957b7ad2e95ad7b17cab858e15282f (pr19872.03 -> pr19872.04) due to the conflicts with #19478 and #19556.

Rebased f594b9a4a2957b7ad2e95ad7b17cab858e15282f -> ff19c7ddb8b8b076e5c538be434abd53af572b8d (pr19872.04 -> pr19872.05) due to the conflict with #19791.

I've reviewed the first three commits, and I'm not sure this is the right direction. Adding locks to lots of places outside the mempool seems unnecessary.

IMO we should move the lock up the stack if it prevents unnecessary unlocks/locks. That's not the case, for instance in CTxMemPool::exists. Looks like it's a premature change.

Concept ACK.

Similar changes are justified, assuming we want to get rid of recursive mutexes.

Recursive mutexes are used by functions that need to own a mutex to do their stuff, but some of the callers of those functions own the mutex and some don't. So the function does not have a clear interface wrt the mutex, rather it has a vague interface like "the caller may own the mutex and I will lock it myself anyway".

IMO the cleaner interface is if the mutex is acquired inside the function and none of the callers own it. If that is not possible then the function must not lock it and mandate that the callers own it. If that is too verbose for the callers, then two functions could be used - one that locks and another that does not:

funcUnprotected() EXCLUSIVE_LOCKS_REQUIRED(cs)
{
    AssertLockHeld(cs);
    ...
}

func() EXCLUSIVE_LOCKS_REQUIRED(!cs)
{
    LOCK(cs);
    funcUnprotected();
}

Many thanks to @jnewbery, @promag and @vasild for your attention to thread safety design!

Let me summarize the two approaches to preventing recursive locking of a mutex that are suggested/discussed/used in the repo:

1. "move the lock up the stack if it prevents unnecessary unlocks/locks" (credits to @promag) E.g., #19833, this PR (in its current state, ff19c7ddb8b8b076e5c538be434abd53af572b8d)

2. "two functions could be used - one that locks and another that does not" (credits to @vasild) E.g., #19238

I believe we should reach the consensus about approach we will use while migrating from RecursiveMutex to Mutex before any further step.

Is it a topic for meeting?

0. (most preferred, if possible) lock the mutex only inside the function that needs it

0. (most preferred, if possible) lock the mutex only inside the function that needs it

Sure! I just described variants when this is not possible :)

@promag

IMO we should move the lock up the stack if it prevents unnecessary unlocks/locks. That's not the case, for instance in CTxMemPool::exists. Looks like it's a premature change.

https://github.com/bitcoin/bitcoin/blob/147d50d63e07f600b414273a9f6b84f9f4ad9696/src/txmempool.h#L755-L761

and mempool_tests.cpp

For unit tests also there is @MarcoFalke's approach (fad3a7c5b9118d4d190401c0fbbd3e2068dffadc from #19909) to subclass CTxMemPool, and add member functions with locking required for tests.

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}