diff options
author | MarcoFalke <falke.marco@gmail.com> | 2019-01-15 13:41:59 -0500 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2019-01-15 13:42:05 -0500 |
commit | 82ffd4d91832c275f791a17f697534cc677c89fd (patch) | |
tree | 0b424542b339305af76a9a54259a953727cf99e6 /src | |
parent | eb2aecfb80662a91c649ea1455d9812ced05c323 (diff) | |
parent | fa5e373365b8e88fc9894f53f618d3e89f1bec14 (diff) | |
download | bitcoin-82ffd4d91832c275f791a17f697534cc677c89fd.tar.xz |
Merge #14963: mempool, validation: Explain cs_main locking semantics
fa5e373365 validation: Add cs_main locking annotations (MarcoFalke)
fa5c346c5a doc: Add comment to cs_main and mempool::cs (MarcoFalke)
fafe941bdd test: Add missing validation locks (MarcoFalke)
fac4558462 sync: Add RecursiveMutex type alias (MarcoFalke)
Pull request description:
Both the chain state and the transaction pool are validation specific, but access to them is protected by two locks. The two locks have the following semantics:
* Writing to the chain state or adding transactions to the transaction pool -> Take both `cs_main` and `mempool::cs`
* Reading either or removing transactions from the the transaction pool -> Take only the appropriate lock
Tree-SHA512: 6f6e612ffc391904c6434a79a4f3f8de1b928bf0a3e3434b73561037b395e2b40a70a5a4bd8472dd230e9eacc8e5d5374c904a3c509910cf3971dd7ff59a626c
Diffstat (limited to 'src')
-rw-r--r-- | src/bench/mempool_eviction.cpp | 4 | ||||
-rw-r--r-- | src/sync.h | 3 | ||||
-rw-r--r-- | src/test/blockencodings_tests.cpp | 6 | ||||
-rw-r--r-- | src/test/mempool_tests.cpp | 10 | ||||
-rw-r--r-- | src/test/miner_tests.cpp | 2 | ||||
-rw-r--r-- | src/test/policyestimator_tests.cpp | 2 | ||||
-rw-r--r-- | src/txmempool.h | 46 | ||||
-rw-r--r-- | src/validation.cpp | 16 |
8 files changed, 67 insertions, 22 deletions
diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp index 49ea6e88b5..ac8a182358 100644 --- a/src/bench/mempool_eviction.cpp +++ b/src/bench/mempool_eviction.cpp @@ -9,7 +9,7 @@ #include <list> #include <vector> -static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs) +static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) { int64_t nTime = 0; unsigned int nHeight = 1; @@ -108,7 +108,7 @@ static void MempoolEviction(benchmark::State& state) 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 index 40709bdd7f..3857eda56b 100644 --- a/src/sync.h +++ b/src/sync.h @@ -20,7 +20,7 @@ //////////////////////////////////////////////// /* -CCriticalSection mutex; +RecursiveMutex mutex; std::recursive_mutex mutex; LOCK(mutex); @@ -104,6 +104,7 @@ public: * Wrapped mutex: supports recursive locking, but no waiting * TODO: We should move away from using the recursive lock by default. */ +using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>; typedef AnnotatedMixin<std::recursive_mutex> CCriticalSection; /** Wrapped mutex: supports waiting but not recursive locking */ diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index 309b8d2d06..607af8a32a 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -62,7 +62,7 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest) TestMemPoolEntryHelper entry; CBlock block(BuildBlockTestCase()); - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); pool.addUnchecked(entry.FromTx(block.vtx[2])); BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0); @@ -162,7 +162,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) TestMemPoolEntryHelper entry; CBlock block(BuildBlockTestCase()); - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); pool.addUnchecked(entry.FromTx(block.vtx[2])); BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0); @@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) TestMemPoolEntryHelper entry; CBlock block(BuildBlockTestCase()); - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); pool.addUnchecked(entry.FromTx(block.vtx[1])); BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[1]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0); diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 2396aba0f1..23ca9d89ae 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -55,7 +55,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) CTxMemPool testPool; - LOCK(testPool.cs); + LOCK2(cs_main, testPool.cs); // Nothing in pool, remove should do nothing: unsigned int poolSize = testPool.size(); @@ -120,7 +120,7 @@ static void CheckSort(CTxMemPool &pool, std::vector<std::string> &sortedOrder) E BOOST_AUTO_TEST_CASE(MempoolIndexingTest) { CTxMemPool pool; - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; /* 3rd highest fee */ @@ -293,7 +293,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) { CTxMemPool pool; - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; /* 3rd highest fee */ @@ -422,7 +422,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) { CTxMemPool pool; - LOCK(pool.cs); + LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; CMutableTransaction tx1 = CMutableTransaction(); @@ -595,7 +595,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestryTests) 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 index f3648e2eee..5ba1df2ec2 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -99,7 +99,7 @@ static bool TestSequenceLocks(const CTransaction &tx, int flags) EXCLUSIVE_LOCKS // Test suite for ancestor feerate transaction selection. // Implemented as an additional function, rather than a separate test case, // to allow reusing the blockchain created in CreateNewBlock_validity. -static void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::mempool.cs) +static void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs) { // Test the ancestor feerate transaction selection. TestMemPoolEntryHelper entry; diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 431b16cfc2..7b274a1658 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -18,7 +18,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { CBlockPolicyEstimator feeEst; CTxMemPool mpool(&feeEst); - LOCK(mpool.cs); + LOCK2(cs_main, mpool.cs); TestMemPoolEntryHelper entry; CAmount basefee(2000); CAmount deltaFee(100); diff --git a/src/txmempool.h b/src/txmempool.h index fadb554723..b10c9f099f 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -485,7 +485,43 @@ public: > > 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); using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator; @@ -541,8 +577,8 @@ public: // Note that addUnchecked is ONLY called from ATMP outside of tests // and any other callers may break wallet's in-mempool tracking (due to // lack of CValidationInterface::TransactionAddedToMempool callbacks). - void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs); - void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs); + void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction &tx, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN); void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -594,7 +630,7 @@ public: * for). Note: vHashesToUpdate should be the set of transactions from the * disconnected block that have been accepted back into the mempool. */ - void UpdateTransactionsFromBlock(const std::vector<uint256> &vHashesToUpdate); + void UpdateTransactionsFromBlock(const std::vector<uint256>& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Try to calculate all in-mempool ancestors of entry. * (these are all calculated including the tx itself) @@ -636,7 +672,7 @@ public: */ 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 index a18e449af6..6a26bf9baa 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -173,7 +173,7 @@ public: CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Block disconnection on our pcoinsTip: - bool DisconnectTip(CValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions *disconnectpool); + bool DisconnectTip(CValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Manual block validity manipulation: bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); @@ -210,9 +210,17 @@ private: bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params) EXCLUSIVE_LOCKS_REQUIRED(cs_main); } g_chainstate; - - -CCriticalSection cs_main; +/** + * 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. + */ +RecursiveMutex cs_main; BlockMap& mapBlockIndex = g_chainstate.mapBlockIndex; CChain& chainActive = g_chainstate.chainActive; |