locks: Annotate CTxMemPool::check to require cs_main by dongcarl · Pull Request #20972 · bitcoin/bitcoin

dongcarl · 2021-01-20T20:40:32Z

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:

hebasto

Concept ACK.

hebasto · 2021-01-20T20:52:20Z

src/txmempool.h

Maybe use explicit global namespace

Suggested change

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

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

?

hebasto · 2021-01-20T20:56:28Z

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

dongcarl · 2021-01-20T21:05:53Z

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 · 2021-01-20T21:07:23Z

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

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 "bitcoin#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.

dongcarl · 2021-01-20T21:18:31Z

Thanks @hebasto, made the changes!

jnewbery · 2021-01-21T08:58:18Z

Code review ACK b396467

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

maflcko

ACK b396467

maflcko · 2021-01-21T09:53:37Z

src/txmempool.cpp


    if (GetRand(m_check_ratio) >= 1) return;

+    AssertLockHeld(::cs_main);


Suggested change

AssertLockHeld(::cs_main);

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?

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:

bitcoin/src/txmempool.cpp

Line 627 in 3734adb

CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins));

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

bitcoin/src/txmempool.cpp

Line 610 in 3734adb

bool fCheckResult = tx.IsCoinBase() || Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, spendheight, txfee);

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.

jonatack

ACK b396467 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 (comment).

maflcko · 2021-01-21T11:50:03Z

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 · 2021-01-21T13:44:20Z

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

…main b396467 locks: Annotate CTxMemPool::check to require cs_main (Carl Dong) Pull request description: ``` 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 "bitcoin#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: 1. bitcoin#20158 (comment) 2. bitcoin#20158 (review) 3. bitcoin#20749 (comment) ACKs for top commit: jnewbery: Code review ACK b396467 MarcoFalke: ACK b396467 jonatack: ACK b396467 review and debug built, verified that `cs_main` is held by callers of `CTxMemPool::check()` in `PeerManagerImpl::ProcessOrphanTx()`, `PeerManagerImpl::ProcessMessage()`, and `CChainState::ActivateBestChainStep()` Tree-SHA512: 4635cddb4aa1af9532bb657b2f9c4deec4568d16ba28c574eae91bb77368cd40e23c3c720a9de11cec78e7ad678a44a5e25af67f13214b86b56e777e0c35a026

Summary: 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 "[[bitcoin/bitcoin#20158 | core#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. This is a backport of [[bitcoin/bitcoin#20972 | core#20972]] Depends on D10842 Test Plan: With TSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10844

dongcarl mentioned this pull request Jan 20, 2021

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Merged

hebasto reviewed Jan 20, 2021

View reviewed changes

dongcarl force-pushed the 2021-01-CTxMemPool-spendheight-lock-inversion branch from b8ac27c to b396467 Compare January 20, 2021 21:16

DrahtBot added the Mempool label Jan 20, 2021

maflcko added the Refactoring label Jan 21, 2021

maflcko approved these changes Jan 21, 2021

View reviewed changes

jonatack reviewed Jan 21, 2021

View reviewed changes

maflcko merged commit 1f45e85 into bitcoin:master Jan 21, 2021

dongcarl mentioned this pull request Feb 1, 2021

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

JaredTate mentioned this pull request Oct 30, 2021

WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code DigiByte-Core/digibyte#77

Closed

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

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


		if (GetRand(m_check_ratio) >= 1) return;

		AssertLockHeld(::cs_main);

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

Conversation

dongcarl commented Jan 20, 2021 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 20, 2021

Uh oh!

dongcarl commented Jan 20, 2021

Uh oh!

hebasto commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongcarl commented Jan 20, 2021

Uh oh!

jnewbery commented Jan 21, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 21, 2021

Uh oh!

jnewbery commented Jan 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dongcarl commented Jan 20, 2021 •

edited by maflcko

Loading

hebasto commented Jan 20, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading