mining: fix -blockreservedweight shadows IPC option

The -blockreservedweight startup option should only affect RPC code,
because IPC clients (currently) do not have a way to signal their intent
to use the node default (the BlockCreateOptions struct defaults
merely document a recommendation for client software).

Before this commit however, if the user set -blockreservedweight
then ApplyArgsManOptions would cause the block_reserved_weight
option passed by IPC clients to be ignored. Users who don't set
this value were not affected.

Fix this by making BlockCreateOptions::block_reserved_weight an
std::optional.

Internal interface users, such as the RPC call sites, don't set a
value so -blockreservedweight is used. Whereas IPC clients do set
a value which is no longer ignored.

Test coverage is added.

mining_basic.py already ensured -blockreservedweight is enforced by
mining RPC methods. This commit adds coverage for Mining interface IPC
clients. It also verifies that -blockreservedweight has no effect on
them.

Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>

src/init.cpp

@@ -685,7 +685,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 | ArgsManager::DEBUG_ONLY, 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("-blockreservedweight=<n>", strprintf("Reserve space for the fixed-size block header plus the largest coinbase transaction the mining software may add to the block. Only affects mining RPC clients, not IPC clients. (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);
 

src/node/miner.cpp

@@ -78,11 +78,12 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 
 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 {
-    options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);
+    // Apply DEFAULT_BLOCK_RESERVED_WEIGHT when the caller left it unset.
+    options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight.value_or(DEFAULT_BLOCK_RESERVED_WEIGHT), MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);
     options.coinbase_output_max_additional_sigops = std::clamp<size_t>(options.coinbase_output_max_additional_sigops, 0, MAX_BLOCK_SIGOPS_COST);
     // 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);
+    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, *options.block_reserved_weight, MAX_BLOCK_WEIGHT);
     return options;
 }
 
@@ -102,13 +103,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);
+    if (!options.block_reserved_weight) {
+        options.block_reserved_weight = args.GetIntArg("-blockreservedweight");
+    }
 }
 
 void BlockAssembler::resetBlock()
 {
     // Reserve space for fixed-size block header, txs count, and coinbase tx.
-    nBlockWeight = m_options.block_reserved_weight;
+    nBlockWeight = *Assert(m_options.block_reserved_weight);
     nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops;
 
     // These counters do not include coinbase tx

src/node/types.h

@@ -44,8 +44,12 @@ struct BlockCreateOptions {
     /**
      * The default reserved weight for the fixed-size block header,
      * transaction count and coinbase transaction.
+     *
+     * Providing a value overrides the `-blockreservedweight` startup setting.
+     * Cap'n Proto IPC clients currently cannot leave this field unset, so they
+     * always provide a value.
      */
-    size_t block_reserved_weight{DEFAULT_BLOCK_RESERVED_WEIGHT};
+    std::optional<size_t> block_reserved_weight{};
     /**
      * The maximum additional sigops which the pool will add in coinbase
      * transaction outputs.

test/functional/interface_ipc_mining.py

@@ -8,6 +8,7 @@ from contextlib import AsyncExitStack
 from io import BytesIO
 from test_framework.blocktools import NULL_OUTPOINT
 from test_framework.messages import (
+    MAX_BLOCK_WEIGHT,
     CTransaction,
     CTxIn,
     CTxOut,
@@ -227,6 +228,37 @@ class IPCMiningTest(BitcoinTestFramework):
 
         asyncio.run(capnp.run(async_routine()))
 
+    def run_ipc_option_override_test(self):
+        self.log.info("Running IPC option override test")
+        # Set an absurd reserved weight. `-blockreservedweight` is RPC-only, so
+        # with this setting RPC templates would be empty. IPC clients set
+        # blockReservedWeight per template request and are unaffected; later in
+        # the test the IPC template includes a mempool transaction.
+        self.restart_node(0, extra_args=[f"-blockreservedweight={MAX_BLOCK_WEIGHT}"])
+
+        async def async_routine():
+            ctx, mining = await self.make_mining_ctx()
+            self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
+
+            async with AsyncExitStack() as stack:
+                opts = self.capnp_modules['mining'].BlockCreateOptions()
+                opts.useMempool = True
+                opts.blockReservedWeight = 4000
+                opts.coinbaseOutputMaxAdditionalSigops = 0
+                template = await mining_create_block_template(mining, stack, ctx, opts)
+                assert template is not None
+                block = await mining_get_block(template, ctx)
+                assert_equal(len(block.vtx), 2)
+
+                self.log.debug("Use absurdly large reserved weight to force an empty template")
+                opts.blockReservedWeight = MAX_BLOCK_WEIGHT
+                empty_template = await mining_create_block_template(mining, stack, ctx, opts)
+                assert empty_template is not None
+                empty_block = await mining_get_block(empty_template, ctx)
+                assert_equal(len(empty_block.vtx), 1)
+
+        asyncio.run(capnp.run(async_routine()))
+
     def run_coinbase_and_submission_test(self):
         """Test coinbase construction (getCoinbaseTx, getCoinbaseCommitment) and block submission (submitSolution)."""
         self.log.info("Running coinbase construction and submission test")
@@ -309,6 +341,7 @@ class IPCMiningTest(BitcoinTestFramework):
         self.run_mining_interface_test()
         self.run_block_template_test()
         self.run_coinbase_and_submission_test()
+        self.run_ipc_option_override_test()
 
 
 if __name__ == '__main__':