Merge bitcoin/bitcoin#25223: [kernel 2e/n] miner: Make `mempool` optional, stop constructing temporary empty mempools

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<CBlockTemplate> 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<CTxMemPool::txiter>& 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<CTransaction>& 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

2 files changed, 2 insertions, 3 deletions

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<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
 {
     auto block = std::make_shared<CBlock>(
-        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 8f03481f72..d9fff85bf5 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) {