Merge bitcoin/bitcoin#31384: mining: bugfix: Fix duplicate coinbase tx weight reservation

386eecff5f doc: add release notes (ismaelsadeeq)
3eaa0a3b66 miner: init: add `-blockreservedweight` startup option (ismaelsadeeq)
777434a2cd doc: rpc: improve `getmininginfo` help text (ismaelsadeeq)
c8acd4032d init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` (ismaelsadeeq)
5bb31633cc test: add `-blockmaxweight` startup option functional test (ismaelsadeeq)
2c7d90a6d6 miner: bugfix: fix duplicate weight reservation in block assembler (ismaelsadeeq)

Pull request description:

  * This PR attempts to fix the duplicate coinbase weight reservation issue we currently have.
  * Fixes #21950

  We reserve 4000 weight units for coinbase transaction in `DEFAULT_BLOCK_MAX_WEIGHT`

  7590e93bc7/src/policy/policy.h (L23)

  And also reserve additional `4000` weight units in the default `BlockCreationOptions` struct.

  7590e93bc7/src/node/types.h (L36-L40)

  **Motivation**

  - This issue was first noticed during a review here https://github.com/bitcoin/bitcoin/pull/11100#discussion_r136157411)
  - It was later reported in issue #21950.
  - I also came across the bug while writing a test for building the block template. I could not create a block template above `3,992,000` in the block assembler, and this was not documented anywhere. It took me a while to realize that we were reserving space for the coinbase transaction weight twice.

  ---
  This PR fixes this by consolidating the reservation to be in a single location in the codebase.

  This PR then adds a new startup option `-blockreservedweight` whose default is `8000` that can be used to lower or increase the block reserved weight for block header, txs count, coinbase tx.

ACKs for top commit:
  Sjors:
    ACK 386eecff5f
  fjahr:
    Code review ACK 386eecff5f
  glozow:
    utACK 386eecff5f, nonblocking nits. I do think the release notes should be clarified more
  pinheadmz:
    ACK 386eecff5f

Tree-SHA512: f27efa1da57947b7f4d42b9322b83d13afe73dd749dd9cac49360002824dd41c99a876a610554ac2d67bad7485020b9dcc423a8e6748fc79d6a10de6d4357d4c

src/init.cpp

@@ -19,6 +19,7 @@
 #include <common/args.h>
 #include <common/system.h>
 #include <consensus/amount.h>
+#include <consensus/consensus.h>
 #include <deploymentstatus.h>
 #include <hash.h>
 #include <httprpc.h>
@@ -648,6 +649,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
 
 
     argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d)", DEFAULT_BLOCK_MAX_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
+    argsman.AddArg("-blockreservedweight=<n>", strprintf("Reserve space for the fixed-size block header plus the largest coinbase transaction the mining software may add to the block. (default: %d).", DEFAULT_BLOCK_RESERVED_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
     argsman.AddArg("-blockmintxfee=<amt>", strprintf("Set lowest fee rate (in %s/kvB) for transactions to be included in block creation. (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_BLOCK_MIN_TX_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
     argsman.AddArg("-blockversion=<n>", "Override block version to test forking scenarios", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::BLOCK_CREATION);
 
@@ -1015,6 +1017,23 @@ bool AppInitParameterInteraction(const ArgsManager& args)
         }
     }
 
+    if (args.IsArgSet("-blockmaxweight")) {
+        const auto max_block_weight = args.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT);
+        if (max_block_weight > MAX_BLOCK_WEIGHT) {
+            return InitError(strprintf(_("Specified -blockmaxweight (%d) exceeds consensus maximum block weight (%d)"), max_block_weight, MAX_BLOCK_WEIGHT));
+        }
+    }
+
+    if (args.IsArgSet("-blockreservedweight")) {
+        const auto block_reserved_weight = args.GetIntArg("-blockreservedweight", DEFAULT_BLOCK_RESERVED_WEIGHT);
+        if (block_reserved_weight > MAX_BLOCK_WEIGHT) {
+            return InitError(strprintf(_("Specified -blockreservedweight (%d) exceeds consensus maximum block weight (%d)"), block_reserved_weight, MAX_BLOCK_WEIGHT));
+        }
+        if (block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
+            return InitError(strprintf(_("Specified -blockreservedweight (%d) is lower than minimum safety value of (%d)"), block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT));
+        }
+    }
+
     nBytesPerSigOp = args.GetIntArg("-bytespersigop", nBytesPerSigOp);
 
     if (!g_wallet_init_interface.ParameterInteraction()) return false;

src/ipc/capnp/mining.capnp

@@ -35,7 +35,7 @@ interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
 
 struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") {
     useMempool @0 :Bool $Proxy.name("use_mempool");
-    coinbaseMaxAdditionalWeight @1 :UInt64 $Proxy.name("coinbase_max_additional_weight");
+    blockReservedWeight @1 :UInt64 $Proxy.name("block_reserved_weight");
     coinbaseOutputMaxAdditionalSigops @2 :UInt64 $Proxy.name("coinbase_output_max_additional_sigops");
 }
 

src/node/miner.cpp

@@ -74,11 +74,12 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 
 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 {
-    Assert(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
+    Assert(options.block_reserved_weight <= MAX_BLOCK_WEIGHT);
+    Assert(options.block_reserved_weight >= MINIMUM_BLOCK_RESERVED_WEIGHT);
     Assert(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
-    // Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
-    // Coinbase (reserved) outputs can safely exceed -blockmaxweight, but the rest of the block template will be empty.
-    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, DEFAULT_BLOCK_MAX_WEIGHT);
+    // Limit weight to between block_reserved_weight and MAX_BLOCK_WEIGHT for sanity:
+    // block_reserved_weight can safely exceed -blockmaxweight, but the rest of the block template will be empty.
+    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.block_reserved_weight, MAX_BLOCK_WEIGHT);
     return options;
 }
 
@@ -98,14 +99,15 @@ void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& optio
         if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed};
     }
     options.print_modified_fee = args.GetBoolArg("-printpriority", options.print_modified_fee);
+    options.block_reserved_weight = args.GetIntArg("-blockreservedweight", options.block_reserved_weight);
 }
 
 void BlockAssembler::resetBlock()
 {
     inBlock.clear();
 
-    // Reserve space for coinbase tx
-    nBlockWeight = m_options.coinbase_max_additional_weight;
+    // Reserve space for fixed-size block header, txs count, and coinbase tx.
+    nBlockWeight = m_options.block_reserved_weight;
     nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops;
 
     // These counters do not include coinbase tx
@@ -393,7 +395,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
             ++nConsecutiveFailed;
 
             if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
-                    m_options.nBlockMaxWeight - m_options.coinbase_max_additional_weight) {
+                    m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
                 // Give up if we're close to full and haven't succeeded in a while
                 break;
             }

src/node/miner.h

@@ -176,7 +176,9 @@ public:
     /** Construct a new block template */
     std::unique_ptr<CBlockTemplate> CreateNewBlock();
 
+    /** The number of transactions in the last assembled block (excluding coinbase transaction) */
     inline static std::optional<int64_t> m_last_block_num_txs{};
+    /** The weight of the last assembled block (including reserved weight for block header, txs count and coinbase tx) */
     inline static std::optional<int64_t> m_last_block_weight{};
 
 private:

src/node/types.h

@@ -14,6 +14,7 @@
 #define BITCOIN_NODE_TYPES_H
 
 #include <cstddef>
+#include <policy/policy.h>
 #include <script/script.h>
 
 namespace node {
@@ -34,11 +35,10 @@ struct BlockCreateOptions {
      */
     bool use_mempool{true};
     /**
-     * The maximum additional weight which the pool will add to the coinbase
-     * scriptSig, witness and outputs. This must include any additional
-     * weight needed for larger CompactSize encoded lengths.
+     * The default reserved weight for the fixed-size block header,
+     * transaction count and coinbase transaction.
      */
-    size_t coinbase_max_additional_weight{4000};
+    size_t block_reserved_weight{DEFAULT_BLOCK_RESERVED_WEIGHT};
     /**
      * The maximum additional sigops which the pool will add in coinbase
      * transaction outputs.

src/policy/policy.h

@@ -20,7 +20,14 @@ class CFeeRate;
 class CScript;
 
 /** Default for -blockmaxweight, which controls the range of block weights the mining code will create **/
-static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};
+static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT};
+/** Default for -blockreservedweight **/
+static constexpr unsigned int DEFAULT_BLOCK_RESERVED_WEIGHT{8000};
+/** This accounts for the block header, var_int encoding of the transaction count and a minimally viable
+ * coinbase transaction. It adds an additional safety margin, because even with a thorough understanding
+ * of block serialization, it's easy to make a costly mistake when trying to squeeze every last byte.
+ * Setting a lower value is prevented at startup. */
+static constexpr unsigned int MINIMUM_BLOCK_RESERVED_WEIGHT{2000};
 /** Default for -blockmintxfee, which sets the minimum feerate for a transaction in blocks created by mining code **/
 static constexpr unsigned int DEFAULT_BLOCK_MIN_TX_FEE{1000};
 /** The maximum weight for transactions we're willing to relay/mine */

src/rpc/mining.cpp

@@ -420,8 +420,8 @@ static RPCHelpMan getmininginfo()
                     RPCResult::Type::OBJ, "", "",
                     {
                         {RPCResult::Type::NUM, "blocks", "The current block"},
-                        {RPCResult::Type::NUM, "currentblockweight", /*optional=*/true, "The block weight of the last assembled block (only present if a block was ever assembled)"},
-                        {RPCResult::Type::NUM, "currentblocktx", /*optional=*/true, "The number of block transactions of the last assembled block (only present if a block was ever assembled)"},
+                        {RPCResult::Type::NUM, "currentblockweight", /*optional=*/true, "The block weight (including reserved weight for block header, txs count and coinbase tx) of the last assembled block (only present if a block was ever assembled)"},
+                        {RPCResult::Type::NUM, "currentblocktx", /*optional=*/true, "The number of block transactions (excluding coinbase) of the last assembled block (only present if a block was ever assembled)"},
                         {RPCResult::Type::STR_HEX, "bits", "The current nBits, compact representation of the block difficulty target"},
                         {RPCResult::Type::NUM, "difficulty", "The current difficulty"},
                         {RPCResult::Type::STR_HEX, "target", "The current target"},

src/test/fuzz/mini_miner.cpp

@@ -13,6 +13,7 @@
 
 #include <node/miner.h>
 #include <node/mini_miner.h>
+#include <node/types.h>
 #include <primitives/transaction.h>
 #include <random.h>
 #include <txmempool.h>
@@ -157,9 +158,10 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
             }
         }
 
-        // Stop if pool reaches DEFAULT_BLOCK_MAX_WEIGHT because BlockAssembler will stop when the
+        const auto block_adjusted_max_weight = MAX_BLOCK_WEIGHT - DEFAULT_BLOCK_RESERVED_WEIGHT;
+        // Stop if pool reaches block_adjusted_max_weight because BlockAssembler will stop when the
         // block template reaches that, but the MiniMiner will keep going.
-        if (pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx) >= DEFAULT_BLOCK_MAX_WEIGHT) break;
+        if (pool.GetTotalTxSize() + GetVirtualTransactionSize(*tx) >= block_adjusted_max_weight) break;
         TestMemPoolEntryHelper entry;
         const CAmount fee{ConsumeMoney(fuzzed_data_provider, /*max=*/MAX_MONEY/100000)};
         assert(MoneyRange(fee));
@@ -181,7 +183,7 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
 
     node::BlockAssembler::Options miner_options;
     miner_options.blockMinFeeRate = target_feerate;
-    miner_options.nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT;
+    miner_options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
     miner_options.test_block_validity = false;
     miner_options.coinbase_output_script = CScript() << OP_0;