diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 4393c9e84f..3f3403983e 100755 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -75,7 +75,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) @@ -100,7 +100,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() @@ -136,7 +136,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; @@ -171,7 +171,10 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc int nPackagesSelected = 0; int nDescendantsUpdated = 0; - addPackageTxs(nPackagesSelected, nDescendantsUpdated, min_tx_age); + if (m_mempool) { + LOCK(m_mempool->cs); + addPackageTxs(*m_mempool, nPackagesSelected, nDescendantsUpdated, min_tx_age); + } int64_t nTime1 = GetTimeMicros(); @@ -285,15 +288,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)) { @@ -316,23 +323,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 @@ -354,9 +344,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, std::chrono::seconds min_tx_age) +void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated, std::chrono::seconds min_tx_age) { - 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 @@ -364,8 +354,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda // Keep track of entries that failed inclusion, to avoid duplicate work CTxMemPool::setEntries failedTx; - // ELEMENTS: we use confidential_score instead of ancestor_score - 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 @@ -374,12 +363,27 @@ 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. - 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 != 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; + } } // Now that mi is not stale, determine which transaction to evaluate: @@ -387,15 +391,15 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda bool fUsingModified = false; modconftxscoreiter 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() && - CompareTxMemPoolEntryByConfidentialFee()(*modit, CTxMemPoolModifiedEntry(iter))) { + CompareTxMemPoolEntryByAncestorFee()(*modit, CTxMemPoolModifiedEntry(iter))) { // The best entry in mapModifiedTx has higher score // than the one from mapTx. // Switch which transaction (package) to consider @@ -454,7 +458,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); @@ -484,7 +488,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda ++nPackagesSelected; // Update transactions that depend on each of these - nDescendantsUpdated += UpdatePackagesForAdded(ancestors, mapModifiedTx); + nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx); } } } // namespace node diff --git a/src/node/miner.h b/src/node/miner.h old mode 100644 new mode 100755 index 1933541687..a10b4cefa8 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -158,7 +158,7 @@ private: int64_t m_lock_time_cutoff; const CChainParams& chainparams; - const CTxMemPool& m_mempool; + const CTxMemPool* const m_mempool; CChainState& m_chainstate; public: @@ -168,8 +168,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, std::chrono::seconds min_tx_age = std::chrono::seconds(0), DynaFedParamEntry* = nullptr, const std::vector* commit_scripts = nullptr); @@ -188,7 +188,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, std::chrono::seconds required_wait = std::chrono::seconds(0)) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs); + void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated, std::chrono::seconds required_wait = std::chrono::seconds(0)) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs); // helper functions for addPackageTxs() /** Remove confirmed (inBlock) entries from given set */ @@ -200,15 +200,8 @@ 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 - * 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); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 072dd8dd49..e2b37f7ef8 100755 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -163,7 +163,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; @@ -373,8 +373,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"); } @@ -774,7 +773,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"); @@ -1177,7 +1176,7 @@ static RPCHelpMan getnewblockhex() CScript feeDestinationScript = Params().GetConsensus().mandatory_coinbase_destination; if (feeDestinationScript == CScript()) feeDestinationScript = CScript() << OP_TRUE; const NodeContext& node = EnsureAnyNodeContext(request.context); - std::unique_ptr pblocktemplate(BlockAssembler(chainman.ActiveChainstate(), *node.mempool).CreateNewBlock(feeDestinationScript, std::chrono::seconds(required_wait), &proposed, data_commitments.empty() ? nullptr : &data_commitments)); + std::unique_ptr pblocktemplate(BlockAssembler(chainman.ActiveChainstate(), node.mempool.get()).CreateNewBlock(feeDestinationScript, std::chrono::seconds(required_wait), &proposed, data_commitments.empty() ? nullptr : &data_commitments)); if (!pblocktemplate.get()) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Wallet keypool empty"); } diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index ba1eacfc78..c31e4e51f7 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 58cdabd667..8c67e6e708 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 8a3b7ebd1a..cf7ba2b336 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 a6d624fe84..88cf9647e7 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 bfd23f82bc..43f665fa00 100755 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -301,8 +301,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 3ff7955b6a..1a3817df1f 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; @@ -330,7 +330,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;