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

dongcarl · 2022-05-26T17:57:52Z

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.

DrahtBot · 2022-05-27T00:36:23Z

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

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.

maflcko · 2022-05-27T08:09:51Z

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

ryanofsky · 2022-05-27T14:16:34Z

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.

dongcarl · 2022-05-27T19:18:39Z

@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.

dongcarl · 2022-05-27T19:28:43Z

@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.

SkipMapTxEntry is a short helper function that's only used in addPackageTxs, we can just inline it, keep the comments, and avoid the unnecessary interface and lock annotations.

Since UpdatePackagesForAdded is a helper function that's only used in addPackageTxs we can make it static and avoid the unnecessary interface and in-header lock annotation.

dongcarl · 2022-05-27T20:26:04Z

Pushed f159dbb -> 10827ac

Rebased over master

maflcko · 2022-05-31T15:24:03Z

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.

dongcarl · 2022-05-31T15:42:57Z

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:

bitcoin/src/rpc/mining.cpp

Lines 357 to 358 in 640eb77

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

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.

maflcko · 2022-05-31T15:52:29Z

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?

dongcarl · 2022-05-31T16:29:57Z

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.

maflcko · 2022-05-31T16:41:27Z

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.

src/node/miner.cpp

dongcarl · 2022-05-31T19:21:12Z

Pushed 10827ac -> faddefe

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

glozow

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.

src/node/miner.h

...also adjust callers Changes: - In BlockAssembler::CreateNewBlock, we now only lock m_mempool->cs and call addPackageTxs if m_mempool is not nullptr - BlockAssembler::addPackageTxs now takes in a mempool reference, and is annotated to require that mempool's lock. - In TestChain100Setup::CreateBlock and generateblock, don't construct an empty mempool, just pass in a nullptr for mempool

dongcarl · 2022-06-06T19:40:05Z

Pushed faddefe -> 0f1a259

Addressed [kernel 2e/n] miner: Make mempool optional, stop constructing temporary empty mempools #25223 (comment) (thanks Gloria!)

glozow

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: glozow@9590c8d

glozow · 2022-06-08T09:13:52Z

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<CTxMemPool*>(&tx_pool), options};
+        auto assembler = BlockAssembler{chainstate, &tx_pool, options};


Question: why'd you remove the static cast? Not suggesting you add it back, just wondering

Oh, I just thought it was unnecessary. I think Marco added it in fa03d0a

I don't care either way what we do here!

dongcarl · 2022-06-13T14:10:03Z

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":

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"
It seems from previous PRs (Remove mempool global #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

glozow · 2022-06-13T14:26:04Z

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.

maflcko · 2022-06-13T14:27:47Z

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.

glozow · 2022-06-13T14:32:33Z

ACK 0f1a259

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.

dongcarl · 2022-06-13T14:54:53Z

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!

maflcko

lgtm

ACK 0f1a259 🐊

Show signature

Signature:

maflcko · 2022-06-15T09:21:33Z

src/node/miner.cpp

-// 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)


in the first commit:

Any reason to not use the same approach from the second commit and make this function static? I understand that this would require passing the mempool solely to check the lock annotation, but I think this makes the diff smaller, keeps the structure of the file, and avoids making a large function even larger by inlining.

If there are no other reasons I'd prefer to keep this as-is for now and leave restructuring to changes that actually need it?

When I was looking at it, this function just seemed so intertwined with the rest of addPackageTxs. Both mapModifiedTx and failedTx are local variables to addPackageTxs and without the function abstraction we can also drop the AssertLockHeld. I agree that addPackageTxs is quite large and we could break it down into pieces where appropriate but I'm not sure that this is the right place.

If we were to make it static we'd have to pass in the mempool and inBlock.

laanwj · 2022-06-15T13:17:12Z

Code review ACK 0f1a259

ryanofsky · 2022-06-15T16:02:44Z

src/node/miner.cpp

@@ -300,17 +290,17 @@ 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)
+void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated)


The BlockAssembler::addPackageTxs function now has access two different mempool variables: a mempool argument, and a this->m_mempool member variable. Right now these point to the same mempool, and the current addPackageTxs code only refers to mempool instead of m_mempool after this PR. But there is no enforcement for these things, so they can easily change and bugs could show up with no one noticing.

IMO, either this new mempool argument should be dropped and function should go back to using m_mempool, or the m_mempool member should be dropped so it is guaranteed that function is only using mempool. But having multiple aliases for no reason is confusing and asking for bugs

Added to the project issue TODO list: #24303

I guess the function could be made static and all needed members be passed in?

I mean honestly all the "helper functions for addPackageTxs()" listed in the header could be made static, also AddToBlock, and then inBlock could be a local var in addPackageTxs. That would minimize the BlockAssembler interface quite nicely. Lots of shuffling around tho.

… cons…

d273e53 bench/rpc_mempool: Create ChainTestingSetup, use its CTxMemPool (Carl Dong) 020caba bench: Use existing CTxMemPool in TestingSetup (Carl Dong) 86e732d scripted-diff: test: Use CTxMemPool in TestingSetup (Carl Dong) 213457e test/policyestimator: Use ChainTestingSetup's CTxMemPool (Carl Dong) 319f0ce rest/getutxos: Don't construct empty mempool (Carl Dong) 03574b9 tree-wide: clang-format CTxMemPool references (Carl Dong) Pull request description: This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18 This PR 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`, eventually all of libbitcoinkernel will be decoupled from `ArgsManager`. The changes in this PR: - Allows us to have less code churn as we modify `CTxMemPool`'s constructor in later PRs - In many cases, we can make use of existing `CTxMemPool` instances, getting rid of extraneous constructions - In other cases, we construct a `ChainTestingSetup` and use the `CTxMemPool` there, so that we can rely on the logic in `setup_common` to set things up correctly ## Notes for Reviewers ### A note on using existing mempools When evaluating whether or not it's appropriate to use an existing mempool in a `*TestingSetup` struct, the key is to make sure that the mempool has the same lifetime as the `*TestingSetup` struct. Example 1: In [`src/fuzz/tx_pool.cpp`](https://github.com/bitcoin/bitcoin/blob/b4f686952a60bbadc7ed2250651d0d6af0959f4d/src/test/fuzz/tx_pool.cpp), the `TestingSetup` is initialized in `initialize_tx_pool` and lives as a static global, while the `CTxMemPool` is in the `tx_pool_standard` fuzz target, meaning that each time the `tx_pool_standard` fuzz target gets run, a new `CTxMemPool` is created. If we were to use the static global `TestingSetup`'s CTxMemPool we might run into problems since its `CTxMemPool` will carry state between subsequent runs. This is why we don't modify `src/fuzz/tx_pool.cpp` in this PR. Example 2: In [`src/bench/mempool_eviction.cpp`](https://github.com/bitcoin/bitcoin/blob/b4f686952a60bbadc7ed2250651d0d6af0959f4d/src/bench/mempool_eviction.cpp), we see that the `TestingSetup` is in the same scope as the constructed `CTxMemPool`, so it is safe to use its `CTxMemPool`. ### A note on checking `CTxMemPool` ctor call sites After the "tree-wide: clang-format CTxMemPool references" commit, you can find all `CTxMemPool` ctor call sites with the following command: ```sh git grep -E -e 'make_unique<CTxMemPool>' \ -e '\bCTxMemPool\s+[^({;]+[({]' \ -e '\bCTxMemPool\s+[^;]+;' \ -e '\bnew\s+CTxMemPool\b' ``` At the end of the PR, you will find that there are still quite a few call sites that we can seemingly get rid of: ```sh $ git grep -E -e 'make_unique<CTxMemPool>' -e '\bCTxMemPool\s+[^({;]+[({]' -e '\bCTxMemPool\s+[^;]+;' -e '\bnew\s+CTxMemPool\b' # rearranged for easier explication src/init.cpp: node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio); src/test/util/setup_common.cpp: m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1); src/rpc/mining.cpp: CTxMemPool empty_mempool; src/test/util/setup_common.cpp: CTxMemPool empty_pool; src/bench/mempool_stress.cpp: CTxMemPool pool; src/bench/mempool_stress.cpp: CTxMemPool pool; src/test/fuzz/rbf.cpp: CTxMemPool pool; src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1}; src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1}; src/test/fuzz/validation_load_mempool.cpp: CTxMemPool pool{}; src/txmempool.h: /** Create a new CTxMemPool. ``` Let's break them down one by one: ``` src/init.cpp: node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio); src/test/util/setup_common.cpp: m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1); ``` Necessary ----- ``` src/rpc/mining.cpp: CTxMemPool empty_mempool; src/test/util/setup_common.cpp: CTxMemPool empty_pool; ``` These are fixed in #25223 where we stop requiring the `BlockAssembler` to have a `CTxMemPool` if it's not going to consult it anyway (as is the case in these two call sites) ----- ``` src/bench/mempool_stress.cpp: CTxMemPool pool; src/bench/mempool_stress.cpp: CTxMemPool pool; ``` Fixed in #24927. ----- ``` src/test/fuzz/rbf.cpp: CTxMemPool pool; src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1}; src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1}; src/test/fuzz/validation_load_mempool.cpp: CTxMemPool pool{}; ``` These are all cases where we don't want the `CTxMemPool` state to persist between runs, see the previous section "A note on using existing mempools" ----- ``` src/txmempool.h: /** Create a new CTxMemPool. ``` It's a comment (someone link me to a grep that understands syntax plz thx) ACKs for top commit: laanwj: Code review ACK d273e53 Tree-SHA512: c4ff3d23217a7cc4a7145defc7b901725073ef73bcac3a252ed75f672c87e98ca0368d1d8c3f606b5b49f641e7d8387d26ef802141b650b215876f191fb6d5f9

dongcarl changed the title ~~miner: Make mempool optional, stop constructing temporary empty mempools~~ [kernel 2e/n] miner: Make mempool optional, stop constructing temporary empty mempools May 26, 2022

dongcarl mentioned this pull request May 26, 2022

The libbitcoinkernel Project #24303

Closed

14 tasks

DrahtBot added Mining RPC/REST/ZMQ labels May 26, 2022

This was referenced May 27, 2022

refactor, miner: Delete call to UpdatePackagesForAdded at beginning of addPackageTxs #24934

Merged

CChainState -> Chainstate #24513

Merged

DrahtBot added the Needs rebase label May 27, 2022

dongcarl mentioned this pull request May 27, 2022

[kernel 2d/n] Reduce CTxMemPool constructor call sites #25215

Merged

dongcarl added 2 commits May 27, 2022 15:31

miner: Absorb SkipMapTxEntry into addPackageTxs

f024578

SkipMapTxEntry is a short helper function that's only used in addPackageTxs, we can just inline it, keep the comments, and avoid the unnecessary interface and lock annotations.

miner: Make UpdatePackagesForAdded static

cc5739b

Since UpdatePackagesForAdded is a helper function that's only used in addPackageTxs we can make it static and avoid the unnecessary interface and in-header lock annotation.

dongcarl force-pushed the 2022-05-kernelargs-mempool-miner branch from f159dbb to 10827ac Compare May 27, 2022 19:35

DrahtBot removed the Needs rebase label May 27, 2022

DrahtBot mentioned this pull request May 28, 2022

test: Cleanup miner_tests #25073

Merged

maflcko reviewed May 31, 2022

View reviewed changes

src/node/miner.cpp Show resolved Hide resolved

dongcarl force-pushed the 2022-05-kernelargs-mempool-miner branch from 10827ac to faddefe Compare May 31, 2022 19:20

glozow reviewed Jun 3, 2022

View reviewed changes

src/node/miner.h Outdated Show resolved Hide resolved

dongcarl force-pushed the 2022-05-kernelargs-mempool-miner branch from faddefe to 0f1a259 Compare June 6, 2022 19:38

dongcarl mentioned this pull request Jun 7, 2022

[kernel 3a/n] Decouple CTxMemPool from ArgsManager #25290

Merged

2 tasks

glozow added Refactoring and removed RPC/REST/ZMQ labels Jun 8, 2022

glozow reviewed Jun 13, 2022

View reviewed changes

maflcko approved these changes Jun 15, 2022

View reviewed changes

fanquake merged commit a7a3659 into bitcoin:master Jun 15, 2022

ryanofsky reviewed Jun 15, 2022

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 15, 2022

Merge bitcoin#25223: [kernel 2e/n] miner: Make mempool optional, stop…

696c919

… cons…

maflcko mentioned this pull request Oct 7, 2022

refactor: Make m_mempool optional in PeerManager #26247

Closed

TheCharlatan mentioned this pull request May 6, 2023

Bitcoin Kernel Library Project Tracking #27587

Open

2 tasks

bitcoin locked and limited conversation to collaborators Jun 15, 2023

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

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

Uh oh!

Conversation

dongcarl commented May 26, 2022

Abstract

Alternatives

Future work

Uh oh!

DrahtBot commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented May 27, 2022

Uh oh!

ryanofsky commented May 27, 2022

Uh oh!

dongcarl commented May 27, 2022

Uh oh!

dongcarl commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongcarl commented May 27, 2022

Uh oh!

maflcko commented May 31, 2022

Uh oh!

dongcarl commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 31, 2022

Uh oh!

dongcarl commented May 31, 2022

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

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

Uh oh!

maflcko commented May 31, 2022

Uh oh!

Uh oh!

dongcarl commented May 31, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongcarl commented Jun 6, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Jun 13, 2022

Uh oh!

glozow commented Jun 13, 2022

Uh oh!

maflcko commented Jun 13, 2022

Uh oh!

glozow commented Jun 13, 2022

Uh oh!

dongcarl commented Jun 13, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

DrahtBot commented May 27, 2022 •

edited

Loading

dongcarl commented May 27, 2022 •

edited

Loading

dongcarl commented May 31, 2022 •

edited

Loading

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

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