diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp --- a/src/bench/mempool_eviction.cpp +++ b/src/bench/mempool_eviction.cpp @@ -10,7 +10,7 @@ #include static void AddTx(const CTransactionRef &tx, const Amount &nFee, - CTxMemPool &pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs) { + CTxMemPool &pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) { int64_t nTime = 0; double dPriority = 10.0; unsigned int nHeight = 1; @@ -98,7 +98,7 @@ tx7.vout[1].nValue = 10 * COIN; CTxMemPool pool; - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); // Create transaction references outside the "hot loop" const CTransactionRef tx1_r{MakeTransactionRef(tx1)}; const CTransactionRef tx2_r{MakeTransactionRef(tx2)}; diff --git a/src/sync.h b/src/sync.h --- a/src/sync.h +++ b/src/sync.h @@ -19,7 +19,7 @@ ///////////////////////////////////////////////// /* -CCriticalSection mutex; +RecursiveMutex mutex; std::recursive_mutex mutex; LOCK(mutex); @@ -101,6 +101,7 @@ * Wrapped mutex: supports recursive locking, but no waiting * TODO: We should move away from using the recursive lock by default. */ +using RecursiveMutex = AnnotatedMixin; typedef AnnotatedMixin CCriticalSection; /** Wrapped mutex: supports waiting but not recursive locking */ diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -72,7 +72,7 @@ TestMemPoolEntryHelper entry; CBlock block(BuildBlockTestCase()); - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); pool.addUnchecked(block.vtx[2]->GetId(), entry.FromTx(block.vtx[2])); BOOST_CHECK_EQUAL( pool.mapTx.find(block.vtx[2]->GetId())->GetSharedTx().use_count(), @@ -183,7 +183,7 @@ TestMemPoolEntryHelper entry; CBlock block(BuildBlockTestCase()); - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); pool.addUnchecked(block.vtx[2]->GetId(), entry.FromTx(block.vtx[2])); BOOST_CHECK_EQUAL( pool.mapTx.find(block.vtx[2]->GetId())->GetSharedTx().use_count(), @@ -278,7 +278,7 @@ TestMemPoolEntryHelper entry; CBlock block(BuildBlockTestCase()); - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); pool.addUnchecked(block.vtx[1]->GetId(), entry.FromTx(block.vtx[1])); BOOST_CHECK_EQUAL( pool.mapTx.find(block.vtx[1]->GetId())->GetSharedTx().use_count(), diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -163,7 +163,7 @@ } CTxMemPool testPool; - LOCK(testPool.cs); + LOCK2(cs_main, testPool.cs); // Nothing in pool, remove should do nothing: unsigned int poolSize = testPool.size(); @@ -271,7 +271,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) { CTxMemPool pool; - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; /* 3rd highest fee */ @@ -462,7 +462,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) { CTxMemPool pool; - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; /* 3rd highest fee */ @@ -602,7 +602,7 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) { CTxMemPool pool; - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; entry.dPriority = 10.0; Amount feeIncrement = MEMPOOL_FULL_FEE_INCREMENT.GetFeePerK(); @@ -896,7 +896,7 @@ size_t ancestors, descendants; CTxMemPool pool; - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; /* Base transaction */ diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -106,7 +106,7 @@ static void TestPackageSelection(const CChainParams &chainparams, const CScript &scriptPubKey, const std::vector &txFirst) - EXCLUSIVE_LOCKS_REQUIRED(::g_mempool.cs) { + EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::g_mempool.cs) { // Test the ancestor feerate transaction selection. TestMemPoolEntryHelper entry; diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -17,7 +17,7 @@ BOOST_AUTO_TEST_CASE(MempoolMinimumFeeEstimate) { CTxMemPool mpool; - LOCK(mpool.cs); + LOCK2(cs_main, mpool.cs); TestMemPoolEntryHelper entry; // Create a transaction template diff --git a/src/txmempool.h b/src/txmempool.h --- a/src/txmempool.h +++ b/src/txmempool.h @@ -522,7 +522,43 @@ CompareTxMemPoolEntryByAncestorFee>>> indexed_transaction_set; - mutable CCriticalSection cs; + /** + * This mutex needs to be locked when accessing `mapTx` or other members + * that are guarded by it. + * + * @par Consistency guarantees + * + * By design, it is guaranteed that: + * + * 1. Locking both `cs_main` and `mempool.cs` will give a view of mempool + * that is consistent with current chain tip (`chainActive` and + * `pcoinsTip`) and is fully populated. Fully populated means that if the + * current active chain is missing transactions that were present in a + * previously active chain, all the missing transactions will have been + * re-added to the mempool and should be present if they meet size and + * consistency constraints. + * + * 2. Locking `mempool.cs` without `cs_main` will give a view of a mempool + * consistent with some chain that was active since `cs_main` was last + * locked, and that is fully populated as described above. It is ok for + * code that only needs to query or remove transactions from the mempool + * to lock just `mempool.cs` without `cs_main`. + * + * To provide these guarantees, it is necessary to lock both `cs_main` and + * `mempool.cs` whenever adding transactions to the mempool and whenever + * changing the chain tip. It's necessary to keep both mutexes locked until + * the mempool is consistent with the new chain tip and fully populated. + * + * @par Consistency bug + * + * The second guarantee above is not currently enforced, but + * https://github.com/bitcoin/bitcoin/pull/14193 will fix it. No known code + * in bitcoin currently depends on second guarantee, but it is important to + * fix for third party code that needs be able to frequently poll the + * mempool without locking `cs_main` and without encountering missing + * transactions during reorgs. + */ + mutable RecursiveMutex cs; indexed_transaction_set mapTx GUARDED_BY(cs); typedef indexed_transaction_set::nth_index<0>::type::iterator txiter; @@ -590,9 +626,10 @@ // and any other callers may break wallet's in-mempool tracking (due to // lack of CValidationInterface::TransactionAddedToMempool callbacks). void addUnchecked(const uint256 &hash, const CTxMemPoolEntry &entry) - EXCLUSIVE_LOCKS_REQUIRED(cs); + EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void addUnchecked(const uint256 &hash, const CTxMemPoolEntry &entry, - setEntries &setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs); + setEntries &setAncestors) + EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive( const CTransaction &tx, @@ -650,7 +687,8 @@ * Note: txidsToUpdate should be the set of transactions from the * disconnected block that have been accepted back into the mempool. */ - void UpdateTransactionsFromBlock(const std::vector &txidsToUpdate); + void UpdateTransactionsFromBlock(const std::vector &txidsToUpdate) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** * Try to calculate all in-mempool ancestors of entry. @@ -715,7 +753,7 @@ void GetTransactionAncestry(const uint256 &txid, size_t &ancestors, size_t &descendants) const; - unsigned long size() { + unsigned long size() const { LOCK(cs); return mapTx.size(); } diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -225,8 +225,17 @@ /** * Global state + * + * Mutex to guard access to validation specific variables, such as reading + * or changing the chainstate. + * + * This may also need to be locked when updating the transaction pool, e.g. on + * AcceptToMemoryPool. See CTxMemPool::cs comment for details. + * + * The transaction pool has a separate lock to allow reading from it and the + * chainstate at the same time. */ -CCriticalSection cs_main; +RecursiveMutex cs_main; BlockMap &mapBlockIndex = g_chainstate.mapBlockIndex; CChain &chainActive = g_chainstate.chainActive;