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:
0diff --git a/src/node/miner.h b/src/node/miner.h
1index 7cf8e3fb9e..7e9f503602 100644
2--- a/src/node/miner.h
3+++ b/src/node/miner.h
4@@ -147,7 +147,7 @@ private:
5 int64_t m_lock_time_cutoff;
6
7 const CChainParams& chainparams;
8- const CTxMemPool& m_mempool;
9+ const CTxMemPool* m_mempool;
10 CChainState& m_chainstate;
11
12 public:
13@@ -157,8 +157,8 @@ public:
14 CFeeRate blockMinFeeRate;
15 };
16
17- explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool);
18- explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options);
19+ explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool);
20+ explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool, const Options& options);
21
22 /** Construct a new block template with coinbase to scriptPubKeyIn */
23 std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
24@@ -177,7 +177,7 @@ private:
25 /** Add transactions based on feerate including unconfirmed ancestors
26 * Increments nPackagesSelected / nDescendantsUpdated with corresponding
27 * statistics from the package selection (for logging statistics). */
28- void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
29+ void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);
30
31 // helper functions for addPackageTxs()
32 /** Remove confirmed (inBlock) entries from given set */
33@@ -189,15 +189,8 @@ private:
34 * These checks should always succeed, and they're here
35 * only as an extra check in case of suboptimal node configuration */
36 bool TestPackageTransactions(const CTxMemPool::setEntries& package) const;
37- /** Return true if given transaction from mapTx has already been evaluated,
38- * or if the transaction's cached data in mapTx is incorrect. */
39- bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
40 /** Sort the package in an order that is valid to appear in a block */
41 void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
42- /** Add descendants of given transactions to mapModifiedTx with ancestor
43- * state updated assuming given transactions are inBlock. Returns number
44- * of updated descendants. */
45- int UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set& mapModifiedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
46 };
47
48 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:
0BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options);
1BlockAssembler::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:
0BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options);
1BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, std::vector<CTransaction>& txs);
2BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool& mempool);
Whereby TestChain100Setup::CreateBlock and generateblock would call the BlockAssembler::CreateNewBlock that takes in CTransactions and we can potentially remove RegenerateCommitments altogether. All other callers can use the CTxMemPool version.
