diff options
author | fanquake <fanquake@gmail.com> | 2022-06-15 16:27:51 +0100 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2022-06-15 16:40:48 +0100 |
commit | a7a36590f590f57d46b4b904d5a8c3a1002f336d (patch) | |
tree | db08446005a27157abc4228de32ae8ab91ea23f7 /src/rpc | |
parent | fa07ee165ed7a08699f4fc513eff6608aedeef16 (diff) | |
parent | 0f1a259657280afc727db97689512aef5ca928fc (diff) | |
download | bitcoin-a7a36590f590f57d46b4b904d5a8c3a1002f336d.tar.xz |
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
Diffstat (limited to 'src/rpc')
-rw-r--r-- | src/rpc/mining.cpp | 7 |
1 files changed, 3 insertions, 4 deletions
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 8fb6daf0cb..ea6db1e9a0 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -144,7 +144,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& me { UniValue blockHashes(UniValue::VARR); while (nGenerate > 0 && !ShutdownRequested()) { - std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler{chainman.ActiveChainstate(), mempool}.CreateNewBlock(coinbase_script)); + std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler{chainman.ActiveChainstate(), &mempool}.CreateNewBlock(coinbase_script)); if (!pblocktemplate.get()) throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block"); CBlock *pblock = &pblocktemplate->block; @@ -354,8 +354,7 @@ static RPCHelpMan generateblock() { LOCK(cs_main); - CTxMemPool empty_mempool; - std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler{chainman.ActiveChainstate(), empty_mempool}.CreateNewBlock(coinbase_script)); + std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler{chainman.ActiveChainstate(), nullptr}.CreateNewBlock(coinbase_script)); if (!blocktemplate) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block"); } @@ -753,7 +752,7 @@ static RPCHelpMan getblocktemplate() // Create new block CScript scriptDummy = CScript() << OP_TRUE; - pblocktemplate = BlockAssembler{active_chainstate, mempool}.CreateNewBlock(scriptDummy); + pblocktemplate = BlockAssembler{active_chainstate, &mempool}.CreateNewBlock(scriptDummy); if (!pblocktemplate) throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory"); |