diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -565,7 +565,7 @@ nTransactionsUpdatedLastLP = nTransactionsUpdatedLast; } - // Release the wallet and main lock while waiting + // Release lock while waiting LEAVE_CRITICAL_SECTION(cs_main); { checktxtime = 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 @@ -883,27 +883,30 @@ vtx.push_back(MakeTransactionRef(*tx)); } DisconnectedBlockTransactions disconnectPool; - disconnectPool.addForBlock(vtx, testPool); - CheckDisconnectPoolOrder(disconnectPool, correctlyOrderedIds, - disconnectedTxns.size()); - - // If the mempool is empty, importMempool doesn't change - // disconnectPool - disconnectPool.importMempool(testPool); - CheckDisconnectPoolOrder(disconnectPool, correctlyOrderedIds, - disconnectedTxns.size()); - + LOCK(testPool.cs); { - LOCK2(cs_main, testPool.cs); - // Add all unconfirmed transactions in testPool - for (auto tx : unconfTxns) { - TestMemPoolEntryHelper entry; - testPool.addUnchecked(entry.FromTx(*tx)); + disconnectPool.addForBlock(vtx, testPool); + CheckDisconnectPoolOrder(disconnectPool, correctlyOrderedIds, + disconnectedTxns.size()); + + // If the mempool is empty, importMempool doesn't change + // disconnectPool + disconnectPool.importMempool(testPool); + CheckDisconnectPoolOrder(disconnectPool, correctlyOrderedIds, + disconnectedTxns.size()); + + { + LOCK(cs_main); + // Add all unconfirmed transactions in testPool + for (auto tx : unconfTxns) { + TestMemPoolEntryHelper entry; + testPool.addUnchecked(entry.FromTx(*tx)); + } } - } - // Now we test importMempool with a non empty mempool - disconnectPool.importMempool(testPool); + // Now we test importMempool with a non empty mempool + disconnectPool.importMempool(testPool); + } CheckDisconnectPoolOrder(disconnectPool, correctlyOrderedIds, disconnectedTxns.size() + unconfTxns.size()); diff --git a/src/txmempool.h b/src/txmempool.h --- a/src/txmempool.h +++ b/src/txmempool.h @@ -538,22 +538,13 @@ * `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; //! All tx hashes/entries in mapTx, in random order - std::vector> vTxHashes; + std::vector> vTxHashes GUARDED_BY(cs); struct CompareIteratorById { bool operator()(const txiter &a, const txiter &b) const { @@ -620,13 +611,14 @@ void addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); - void removeRecursive(const CTransaction &tx, MemPoolRemovalReason reason); + void removeRecursive(const CTransaction &tx, MemPoolRemovalReason reason) + EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForReorg(const Config &config, const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) - EXCLUSIVE_LOCKS_REQUIRED(cs_main); + EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector &vtx, - unsigned int nBlockHeight); + unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); void clear(); // lock free @@ -641,7 +633,8 @@ * the tx is not dependent on other mempool transactions to be included in a * block. */ - bool HasNoInputsOf(const CTransaction &tx) const; + bool HasNoInputsOf(const CTransaction &tx) const + EXCLUSIVE_LOCKS_REQUIRED(cs); /** Affect CreateNewBlock prioritisation of transactions */ void PrioritiseTransaction(const TxId &txid, const Amount nFeeDelta); @@ -685,7 +678,7 @@ * disconnected block that have been accepted back into the mempool. */ void UpdateTransactionsFromBlock(const std::vector &txidsToUpdate) - EXCLUSIVE_LOCKS_REQUIRED(cs_main); + EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); /** * Try to calculate all in-mempool ancestors of entry. @@ -730,18 +723,20 @@ * this mempool. */ void TrimToSize(size_t sizelimit, - std::vector *pvNoSpendsRemaining = nullptr); + std::vector *pvNoSpendsRemaining = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(cs); /** * Expire all transaction (and their dependencies) in the mempool older than * time. Return the number of removed transactions. */ - int Expire(std::chrono::seconds time); + int Expire(std::chrono::seconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); /** * Reduce the size of the mempool by expiring and then trimming the mempool. */ - void LimitSize(size_t limit, std::chrono::seconds age); + void LimitSize(size_t limit, std::chrono::seconds age) + EXCLUSIVE_LOCKS_REQUIRED(cs); /** * Calculate the ancestor and descendant count for the given transaction. @@ -922,11 +917,12 @@ // Import mempool entries in topological order into queuedTx and clear the // mempool. Caller should call updateMempoolForReorg to reprocess these // transactions - void importMempool(CTxMemPool &pool); + void importMempool(CTxMemPool &pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs); // Add entries for a block while reconstructing the topological ordering so // they can be added back to the mempool simply. - void addForBlock(const std::vector &vtx, CTxMemPool &pool); + void addForBlock(const std::vector &vtx, CTxMemPool &pool) + EXCLUSIVE_LOCKS_REQUIRED(pool.cs); // Remove entries based on txid_index, and update memory usage. void removeForBlock(const std::vector &vtx) { @@ -971,7 +967,8 @@ * back, and instead just erase from the mempool as needed. */ void updateMempoolForReorg(const Config &config, bool fAddToMempool, - CTxMemPool &pool); + CTxMemPool &pool) + EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs); }; #endif // BITCOIN_TXMEMPOOL_H diff --git a/src/txmempool.cpp b/src/txmempool.cpp --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -134,7 +134,7 @@ // state to include the parent. void CTxMemPool::UpdateTransactionsFromBlock( const std::vector &txidsToUpdate) { - LOCK(cs); + AssertLockHeld(cs); // For each entry in txidsToUpdate, store the set of in-mempool, but not // in-txidsToUpdate transactions, so that we don't have to recalculate // descendants when we come across a previously seen entry. @@ -526,7 +526,7 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReason reason) { // Remove transaction from memory pool. - LOCK(cs); + AssertLockHeld(cs); setEntries txToRemove; txiter origit = mapTx.find(origTx.GetId()); if (origit != mapTx.end()) { @@ -561,7 +561,7 @@ unsigned int nMemPoolHeight, int flags) { // Remove transactions spending a coinbase which are now immature and // no-longer-final transactions. - LOCK(cs); + AssertLockHeld(cs); setEntries txToRemove; for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { @@ -631,7 +631,7 @@ */ void CTxMemPool::removeForBlock(const std::vector &vtx, unsigned int nBlockHeight) { - LOCK(cs); + AssertLockHeld(cs); DisconnectedBlockTransactions disconnectpool; disconnectpool.addForBlock(vtx, *this); @@ -1074,7 +1074,7 @@ } int CTxMemPool::Expire(std::chrono::seconds time) { - LOCK(cs); + AssertLockHeld(cs); indexed_transaction_set::index::type::iterator it = mapTx.get().begin(); setEntries toremove; @@ -1182,7 +1182,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector *pvNoSpendsRemaining) { - LOCK(cs); + AssertLockHeld(cs); unsigned nTxnRemoved = 0; CFeeRate maxFeeRateRemoved(Amount::zero()); @@ -1283,6 +1283,7 @@ void DisconnectedBlockTransactions::addForBlock( const std::vector &vtx, CTxMemPool &pool) { + AssertLockHeld(pool.cs); for (const auto &tx : reverse_iterate(vtx)) { // If we already added it, just skip. auto it = queuedTx.find(tx->GetId()); @@ -1340,6 +1341,7 @@ } void DisconnectedBlockTransactions::importMempool(CTxMemPool &pool) { + AssertLockHeld(pool.cs); // addForBlock's algorithm sorts a vector of transactions back into // topological order. We use it in a separate object to create a valid // ordering of all mempool transactions, which we then splice in front of @@ -1350,14 +1352,12 @@ // addForBlocks (which iterates in reverse order), as vtx probably end in // the correct ordering for queuedTx. std::vector vtx; - { - LOCK(pool.cs); - vtx.reserve(pool.mapTx.size()); - for (const CTxMemPoolEntry &e : pool.mapTx.get()) { - vtx.push_back(e.GetSharedTx()); - } - pool.clear(); + + vtx.reserve(pool.mapTx.size()); + for (const CTxMemPoolEntry &e : pool.mapTx.get()) { + vtx.push_back(e.GetSharedTx()); } + pool.clear(); // Use addForBlocks to sort the transactions and then splice them in front // of queuedTx @@ -1380,6 +1380,7 @@ bool fAddToMempool, CTxMemPool &pool) { AssertLockHeld(cs_main); + AssertLockHeld(pool.cs); std::vector txidsUpdate; // disconnectpool's insertion_order index sorts the entries from oldest to diff --git a/src/validation.h b/src/validation.h --- a/src/validation.h +++ b/src/validation.h @@ -23,6 +23,7 @@ #include