Merge 1847ce2d49 into merged_master (Bitcoin PR bitcoin/bitcoin#23157)

src/bench/mempool_stress.cpp

@@ -6,6 +6,7 @@
 #include <policy/policy.h>
 #include <test/util/setup_common.h>
 #include <txmempool.h>
+#include <validation.h>
 
 #include <vector>
 
@@ -27,14 +28,8 @@ struct Available {
     Available(CTransactionRef& ref, size_t tx_count) : ref(ref), tx_count(tx_count){}
 };
 
-static void ComplexMemPool(benchmark::Bench& bench)
+static std::vector<CTransactionRef> CreateOrderedCoins(FastRandomContext& det_rand, int childTxs, int min_ancestors)
 {
-    int childTxs = 800;
-    if (bench.complexityN() > 1) {
-        childTxs = static_cast<int>(bench.complexityN());
-    }
-
-    FastRandomContext det_rand{true};
     std::vector<Available> available_coins;
     std::vector<CTransactionRef> ordered_coins;
     // Create some base transactions
@@ -60,8 +55,10 @@ static void ComplexMemPool(benchmark::Bench& bench)
             size_t idx = det_rand.randrange(available_coins.size());
             Available coin = available_coins[idx];
             uint256 hash = coin.ref->GetHash();
-            // biased towards taking just one ancestor, but maybe more
+            // biased towards taking min_ancestors parents, but maybe more
-            size_t n_to_take = det_rand.randrange(2) == 0 ? 1 : 1+det_rand.randrange(coin.ref->vout.size() - coin.vin_left);
+            size_t n_to_take = det_rand.randrange(2) == 0 ?
+                               min_ancestors :
+                               min_ancestors + det_rand.randrange(coin.ref->vout.size() - coin.vin_left);
             for (size_t i = 0; i < n_to_take; ++i) {
                 tx.vin.emplace_back();
                 tx.vin.back().prevout = COutPoint(hash, coin.vin_left++);
@@ -82,6 +79,17 @@ static void ComplexMemPool(benchmark::Bench& bench)
         ordered_coins.emplace_back(MakeTransactionRef(tx));
         available_coins.emplace_back(ordered_coins.back(), tx_counter++);
     }
+    return ordered_coins;
+}
+
+static void ComplexMemPool(benchmark::Bench& bench)
+{
+    FastRandomContext det_rand{true};
+    int childTxs = 800;
+    if (bench.complexityN() > 1) {
+        childTxs = static_cast<int>(bench.complexityN());
+    }
+    std::vector<CTransactionRef> ordered_coins = CreateOrderedCoins(det_rand, childTxs, /* min_ancestors */ 1);
     const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::MAIN);
     CTxMemPool pool;
     LOCK2(cs_main, pool.cs);
@@ -94,4 +102,22 @@ static void ComplexMemPool(benchmark::Bench& bench)
     });
 }
 
+static void MempoolCheck(benchmark::Bench& bench)
+{
+    FastRandomContext det_rand{true};
+    const int childTxs = bench.complexityN() > 1 ? static_cast<int>(bench.complexityN()) : 2000;
+    const std::vector<CTransactionRef> ordered_coins = CreateOrderedCoins(det_rand, childTxs, /* min_ancestors */ 5);
+    const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::MAIN, {"-checkmempool=1"});
+    CTxMemPool pool;
+    LOCK2(cs_main, pool.cs);
+    const CBlockIndex* chain_tip = testing_setup.get()->m_node.chainman->ActiveChainstate().m_chain.Tip();
+    const CCoinsViewCache& coins_tip = testing_setup.get()->m_node.chainman->ActiveChainstate().CoinsTip();
+    for (auto& tx : ordered_coins) AddTx(tx, pool);
+
+    bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {
+        pool.check(chain_tip, coins_tip, /* spendheight */ 2);
+    });
+}
+
 BENCHMARK(ComplexMemPool);
+BENCHMARK(MempoolCheck);

src/net_processing.cpp

@@ -2350,7 +2350,8 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
             break;
         }
     }
-    m_mempool.check(m_chainman.ActiveChainstate());
+    CChainState& active_chainstate = m_chainman.ActiveChainstate();
+    m_mempool.check(active_chainstate.m_chain.Tip(), active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1);
 }
 
 bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer,
@@ -3320,7 +3321,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         const TxValidationState& state = result.m_state;
 
         if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
-            m_mempool.check(m_chainman.ActiveChainstate());
+            CChainState& active_chainstate = m_chainman.ActiveChainstate();
+            m_mempool.check(active_chainstate.m_chain.Tip(), active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1);
             // As this version of the transaction was acceptable, we can forget about any
             // requests for it.
             m_txrequest.ForgetTxHash(tx.GetHash());

src/test/fuzz/tx_pool.cpp

@@ -81,7 +81,7 @@ void SetMempoolConstraints(ArgsManager& args, FuzzedDataProvider& fuzzed_data_pr
 
 void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, CChainState& chainstate)
 {
-    WITH_LOCK(::cs_main, tx_pool.check(chainstate));
+    WITH_LOCK(::cs_main, tx_pool.check(chainstate.m_chain.Tip(), chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
     {
         BlockAssembler::Options options;
         options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT);
@@ -97,7 +97,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, CCh
         std::vector<uint256> all_txids;
         tx_pool.queryHashes(all_txids);
         assert(all_txids.size() < info_all.size());
-        WITH_LOCK(::cs_main, tx_pool.check(chainstate));
+        WITH_LOCK(::cs_main, tx_pool.check(chainstate.m_chain.Tip(), chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
     }
     SyncWithValidationInterfaceQueue();
 }

src/txmempool.cpp

@@ -5,7 +5,9 @@
 
 #include <txmempool.h>
 
+
 #include <chainparams.h> // removeForBlock paklist transition
+#include <coins.h>
 #include <consensus/consensus.h>
 #include <consensus/tx_verify.h>
 #include <consensus/validation.h>
@@ -728,25 +730,7 @@ void CTxMemPool::clear()
     _clear();
 }
 
-static void CheckInputsAndUpdateCoins(CChainState& active_chainstate, const CTxMemPoolEntry& entry, CCoinsViewCache& mempoolDuplicate, const int64_t spendheight, std::set<std::pair<uint256, COutPoint>>& setGlobalPeginsSpent)
+void CTxMemPool::check(const CBlockIndex* active_chain_tip, const CCoinsViewCache& active_coins_tip, int64_t spendheight) const
-{
-    CTransaction tx = entry.GetTx();
-    TxValidationState dummy_state; // Not used. CheckTxInputs() should always pass
-    CAmountMap fee_map;
-    std::set<std::pair<uint256, COutPoint> > setPeginsSpent;
-    const auto& fedpegscripts = GetValidFedpegScripts(active_chainstate.m_chain.Tip(), Params().GetConsensus(), true /* nextblock_validation */);
-    bool fCheckResult = tx.IsCoinBase() || Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, spendheight, fee_map, setPeginsSpent, NULL, false, true, fedpegscripts);
-    assert(fCheckResult);
-    UpdateCoins(tx, mempoolDuplicate, std::numeric_limits<int>::max());
-
-    // ELEMENTS:
-    assert(setPeginsSpent == entry.setPeginsSpent);
-    size_t prevPeginsCount = setGlobalPeginsSpent.size();
-    setGlobalPeginsSpent.insert(setPeginsSpent.begin(), setPeginsSpent.end());
-    assert(setGlobalPeginsSpent.size() == prevPeginsCount + setPeginsSpent.size());
-}
-
-void CTxMemPool::check(CChainState& active_chainstate) const
 {
     if (m_check_ratio == 0) return;
 
@@ -759,23 +743,19 @@ void CTxMemPool::check(CChainState& active_chainstate) const
     uint64_t checkTotal = 0;
     CAmount check_total_fee{0};
     uint64_t innerUsage = 0;
+    uint64_t prev_ancestor_count{0};
 
-    CCoinsViewCache& active_coins_tip = active_chainstate.CoinsTip();
     CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(&active_coins_tip));
-    const int64_t spendheight = active_chainstate.m_chain.Height() + 1;
 
-    std::list<const CTxMemPoolEntry*> waitingOnDependants;
     // ELEMENTS:
-    std::set<std::pair<uint256, COutPoint> > setGlobalPeginsSpent;
+    std::set<std::pair<uint256, COutPoint>> setGlobalPeginsSpent;
 
-    for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) {
+    for (const auto& it : GetSortedDepthAndScore()) {
-        unsigned int i = 0;
         checkTotal += it->GetTxSize();
         check_total_fee += it->GetFee();
         innerUsage += it->DynamicMemoryUsage();
         const CTransaction& tx = it->GetTx();
         innerUsage += memusage::DynamicUsage(it->GetMemPoolParentsConst()) + memusage::DynamicUsage(it->GetMemPoolChildrenConst());
-        bool fDependsWait = false;
         CTxMemPoolEntry::Parents setParentCheck;
         for (const CTxIn &txin : tx.vin) {
             // Check that every mempool transaction's inputs refer to available coins, or other mempool tx's.
@@ -783,18 +763,20 @@ void CTxMemPool::check(CChainState& active_chainstate) const
             if (it2 != mapTx.end()) {
                 const CTransaction& tx2 = it2->GetTx();
                 assert(tx2.vout.size() > txin.prevout.n && !tx2.vout[txin.prevout.n].IsNull());
-                fDependsWait = true;
                 setParentCheck.insert(*it2);
             } else {
                 // peg-in inputs are not sanity-checked to be valid
-                assert(txin.m_is_pegin || active_coins_tip.HaveCoin(txin.prevout));
+                // assert(txin.m_is_pegin); FIXME
             }
+            // We are iterating through the mempool entries sorted in order by ancestor count.
+            // All parents must have been checked before their children and their coins added to
+            // the mempoolDuplicate coins cache.
+            assert(mempoolDuplicate.HaveCoin(txin.prevout));
             // Check whether its inputs are marked in mapNextTx.
             auto it3 = mapNextTx.find(txin.prevout);
             assert(it3 != mapNextTx.end());
             assert(it3->first == &txin.prevout);
             assert(it3->second == &tx);
-            i++;
         }
         auto comp = [](const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) -> bool {
             return a.GetTx().GetHash() == b.GetTx().GetHash();
@@ -821,6 +803,9 @@ void CTxMemPool::check(CChainState& active_chainstate) const
         assert(it->GetSizeWithAncestors() == nSizeCheck);
         assert(it->GetSigOpCostWithAncestors() == nSigOpCheck);
         assert(it->GetModFeesWithAncestors() == nFeesCheck);
+        // Sanity check: we are walking in ascending ancestor count order.
+        assert(prev_ancestor_count <= it->GetCountWithAncestors());
+        prev_ancestor_count = it->GetCountWithAncestors();
 
         // Check children against mapNextTx
         CTxMemPoolEntry::Children setChildrenCheck;
@@ -839,24 +824,20 @@ void CTxMemPool::check(CChainState& active_chainstate) const
         // just a sanity check, not definitive that this calc is correct...
         assert(it->GetSizeWithDescendants() >= child_sizes + it->GetTxSize());
 
-        if (fDependsWait)
+        TxValidationState dummy_state; // Not used. CheckTxInputs() should always pass
-            waitingOnDependants.push_back(&(*it));
+        CAmount txfee = 0;
-        else {
+        assert(!tx.IsCoinBase());
-            CheckInputsAndUpdateCoins(active_chainstate, *it, mempoolDuplicate, spendheight, setGlobalPeginsSpent);
+
-        }
+        // ELEMENTS
-    }
+        CAmountMap fee_map;
-    unsigned int stepsSinceLastRemove = 0;
+        std::set<std::pair<uint256, COutPoint>> setPeginsSpent;
-    while (!waitingOnDependants.empty()) {
+        const auto& fedpegscripts = GetValidFedpegScripts(active_chain_tip, Params().GetConsensus(), true /* nextblock_validation */);
-        const CTxMemPoolEntry* entry = waitingOnDependants.front();
+        std::vector<CCheck*> pvChecks; // FIXME
-        waitingOnDependants.pop_front();
+        bool cacheStore = true;
-        if (!mempoolDuplicate.HaveInputs(entry->GetTx())) {
+        bool fScriptChecks = true;
-            waitingOnDependants.push_back(entry);
+        assert(Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, spendheight, fee_map, setPeginsSpent, &pvChecks, cacheStore, fScriptChecks, fedpegscripts));
-            stepsSinceLastRemove++;
+        for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout);
-            assert(stepsSinceLastRemove < waitingOnDependants.size());
+        AddCoins(mempoolDuplicate, tx, std::numeric_limits<int>::max());
-        } else {
-            CheckInputsAndUpdateCoins(active_chainstate, *entry, mempoolDuplicate, spendheight, setGlobalPeginsSpent);
-            stepsSinceLastRemove = 0;
-        }
     }
     for (auto it = mapNextTx.cbegin(); it != mapNextTx.cend(); it++) {
         uint256 hash = it->second->GetHash();

src/txmempool.h

@@ -628,7 +628,7 @@ public:
      * all inputs are in the mapNextTx array). If sanity-checking is turned off,
      * check does nothing.
      */
-    void check(CChainState& active_chainstate) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    void check(const CBlockIndex* active_chain_tip, const CCoinsViewCache& active_coins_tip, int64_t spendheight) 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

src/validation.cpp

@@ -1335,12 +1335,6 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txund
     AddCoins(inputs, tx, nHeight);
 }
 
-void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, int nHeight)
-{
-    CTxUndo txundo;
-    UpdateCoins(tx, inputs, txundo, nHeight);
-}
-
 bool CScriptCheck::operator()() {
     const CScript &scriptSig = ptxTo->vin[nIn].scriptSig;
     const CScriptWitness *witness = ptxTo->witness.vtxinwit.size() > nIn ? &ptxTo->witness.vtxinwit[nIn].scriptWitness : NULL;
@@ -2771,7 +2765,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, CBlockIndex
         // any disconnected transactions back to the mempool.
         MaybeUpdateMempoolForReorg(disconnectpool, true);
     }
-    if (m_mempool) m_mempool->check(*this);
+    if (m_mempool) m_mempool->check(this->m_chain.Tip(), this->CoinsTip(), this->m_chain.Height() + 1);
 
     CheckForkWarningConditions();
 

src/validation.h

@@ -239,9 +239,6 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx
                                                    const Package& txns, bool test_accept)
                                                    EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
-/** Apply the effects of this transaction on the UTXO set represented by view */
-void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, int nHeight);
-
 /** Transaction validation functions */
 
 /**