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

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 --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 CTransactions and we can potentially remove RegenerateCommitments altogether. All other callers can use the CTxMemPool version.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25073 (test: Cleanup miner_tests by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Can you explain what this change has to do with the kernel? I don't see any modifications in the kernel folder or the kernel build target.s

Can you explain what this change has to do with the kernel?

I'm also not clear on how making BlockAssembler constructor not require an empty mempool relates to making kernel library better, or lets it drop dependencies. (This still does seem like a positive change, just connection is unclear)

I don't see any modifications in the kernel folder or the kernel build target.

Maybe you already know this, but one thing that I wasn't expecting looking at Carl's branch is that it mostly doesn't move source code into src/kernel/, instead it just changes build rules to build source files in other directories as part of the kernel library. I think the only source code that does moves into src/kernel/ initially is from files that have to be split up because they pull in wanted dependencies. If existing source files can be wholly moved into the kernel without being split up, they aren't moved. Probably src/node/miner.cpp is one of these files.

@MarcoFalke

Can you explain what this change has to do with the kernel? I don't see any modifications in the kernel folder or the kernel build target.s

Ah yes, as the companion PR to #25215, this PR also "reduces the number of call sites where we explicitly construct CTxMemPool. This is done in preparation for later PRs which decouple the mempool module from ArgsManager..."

One of the challenges I found with making the upcoming CTxMemPool+MemPoolAccept+mempool-relevant parts of CChainState <-> ArgsManager decoupling PRs easy to review was that from the view point of a reviewer, it was hard to reason that the behavior at each CTxMemPool construction call site was preserved. This is because after we decouple from ArgsMan, instead of reaching for gArgs deep in the mempool code, we now have to make sure that the options are set correctly and passed to each CTxMemPool that we construct. During development I also ran into some inadvertent bugs stemming from this.

As an example: When we stop asking gArgs for -mempoolexpiry or -maxmempool in CTxMemPool+MemPoolAccept+mempool-relevant parts of CChainState, and instead construct CTxMemPool with an CTxMemPool::Options with these parameters as struct members, preserving existing behaviour means that every CTxMemPool construction call site would have to get the default CTxMemPool::Options and apply gArgs overrides.

I reasoned that since these CTxMemPool constructor calls were entirely unnecessary in the first place, and the fix wasn't that involved, we could kill two birds with one stone: remove what's unnecessary and also make the ArgsMan decoupling PRs easier to reason about.

@ryanofsky

Maybe you already know this, but one thing that I wasn't expecting looking at Carl's branch is that it mostly doesn't move source code into src/kernel/, instead it just changes build rules to build source files in other directories as part of the kernel library. I think the only source code that does moves into src/kernel/ initially is from files that have to be split up because they pull in wanted dependencies. If existing source files can be wholly moved into the kernel without being split up, they aren't moved. Probably src/node/miner.cpp is one of these files.

Yeah I think that's right. Mostly I just don't think moving things around and changing namespaces for everything needs to be done right now. Looking at past reviews it also seems that people expect things in src/kernel/ to have some kind of cleanliness, even though we're not going to be refining the API until Phase 2, so I'd like to avoid the sum of these little expectations to make it harder for the main thrust of Phase 1 to get through review.

Pushed f159dbb92c88840ad44c6e0f1d8357449fd65141 -> 10827ac257689f8945166163b1ae496d792c790c

• Rebased over master

I also wanted to ask what libbitcoinkernel has to do with mining. And if there is mining in it, why should the mempool be optional.

libbitcoinkernel doesn't have anything to do with mining. However, mining does require (prior to this PR) a mempool. In many cases, these mempool are sometimes constructed on-the-fly (not as part of init). For example:

https://github.com/bitcoin/bitcoin/blob/640eb772e55671c5dab29843cebe42ec35cb703f/src/rpc/mining.cpp#L357-L358

Therefore, when I start making CTxMemPool options explicitly passed in to CTxMemPool's constructor instead of just being globals inside gArgs, the on-the-fly calls to CTxMemPool's constructor become much more involved.

        CTxMemPool::Options mempool_opts{};
        OverlayArgsManSettings(mempool_opts);
        CTxMemPool empty_mempool{mempool_opts};
        std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler{chainman.ActiveChainstate(), empty_mempool}.CreateNewBlock(coinbase_script));

Now, it would be another story if any of these empty_mempools were being useful, but it seems that they're basically unused, so there's no need for this charade. We can just remove BlockAssembler's dependence on CTxMemPool by making it nullable and call it a day.

libbitcoinkernel doesn't have anything to do with mining

Wouldn't it be better to move mining out of libbitcoinkernel and not motivate this pull request on [kernel 2e/n] and instead make it an orthogonal change?

constructor become much more involved

So an alternative would be to make CTxMemPool::Options optional to be passed to the constructor?

Wouldn't it be better to move mining out of libbitcoinkernel and not motivate this pull request on [kernel 2e/n] and instead make it an orthogonal change?

Mining is not in libbitcoinkernel right now, I put this PR as [kernel 2e/n] because there's a lot of future work in libbitcoinkernel's ArgsManager decoupling that depends on this. Another way I could have done it would be to publish the vision quest branch for libbitcoinkernel's ArgsManager decoupling then split out this PR, but I didn't think it'd matter that much which way I did it.

So an alternative would be to make CTxMemPool::Options optional to be passed to the constructor?

Yeah I suppose so, but it's also fraught with danger:

Does the CTxMemPool constructor consult gArgs when the CTxMemPool::Options is null?

Then we've made CTxMemPool dependent on ArgsManager again.

Does the CTxMemPool constructor just use the static const sane defaults when the CTxMemPool::Options is null?

Then you're likely introducing a behaviour change since the gArgs settings are no longer respected.

Then you're likely introducing a behaviour change

At least for the purposes of the miner this should not be the case. It will result in an empty block regardless of the options.

Personally I think it is fine to fall back to the defaults. After all this is also what happens with gArgs when no options are passed to the ArgsManager.

Though, I am also fine with your approach.

Pushed 10827ac257689f8945166163b1ae496d792c790c -> faddefe547cad0e4ba0f1cab635e48b42e153a11

• Addressed: #25223 (review)

Concept ACK, agree with narrowing the interface for constructing mempools. In a number of ways, CTxMemPool is dependent on the caller to manage things for it; it requires somebody else to ask gArgs for maxmempool and call TrimToSize, get the active chainstate and pass in the coinsview to do consistency checks, etc. Constructing a mempool outside of the "main" path means we might forget to do something that CTxMemPool doesn't take responsibility for; #24634 essentially stems from this.

My initial reaction to seeing these empty mempools was "why aren't they using node_context.mempool or chainstate.GetMempool()?" and the answer was that they need to skirt around the fact that BlockAssembler will try to fill up the block template using transactions from the mempool.

we could potentially refine the BlockAssembler interface further...

Also concept ACK to this - we have 2 ways of making a block template: (1) manually add the txns we want or (2) use the miner algo to maximize fees, and BlockAssembler should be responsible for creating both. The latter case (i.e. in generateblock and CreateBlock) is currently implemented by manually adding transactions to block.vtx and then calling RegenerateCommitments() - a bit of a hack, repeated across multiple tests, and potentially unsafe. It should be replaced with something like BlockAssembler::CreateNewBlock(manual_txns) which would internally handle the merkle commitments, call TestBlockValidity(), etc.

One question though - it seems to me that we'd still want mempool in both cases? Why would we manually insert transactions that aren't in our mempool?

Regardless, I agree that making the mempool parameter optional is much better than creating placeholder mempools.

Pushed faddefe547cad0e4ba0f1cab635e48b42e153a11 -> 0f1a259657280afc727db97689512aef5ca928fc

• Addressed #25223 (review) (thanks Gloria!)

What do you think about adding a boolean to CreateNewBlock indicating "I don't want any transactions other than the coinbase," instead of making mempool optional? This can later easily be replaced with some txids/txns indicating "please manually add these to the block template." Would it still help you detangle mempool from the argsmanager? I wrote a branch for this approach: https://github.com/glozow/bitcoin/commit/9590c8d5e844327d384795b4e3d97d8486dc8903

Talked with Gloria offline, I'm fine with either approach as long as we don't create extraneous CTxMemPools. However, there are two things to consider if we take the route of "even in the case of supplying BlockAssembler with a custom list of transactions we insert them into the mempool then use the mempool logic to select txs to include in the block":

1. Do we want to allow for cases where we want to assemble a block with transactions which would not be selected using the mempool selection logic? For example: The mempool already has a block's worth of high feerate txs, we want to create a block with a list of low feerate txs. This wouldn't be possible if we take the aforementioned route of "even in the case of supplying BlockAssembler with a custom list of transactions we insert them into the mempool then use the mempool logic to select txs to include in the block"
2. It seems from previous PRs (#19556) that we want to move towards a world where it's possible to not have a mempool at all. In that -blocksonly world, do we still want to retain the ability to use the BlockAssembler?

Ping @MarcoFalke @glozow

Ye thanks for summarizing @dongcarl, also asked @MarcoFalke offline

Do we want to allow for cases where we want to assemble a block with transactions which would not be selected using the mempool selection logic?

It seems like the answer to this is yes, and we already rely on this for things like generateblock rpc. A few tests broke when I played around with things, suggesting they rely on generateblock() only putting the manually specified transactions. In non-test-only scenarios, the option for manual block assembly also allows someone to plug in an external block solver.

It seems from previous PRs that we want to move towards a world where it's possible to not have a mempool at all. In that -blocksonly world, do we still want to retain the ability to use the BlockAssembler?

At the very least, we use generateblock() for bootstrapping chains and that uses BlockAssembler to make blocks. It seems like there is a world where we want to generate empty blocks on a no-mempool node. There are multiple ways to do this, including (1) optional mempool for BlockAssembler ctor i.e. this PR, (2) a separate helper function that creates a valid but empty block.

I think:

• if you want low-fee txs to be selected from the mempool usually you'd call prioritisetransaction on them.
• if you want txs not in the mempool to be included, you'd pass them as raw hex, to be added directly the block, as you wouldn't know whether they are standard enough to be added to the mempool. Consensus validity is obviously still checked by TestBlockValidity before returning the block.

About the implementation: I think anything works. You could even go with both approaches at the same time. For example, you could make mempool a pointer to indicate whether to fill the initial block template with txs from the mempool. And you could add another option to pass a list of txs unconditionally added to the initial template.

ACK 0f1a259657280afc727db97689512aef5ca928fc

Beyond avoiding the construction of temporary mempools, I was confused about why we'd ever use BlockAssembler() without a mempool, but that's clarified now. As stated above there are multiple approaches, but I think this is fine and doesn't hinder others. Reviewed the code to verify there's no change in behavior.

Many thanks for the insightful comments Marco and Gloria! Lots to refer back to in future convos for sure.

Glad that the current approach is good to move forward with!

lgtm

ACK 0f1a259657280afc727db97689512aef5ca928fc 🐊

<details><summary>Show signature</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 0f1a259657280afc727db97689512aef5ca928fc 🐊
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhBFAv+Mu3PolWzpLYwd2nrk8Zpq6v9Jw7/TPIRCG2BgmUs0DQ5dht7aqKhiFEp
PiY8mbyM4A9XRHTZcJ1Up+G0tG+o3iMnE5z+9l6AnDRUHZ1N9j0OpxXMq6/ZwvuN
y5CNje2QIARf8oytize8w9Y/l+PxfLwSuFdOx0YHxwE3ZcosvVUUTeZO41QrI6Hu
Hhdsutm/qf25xsRTo/cs3ROrnTBI7wBi+VK6wggdyACSkFKQZWfA7mQiPBf3y+Ji
/vKXWD6P1n2tQJxDydKsHQ7r13/y5wKJQjJ5oN+0DgeWFCkbmiLY7r+jH3I1Yt8m
TG1vSj83awiC2SPnyOto1RYyA7AIWO+XArDuxYbFtYneM1dwj00oJmihUfguI47y
+CD26TsV6jCTXD1/l1TCAH6SCBR24tj1GJ/UbYPGlNAdpzS02UPGVGUttOm7wivS
8YVc7iw/CykiNdOAaqtiTbrRxh2QcGjDWyOhORMit7Ft94RJjstbnGVspZdrChK5
4tBjHzqC
=Ha/h
-----END PGP SIGNATURE-----

</details>

Code review ACK 0f1a259657280afc727db97689512aef5ca928fc