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

hebasto · 2020-09-04T15:51:00Z

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.

hebasto · 2020-09-04T15:53:31Z

cc @ajtowns @jnewbery @promag @vasild

hebasto · 2020-09-04T18:03:08Z

Updated c22c4d2 -> e79bc0f (pr19872.01 -> pr19872.02, diff):

fixed CentOS 32-bit build on Travis CI

DrahtBot · 2020-09-04T19:01:28Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove mempool global from interfaces #19848 (Remove mempool global from interfaces by MarcoFalke)
[net processing] Move Misbehaving() to PeerManager #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
p2p: don't add AlreadyHave transactions to recentRejects #19753 (p2p: don't add AlreadyHave transactions to recentRejects by troygiorshev)
Allow updating mempool-txn with cheaper witnesses #19645 (Allow updating mempool-txn with cheaper witnesses by ariard)
ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
Remove mempool global #19556 (Remove mempool global by MarcoFalke)
Tidy up ProcessOrphanTx #19498 (Tidy up ProcessOrphanTx by jnewbery)
Refactor mempool.dat to be extensible, and store missing info #19488 (Refactor mempool.dat to be extensible, and store missing info by luke-jr)
Remove CTxMempool::mapLinks data structure member #19478 (Remove CTxMempool::mapLinks data structure member by JeremyRubin)
validation: re-delegate absurd fee checking from mempool to clients #19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)
RPC: Add ancestor{count,size,fees} to listunspent output #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.

hebasto · 2020-09-05T15:33:41Z

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

hebasto · 2020-09-07T11:26:16Z

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

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

hebasto · 2020-09-07T18:16:29Z

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

jnewbery

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.

jnewbery · 2020-09-08T10:55:23Z

src/txmempool.cpp

 void CTxMemPool::check(const CCoinsViewCache *pcoins) const
 {
-    LOCK(cs);
+    AssertLockHeld(cs);


I don't understand why this change is necessary. check() is never currently called with the lock held.

It is here (mutex locked a little bit earlier).

jnewbery · 2020-09-08T11:19:40Z

src/txmempool.cpp

 void CTxMemPool::check(const CCoinsViewCache *pcoins) const
 {
-    LOCK(cs);
+    AssertLockHeld(cs);


I don't think this change is necessary. check() isn't currently called anywhere with the lock held.

jnewbery · 2020-09-08T11:20:16Z

src/txmempool.cpp

-    LOCK(cs);
+void CTxMemPool::GetTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) const
+{
+    AssertLockHeld(cs);


Again, I don't think this change is necessary. GetTransactionAncestry() is only called with the lock held in the tests, which can be updated.

Right, I also saw that. I guess @hebasto preferred this approach because the changes to tests would be too big and ugly? (if we mandate that GetTransactionAncestry() will lock the mutex itself and the callers should not be holding it)

@vasild Correct.

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.

vasild · 2020-09-08T12:20:55Z

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();
}

hebasto · 2020-09-08T13:05:04Z

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:

"move the lock up the stack if it prevents unnecessary unlocks/locks" (credits to @promag)
E.g., wallet: Avoid locking cs_wallet recursively #19833, this PR (in its current state, ff19c7d)
"two functions could be used - one that locks and another that does not" (credits to @vasild)
E.g., refactor: Make CAddrMan::cs non-recursive #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?

vasild · 2020-09-08T13:10:38Z

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

hebasto · 2020-09-08T13:13:16Z

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

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

hebasto · 2020-09-08T13:25:03Z

@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.

bitcoin/src/txmempool.h

Lines 755 to 761 in 147d50d

    
           void AddUnbroadcastTx(const uint256& txid, const uint256& wtxid) { 
        
               LOCK(cs); 
        
               // Sanity Check: the transaction should also be in the mempool 
        
               if (exists(txid)) { 
        
                   m_unbroadcast_txids[txid] = wtxid; 
        
               } 
        
           }

and mempool_tests.cpp

hebasto · 2020-09-08T13:36:32Z

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

DrahtBot · 2020-09-16T00:14:20Z

🐙 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".}

hebasto changed the title ~~200904 mmx4~~ Avoid locking CTxMemPool::cs recursively in some cases Sep 4, 2020

hebasto mentioned this pull request Sep 4, 2020

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

Open

35 tasks

DrahtBot added Mempool Mining P2P RPC/REST/ZMQ Validation labels Sep 4, 2020

hebasto force-pushed the 200904-mmx4 branch from c22c4d2 to e79bc0f Compare September 4, 2020 17:37

DrahtBot mentioned this pull request Sep 5, 2020

Remove mempool global from interfaces #19848

Merged

DrahtBot added the Needs rebase label Sep 5, 2020

hebasto force-pushed the 200904-mmx4 branch from e79bc0f to 2a3e7cb Compare September 5, 2020 15:32

DrahtBot removed the Needs rebase label Sep 5, 2020

hebasto mentioned this pull request Sep 6, 2020

Avoid locking CTxMemPool::cs recursively in CTxMemPool::DynamicMemoryUsage() #19901

Closed

DrahtBot mentioned this pull request Sep 7, 2020

[net processing] Move Misbehaving() to PeerManager #19791

Merged

DrahtBot added the Needs rebase label Sep 7, 2020

hebasto force-pushed the 200904-mmx4 branch from 2a3e7cb to f594b9a Compare September 7, 2020 11:24

DrahtBot added Needs rebase and removed Needs rebase labels Sep 7, 2020

hebasto added 7 commits September 7, 2020 21:13

refactor: CTxMemPool::check() requires CTxMemPool::cs lock

0314abb

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

refactor: CTxMemPool::GetMinFee() requires CTxMemPool::cs lock

e0f3d72

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

refactor: CTxMemPool::GetTransactionAncestry() requires cs lock

ccb4694

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

refactor: CTxMemPool::IsLoaded() requires CTxMemPool::cs lock

b256cc3

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

refactor: CTxMemPool::size() requires CTxMemPool::cs lock

e79f2a4

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

refactor: CTxMemPool::exists() requires CTxMemPool::cs lock

049d8c5

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

refactor: CTxMemPool::infoAll() requires CTxMemPool::cs lock

598150b

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

hebasto added 3 commits September 7, 2020 21:13

refactor: CTxMemPool::RemoveUnbroadcastTx() requires CTxMemPool::cs lock

551cf1b

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

refactor: CTxMemPool::GetUnbroadcastTxs() requires CTxMemPool::cs lock

e546ffd

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

refactor: CTxMemPool::queryHashes() requires CTxMemPool::cs lock

ff19c7d

No change in behavior, some call sites already held the lock and a lock is added in the remaining sites.

hebasto force-pushed the 200904-mmx4 branch from f594b9a to ff19c7d Compare September 7, 2020 18:16

DrahtBot removed the Needs rebase label Sep 7, 2020

DrahtBot mentioned this pull request Sep 8, 2020

p2p: don't add AlreadyHave transactions to recentRejects #19753

Merged

jnewbery reviewed Sep 8, 2020

View reviewed changes

promag reviewed Sep 8, 2020

View reviewed changes

This was referenced Sep 8, 2020

refactor: Remove unused CTxMemPool::clear() helper #19909

Merged

Yet another change to reduce recursive mempool locking #19917

Closed

DrahtBot added the Needs rebase label Sep 16, 2020

hebasto marked this pull request as draft November 8, 2020 20:58

hebasto closed this Aug 24, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 24, 2022

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

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

Uh oh!

Conversation

hebasto commented Sep 4, 2020

Uh oh!

hebasto commented Sep 4, 2020

Uh oh!

hebasto commented Sep 4, 2020

Uh oh!

DrahtBot commented Sep 4, 2020

Conflicts

Uh oh!

hebasto commented Sep 5, 2020

Uh oh!

hebasto commented Sep 7, 2020

Uh oh!

hebasto commented Sep 7, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

vasild Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

vasild Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Sep 8, 2020

Uh oh!

hebasto commented Sep 8, 2020

Uh oh!

vasild commented Sep 8, 2020

Uh oh!

hebasto commented Sep 8, 2020

Uh oh!

hebasto commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Sep 8, 2020

Uh oh!

DrahtBot commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hebasto commented Sep 8, 2020 •

edited

Loading