foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-02 09:46:52 -05:00
Code Issues Activity

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

Browse source 
b396467053 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 "#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. https://github.com/bitcoin/bitcoin/pull/20158#discussion_r520639845
  2. https://github.com/bitcoin/bitcoin/pull/20158#pullrequestreview-557117202
  3. https://github.com/bitcoin/bitcoin/pull/20749#discussion_r559425521

ACKs for top commit:
  jnewbery:
    Code review ACK b396467053
  MarcoFalke:
    ACK b396467053
  jonatack:
    ACK b396467053 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
This commit is contained in:
MarcoFalke 2021-01-21 16:45:40 +01:00
commit 1f45e85509
No known key found for this signature in database
GPG key ID: D2EA4850E7528B25
2 changed files with 2 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/txmempool.cpp
View file

@ -618,6 +618,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
if (GetRand(m_check_ratio) >= 1) return;
AssertLockHeld(::cs_main);
LOCK(cs);
LogPrint(BCLog::MEMPOOL, "Checking mempool with %u transactions and %u inputs\n", (unsigned int)mapTx.size(), (unsigned int)mapNextTx.size());

2
src/txmempool.h
View file

@ -602,7 +602,7 @@ public:
* all inputs are in the mapNextTx array). If sanity-checking is turned off,
* check does nothing.
*/
void check(const CCoinsViewCache *pcoins) const;
void check(const CCoinsViewCache *pcoins) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
// addUnchecked must updated state for all ancestors of a given transaction,
// to track size/count of descendant transactions. First version of