From a21bfd02e9f116ebd85556858594217747e76d6f Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 30 Sep 2021 09:25:11 +0100 Subject: [PATCH] merge bitcoin#23157: improve performance of check() and remove dependency on validation --- src/bench/mempool_stress.cpp | 43 ++++++++++++++++++++++------ src/net_processing.cpp | 6 ++-- src/test/fuzz/tx_pool.cpp | 4 +-- src/txmempool.cpp | 55 +++++++++++------------------------- src/txmempool.h | 2 +- src/validation.cpp | 8 +----- src/validation.h | 3 -- 7 files changed, 59 insertions(+), 62 deletions(-) diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp index 29a835ad21..3837a1427d 100644 --- a/src/bench/mempool_stress.cpp +++ b/src/bench/mempool_stress.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -26,14 +27,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 CreateOrderedCoins(FastRandomContext& det_rand, int childTxs, int min_ancestors) { - int childTxs = 800; - if (bench.complexityN() > 1) { - childTxs = static_cast(bench.complexityN()); - } - - FastRandomContext det_rand{true}; std::vector available_coins; std::vector ordered_coins; // Create some base transactions @@ -57,8 +52,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 - size_t n_to_take = det_rand.randrange(2) == 0 ? 1 : 1+det_rand.randrange(coin.ref->vout.size() - coin.vin_left); + // biased towards taking min_ancestors parents, but maybe more + 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++); @@ -77,6 +74,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(bench.complexityN()); + } + std::vector ordered_coins = CreateOrderedCoins(det_rand, childTxs, /* min_ancestors */ 1); const auto testing_setup = MakeNoLogFileContext(CBaseChainParams::MAIN); CTxMemPool pool; LOCK2(cs_main, pool.cs); @@ -89,4 +97,21 @@ static void ComplexMemPool(benchmark::Bench& bench) }); } +static void MempoolCheck(benchmark::Bench& bench) +{ + FastRandomContext det_rand{true}; + const int childTxs = bench.complexityN() > 1 ? static_cast(bench.complexityN()) : 2000; + const std::vector ordered_coins = CreateOrderedCoins(det_rand, childTxs, /* min_ancestors */ 5); + const auto testing_setup = MakeNoLogFileContext(CBaseChainParams::MAIN, {"-checkmempool=1"}); + CTxMemPool pool; + LOCK2(cs_main, pool.cs); + 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(coins_tip, /* spendheight */ 2); + }); +} + BENCHMARK(ComplexMemPool); +BENCHMARK(MempoolCheck); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5fa577ce05..d14d4d2a0c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3065,7 +3065,8 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) break; } } - m_mempool.check(m_chainman.ActiveChainstate()); + CChainState& active_chainstate = m_chainman.ActiveChainstate(); + m_mempool.check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); } bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer, @@ -4234,7 +4235,8 @@ void PeerManagerImpl::ProcessMessage( m_cj_ctx->dstxman->AddDSTX(dstx); } - m_mempool.check(m_chainman.ActiveChainstate()); + CChainState& active_chainstate = m_chainman.ActiveChainstate(); + m_mempool.check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); RelayTransaction(tx.GetHash()); m_orphanage.AddChildrenToWorkSet(tx, peer->m_orphan_work_set); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index c689b52cc6..d42b42122c 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -80,7 +80,7 @@ void SetMempoolConstraints(ArgsManager& args, FuzzedDataProvider& fuzzed_data_pr void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, const NodeContext& node, CChainState& chainstate) { - WITH_LOCK(::cs_main, tx_pool.check(chainstate)); + WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1)); { BlockAssembler::Options options; options.nBlockMaxSize = fuzzed_data_provider.ConsumeIntegralInRange(0U, MaxBlockSize(true)); @@ -97,7 +97,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, con std::vector 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.CoinsTip(), chainstate.m_chain.Height() + 1)); } SyncWithValidationInterfaceQueue(); } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 9bf3dc3d25..0f6711aefe 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -5,6 +5,7 @@ #include +#include #include #include #include @@ -1101,16 +1102,7 @@ void CTxMemPool::clear() _clear(); } -static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& mempoolDuplicate, const int64_t spendheight) -{ - TxValidationState dummy_state; // Not used. CheckTxInputs() should always pass - CAmount txfee = 0; - bool fCheckResult = tx.IsCoinBase() || Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, spendheight, txfee); - assert(fCheckResult); - UpdateCoins(tx, mempoolDuplicate, std::numeric_limits::max()); -} - -void CTxMemPool::check(CChainState& active_chainstate) const +void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendheight) const { if (m_check_ratio == 0) return; @@ -1123,20 +1115,16 @@ 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(&active_coins_tip)); - const int64_t spendheight = active_chainstate.m_chain.Height() + 1; - std::list waitingOnDependants; - for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { - unsigned int i = 0; + for (const auto& it : GetSortedDepthAndScore()) { 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. @@ -1144,17 +1132,17 @@ 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 { - assert(active_coins_tip.HaveCoin(txin.prevout)); } + // 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(); @@ -1181,6 +1169,9 @@ void CTxMemPool::check(CChainState& active_chainstate) const assert(it->GetSizeWithAncestors() == nSizeCheck); assert(it->GetSigOpCountWithAncestors() == 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; @@ -1199,24 +1190,12 @@ 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) - waitingOnDependants.push_back(&(*it)); - else { - CheckInputsAndUpdateCoins(tx, mempoolDuplicate, spendheight); - } - } - unsigned int stepsSinceLastRemove = 0; - while (!waitingOnDependants.empty()) { - const CTxMemPoolEntry* entry = waitingOnDependants.front(); - waitingOnDependants.pop_front(); - if (!mempoolDuplicate.HaveInputs(entry->GetTx())) { - waitingOnDependants.push_back(entry); - stepsSinceLastRemove++; - assert(stepsSinceLastRemove < waitingOnDependants.size()); - } else { - CheckInputsAndUpdateCoins(entry->GetTx(), mempoolDuplicate, spendheight); - stepsSinceLastRemove = 0; - } + TxValidationState dummy_state; // Not used. CheckTxInputs() should always pass + CAmount txfee = 0; + assert(!tx.IsCoinBase()); + assert(Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, spendheight, txfee)); + for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout); + AddCoins(mempoolDuplicate, tx, std::numeric_limits::max()); } for (auto it = mapNextTx.cbegin(); it != mapNextTx.cend(); it++) { uint256 hash = it->second->GetHash(); diff --git a/src/txmempool.h b/src/txmempool.h index d3f878a13d..5193856924 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -645,7 +645,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 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 diff --git a/src/validation.cpp b/src/validation.cpp index ef8eeb920a..51c5d28fc8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1386,12 +1386,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; PrecomputedTransactionData txdata(*ptxTo); @@ -2939,7 +2933,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->CoinsTip(), this->m_chain.Height() + 1); CheckForkWarningConditions(); diff --git a/src/validation.h b/src/validation.h index 1f11e3fab4..9b27f340da 100644 --- a/src/validation.h +++ b/src/validation.h @@ -266,9 +266,6 @@ bool GetUTXOCoin(CChainState& active_chainstate, const COutPoint& outpoint, Coin int GetUTXOHeight(CChainState& active_chainstate, const COutPoint& outpoint); int GetUTXOConfirmations(CChainState& active_chainstate, const COutPoint& outpoint); -/** 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 */ /**