Merge 7452638559 into 433412fd84

- The reserved weight and sigops count of the coinbase transaction
  is an estimate and may not represent the exact values; it can be lower.
  Adding these values to the total block weight, and total block sigop count
  of the assembled block might result in an incorrect total values when the
  real values are lower than the reserved ones.

- Fix this by not adding the reserved values to the total.

- Also make it explicit that total block weight, sigops count, and transaction count
  does not include the coinbase transaction.

doc/release-notes-31384.md

@@ -0,0 +1,17 @@
+- Node and Mining
+
+---
+
+- PR #31384 fixed an issue where the coinbase transaction weight was being reserved in two different places.
+  The fix ensures that the reservation happens in a single place.
+
+- This specifies how many weight units to reserve for the coinbase transaction.
+
+- Upgrade note: the default of `-maxcoinbaseweight` ensures backward compatibility for users who did
+  not override `-blockmaxweight`.
+
+- If you previously configured `-blockmaxweight=4000000` then you can
+  safely set `-maxcoinbaseweight=4000` to maintain existing behavior.
+
+- Bitcoin Core will now **fail to start** if the `-blockmaxweight` or `-maxcoinbaseweight` init parameter exceeds
+  the consensus limit of `4,000,000` weight units.

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>
@@ -54,6 +55,7 @@
 #include <node/mempool_persist_args.h>
 #include <node/miner.h>
 #include <node/peerman_args.h>
+#include <node/types.h>
 #include <policy/feerate.h>
 #include <policy/fees.h>
 #include <policy/fees_args.h>
@@ -647,7 +649,8 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
     argsman.AddArg("-whitelistrelay", strprintf("Add 'relay' permission to whitelisted peers with default permissions. This will accept relayed transactions even when not relaying transactions (default: %d)", DEFAULT_WHITELISTRELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
 
 
-    argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d)", DEFAULT_BLOCK_MAX_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
+    argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d).", MAX_BLOCK_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
+    argsman.AddArg("-maxcoinbaseweight=<n>", strprintf("Set the block maximum reserved coinbase transaction weight (default: %d).", node::BlockCreateOptions{}.coinbase_max_additional_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 +1018,20 @@ bool AppInitParameterInteraction(const ArgsManager& args)
         }
     }
 
+    if (args.IsArgSet("-blockmaxweight")) {
+        const auto max_block_weight = args.GetIntArg("-blockmaxweight", MAX_BLOCK_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("-maxcoinbaseweight")) {
+        const auto max_coinbase_weight = args.GetIntArg("-maxcoinbaseweight", node::BlockCreateOptions{}.coinbase_max_additional_weight);
+        if (max_coinbase_weight > MAX_BLOCK_WEIGHT) {
+            return InitError(strprintf(_("Specified -maxcoinbaseweight (%d) exceeds consensus maximum block weight (%d)"), max_coinbase_weight, MAX_BLOCK_WEIGHT));
+        }
+    }
+
     nBytesPerSigOp = args.GetIntArg("-bytespersigop", nBytesPerSigOp);
 
     if (!g_wallet_init_interface.ParameterInteraction()) return false;

src/node/miner.cpp

@@ -67,11 +67,11 @@ 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.coinbase_max_additional_weight <= MAX_BLOCK_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:
+    // Limit weight to between coinbase_max_additional_weight and MAX_BLOCK_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);
+    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, MAX_BLOCK_WEIGHT);
     return options;
 }
 
@@ -90,18 +90,17 @@ void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& optio
     if (const auto blockmintxfee{args.GetArg("-blockmintxfee")}) {
         if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed};
     }
+
     options.print_modified_fee = args.GetBoolArg("-printpriority", options.print_modified_fee);
+    options.coinbase_max_additional_weight = args.GetIntArg("-maxcoinbaseweight", options.coinbase_max_additional_weight);
 }
 
 void BlockAssembler::resetBlock()
 {
     inBlock.clear();
-
-    // Reserve space for coinbase tx
-    nBlockWeight = m_options.coinbase_max_additional_weight;
-    nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops;
-
     // These counters do not include coinbase tx
+    nBlockWeight = 0;
+    nBlockSigOpsCost = 0;
     nBlockTx = 0;
     nFees = 0;
 }
@@ -158,7 +157,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     pblocktemplate->vchCoinbaseCommitment = m_chainstate.m_chainman.GenerateCoinbaseCommitment(*pblock, pindexPrev);
     pblocktemplate->vTxFees[0] = -nFees;
 
-    LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d\n", GetBlockWeight(*pblock), nBlockTx, nFees, nBlockSigOpsCost);
+    LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d. (excluding coinbase tx)\n", nBlockWeight, nBlockTx, nFees, nBlockSigOpsCost);
 
     // Fill in header
     pblock->hashPrevBlock  = pindexPrev->GetBlockHash();
@@ -197,10 +196,12 @@ void BlockAssembler::onlyUnconfirmed(CTxMemPool::setEntries& testSet)
 bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost) const
 {
     // TODO: switch to weight-based accounting for packages instead of vsize-based accounting.
-    if (nBlockWeight + WITNESS_SCALE_FACTOR * packageSize >= m_options.nBlockMaxWeight) {
+    auto coinbase_adjusted_block_weight = nBlockWeight + m_options.coinbase_max_additional_weight; // Account for coinbase tx weight.
+    if (coinbase_adjusted_block_weight + WITNESS_SCALE_FACTOR * packageSize >= m_options.nBlockMaxWeight) {
         return false;
     }
-    if (nBlockSigOpsCost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) {
+    auto coinbase_adjusted_block_sigopcost = nBlockSigOpsCost + m_options.coinbase_output_max_additional_sigops; // Account for the coinbase tx sigop cost.
+    if (coinbase_adjusted_block_sigopcost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) {
         return false;
     }
     return true;

src/node/miner.h

@@ -6,8 +6,8 @@
 #ifndef BITCOIN_NODE_MINER_H
 #define BITCOIN_NODE_MINER_H
 
+#include <consensus/consensus.h>
 #include <node/types.h>
-#include <policy/policy.h>
 #include <primitives/block.h>
 #include <txmempool.h>
 
@@ -160,7 +160,7 @@ private:
 public:
     struct Options : BlockCreateOptions {
         // Configuration parameters for the block size
-        size_t nBlockMaxWeight{DEFAULT_BLOCK_MAX_WEIGHT};
+        size_t nBlockMaxWeight{MAX_BLOCK_WEIGHT};
         CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
         // Whether to call TestBlockValidity() at the end of CreateNewBlock().
         bool test_block_validity{true};
@@ -172,7 +172,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 (excluding coinbase transaction) */
     inline static std::optional<int64_t> m_last_block_weight{};
 
 private:

src/node/types.h

@@ -38,7 +38,7 @@ struct BlockCreateOptions {
      * scriptSig, witness and outputs. This must include any additional
      * weight needed for larger CompactSize encoded lengths.
      */
-    size_t coinbase_max_additional_weight{4000};
+    size_t coinbase_max_additional_weight{8000};
     /**
      * The maximum additional sigops which the pool will add in coinbase
      * transaction outputs.

src/policy/policy.h

@@ -19,8 +19,6 @@ class CCoinsViewCache;
 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};
 /** 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

@@ -419,8 +419,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 (excluding coinbase) 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::NUM, "difficulty", "The current difficulty"},
                         {RPCResult::Type::NUM, "networkhashps", "The network hashes per second"},
                         {RPCResult::Type::NUM, "pooledtx", "The size of the mempool"},

src/test/fuzz/mini_miner.cpp

@@ -9,6 +9,7 @@
 
 #include <node/miner.h>
 #include <node/mini_miner.h>
+#include <node/types.h>
 #include <primitives/transaction.h>
 #include <random.h>
 #include <txmempool.h>
@@ -150,9 +151,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 - node::BlockCreateOptions{}.coinbase_max_additional_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));
@@ -174,7 +176,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;
 

test/functional/mining_basic.py

@@ -22,7 +22,10 @@ from test_framework.messages import (
     CBlock,
     CBlockHeader,
     COIN,
+    DEFAULT_COINBASE_TX_WEIGHT,
+    MAX_BLOCK_WEIGHT,
     ser_uint256,
+    WITNESS_SCALE_FACTOR
 )
 from test_framework.p2p import P2PDataStore
 from test_framework.test_framework import BitcoinTestFramework
@@ -73,7 +76,7 @@ class MiningTest(BitcoinTestFramework):
         mining_info = self.nodes[0].getmininginfo()
         assert_equal(mining_info['blocks'], 200)
         assert_equal(mining_info['currentblocktx'], 0)
-        assert_equal(mining_info['currentblockweight'], 4000)
+        assert_equal(mining_info['currentblockweight'], 0)
 
         self.log.info('test blockversion')
         self.restart_node(0, extra_args=[f'-mocktime={t}', '-blockversion=1337'])
@@ -189,6 +192,109 @@ class MiningTest(BitcoinTestFramework):
         assert_equal(result, "inconclusive")
         assert_equal(prune_node.getblock(pruned_blockhash, verbosity=0), pruned_block)
 
+
+    def send_transactions(self, utxos, fee_rate, target_vsize):
+        """
+        Helper to create and send transactions with the specified target virtual size and fee rate.
+        """
+        for utxo in utxos:
+            self.wallet.send_self_transfer(
+                from_node=self.nodes[0],
+                utxo_to_spend=utxo,
+                target_vsize=target_vsize,
+                fee_rate=fee_rate,
+            )
+
+    def verify_block_template(self, expected_tx_count, expected_weight):
+        """
+        Create a block template and check that it satisfies the expected transaction count and total weight.
+        """
+        response = self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
+        self.log.info(f"Testing block template: contains {expected_tx_count} transactions, and total weight <= {expected_weight}")
+        assert_equal(len(response["transactions"]), expected_tx_count)
+        total_weight = sum(transaction["weight"] for transaction in response["transactions"])
+        assert_greater_than_or_equal(expected_weight, total_weight)
+
+    def test_block_max_weight(self):
+        self.log.info("Testing default and custom -blockmaxweight startup options.")
+
+        # Restart the node to allow large transactions
+        LARGE_TXS_COUNT = 10
+        LARGE_VSIZE = int(((MAX_BLOCK_WEIGHT - DEFAULT_COINBASE_TX_WEIGHT) / WITNESS_SCALE_FACTOR) / LARGE_TXS_COUNT)
+        HIGH_FEERATE = Decimal("0.0003")
+        self.restart_node(0, extra_args=[f"-datacarriersize={LARGE_VSIZE}"])
+
+        # Ensure the mempool is empty
+        assert_equal(len(self.nodes[0].getrawmempool()), 0)
+
+        # Generate UTXOs and send 10 large transactions with a high fee rate
+        utxos = [self.wallet.get_utxo(confirmed_only=True) for _ in range(LARGE_TXS_COUNT + 4)] # Add 4 more utxos that will be used in the test later
+        self.send_transactions(utxos[:LARGE_TXS_COUNT], HIGH_FEERATE, LARGE_VSIZE)
+
+        # Send 2 normal transactions with a lower fee rate
+        NORMAL_VSIZE = int(2000 / WITNESS_SCALE_FACTOR)
+        NORMAL_FEERATE = Decimal("0.0001")
+        self.send_transactions(utxos[LARGE_TXS_COUNT:LARGE_TXS_COUNT + 2], NORMAL_FEERATE, NORMAL_VSIZE)
+
+        # Check that the mempool contains all transactions
+        self.log.info(f"Testing that the mempool contains {LARGE_TXS_COUNT + 2} transactions.")
+        assert_equal(len(self.nodes[0].getrawmempool()), LARGE_TXS_COUNT + 2)
+
+        # Verify the block template includes only the 10 high-fee transactions
+        self.log.info("Testing that the block template includes only the 10 large transactions.")
+        self.verify_block_template(
+            expected_tx_count=LARGE_TXS_COUNT,
+            expected_weight=MAX_BLOCK_WEIGHT - DEFAULT_COINBASE_TX_WEIGHT,
+        )
+
+        # Test block template creation with custom -blockmaxweight
+        custom_block_weight = MAX_BLOCK_WEIGHT - 2000
+        # Reducing the weight by 2000 units will prevent 1 large transaction from fitting into the block.
+        self.restart_node(0, extra_args=[f"-datacarriersize={LARGE_VSIZE}", f"-blockmaxweight={custom_block_weight}"])
+
+        self.log.info("Testing the block template with custom -blockmaxweight to include 9 large and 2 normal transactions.")
+        self.verify_block_template(
+            expected_tx_count=11,
+            expected_weight=MAX_BLOCK_WEIGHT - DEFAULT_COINBASE_TX_WEIGHT - 2000,
+        )
+
+        # Ensure the block weight does not exceed the maximum
+        self.log.info(f"Testing that the block weight will never exceed {MAX_BLOCK_WEIGHT - DEFAULT_COINBASE_TX_WEIGHT}.")
+        self.restart_node(0, extra_args=[f"-datacarriersize={LARGE_VSIZE}", f"-blockmaxweight={MAX_BLOCK_WEIGHT}"])
+        self.log.info("Sending 2 additional normal transactions to fill the mempool to the maximum block weight.")
+        self.send_transactions(utxos[LARGE_TXS_COUNT + 2:], NORMAL_FEERATE, NORMAL_VSIZE)
+        self.log.info(f"Testing that the mempool's weight matches the maximum block weight: {MAX_BLOCK_WEIGHT}.")
+        assert_equal(self.nodes[0].getmempoolinfo()['bytes'] * WITNESS_SCALE_FACTOR, MAX_BLOCK_WEIGHT)
+
+        self.log.info("Testing that the block template includes only 10 transactions and cannot reach full block weight.")
+        self.verify_block_template(
+            expected_tx_count=LARGE_TXS_COUNT,
+            expected_weight=MAX_BLOCK_WEIGHT - DEFAULT_COINBASE_TX_WEIGHT,
+        )
+
+        self.log.info("Test -maxcoinbaseweight startup option.")
+        # Lowering the -maxcoinbaseweight by 4000 will allow for two more transactions.
+        self.restart_node(0, extra_args=[f"-datacarriersize={LARGE_VSIZE}", "-maxcoinbaseweight=4000"])
+        self.verify_block_template(
+            expected_tx_count=12,
+            expected_weight=MAX_BLOCK_WEIGHT - 4000,
+        )
+
+        self.log.info("Test that node will fail to start when user provide consensus invalid -maxcoinbaseweight")
+        self.stop_node(0)
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=[f"-maxcoinbaseweight={MAX_BLOCK_WEIGHT + 1}"],
+            expected_msg=f"Error: Specified -maxcoinbaseweight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus maximum block weight ({MAX_BLOCK_WEIGHT})",
+        )
+
+        self.log.info("Test that node will fail to start when user provide consensus invalid -blockmaxweight")
+        self.stop_node(0)
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=[f"-blockmaxweight={MAX_BLOCK_WEIGHT + 1}"],
+            expected_msg=f"Error: Specified -blockmaxweight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus maximum block weight ({MAX_BLOCK_WEIGHT})",
+        )
+
+
     def run_test(self):
         node = self.nodes[0]
         self.wallet = MiniWallet(node)
@@ -406,6 +512,7 @@ class MiningTest(BitcoinTestFramework):
         assert_equal(node.submitblock(hexdata=block.serialize().hex()), 'duplicate')  # valid
 
         self.test_blockmintxfee_parameter()
+        self.test_block_max_weight()
         self.test_timewarp()
         self.test_pruning()
 

test/functional/test_framework/messages.py

@@ -33,6 +33,7 @@ from test_framework.util import assert_equal
 
 MAX_LOCATOR_SZ = 101
 MAX_BLOCK_WEIGHT = 4000000
+DEFAULT_COINBASE_TX_WEIGHT = 8000
 MAX_BLOOM_FILTER_SIZE = 36000
 MAX_BLOOM_HASH_FUNCS = 50