Merge bitcoin/bitcoin#26695: bench: BlockAssembler on a mempool with packages

04528054fc [bench] BlockAssembler with mempool packages (glozow)
6ce265acf4 [test util] lock cs_main before pool.cs in PopulateMempool (glozow)
8791410662 [test util] randomize fee in PopulateMempool (glozow)
cba5934eb6 [miner] allow bypassing TestBlockValidity (glozow)
c058852308 [refactor] parameterize BlockAssembler::Options in PrepareBlock (glozow)
a2de971ba1 [refactor] add helper to apply ArgsManager to BlockAssembler::Options (glozow)

Pull request description:

  Performance of block template building matters as miners likely want to be able to start mining on a block with transactions asap after a block is found. We would want to know if a mempool PR accidentally caused, for example, a 100x slowdown. An `AssembleBlock()` bench exists, but it operates on a mempool with 101 transactions, each with 0 ancestors or descendants and with the same fee. Adding a bench with a more complex mempool is useful because (1) it's more realistic (2) updating packages can potentially cause the algorithm to take a long time.

ACKs for top commit:
  kevkevinpal:
    Tested ACK [0452805](04528054fc)
  achow101:
    ACK 04528054fc
  stickies-v:
    ACK 04528054f

Tree-SHA512: 38c138d6a75616651f9b1faf4e3a1cd833437a486f4e84308fbee958e8462bb570582c88f7ba7ab99d80191e97855ac2cf27c43cc21585d3e4b0e227effe2fb5

src/bench/block_assemble.cpp

@@ -5,6 +5,7 @@
 #include <bench/bench.h>
 #include <consensus/validation.h>
 #include <crypto/sha256.h>
+#include <node/miner.h>
 #include <test/util/mining.h>
 #include <test/util/script.h>
 #include <test/util/setup_common.h>
@@ -45,5 +46,18 @@ static void AssembleBlock(benchmark::Bench& bench)
         PrepareBlock(test_setup->m_node, P2WSH_OP_TRUE);
     });
 }
+static void BlockAssemblerAddPackageTxns(benchmark::Bench& bench)
+{
+    FastRandomContext det_rand{true};
+    auto testing_setup{MakeNoLogFileContext<TestChain100Setup>()};
+    testing_setup->PopulateMempool(det_rand, /*num_transactions=*/1000, /*submit=*/true);
+    node::BlockAssembler::Options assembler_options;
+    assembler_options.test_block_validity = false;
+
+    bench.run([&] {
+        PrepareBlock(testing_setup->m_node, P2WSH_OP_TRUE, assembler_options);
+    });
+}
 
 BENCHMARK(AssembleBlock, benchmark::PriorityLevel::HIGH);
+BENCHMARK(BlockAssemblerAddPackageTxns, benchmark::PriorityLevel::LOW);

src/node/miner.cpp

@@ -60,10 +60,12 @@ BlockAssembler::Options::Options()
 {
     blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE);
     nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT;
+    test_block_validity = true;
 }
 
 BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options)
-    : chainparams{chainstate.m_chainman.GetParams()},
+    : test_block_validity{options.test_block_validity},
+      chainparams{chainstate.m_chainman.GetParams()},
       m_mempool(mempool),
       m_chainstate(chainstate)
 {
@@ -72,11 +74,10 @@ BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool
     nBlockMaxWeight = std::max<size_t>(4000, std::min<size_t>(MAX_BLOCK_WEIGHT - 4000, options.nBlockMaxWeight));
 }
 
-static BlockAssembler::Options DefaultOptions()
+void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options)
 {
     // Block resource limits
     // If -blockmaxweight is not given, limit to DEFAULT_BLOCK_MAX_WEIGHT
-    BlockAssembler::Options options;
     options.nBlockMaxWeight = gArgs.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT);
     if (gArgs.IsArgSet("-blockmintxfee")) {
         std::optional<CAmount> parsed = ParseMoney(gArgs.GetArg("-blockmintxfee", ""));
@@ -84,11 +85,16 @@ static BlockAssembler::Options DefaultOptions()
     } else {
         options.blockMinFeeRate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
     }
+}
+static BlockAssembler::Options ConfiguredOptions()
+{
+    BlockAssembler::Options options;
+    ApplyArgsManOptions(gArgs, options);
     return options;
 }
 
 BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool)
-    : BlockAssembler(chainstate, mempool, DefaultOptions()) {}
+    : BlockAssembler(chainstate, mempool, ConfiguredOptions()) {}
 
 void BlockAssembler::resetBlock()
 {
@@ -170,7 +176,8 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]);
 
     BlockValidationState state;
-    if (!TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, GetAdjustedTime, false, false)) {
+    if (test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev,
+                                                  GetAdjustedTime, /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
         throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString()));
     }
     const auto time_2{SteadyClock::now()};

src/node/miner.h

@@ -16,6 +16,7 @@
 #include <boost/multi_index/ordered_index.hpp>
 #include <boost/multi_index_container.hpp>
 
+class ArgsManager;
 class ChainstateManager;
 class CBlockIndex;
 class CChainParams;
@@ -135,6 +136,9 @@ private:
     unsigned int nBlockMaxWeight;
     CFeeRate blockMinFeeRate;
 
+    // Whether to call TestBlockValidity() at the end of CreateNewBlock().
+    const bool test_block_validity;
+
     // Information on the current status of the block
     uint64_t nBlockWeight;
     uint64_t nBlockTx;
@@ -155,6 +159,7 @@ public:
         Options();
         size_t nBlockMaxWeight;
         CFeeRate blockMinFeeRate;
+        bool test_block_validity;
     };
 
     explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool);
@@ -197,6 +202,9 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
 
 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
 void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
+
+/** Apply -blockmintxfee and -blockmaxweight options from ArgsManager to BlockAssembler options. */
+void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options);
 } // namespace node
 
 #endif // BITCOIN_NODE_MINER_H

src/test/util/mining.cpp

@@ -8,7 +8,6 @@
 #include <consensus/merkle.h>
 #include <key_io.h>
 #include <node/context.h>
-#include <node/miner.h>
 #include <pow.h>
 #include <script/standard.h>
 #include <test/util/script.h>
@@ -74,10 +73,11 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
     return CTxIn{block->vtx[0]->GetHash(), 0};
 }
 
-std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
+std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey,
+                                     const BlockAssembler::Options& assembler_options)
 {
     auto block = std::make_shared<CBlock>(
-        BlockAssembler{Assert(node.chainman)->ActiveChainstate(), Assert(node.mempool.get())}
+        BlockAssembler{Assert(node.chainman)->ActiveChainstate(), Assert(node.mempool.get()), assembler_options}
             .CreateNewBlock(coinbase_scriptPubKey)
             ->block);
 
@@ -87,3 +87,9 @@ std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coi
 
     return block;
 }
+std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
+{
+    BlockAssembler::Options assembler_options;
+    ApplyArgsManOptions(*node.args, assembler_options);
+    return PrepareBlock(node, coinbase_scriptPubKey, assembler_options);
+}

src/test/util/mining.h

@@ -5,6 +5,8 @@
 #ifndef BITCOIN_TEST_UTIL_MINING_H
 #define BITCOIN_TEST_UTIL_MINING_H
 
+#include <node/miner.h>
+
 #include <memory>
 #include <string>
 #include <vector>
@@ -25,6 +27,8 @@ CTxIn MineBlock(const node::NodeContext&, const CScript& coinbase_scriptPubKey);
 
 /** Prepare a block to be mined */
 std::shared_ptr<CBlock> PrepareBlock(const node::NodeContext&, const CScript& coinbase_scriptPubKey);
+std::shared_ptr<CBlock> PrepareBlock(const node::NodeContext& node, const CScript& coinbase_scriptPubKey,
+                                     const node::BlockAssembler::Options& assembler_options);
 
 /** RPC-like helper function, returns the generated coin */
 CTxIn generatetoaddress(const node::NodeContext&, const std::string& address);

src/test/util/setup_common.cpp

@@ -397,15 +397,15 @@ std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContex
             unspent_prevouts.pop_front();
         }
         const size_t num_outputs = det_rand.randrange(24) + 1;
-        // Approximately 1000sat "fee," equal output amounts.
-        const CAmount amount_per_output = (total_in - 1000) / num_outputs;
+        const CAmount fee = 100 * det_rand.randrange(30);
+        const CAmount amount_per_output = (total_in - fee) / num_outputs;
         for (size_t n{0}; n < num_outputs; ++n) {
             CScript spk = CScript() << CScriptNum(num_transactions + n);
             mtx.vout.push_back(CTxOut(amount_per_output, spk));
         }
         CTransactionRef ptx = MakeTransactionRef(mtx);
         mempool_transactions.push_back(ptx);
-        if (amount_per_output > 2000) {
+        if (amount_per_output > 3000) {
             // If the value is high enough to fund another transaction + fees, keep track of it so
             // it can be used to build a more complex transaction graph. Insert randomly into
             // unspent_prevouts for extra randomness in the resulting structures.
@@ -415,9 +415,11 @@ std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContex
             }
         }
         if (submit) {
-            LOCK2(m_node.mempool->cs, cs_main);
+            LOCK2(cs_main, m_node.mempool->cs);
             LockPoints lp;
-            m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, 1000, 0, 1, false, 4, lp));
+            m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, /*fee=*/(total_in - num_outputs * amount_per_output),
+                                                         /*time=*/0, /*entry_height=*/1,
+                                                         /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
         }
         --num_transactions;
     }