From f024578b3a5c40e275e23d1c8e82530e235fdbf9 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 20 May 2022 13:22:47 -0400 Subject: [PATCH 1/3] miner: Absorb SkipMapTxEntry into addPackageTxs SkipMapTxEntry is a short helper function that's only used in addPackageTxs, we can just inline it, keep the comments, and avoid the unnecessary interface and lock annotations. --- src/node/miner.cpp | 40 +++++++++++++++++++--------------------- src/node/miner.h | 3 --- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 01db49d5cf1..fd148640e7f 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -262,23 +262,6 @@ int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& already return nDescendantsUpdated; } -// Skip entries in mapTx that are already in a block or are present -// in mapModifiedTx (which implies that the mapTx ancestor state is -// stale due to ancestor inclusion in the block) -// Also skip transactions that we've already failed to add. This can happen if -// we consider a transaction in mapModifiedTx and it fails: we can then -// potentially consider it again while walking mapTx. It's currently -// guaranteed to fail again, but as a belt-and-suspenders check we put it in -// failedTx and avoid re-evaluation, since the re-evaluation would be using -// cached size/sigops/fee values that are not actually correct. -bool BlockAssembler::SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) -{ - AssertLockHeld(m_mempool.cs); - - assert(it != m_mempool.mapTx.end()); - return mapModifiedTx.count(it) || inBlock.count(it) || failedTx.count(it); -} - void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::vector& sortedEntries) { // Sort package by ancestor count @@ -321,10 +304,25 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda while (mi != m_mempool.mapTx.get().end() || !mapModifiedTx.empty()) { // First try to find a new transaction in mapTx to evaluate. - if (mi != m_mempool.mapTx.get().end() && - SkipMapTxEntry(m_mempool.mapTx.project<0>(mi), mapModifiedTx, failedTx)) { - ++mi; - continue; + // + // Skip entries in mapTx that are already in a block or are present + // in mapModifiedTx (which implies that the mapTx ancestor state is + // stale due to ancestor inclusion in the block) + // Also skip transactions that we've already failed to add. This can happen if + // we consider a transaction in mapModifiedTx and it fails: we can then + // potentially consider it again while walking mapTx. It's currently + // guaranteed to fail again, but as a belt-and-suspenders check we put it in + // failedTx and avoid re-evaluation, since the re-evaluation would be using + // cached size/sigops/fee values that are not actually correct. + /** Return true if given transaction from mapTx has already been evaluated, + * or if the transaction's cached data in mapTx is incorrect. */ + if (mi != m_mempool.mapTx.get().end()) { + auto it = m_mempool.mapTx.project<0>(mi); + assert(it != m_mempool.mapTx.end()); + if (mapModifiedTx.count(it) || inBlock.count(it) || failedTx.count(it)) { + ++mi; + continue; + } } // Now that mi is not stale, determine which transaction to evaluate: diff --git a/src/node/miner.h b/src/node/miner.h index 7cf8e3fb9e1..b5f70d2fc0c 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -189,9 +189,6 @@ private: * These checks should always succeed, and they're here * only as an extra check in case of suboptimal node configuration */ bool TestPackageTransactions(const CTxMemPool::setEntries& package) const; - /** Return true if given transaction from mapTx has already been evaluated, - * or if the transaction's cached data in mapTx is incorrect. */ - bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs); /** Sort the package in an order that is valid to appear in a block */ void SortForBlock(const CTxMemPool::setEntries& package, std::vector& sortedEntries); /** Add descendants of given transactions to mapModifiedTx with ancestor From cc5739b27df830d138119eaa13f2286d91d0dadd Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 20 May 2022 13:28:35 -0400 Subject: [PATCH 2/3] miner: Make UpdatePackagesForAdded static Since UpdatePackagesForAdded is a helper function that's only used in addPackageTxs we can make it static and avoid the unnecessary interface and in-header lock annotation. --- src/node/miner.cpp | 14 +++++++++----- src/node/miner.h | 4 ---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index fd148640e7f..167499ee415 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -232,15 +232,19 @@ void BlockAssembler::AddToBlock(CTxMemPool::txiter iter) } } -int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, - indexed_modified_transaction_set &mapModifiedTx) +/** Add descendants of given transactions to mapModifiedTx with ancestor + * state updated assuming given transactions are inBlock. Returns number + * of updated descendants. */ +static int UpdatePackagesForAdded(const CTxMemPool& mempool, + const CTxMemPool::setEntries& alreadyAdded, + indexed_modified_transaction_set& mapModifiedTx) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs) { - AssertLockHeld(m_mempool.cs); + AssertLockHeld(mempool.cs); int nDescendantsUpdated = 0; for (CTxMemPool::txiter it : alreadyAdded) { CTxMemPool::setEntries descendants; - m_mempool.CalculateDescendants(it, descendants); + mempool.CalculateDescendants(it, descendants); // Insert all descendants (not yet in block) into the modified set for (CTxMemPool::txiter desc : descendants) { if (alreadyAdded.count(desc)) { @@ -421,7 +425,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda ++nPackagesSelected; // Update transactions that depend on each of these - nDescendantsUpdated += UpdatePackagesForAdded(ancestors, mapModifiedTx); + nDescendantsUpdated += UpdatePackagesForAdded(m_mempool, ancestors, mapModifiedTx); } } } // namespace node diff --git a/src/node/miner.h b/src/node/miner.h index b5f70d2fc0c..4e4f02ce4d2 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -191,10 +191,6 @@ private: bool TestPackageTransactions(const CTxMemPool::setEntries& package) const; /** Sort the package in an order that is valid to appear in a block */ void SortForBlock(const CTxMemPool::setEntries& package, std::vector& sortedEntries); - /** Add descendants of given transactions to mapModifiedTx with ancestor - * state updated assuming given transactions are inBlock. Returns number - * of updated descendants. */ - int UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set& mapModifiedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs); }; int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev); From 0f1a259657280afc727db97689512aef5ca928fc Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 25 May 2022 22:06:23 -0400 Subject: [PATCH 3/3] miner: Make mempool optional for BlockAssembler ...also adjust callers Changes: - In BlockAssembler::CreateNewBlock, we now only lock m_mempool->cs and call addPackageTxs if m_mempool is not nullptr - BlockAssembler::addPackageTxs now takes in a mempool reference, and is annotated to require that mempool's lock. - In TestChain100Setup::CreateBlock and generateblock, don't construct an empty mempool, just pass in a nullptr for mempool --- src/node/miner.cpp | 33 +++++++++++++++------------- src/node/miner.h | 8 +++---- src/rpc/mining.cpp | 7 +++--- src/test/blockfilter_index_tests.cpp | 2 +- src/test/fuzz/tx_pool.cpp | 2 +- src/test/miner_tests.cpp | 2 +- src/test/util/mining.cpp | 2 +- src/test/util/setup_common.cpp | 3 +-- src/test/validation_block_tests.cpp | 4 ++-- 9 files changed, 32 insertions(+), 31 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 167499ee415..9db10feae48 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -62,7 +62,7 @@ BlockAssembler::Options::Options() nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT; } -BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options) +BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool, const Options& options) : chainparams{chainstate.m_chainman.GetParams()}, m_mempool(mempool), m_chainstate(chainstate) @@ -87,7 +87,7 @@ static BlockAssembler::Options DefaultOptions() return options; } -BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool) +BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool) : BlockAssembler(chainstate, mempool, DefaultOptions()) {} void BlockAssembler::resetBlock() @@ -121,7 +121,7 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc pblocktemplate->vTxFees.push_back(-1); // updated at end pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end - LOCK2(cs_main, m_mempool.cs); + LOCK(::cs_main); CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip(); assert(pindexPrev != nullptr); nHeight = pindexPrev->nHeight + 1; @@ -138,7 +138,10 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc int nPackagesSelected = 0; int nDescendantsUpdated = 0; - addPackageTxs(nPackagesSelected, nDescendantsUpdated); + if (m_mempool) { + LOCK(m_mempool->cs); + addPackageTxs(*m_mempool, nPackagesSelected, nDescendantsUpdated); + } int64_t nTime1 = GetTimeMicros(); @@ -287,9 +290,9 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve // Each time through the loop, we compare the best transaction in // mapModifiedTxs with the next transaction in the mempool to decide what // transaction package to work on next. -void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) +void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) { - AssertLockHeld(m_mempool.cs); + AssertLockHeld(mempool.cs); // mapModifiedTx will store sorted packages after they are modified // because some of their txs are already in the block @@ -297,7 +300,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda // Keep track of entries that failed inclusion, to avoid duplicate work CTxMemPool::setEntries failedTx; - CTxMemPool::indexed_transaction_set::index::type::iterator mi = m_mempool.mapTx.get().begin(); + CTxMemPool::indexed_transaction_set::index::type::iterator mi = mempool.mapTx.get().begin(); CTxMemPool::txiter iter; // Limit the number of attempts to add transactions to the block when it is @@ -306,7 +309,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda const int64_t MAX_CONSECUTIVE_FAILURES = 1000; int64_t nConsecutiveFailed = 0; - while (mi != m_mempool.mapTx.get().end() || !mapModifiedTx.empty()) { + while (mi != mempool.mapTx.get().end() || !mapModifiedTx.empty()) { // First try to find a new transaction in mapTx to evaluate. // // Skip entries in mapTx that are already in a block or are present @@ -320,9 +323,9 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda // cached size/sigops/fee values that are not actually correct. /** Return true if given transaction from mapTx has already been evaluated, * or if the transaction's cached data in mapTx is incorrect. */ - if (mi != m_mempool.mapTx.get().end()) { - auto it = m_mempool.mapTx.project<0>(mi); - assert(it != m_mempool.mapTx.end()); + if (mi != mempool.mapTx.get().end()) { + auto it = mempool.mapTx.project<0>(mi); + assert(it != mempool.mapTx.end()); if (mapModifiedTx.count(it) || inBlock.count(it) || failedTx.count(it)) { ++mi; continue; @@ -334,13 +337,13 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda bool fUsingModified = false; modtxscoreiter modit = mapModifiedTx.get().begin(); - if (mi == m_mempool.mapTx.get().end()) { + if (mi == mempool.mapTx.get().end()) { // We're out of entries in mapTx; use the entry from mapModifiedTx iter = modit->iter; fUsingModified = true; } else { // Try to compare the mapTx entry to the mapModifiedTx entry - iter = m_mempool.mapTx.project<0>(mi); + iter = mempool.mapTx.project<0>(mi); if (modit != mapModifiedTx.get().end() && CompareTxMemPoolEntryByAncestorFee()(*modit, CTxMemPoolModifiedEntry(iter))) { // The best entry in mapModifiedTx has higher score @@ -395,7 +398,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda CTxMemPool::setEntries ancestors; uint64_t nNoLimit = std::numeric_limits::max(); std::string dummy; - m_mempool.CalculateMemPoolAncestors(*iter, ancestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false); + mempool.CalculateMemPoolAncestors(*iter, ancestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false); onlyUnconfirmed(ancestors); ancestors.insert(iter); @@ -425,7 +428,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda ++nPackagesSelected; // Update transactions that depend on each of these - nDescendantsUpdated += UpdatePackagesForAdded(m_mempool, ancestors, mapModifiedTx); + nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx); } } } // namespace node diff --git a/src/node/miner.h b/src/node/miner.h index 4e4f02ce4d2..26454df3dff 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -147,7 +147,7 @@ private: int64_t m_lock_time_cutoff; const CChainParams& chainparams; - const CTxMemPool& m_mempool; + const CTxMemPool* const m_mempool; CChainState& m_chainstate; public: @@ -157,8 +157,8 @@ public: CFeeRate blockMinFeeRate; }; - explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool); - explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options); + explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool); + explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool, const Options& options); /** Construct a new block template with coinbase to scriptPubKeyIn */ std::unique_ptr CreateNewBlock(const CScript& scriptPubKeyIn); @@ -177,7 +177,7 @@ private: /** Add transactions based on feerate including unconfirmed ancestors * Increments nPackagesSelected / nDescendantsUpdated with corresponding * statistics from the package selection (for logging statistics). */ - void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs); + void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs); // helper functions for addPackageTxs() /** Remove confirmed (inBlock) entries from given set */ diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 8fb6daf0cb4..ea6db1e9a0e 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -144,7 +144,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& me { UniValue blockHashes(UniValue::VARR); while (nGenerate > 0 && !ShutdownRequested()) { - std::unique_ptr pblocktemplate(BlockAssembler{chainman.ActiveChainstate(), mempool}.CreateNewBlock(coinbase_script)); + std::unique_ptr pblocktemplate(BlockAssembler{chainman.ActiveChainstate(), &mempool}.CreateNewBlock(coinbase_script)); if (!pblocktemplate.get()) throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block"); CBlock *pblock = &pblocktemplate->block; @@ -354,8 +354,7 @@ static RPCHelpMan generateblock() { LOCK(cs_main); - CTxMemPool empty_mempool; - std::unique_ptr blocktemplate(BlockAssembler{chainman.ActiveChainstate(), empty_mempool}.CreateNewBlock(coinbase_script)); + std::unique_ptr blocktemplate(BlockAssembler{chainman.ActiveChainstate(), nullptr}.CreateNewBlock(coinbase_script)); if (!blocktemplate) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block"); } @@ -753,7 +752,7 @@ static RPCHelpMan getblocktemplate() // Create new block CScript scriptDummy = CScript() << OP_TRUE; - pblocktemplate = BlockAssembler{active_chainstate, mempool}.CreateNewBlock(scriptDummy); + pblocktemplate = BlockAssembler{active_chainstate, &mempool}.CreateNewBlock(scriptDummy); if (!pblocktemplate) throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory"); diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index ba1eacfc784..c31e4e51f7c 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -65,7 +65,7 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev, const std::vector& txns, const CScript& scriptPubKey) { - std::unique_ptr pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), *m_node.mempool}.CreateNewBlock(scriptPubKey); + std::unique_ptr pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get()}.CreateNewBlock(scriptPubKey); CBlock& block = pblocktemplate->block; block.hashPrevBlock = prev->GetBlockHash(); block.nTime = prev->nTime + 1; diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 771d7a11cbb..4f40608c4fa 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -97,7 +97,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, CCh BlockAssembler::Options options; options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT); options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; - auto assembler = BlockAssembler{chainstate, *static_cast(&tx_pool), options}; + auto assembler = BlockAssembler{chainstate, &tx_pool, options}; auto block_template = assembler.CreateNewBlock(CScript{} << OP_TRUE); Assert(block_template->block.vtx.size() >= 1); } diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 439ad174b30..eca4fbf15c5 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -52,7 +52,7 @@ BlockAssembler MinerTestingSetup::AssemblerForTest(const CChainParams& params) options.nBlockMaxWeight = MAX_BLOCK_WEIGHT; options.blockMinFeeRate = blockMinFeeRate; - return BlockAssembler{m_node.chainman->ActiveChainstate(), *m_node.mempool, options}; + return BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get(), options}; } constexpr static struct { diff --git a/src/test/util/mining.cpp b/src/test/util/mining.cpp index a6d624fe84e..88cf9647e7f 100644 --- a/src/test/util/mining.cpp +++ b/src/test/util/mining.cpp @@ -77,7 +77,7 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey) std::shared_ptr PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey) { auto block = std::make_shared( - BlockAssembler{Assert(node.chainman)->ActiveChainstate(), *Assert(node.mempool)} + BlockAssembler{Assert(node.chainman)->ActiveChainstate(), Assert(node.mempool.get())} .CreateNewBlock(coinbase_scriptPubKey) ->block); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index b7566bd1fab..67820d50bdc 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -277,8 +277,7 @@ CBlock TestChain100Setup::CreateBlock( const CScript& scriptPubKey, CChainState& chainstate) { - CTxMemPool empty_pool; - CBlock block = BlockAssembler{chainstate, empty_pool}.CreateNewBlock(scriptPubKey)->block; + CBlock block = BlockAssembler{chainstate, nullptr}.CreateNewBlock(scriptPubKey)->block; Assert(block.vtx.size() == 1); for (const CMutableTransaction& tx : txns) { diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 331da691b58..7ade4d81959 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -65,7 +65,7 @@ std::shared_ptr MinerTestingSetup::Block(const uint256& prev_hash) static int i = 0; static uint64_t time = Params().GenesisBlock().nTime; - auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), *m_node.mempool}.CreateNewBlock(CScript{} << i++ << OP_TRUE); + auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get()}.CreateNewBlock(CScript{} << i++ << OP_TRUE); auto pblock = std::make_shared(ptemplate->block); pblock->hashPrevBlock = prev_hash; pblock->nTime = ++time; @@ -327,7 +327,7 @@ BOOST_AUTO_TEST_CASE(witness_commitment_index) { CScript pubKey; pubKey << 1 << OP_TRUE; - auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), *m_node.mempool}.CreateNewBlock(pubKey); + auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get()}.CreateNewBlock(pubKey); CBlock pblock = ptemplate->block; CTxOut witness;