locks: Annotate CTxMemPool::check to require cs

dongcarl commented at 8:40 pm on January 20, 2021: member

 0Currently, CTxMemPool::check locks CTxMemPool's own cs member, then
 1calls GetSpendHeight which locks cs_main. This can potentially cause an
 2undesirable lock invesion since CTxMemPool's cs is supposed to be locked
 3after cs_main.
 4
 5This does not cause us any problems right now because all callers of
 6CTxMemPool already lock cs_main before calling CTxMemPool::check, which
 7means that the LOCK(cs_main) in GetSpendHeight becomes benign.
 8
 9However, it is currently possible for new code to be added which calls
10CTxMemPool::check without locking cs_main (which would be dangerous).
11Therefore we should make it explicit that cs_main needs to be held
12before calling CTxMemPool::check.
13
14NOTE: After all review-only assertions are removed in "#20158 |
15      tree-wide: De-globalize ChainstateManager", and assuming that we
16      keep the changes in "validation: Pass in spendheight to
17      CTxMemPool::check", we can re-evaluate to see if this annotation
18      is still necessary.

Previous discussions:

#20158 (review)
#20158#pullrequestreview-557117202
#20749 (review)

in src/txmempool.h:605 in b8ac27c636 outdated

601@@ -602,7 +602,7 @@ class CTxMemPool
602      * all inputs are in the mapNextTx array). If sanity-checking is turned off,
603      * check does nothing.
604      */
605-    void check(const CCoinsViewCache *pcoins) const;
606+    void check(const CCoinsViewCache *pcoins) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

hebasto commented at 8:52 pm on January 20, 2021:

Maybe use explicit global namespace

0    void check(const CCoinsViewCache *pcoins) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

?

hebasto commented at 8:52 pm on January 20, 2021: member

Concept ACK.

hebasto commented at 8:56 pm on January 20, 2021: member

It seems reasonable to add a lock assertion into the CTxMemPool::check, no?

dongcarl commented at 9:05 pm on January 20, 2021: member

It seems reasonable to add a lock assertion into the CTxMemPool::check, no?

Not sure what you mean by this? Do you mean adding an AssertLockHeld(::cs_main) instead of the annotation?

hebasto commented at 9:07 pm on January 20, 2021: member

It seems reasonable to add a lock assertion into the CTxMemPool::check, no?

Not sure what you mean by this? Do you mean adding an AssertLockHeld(::cs_main) instead of the annotation?

Both. Annotation for compile-time check, and assertion for run-time check.

EDIT: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

locks: Annotate CTxMemPool::check to require cs_main

Currently, CTxMemPool::check locks CTxMemPool's own cs member, then
calls GetSpendHeight which locks cs_main. This can potentially cause an
undesirable lock invesion since CTxMemPool's cs is supposed to be locked
after cs_main.

This does not cause us any problems right now because all callers of
CTxMemPool already lock cs_main before calling CTxMemPool::check, which
means that the LOCK(cs_main) in GetSpendHeight becomes benign.

However, it is currently possible for new code to be added which calls
CTxMemPool::check without locking cs_main (which would be dangerous).
Therefore we should make it explicit that cs_main needs to be held
before calling CTxMemPool::check.

NOTE: After all review-only assertions are removed in "#20158 |
      tree-wide: De-globalize ChainstateManager", and assuming that we
      keep the changes in "validation: Pass in spendheight to
      CTxMemPool::check", we can re-evaluate to see if this annotation
      is still necessary.

b396467053

dongcarl force-pushed on Jan 20, 2021

dongcarl commented at 9:18 pm on January 20, 2021: member

Thanks @hebasto, made the changes!

DrahtBot added the label Mempool on Jan 20, 2021

jnewbery commented at 8:58 am on January 21, 2021: member

Code review ACK b3964670537d0943b8fb2d8f2ea419cbefd4835a

Verified that the three call sites (two in net_processing.cpp and one in validation.cpp) all hold cs_main before calling mempool.check().

MarcoFalke added the label Refactoring on Jan 21, 2021

in src/txmempool.cpp:621 in b396467053

617@@ -618,6 +618,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
618 
619     if (GetRand(m_check_ratio) >= 1) return;
620 
621+    AssertLockHeld(::cs_main);

MarcoFalke commented at 9:53 am on January 21, 2021:

0    AssertLockHeld(::cs_main); // for GetSpendHeight

nit: Could mention for which function this is needed?

nit: Since all callers of GetSpendHeight already have cs_main, would it make sense to remove the recursive lock from GetSpendHeight itself and replace it with a debug-only/compile-only AssertLockHeld/EXCLUSIVE_LOCKS_REQUIRED?

jnewbery commented at 10:37 am on January 21, 2021:

Could mention for which function this is needed?

I’ve dug into this function a bit more, and I think it’s more than that. check() takes a copy of the CoinsTip:

https://github.com/bitcoin/bitcoin/blob/3734adba390cef881445c4de780e2a3bb080c512/src/txmempool.cpp#L627

and is then fetching coins from that CCoinsViewCache via CheckTxInputs():

https://github.com/bitcoin/bitcoin/blob/3734adba390cef881445c4de780e2a3bb080c512/src/txmempool.cpp#L610

If the coins in that CCoinsView were to be updated by a different thread while check() is running, then we could hit an assert. There’s an implicit assumption that the CCoinsView won’t change while this function is running, so I think we need cs_main throughout this function.

It’s a shame that mempool calls back into validation in this way and has a circular dependency, but that’s how it is for now.

MarcoFalke approved

MarcoFalke commented at 9:53 am on January 21, 2021: member

ACK b3964670537d0943b8fb2d8f2ea419cbefd4835a

jonatack commented at 11:37 am on January 21, 2021: member

ACK b3964670537d0943b8fb2d8f2ea419cbefd4835a review and debug built, verified that cs_main is held by callers of CTxMemPool::check() in PeerManagerImpl::ProcessOrphanTx(), PeerManagerImpl::ProcessMessage(), and CChainState::ActivateBestChainStep()

Happy to re-ACK if you add documentation or update the commit message with the info in #20972 (review).

MarcoFalke commented at 11:50 am on January 21, 2021: member

if you add documentation

If this is changed, I’d prefer to add the lock annotations (to GetSpendHeight and maybe others), so that the code is self-documenting. As in: Accidentally removing a lock(annotation) will fail to compile with a verbose reason.

jnewbery commented at 1:44 pm on January 21, 2021: member

Happy to re-ACK if you add documentation or update the commit message with the info in #20972 (comment).

I wasn’t really suggesting that we add documentation, just that we don’t add misleading documentation that cs_main is only needed for GetSpendHeight. I can see the benefit to documenting the reasoning in the commit log, and would be happy to reACK a push that adds that commit message.

I’d prefer to add the lock annotations (to GetSpendHeight and maybe others), so that the code is self-documenting.

I’ll commit to reviewing this (here or in a follow-up PR).

MarcoFalke merged this on Jan 21, 2021

MarcoFalke closed this on Jan 21, 2021

sidhujag referenced this in commit c0b3bb2d2a on Jan 21, 2021

Fabcien referenced this in commit ab4c27192a on Jan 19, 2022

DrahtBot locked this on Aug 16, 2022

locks: Annotate CTxMemPool::check to require cs_main #20972