miner: allow CreateNewBlock without non-coinbase txns

glozow · glozow · commit 9590c8d5e844 · 2022-06-08T11:12:00.000+01:00
There are a few places (in tests) where we want to manually add
transactions to the block template. We currently do this by constructing
an empty mempool for the BlockAssembler to pull transactions from. This
is hacky and potentially unsafe. We should narrow the interface for
constructing new mempools.

Instead of passing in an empty mempool, just add the option to not add
any transactions to the template. In the future, this param be easily
changed to a set of transactions to ask BlockAssembler to force-include.
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
@@ -103,7 +103,7 @@ void BlockAssembler::resetBlock()
     nFees = 0;
 }
 
-std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
+std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, bool manual_txns)
 {
     int64_t nTimeStart = GetTimeMicros();
 
@@ -138,7 +138,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
 
     int nPackagesSelected = 0;
     int nDescendantsUpdated = 0;
-    addPackageTxs(nPackagesSelected, nDescendantsUpdated);
+    if (!manual_txns) {
+        addPackageTxs(nPackagesSelected, nDescendantsUpdated);
+    }
 
     int64_t nTime1 = GetTimeMicros();
 
diff --git a/src/node/miner.h b/src/node/miner.h
@@ -160,8 +160,9 @@ class BlockAssembler
     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);
+    /** Construct a new block template with coinbase to scriptPubKeyIn.
+     * @param[in]   manual_txns     if true, don't add any non-coinbase transactions. */
+    std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn, bool manual_txns = false);
 
     inline static std::optional<int64_t> m_last_block_num_txs{};
     inline static std::optional<int64_t> m_last_block_weight{};
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
@@ -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(), mempool}.CreateNewBlock(coinbase_script, /*manual_txns=*/ true));
         if (!blocktemplate) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
         }
diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
@@ -65,7 +65,7 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev,
     const std::vector<CMutableTransaction>& txns,
     const CScript& scriptPubKey)
 {
-    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), *m_node.mempool}.CreateNewBlock(scriptPubKey);
+    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), *m_node.mempool}.CreateNewBlock(scriptPubKey, /*manual_txns=*/ true);
     CBlock& block = pblocktemplate->block;
     block.hashPrevBlock = prev->GetBlockHash();
     block.nTime = prev->nTime + 1;
diff --git 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, *m_node.mempool.get()}.CreateNewBlock(scriptPubKey, /*manual_txns=*/ true)->block;
 
     Assert(block.vtx.size() == 1);
     for (const CMutableTransaction& tx : txns) {

Original file line number	Diff line number	Diff line change
`@@ -103,7 +103,7 @@ void BlockAssembler::resetBlock()`
`103`	`103`	`nFees = 0;`
`104`	`104`	`}`
`105`	`105`
`106`		`-std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)`
	`106`	`+std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, bool manual_txns)`
`107`	`107`	`{`
`108`	`108`	`int64_t nTimeStart = GetTimeMicros();`
`109`	`109`
`@@ -138,7 +138,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc`
`138`	`138`
`139`	`139`	`int nPackagesSelected = 0;`
`140`	`140`	`int nDescendantsUpdated = 0;`
`141`		`- addPackageTxs(nPackagesSelected, nDescendantsUpdated);`
	`141`	`+ if (!manual_txns) {`
	`142`	`+ addPackageTxs(nPackagesSelected, nDescendantsUpdated);`
	`143`	`+ }`
`142`	`144`
`143`	`145`	`int64_t nTime1 = GetTimeMicros();`
`144`	`146`
Original file line number	Diff line number	Diff line change
`@@ -354,8 +354,7 @@ static RPCHelpMan generateblock()`
`354`	`354`	`{`
`355`	`355`	`LOCK(cs_main);`
`356`	`356`
`357`		`- CTxMemPool empty_mempool;`
`358`		`- std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler{chainman.ActiveChainstate(), empty_mempool}.CreateNewBlock(coinbase_script));`
	`357`	`+ std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler{chainman.ActiveChainstate(), mempool}.CreateNewBlock(coinbase_script, /manual_txns=/ true));`
`359`	`358`	`if (!blocktemplate) {`
`360`	`359`	`throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");`
`361`	`360`	`}`
Original file line number	Diff line number	Diff line change
`@@ -65,7 +65,7 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev,`
`65`	`65`	`const std::vector<CMutableTransaction>& txns,`
`66`	`66`	`const CScript& scriptPubKey)`
`67`	`67`	`{`
`68`		`- std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), *m_node.mempool}.CreateNewBlock(scriptPubKey);`
	`68`	`+ std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool}.CreateNewBlock(scriptPubKey, /manual_txns=*/ true);`
`69`	`69`	`CBlock& block = pblocktemplate->block;`
`70`	`70`	`block.hashPrevBlock = prev->GetBlockHash();`
`71`	`71`	`block.nTime = prev->nTime + 1;`
Original file line number	Diff line number	Diff line change
`@@ -277,8 +277,7 @@ CBlock TestChain100Setup::CreateBlock(`
`277`	`277`	`const CScript& scriptPubKey,`
`278`	`278`	`CChainState& chainstate)`
`279`	`279`	`{`
`280`		`- CTxMemPool empty_pool;`
`281`		`- CBlock block = BlockAssembler{chainstate, empty_pool}.CreateNewBlock(scriptPubKey)->block;`
	`280`	`+ CBlock block = BlockAssembler{chainstate, m_node.mempool.get()}.CreateNewBlock(scriptPubKey, /manual_txns=*/ true)->block;`
`282`	`281`
`283`	`282`	`Assert(block.vtx.size() == 1);`
`284`	`283`	`for (const CMutableTransaction& tx : txns) {`