Merge bitcoin/bitcoin#33965: mining: fix -blockreservedweight shadows IPC option

b623fab1ba mining: enforce minimum reserved weight for IPC (Sjors Provoost)
d3e49528d4 mining: fix -blockreservedweight shadows IPC option (Sjors Provoost)
418b7995dd test: have mining template helpers return None (Sjors Provoost)

Pull request description:

  Also enforce `MINIMUM_BLOCK_RESERVED_WEIGHT` for IPC clients.

  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 PR 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, with a preliminary commit that refactors the `create_block_template` and `wait_next_template` helpers.

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

  The third commit enforces `MINIMUM_BLOCK_RESERVED_WEIGHT` for IPC clients. Previously lower values were quietly clamped.

  ---

  Merge order preference: #34452 should ideally go first.

ACKs for top commit:
  sedited:
    Re-ACK b623fab1ba
  ryanofsky:
    Code review ACK b623fab1ba. Was rebased and test split up and comment updated since last review.
  ismaelsadeeq:
    ACK b623fab1ba

Tree-SHA512: 9e651a520d8e4aeadb330da86788744b6ecad8060fa21d50dc8e6012a60083e7b262aaa08a64676b9ef18ba65b651bc1272d8383d184030342e4c0f2c6a9866d

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/interfaces.cpp

@@ -67,6 +67,7 @@
 #include <any>
 #include <memory>
 #include <optional>
+#include <stdexcept>
 #include <utility>
 
 #include <boost/signals2/signal.hpp>
@@ -969,6 +970,16 @@ public:
 
     std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
     {
+        // Reject too-small values instead of clamping so callers don't silently
+        // end up mining with different options than requested. This matches the
+        // behavior of the `-blockreservedweight` startup option, which rejects
+        // values below MINIMUM_BLOCK_RESERVED_WEIGHT.
+        if (options.block_reserved_weight && options.block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
+            throw std::runtime_error(strprintf("block_reserved_weight (%zu) must be at least %u weight units",
+                                               *options.block_reserved_weight,
+                                               MINIMUM_BLOCK_RESERVED_WEIGHT));
+        }
+
         // Ensure m_tip_block is set so consumers of BlockTemplate can rely on that.
         if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};
 

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

@@ -43,9 +43,14 @@ struct BlockCreateOptions {
     bool use_mempool{true};
     /**
      * The default reserved weight for the fixed-size block header,
-     * transaction count and coinbase transaction.
+     * transaction count and coinbase transaction. Minimum: 2000 weight units
+     * (MINIMUM_BLOCK_RESERVED_WEIGHT).
+     *
+     * 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,
@@ -160,6 +161,7 @@ class IPCMiningTest(BitcoinTestFramework):
             async with AsyncExitStack() as stack:
                 self.log.debug("Create a template")
                 template = await mining_create_block_template(mining, stack, ctx, self.default_block_create_options)
+                assert template is not None
 
                 self.log.debug("Test some inspectors of Template")
                 header = (await template.getBlockHeader(ctx)).result
@@ -179,23 +181,26 @@ class IPCMiningTest(BitcoinTestFramework):
                 template2 = await wait_and_do(
                     mining_wait_next_template(template, stack, ctx, waitoptions),
                     lambda: self.generate(self.nodes[0], 1))
+                assert template2 is not None
                 block2 = await mining_get_block(template2, ctx)
                 assert_equal(len(block2.vtx), 1)
 
                 self.log.debug("Wait for another, but time out")
-                template3 = await template2.waitNext(ctx, waitoptions)
-                assert_equal(template3._has("result"), False)
+                template3 = await mining_wait_next_template(template2, stack, ctx, waitoptions)
+                assert template3 is None
 
                 self.log.debug("Wait for another, get one after increase in fees in the mempool")
                 template4 = await wait_and_do(
                     mining_wait_next_template(template2, stack, ctx, waitoptions),
                     lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
+                assert template4 is not None
                 block3 = await mining_get_block(template4, ctx)
                 assert_equal(len(block3.vtx), 2)
 
                 self.log.debug("Wait again, this should return the same template, since the fee threshold is zero")
                 waitoptions.feeThreshold = 0
                 template5 = await mining_wait_next_template(template4, stack, ctx, waitoptions)
+                assert template5 is not None
                 block4 = await mining_get_block(template5, ctx)
                 assert_equal(len(block4.vtx), 2)
                 waitoptions.feeThreshold = 1
@@ -204,24 +209,65 @@ class IPCMiningTest(BitcoinTestFramework):
                 template6 = await wait_and_do(
                     mining_wait_next_template(template5, stack, ctx, waitoptions),
                     lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
+                assert template6 is not None
                 block4 = await mining_get_block(template6, ctx)
                 assert_equal(len(block4.vtx), 3)
 
                 self.log.debug("Wait for another, but time out, since the fee threshold is set now")
-                template7 = await template6.waitNext(ctx, waitoptions)
-                assert_equal(template7._has("result"), False)
+                template7 = await mining_wait_next_template(template6, stack, ctx, waitoptions)
+                assert template7 is None
 
                 self.log.debug("interruptWait should abort the current wait")
                 async def wait_for_block():
                     new_waitoptions = self.capnp_modules['mining'].BlockWaitOptions()
                     new_waitoptions.timeout = timeout * 60 # 1 minute wait
                     new_waitoptions.feeThreshold = 1
-                    template7 = await template6.waitNext(ctx, new_waitoptions)
-                    assert_equal(template7._has("result"), False)
+                    template7 = await mining_wait_next_template(template6, stack, ctx, new_waitoptions)
+                    assert template7 is None
                 await wait_and_do(wait_for_block(), template6.interruptWait())
 
         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)
+
+                self.log.debug("Enforce minimum reserved weight for IPC clients too")
+                opts.blockReservedWeight = 0
+                try:
+                    await mining.createNewBlock(opts)
+                    raise AssertionError("createNewBlock unexpectedly succeeded")
+                except capnp.lib.capnp.KjException as e:
+                    assert_equal(e.description, "remote exception: std::exception: block_reserved_weight (0) must be at least 2000 weight units")
+                    assert_equal(e.type, "FAILED")
+
+        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")
@@ -304,6 +350,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__':

test/functional/test_framework/ipc_util.py

@@ -108,12 +108,18 @@ async def make_capnp_init_ctx(self):
 
 async def mining_create_block_template(mining, stack, ctx, opts):
     """Call mining.createNewBlock() and return template, then call template.destroy() when stack exits."""
-    return await stack.enter_async_context(destroying((await mining.createNewBlock(opts)).result, ctx))
+    response = await mining.createNewBlock(opts)
+    if not response._has("result"):
+        return None
+    return await stack.enter_async_context(destroying(response.result, ctx))
 
 
 async def mining_wait_next_template(template, stack, ctx, opts):
     """Call template.waitNext() and return template, then call template.destroy() when stack exits."""
-    return await stack.enter_async_context(destroying((await template.waitNext(ctx, opts)).result, ctx))
+    response = await template.waitNext(ctx, opts)
+    if not response._has("result"):
+        return None
+    return await stack.enter_async_context(destroying(response.result, ctx))
 
 
 async def mining_get_block(block_template, ctx):