From 130981fbbd9bb89ae7f874cc3b927e63b5ff6ac9 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 15 Jun 2022 16:27:51 +0100 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#25223: [kernel 2e/n] miner: Make `mempool` optional, stop constructing temporary empty mempools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0f1a259657280afc727db97689512aef5ca928fc miner: Make mempool optional for BlockAssembler (Carl Dong) cc5739b27df830d138119eaa13f2286d91d0dadd miner: Make UpdatePackagesForAdded static (Carl Dong) f024578b3a5c40e275e23d1c8e82530e235fdbf9 miner: Absorb SkipMapTxEntry into addPackageTxs (Carl Dong) Pull request description: This is part of the libbitcoinkernel project: #24303, https://github.com/bitcoin/bitcoin/projects/18 This is **_NOT_** dependent on, but is a "companion-PR" to #25215. ### Abstract This PR removes the need to construct `BlockAssembler` with temporary, empty mempools in cases where we don't want to source transactions from the mempool (e.g. in `TestChain100Setup::CreateBlock` and `generateblock`). After this PR, `BlockAssembler` will accept a `CTxMemPool` pointer and handle the `nullptr` case instead of requiring a `CTxMemPool` reference. An overview of the changes is best seen in the changes in the header file: ```diff diff --git a/src/node/miner.h b/src/node/miner.h index 7cf8e3fb9e..7e9f503602 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* 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 */ @@ -189,15 +189,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); ``` ### Alternatives Aside from approach in this current PR, we can also take the approach of moving the `CTxMemPool*` argument from the `BlockAssembler` constructor to `BlockAssembler::CreateNewBlock`, since that's where it's needed anyway. I did not push this approach because it requires quite a lot of call sites to be changed. However, I do have it coded up and can do that if people express a strong preference. This would look something like: ``` BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options); BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool* maybe_mempool); ``` ### Future work Although wholly out of scope for this PR, we could potentially refine the `BlockAssembler` interface further, so that we have: ``` BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options); BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, std::vector& txs); BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool& mempool); ``` Whereby `TestChain100Setup::CreateBlock` and `generateblock` would call the `BlockAssembler::CreateNewBlock` that takes in `CTransaction`s and we can potentially remove `RegenerateCommitments` altogether. All other callers can use the `CTxMemPool` version. ACKs for top commit: glozow: ACK 0f1a259657280afc727db97689512aef5ca928fc laanwj: Code review ACK 0f1a259657280afc727db97689512aef5ca928fc MarcoFalke: ACK 0f1a259657280afc727db97689512aef5ca928fc 🐊 Tree-SHA512: 2b4b1dbb43d85719f241ad1f19ceb7fc50cf764721da425a3d1ff71bd16328c4f86acff22e565bc9abee770d3ac8827a6676b66daa93dbf42dd817ad929e9448 --- src/node/miner.cpp | 80 +++++++++++++++------------- src/node/miner.h | 18 +++---- 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 | 13 +++++ src/test/validation_block_tests.cpp | 2 +- 9 files changed, 69 insertions(+), 59 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 4bc58264305a..a4ca62bd09a7 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -67,7 +67,7 @@ BlockAssembler::Options::Options() nBlockMaxSize = DEFAULT_BLOCK_MAX_SIZE; } -BlockAssembler::BlockAssembler(CChainState& chainstate, const NodeContext& node, const CTxMemPool& mempool, const CChainParams& params, const Options& options) : +BlockAssembler::BlockAssembler(CChainState& chainstate, const NodeContext& node, const CTxMemPool* mempool, const CChainParams& params, const Options& options) : m_blockman(chainstate.m_blockman), m_cpoolman(*Assert(node.cpoolman)), m_chain_helper(chainstate.ChainHelper()), @@ -104,7 +104,7 @@ static BlockAssembler::Options DefaultOptions() return options; } -BlockAssembler::BlockAssembler(CChainState& chainstate, const NodeContext& node, const CTxMemPool& mempool, const CChainParams& params) +BlockAssembler::BlockAssembler(CChainState& chainstate, const NodeContext& node, const CTxMemPool* mempool, const CChainParams& params) : BlockAssembler(chainstate, node, mempool, params, DefaultOptions()) {} void BlockAssembler::resetBlock() @@ -138,7 +138,7 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc pblocktemplate->vTxFees.push_back(-1); // updated at end pblocktemplate->vTxSigOps.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; @@ -181,8 +181,10 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc int nPackagesSelected = 0; int nDescendantsUpdated = 0; - - addPackageTxs(nPackagesSelected, nDescendantsUpdated, pindexPrev); + if (m_mempool) { + LOCK(m_mempool->cs); + addPackageTxs(*m_mempool, nPackagesSelected, nDescendantsUpdated, pindexPrev); + } int64_t nTime1 = GetTimeMicros(); @@ -347,15 +349,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)) { @@ -377,23 +383,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 @@ -415,9 +404,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, const CBlockIndex* const pindexPrev) +void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated, const CBlockIndex* const pindexPrev) { - AssertLockHeld(m_mempool.cs); + AssertLockHeld(mempool.cs); // This credit pool is used only to check withdrawal limits and to find // duplicates of indexes. There's used `BlockSubsidy` equaled to 0 @@ -436,7 +425,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 @@ -445,12 +434,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: @@ -458,13 +462,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 @@ -547,7 +551,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); @@ -577,7 +581,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); } } diff --git a/src/node/miner.h b/src/node/miner.h index c64007e269c9..ec86912bc28a 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -177,7 +177,8 @@ class BlockAssembler llmq::CInstantSendManager& m_isman; llmq::CQuorumSnapshotManager& m_qsnapman; const CChainParams& chainparams; - const CTxMemPool& m_mempool; + const CTxMemPool* const m_mempool; + CChainState& m_chainstate; const llmq::CQuorumBlockProcessor& m_quorum_block_processor; const llmq::CQuorumManager& m_qman; @@ -188,8 +189,8 @@ class BlockAssembler CFeeRate blockMinFeeRate; }; - explicit BlockAssembler(CChainState& chainstate, const NodeContext& node, const CTxMemPool& mempool, const CChainParams& params); - explicit BlockAssembler(CChainState& chainstate, const NodeContext& node, const CTxMemPool& mempool, const CChainParams& params, + explicit BlockAssembler(CChainState& chainstate, const NodeContext& node, const CTxMemPool* mempool, const CChainParams& params); + explicit BlockAssembler(CChainState& chainstate, const NodeContext& node, const CTxMemPool* mempool, const CChainParams& params, const Options& options); /** Construct a new block template with coinbase to scriptPubKeyIn */ @@ -209,8 +210,8 @@ class BlockAssembler /** 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, - const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs); + void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated, + const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs); // helper functions for addPackageTxs() /** Remove confirmed (inBlock) entries from given set */ @@ -222,15 +223,8 @@ class BlockAssembler * 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); }; /** Modify the extranonce in a block */ diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 12359f668096..718de0b5df07 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -169,7 +169,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, const NodeContext& n UniValue blockHashes(UniValue::VARR); while (nHeight < nHeightEnd && !ShutdownRequested()) { - std::unique_ptr pblocktemplate(BlockAssembler(chainman.ActiveChainstate(), node, mempool, Params()).CreateNewBlock(coinbase_script)); + std::unique_ptr pblocktemplate(BlockAssembler(chainman.ActiveChainstate(), node, &mempool, Params()).CreateNewBlock(coinbase_script)); if (!pblocktemplate.get()) throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block"); CBlock *pblock = &pblocktemplate->block; @@ -375,8 +375,7 @@ static RPCHelpMan generateblock() { LOCK(cs_main); - CTxMemPool empty_mempool; - std::unique_ptr blocktemplate(BlockAssembler(active_chainstate, node, empty_mempool, chainparams).CreateNewBlock(coinbase_script)); + std::unique_ptr blocktemplate(BlockAssembler(active_chainstate, node, nullptr, chainparams).CreateNewBlock(coinbase_script)); if (!blocktemplate) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block"); } @@ -823,7 +822,7 @@ static RPCHelpMan getblocktemplate() // Create new block CScript scriptDummy = CScript() << OP_TRUE; EnsureLLMQContext(node); - pblocktemplate = BlockAssembler(active_chainstate, node, mempool, Params()).CreateNewBlock(scriptDummy); + pblocktemplate = BlockAssembler(active_chainstate, node, &mempool, Params()).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 a71f140e0660..0383fdda22ed 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -63,7 +63,7 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev, const CScript& scriptPubKey) { const CChainParams& chainparams = Params(); - std::unique_ptr pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, chainparams).CreateNewBlock(scriptPubKey); + std::unique_ptr pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), chainparams).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 36da13ce38be..9849feb1ede7 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -95,7 +95,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, con options.nBlockMaxSize = fuzzed_data_provider.ConsumeIntegralInRange(0U, MaxBlockSize(true)); options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; - auto assembler = BlockAssembler{chainstate, node, *static_cast(&tx_pool), chainstate.m_params, options}; + auto assembler = BlockAssembler{chainstate, node, static_cast(&tx_pool), chainstate.m_params, 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 db9361aaca0d..3ec2c900283a 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -53,7 +53,7 @@ BlockAssembler MinerTestingSetup::AssemblerForTest(const CChainParams& params) options.nBlockMaxSize = DEFAULT_BLOCK_MAX_SIZE; options.blockMinFeeRate = blockMinFeeRate; - return BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, params, options); + return BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), params, options); } constexpr static struct { diff --git a/src/test/util/mining.cpp b/src/test/util/mining.cpp index 270f81bf56ef..653814db5e7a 100644 --- a/src/test/util/mining.cpp +++ b/src/test/util/mining.cpp @@ -77,7 +77,7 @@ std::shared_ptr PrepareBlock(const NodeContext& node, const CScript& coi { assert(node.mempool); auto block = std::make_shared( - BlockAssembler{node.chainman->ActiveChainstate(), node, *node.mempool, Params()} + BlockAssembler{node.chainman->ActiveChainstate(), node, Assert(node.mempool.get()), Params()} .CreateNewBlock(coinbase_scriptPubKey) ->block); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 54d36e8df3b0..367f526ce165 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -428,6 +428,19 @@ void TestChainSetup::mineBlocks(int num_blocks) } } +CBlock TestChain100Setup::CreateBlock( + const std::vector& txns, + const CScript& scriptPubKey, + CChainState& chainstate) +{ + CBlock block = BlockAssembler{chainstate, nullptr}.CreateNewBlock(scriptPubKey)->block; + + Assert(block.vtx.size() == 1); + for (const CMutableTransaction& tx : txns) { + block.vtx.push_back(MakeTransactionRef(tx)); + } +} + CBlock TestChainSetup::CreateAndProcessBlock( const std::vector& txns, const CScript& scriptPubKey, diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 242b4c4838aa..d87929767e33 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -64,7 +64,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, *m_node.mempool, Params()).CreateNewBlock(CScript{} << i++ << OP_TRUE); + auto ptemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(CScript{} << i++ << OP_TRUE); auto pblock = std::make_shared(ptemplate->block); pblock->hashPrevBlock = prev_hash; pblock->nTime = ++time; From d1be47d76efd87d7c9065e0f2f2cb880e83f3aa0 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Fri, 30 May 2025 16:05:12 -0500 Subject: [PATCH 2/2] Fix remaining test files to use mempool pointer Updates test files to use the new BlockAssembler constructor signature that takes CTxMemPool* instead of CTxMemPool&. --- src/node/miner.h | 1 - src/test/block_reward_reallocation_tests.cpp | 14 +++++++------- src/test/dynamic_activation_thresholds_tests.cpp | 2 +- src/test/util/setup_common.cpp | 15 +-------------- 4 files changed, 9 insertions(+), 23 deletions(-) diff --git a/src/node/miner.h b/src/node/miner.h index ec86912bc28a..fcb81390bcbb 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -178,7 +178,6 @@ class BlockAssembler llmq::CQuorumSnapshotManager& m_qsnapman; const CChainParams& chainparams; const CTxMemPool* const m_mempool; - CChainState& m_chainstate; const llmq::CQuorumBlockProcessor& m_quorum_block_processor; const llmq::CQuorumManager& m_qman; diff --git a/src/test/block_reward_reallocation_tests.cpp b/src/test/block_reward_reallocation_tests.cpp index 50a1ecd477f7..14461acbf939 100644 --- a/src/test/block_reward_reallocation_tests.cpp +++ b/src/test/block_reward_reallocation_tests.cpp @@ -182,7 +182,7 @@ BOOST_FIXTURE_TEST_CASE(block_reward_reallocation, TestChainBRRBeforeActivationS BOOST_REQUIRE(dmnman.GetListAtChainTip().HasMN(tx.GetHash())); BOOST_CHECK(tip->nHeight < Params().GetConsensus().BRRHeight); // Creating blocks by different ways - const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, Params()).CreateNewBlock(coinbasePubKey); + const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(coinbasePubKey); } for ([[maybe_unused]] auto _ : irange::range(499)) { CreateAndProcessBlock({}, coinbaseKey); @@ -212,7 +212,7 @@ BOOST_FIXTURE_TEST_CASE(block_reward_reallocation, TestChainBRRBeforeActivationS BOOST_REQUIRE(dmnman.GetListAtChainTip().HasMN(tx.GetHash())); const CAmount block_subsidy = GetBlockSubsidyInner(tip->nBits, tip->nHeight, consensus_params, isV20Active); const CAmount masternode_payment = GetMasternodePayment(tip->nHeight, block_subsidy, isV20Active); - const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, Params()).CreateNewBlock(coinbasePubKey); + const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(coinbasePubKey); BOOST_CHECK_EQUAL(pblocktemplate->voutMasternodePayments[0].nValue, masternode_payment); } @@ -226,7 +226,7 @@ BOOST_FIXTURE_TEST_CASE(block_reward_reallocation, TestChainBRRBeforeActivationS const bool isV20Active{DeploymentActiveAfter(tip, consensus_params, Consensus::DEPLOYMENT_V20)}; const CAmount block_subsidy = GetBlockSubsidyInner(tip->nBits, tip->nHeight, consensus_params, isV20Active); const CAmount masternode_payment = GetMasternodePayment(tip->nHeight, block_subsidy, isV20Active); - const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, Params()).CreateNewBlock(coinbasePubKey); + const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(coinbasePubKey); BOOST_CHECK_EQUAL(pblocktemplate->block.vtx[0]->GetValueOut(), 28847249686); BOOST_CHECK_EQUAL(pblocktemplate->voutMasternodePayments[0].nValue, masternode_payment); BOOST_CHECK_EQUAL(pblocktemplate->voutMasternodePayments[0].nValue, 14423624841); // 0.4999999999 @@ -243,7 +243,7 @@ BOOST_FIXTURE_TEST_CASE(block_reward_reallocation, TestChainBRRBeforeActivationS const bool isV20Active{DeploymentActiveAfter(tip, consensus_params, Consensus::DEPLOYMENT_V20)}; const CAmount block_subsidy = GetBlockSubsidyInner(tip->nBits, tip->nHeight, consensus_params, isV20Active); const CAmount masternode_payment = GetMasternodePayment(tip->nHeight, block_subsidy, isV20Active); - const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, Params()).CreateNewBlock(coinbasePubKey); + const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(coinbasePubKey); BOOST_CHECK_EQUAL(pblocktemplate->voutMasternodePayments[0].nValue, masternode_payment); } } @@ -261,7 +261,7 @@ BOOST_FIXTURE_TEST_CASE(block_reward_reallocation, TestChainBRRBeforeActivationS CAmount expected_block_reward = block_subsidy_potential - block_subsidy_potential / 5; const CAmount masternode_payment = GetMasternodePayment(tip->nHeight, block_subsidy, isV20Active); - const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, Params()).CreateNewBlock(coinbasePubKey); + const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(coinbasePubKey); BOOST_CHECK_EQUAL(pblocktemplate->block.vtx[0]->GetValueOut(), expected_block_reward); BOOST_CHECK_EQUAL(pblocktemplate->block.vtx[0]->GetValueOut(), 141734128); BOOST_CHECK_EQUAL(pblocktemplate->voutMasternodePayments[0].nValue, masternode_payment); @@ -281,7 +281,7 @@ BOOST_FIXTURE_TEST_CASE(block_reward_reallocation, TestChainBRRBeforeActivationS const bool isMNRewardReallocated{DeploymentActiveAfter(tip, consensus_params, Consensus::DEPLOYMENT_MN_RR)}; const CAmount block_subsidy = GetBlockSubsidyInner(tip->nBits, tip->nHeight, consensus_params, isV20Active); CAmount masternode_payment = GetMasternodePayment(tip->nHeight, block_subsidy, isV20Active); - const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, Params()).CreateNewBlock(coinbasePubKey); + const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(coinbasePubKey); if (isMNRewardReallocated) { const CAmount platform_payment = PlatformShare(masternode_payment); @@ -303,7 +303,7 @@ BOOST_FIXTURE_TEST_CASE(block_reward_reallocation, TestChainBRRBeforeActivationS CAmount masternode_payment = GetMasternodePayment(tip->nHeight, block_subsidy, isV20Active); const CAmount platform_payment = PlatformShare(masternode_payment); masternode_payment -= platform_payment; - const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, Params()).CreateNewBlock(coinbasePubKey); + const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(coinbasePubKey); CAmount block_subsidy_potential = block_subsidy + block_subsidy_sb; BOOST_CHECK_EQUAL(tip->nHeight, 2358); diff --git a/src/test/dynamic_activation_thresholds_tests.cpp b/src/test/dynamic_activation_thresholds_tests.cpp index 48cc4b81ce7f..c48ef06aab2b 100644 --- a/src/test/dynamic_activation_thresholds_tests.cpp +++ b/src/test/dynamic_activation_thresholds_tests.cpp @@ -75,7 +75,7 @@ struct TestChainDATSetup : public TestChainSetup BOOST_CHECK_EQUAL(g_versionbitscache.State(m_node.chainman->ActiveChain().Tip(), consensus_params, deployment_id), ThresholdState::STARTED); BOOST_CHECK_EQUAL(g_versionbitscache.Statistics(m_node.chainman->ActiveChain().Tip(), consensus_params, deployment_id).threshold, threshold(0)); // Next block should be signaling by default - const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, *m_node.mempool, Params()).CreateNewBlock(coinbasePubKey); + const auto pblocktemplate = BlockAssembler(m_node.chainman->ActiveChainstate(), m_node, m_node.mempool.get(), Params()).CreateNewBlock(coinbasePubKey); const uint32_t bitmask = ((uint32_t)1) << consensus_params.vDeployments[deployment_id].bit; BOOST_CHECK_EQUAL(m_node.chainman->ActiveChain().Tip()->nVersion & bitmask, 0); BOOST_CHECK_EQUAL(pblocktemplate->block.nVersion & bitmask, bitmask); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 367f526ce165..b1f01a85660c 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -428,19 +428,6 @@ void TestChainSetup::mineBlocks(int num_blocks) } } -CBlock TestChain100Setup::CreateBlock( - const std::vector& txns, - const CScript& scriptPubKey, - CChainState& chainstate) -{ - CBlock block = BlockAssembler{chainstate, nullptr}.CreateNewBlock(scriptPubKey)->block; - - Assert(block.vtx.size() == 1); - for (const CMutableTransaction& tx : txns) { - block.vtx.push_back(MakeTransactionRef(tx)); - } -} - CBlock TestChainSetup::CreateAndProcessBlock( const std::vector& txns, const CScript& scriptPubKey, @@ -475,7 +462,7 @@ CBlock TestChainSetup::CreateBlock( { const CChainParams& chainparams = Params(); CTxMemPool empty_pool; - CBlock block = BlockAssembler(chainstate, m_node, empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block; + CBlock block = BlockAssembler(chainstate, m_node, &empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block; std::vector llmqCommitments; for (const auto& tx : block.vtx) {