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

pull dongcarl wants to merge 1 commits into bitcoin:master from dongcarl:2021-01-CTxMemPool-spendheight-lock-inversion changing 2 files +2 −1

dongcarl commented at 8:40 PM on January 20, 2021: contributor

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.

Previous discussions:

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

dongcarl cross-referenced this on Jan 20, 2021 from issue [Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex by dongcarl

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

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

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

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:

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

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
dongcarl cross-referenced this on Feb 1, 2021 from issue tree-wide: De-globalize ChainstateManager by dongcarl
JaredTate cross-referenced this on Oct 30, 2021 from issue WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code by JaredTate
Fabcien referenced this in commit ab4c27192a on Jan 19, 2022
bitcoin locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#20158 tree-wide: De-globalize ChainstateManager #20749 [Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex