Merge #14193: validation: Add missing mempool locks

fa2b083 [test] Add test to check mempool consistency in case of reorgs (MarcoFalke)
fabeb1f validation: Add missing mempool locks (MarcoFalke)
fa0c9db txpool: Make nTransactionsUpdated atomic (MarcoFalke)

Pull request description:

  Take the mempool read lock during reorgs, so that we don't accidentally read an inconsistent mempool.

ACKs for top commit:
  laanwj:
    code review ACK fa2b083
  ryanofsky:
    utACK fa2b083 [EDIT: was ~e284e422e75189794e24fe482819d8b1407857c3~, from bad copy and paste]. Changes since last review: rebase after #15976, adding vTxHashes lock annotation, adding new commit dropping mempool lock for nTransactionsUpdated and making it atomic to avoid deadlock between mempool lock and g_best_block_mutex

Tree-SHA512: cfe7777993589087753e000e3736d79d320dca412383fb77b56bef8946a04049722bf888c11b6f722adf677165185c7e58b4a269f7c5fa25e84dda375f6c8a7d

Author: laanwj

src/rpc/mining.cpp

@@ -469,7 +469,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
             nTransactionsUpdatedLastLP = nTransactionsUpdatedLast;
         }
 
-        // Release the wallet and main lock while waiting
+        // Release lock while waiting
         LEAVE_CRITICAL_SECTION(cs_main);
         {
             checktxtime = std::chrono::steady_clock::now() + std::chrono::minutes(1);
@@ -480,6 +480,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
                 if (g_best_block_cv.wait_until(lock, checktxtime) == std::cv_status::timeout)
                 {
                     // Timeout: Check transactions for update
+                    // without holding ::mempool.cs to avoid deadlocks
                     if (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP)
                         break;
                     checktxtime += std::chrono::seconds(10);

src/test/validation_block_tests.cpp

@@ -10,6 +10,7 @@
 #include <miner.h>
 #include <pow.h>
 #include <random.h>
+#include <script/standard.h>
 #include <test/setup_common.h>
 #include <util/time.h>
 #include <validation.h>
@@ -21,6 +22,8 @@ struct RegtestingSetup : public TestingSetup {
     RegtestingSetup() : TestingSetup(CBaseChainParams::REGTEST) {}
 };
 
+static const std::vector<unsigned char> V_OP_TRUE{OP_TRUE};
+
 BOOST_FIXTURE_TEST_SUITE(validation_block_tests, RegtestingSetup)
 
 struct TestSubscriber : public CValidationInterface {
@@ -62,8 +65,21 @@ std::shared_ptr<CBlock> Block(const uint256& prev_hash)
     pblock->hashPrevBlock = prev_hash;
     pblock->nTime = ++time;
 
+    pubKey.clear();
+    {
+        WitnessV0ScriptHash witness_program;
+        CSHA256().Write(&V_OP_TRUE[0], V_OP_TRUE.size()).Finalize(witness_program.begin());
+        pubKey << OP_0 << ToByteVector(witness_program);
+    }
+
+    // Make the coinbase transaction with two outputs:
+    // One zero-value one that has a unique pubkey to make sure that blocks at the same height can have a different hash
+    // Another one that has the coinbase reward in a P2WSH with OP_TRUE as witness program to make it easy to spend
     CMutableTransaction txCoinbase(*pblock->vtx[0]);
-    txCoinbase.vout.resize(1);
+    txCoinbase.vout.resize(2);
+    txCoinbase.vout[1].scriptPubKey = pubKey;
+    txCoinbase.vout[1].nValue = txCoinbase.vout[0].nValue;
+    txCoinbase.vout[0].nValue = 0;
     txCoinbase.vin[0].scriptWitness.SetNull();
     pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase));
 
@@ -72,6 +88,9 @@ std::shared_ptr<CBlock> Block(const uint256& prev_hash)
 
 std::shared_ptr<CBlock> FinalizeBlock(std::shared_ptr<CBlock> pblock)
 {
+    LOCK(cs_main); // For LookupBlockIndex
+    GenerateCoinbaseCommitment(*pblock, LookupBlockIndex(pblock->hashPrevBlock), Params().GetConsensus());
+
     pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
 
     while (!CheckProofOfWork(pblock->GetHash(), pblock->nBits, Params().GetConsensus())) {
@@ -82,13 +101,13 @@ std::shared_ptr<CBlock> FinalizeBlock(std::shared_ptr<CBlock> pblock)
 }
 
 // construct a valid block
-const std::shared_ptr<const CBlock> GoodBlock(const uint256& prev_hash)
+std::shared_ptr<const CBlock> GoodBlock(const uint256& prev_hash)
 {
     return FinalizeBlock(Block(prev_hash));
 }
 
 // construct an invalid block (but with a valid header)
-const std::shared_ptr<const CBlock> BadBlock(const uint256& prev_hash)
+std::shared_ptr<const CBlock> BadBlock(const uint256& prev_hash)
 {
     auto pblock = Block(prev_hash);
 
@@ -188,4 +207,131 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
     BOOST_CHECK_EQUAL(sub.m_expected_tip, ::ChainActive().Tip()->GetBlockHash());
 }
 
+/**
+ * Test that mempool updates happen atomically with reorgs.
+ *
+ * This prevents RPC clients, among others, from retrieving immediately-out-of-date mempool data
+ * during large reorgs.
+ *
+ * The test verifies this by creating a chain of `num_txs` blocks, matures their coinbases, and then
+ * submits txns spending from their coinbase to the mempool. A fork chain is then processed,
+ * invalidating the txns and evicting them from the mempool.
+ *
+ * We verify that the mempool updates atomically by polling it continuously
+ * from another thread during the reorg and checking that its size only changes
+ * once. The size changing exactly once indicates that the polling thread's
+ * view of the mempool is either consistent with the chain state before reorg,
+ * or consistent with the chain state after the reorg, and not just consistent
+ * with some intermediate state during the reorg.
+ */
+BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
+{
+    bool ignored;
+    auto ProcessBlock = [&ignored](std::shared_ptr<const CBlock> block) -> bool {
+        return ProcessNewBlock(Params(), block, /* fForceProcessing */ true, /* fNewBlock */ &ignored);
+    };
+
+    // Process all mined blocks
+    BOOST_REQUIRE(ProcessBlock(std::make_shared<CBlock>(Params().GenesisBlock())));
+    auto last_mined = GoodBlock(Params().GenesisBlock().GetHash());
+    BOOST_REQUIRE(ProcessBlock(last_mined));
+
+    // Run the test multiple times
+    for (int test_runs = 3; test_runs > 0; --test_runs) {
+        BOOST_CHECK_EQUAL(last_mined->GetHash(), ::ChainActive().Tip()->GetBlockHash());
+
+        // Later on split from here
+        const uint256 split_hash{last_mined->hashPrevBlock};
+
+        // Create a bunch of transactions to spend the miner rewards of the
+        // most recent blocks
+        std::vector<CTransactionRef> txs;
+        for (int num_txs = 22; num_txs > 0; --num_txs) {
+            CMutableTransaction mtx;
+            mtx.vin.push_back(CTxIn{COutPoint{last_mined->vtx[0]->GetHash(), 1}, CScript{}});
+            mtx.vin[0].scriptWitness.stack.push_back(V_OP_TRUE);
+            mtx.vout.push_back(last_mined->vtx[0]->vout[1]);
+            mtx.vout[0].nValue -= 1000;
+            txs.push_back(MakeTransactionRef(mtx));
+
+            last_mined = GoodBlock(last_mined->GetHash());
+            BOOST_REQUIRE(ProcessBlock(last_mined));
+        }
+
+        // Mature the inputs of the txs
+        for (int j = COINBASE_MATURITY; j > 0; --j) {
+            last_mined = GoodBlock(last_mined->GetHash());
+            BOOST_REQUIRE(ProcessBlock(last_mined));
+        }
+
+        // Mine a reorg (and hold it back) before adding the txs to the mempool
+        const uint256 tip_init{last_mined->GetHash()};
+
+        std::vector<std::shared_ptr<const CBlock>> reorg;
+        last_mined = GoodBlock(split_hash);
+        reorg.push_back(last_mined);
+        for (size_t j = COINBASE_MATURITY + txs.size() + 1; j > 0; --j) {
+            last_mined = GoodBlock(last_mined->GetHash());
+            reorg.push_back(last_mined);
+        }
+
+        // Add the txs to the tx pool
+        {
+            LOCK(cs_main);
+            CValidationState state;
+            std::list<CTransactionRef> plTxnReplaced;
+            for (const auto& tx : txs) {
+                BOOST_REQUIRE(AcceptToMemoryPool(
+                    ::mempool,
+                    state,
+                    tx,
+                    /* pfMissingInputs */ &ignored,
+                    &plTxnReplaced,
+                    /* bypass_limits */ false,
+                    /* nAbsurdFee */ 0));
+            }
+        }
+
+        // Check that all txs are in the pool
+        {
+            LOCK(::mempool.cs);
+            BOOST_CHECK_EQUAL(::mempool.mapTx.size(), txs.size());
+        }
+
+        // Run a thread that simulates an RPC caller that is polling while
+        // validation is doing a reorg
+        std::thread rpc_thread{[&]() {
+            // This thread is checking that the mempool either contains all of
+            // the transactions invalidated by the reorg, or none of them, and
+            // not some intermediate amount.
+            while (true) {
+                LOCK(::mempool.cs);
+                if (::mempool.mapTx.size() == 0) {
+                    // We are done with the reorg
+                    break;
+                }
+                // Internally, we might be in the middle of the reorg, but
+                // externally the reorg to the most-proof-of-work chain should
+                // be atomic. So the caller assumes that the returned mempool
+                // is consistent. That is, it has all txs that were there
+                // before the reorg.
+                assert(::mempool.mapTx.size() == txs.size());
+                continue;
+            }
+            LOCK(cs_main);
+            // We are done with the reorg, so the tip must have changed
+            assert(tip_init != ::ChainActive().Tip()->GetBlockHash());
+        }};
+
+        // Submit the reorg in this thread to invalidate and remove the txs from the tx pool
+        for (const auto& b : reorg) {
+            ProcessBlock(b);
+        }
+        // Check that the reorg was eventually successful
+        BOOST_CHECK_EQUAL(last_mined->GetHash(), ::ChainActive().Tip()->GetBlockHash());
+
+        // We can join the other thread, which returns when the reorg was successful
+        rpc_thread.join();
+    }
+}
 BOOST_AUTO_TEST_SUITE_END()

src/txmempool.cpp

@@ -104,7 +104,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendan
 // for each such descendant, also update the ancestor state to include the parent.
 void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashesToUpdate)
 {
-    LOCK(cs);
+    AssertLockHeld(cs);
     // For each entry in vHashesToUpdate, store the set of in-mempool, but not
     // in-vHashesToUpdate transactions, so that we don't have to recalculate
     // descendants when we come across a previously seen entry.
@@ -322,8 +322,8 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee,
     assert(int(nSigOpCostWithAncestors) >= 0);
 }
 
-CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator) :
-    nTransactionsUpdated(0), minerPolicyEstimator(estimator)
+CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator)
+    : nTransactionsUpdated(0), minerPolicyEstimator(estimator)
 {
     _clear(); //lock free clear
 
@@ -341,13 +341,11 @@ bool CTxMemPool::isSpent(const COutPoint& outpoint) const
 
 unsigned int CTxMemPool::GetTransactionsUpdated() const
 {
-    LOCK(cs);
     return nTransactionsUpdated;
 }
 
 void CTxMemPool::AddTransactionsUpdated(unsigned int n)
 {
-    LOCK(cs);
     nTransactionsUpdated += n;
 }
 
@@ -459,8 +457,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries& setDescendants
 void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReason reason)
 {
     // Remove transaction from memory pool
-    {
-        LOCK(cs);
+    AssertLockHeld(cs);
         setEntries txToRemove;
         txiter origit = mapTx.find(origTx.GetHash());
         if (origit != mapTx.end()) {
@@ -485,13 +482,12 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
         }
 
         RemoveStaged(setAllRemoves, false, reason);
-    }
 }
 
 void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, 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++) {
         const CTransaction& tx = it->GetTx();
@@ -547,7 +543,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
  */
 void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight)
 {
-    LOCK(cs);
+    AssertLockHeld(cs);
     std::vector<const CTxMemPoolEntry*> entries;
     for (const auto& tx : vtx)
     {
@@ -922,7 +918,7 @@ void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants, MemPool
 }
 
 int CTxMemPool::Expire(int64_t time) {
-    LOCK(cs);
+    AssertLockHeld(cs);
     indexed_transaction_set::index<entry_time>::type::iterator it = mapTx.get<entry_time>().begin();
     setEntries toremove;
     while (it != mapTx.get<entry_time>().end() && it->GetTime() < time) {
@@ -1015,7 +1011,7 @@ void CTxMemPool::trackPackageRemoved(const CFeeRate& rate) {
 }
 
 void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<COutPoint>* pvNoSpendsRemaining) {
-    LOCK(cs);
+    AssertLockHeld(cs);
 
     unsigned nTxnRemoved = 0;
     CFeeRate maxFeeRateRemoved(0);

src/txmempool.h

@@ -6,12 +6,13 @@
 #ifndef BITCOIN_TXMEMPOOL_H
 #define BITCOIN_TXMEMPOOL_H
 
+#include <atomic>
+#include <map>
 #include <memory>
 #include <set>
-#include <map>
-#include <vector>
-#include <utility>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include <amount.h>
 #include <coins.h>
@@ -443,7 +444,7 @@ class CTxMemPool
 {
 private:
     uint32_t nCheckFrequency GUARDED_BY(cs); //!< Value n means that n times in 2^32 we check.
-    unsigned int nTransactionsUpdated; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation
+    std::atomic<unsigned int> nTransactionsUpdated; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation
     CBlockPolicyEstimator* minerPolicyEstimator;
 
     uint64_t totalTxSize;      //!< sum of all mempool tx's virtual sizes. Differs from serialized tx size since witness data is discounted. Defined in BIP 141.
@@ -513,21 +514,12 @@ class CTxMemPool
      * `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;
-    std::vector<std::pair<uint256, txiter> > vTxHashes; //!< All tx witness hashes/entries in mapTx, in random order
+    std::vector<std::pair<uint256, txiter>> vTxHashes GUARDED_BY(cs); //!< All tx witness hashes/entries in mapTx, in random order
 
     struct CompareIteratorByHash {
         bool operator()(const txiter &a, const txiter &b) const {
@@ -582,10 +574,10 @@ class CTxMemPool
     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);
-    void removeConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
-    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight);
+    void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
+    void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     void clear();
     void _clear() EXCLUSIVE_LOCKS_REQUIRED(cs); //lock free
@@ -598,7 +590,7 @@ class CTxMemPool
      * Check that none of this transactions inputs are in the mempool, and thus
      * 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 uint256& hash, const CAmount& nFeeDelta);
@@ -632,7 +624,7 @@ class CTxMemPool
      *  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) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    void UpdateTransactionsFromBlock(const std::vector<uint256>& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
 
     /** Try to calculate all in-mempool ancestors of entry.
      *  (these are all calculated including the tx itself)
@@ -663,10 +655,10 @@ class CTxMemPool
       *  pvNoSpendsRemaining, if set, will be populated with the list of outpoints
       *  which are not in mempool which no longer have any spends in this mempool.
       */
-    void TrimToSize(size_t sizelimit, std::vector<COutPoint>* pvNoSpendsRemaining=nullptr);
+    void TrimToSize(size_t sizelimit, std::vector<COutPoint>* 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(int64_t time);
+    int Expire(int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /**
      * Calculate the ancestor and descendant count for the given transaction.