mining: reject invalid block create options

Check BlockCreateOptions before block template creation instead of
clamping runtime values. This makes invalid runtime block creation
options, including those passed by IPC mining clients, fail explicitly
instead of silently mining with different values than the caller
requested.

Runtime option validation now uses the same error wording as startup
option validation. Startup validation also rejects -blockmaxweight
values lower than -blockreservedweight instead of allowing them to be
clamped later.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

src/node/interfaces.cpp

@@ -969,16 +969,6 @@ public:
 
     std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options, bool cooldown) 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.
         std::optional<BlockRef> maybe_tip{waitTipChanged(uint256::ZERO, MillisecondsDouble::max())};
 

src/node/miner.cpp

@@ -29,8 +29,9 @@
 #include <validation.h>
 
 #include <algorithm>
-#include <utility>
 #include <numeric>
+#include <stdexcept>
+#include <utility>
 
 namespace node {
 
@@ -77,24 +78,18 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
     block.hashMerkleRoot = BlockMerkleRoot(block);
 }
 
-static BlockCreateOptions ClampOptions(BlockCreateOptions options)
-{
-    options = FlattenMiningOptions(std::move(options));
-    options.block_reserved_weight = std::clamp<uint64_t>(*options.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.block_max_weight = std::clamp<uint64_t>(*options.block_max_weight, *options.block_reserved_weight, MAX_BLOCK_WEIGHT);
-    return options;
-}
-
 BlockAssembler::BlockAssembler(Chainstate& chainstate,
                                const CTxMemPool* mempool,
                                BlockCreateOptions options)
     : chainparams{chainstate.m_chainman.GetParams()},
       m_mempool{options.use_mempool ? mempool : nullptr},
       m_chainstate{chainstate},
-      m_options{ClampOptions(std::move(options))}
+      m_options{[&] {
+          if (auto result{CheckMiningOptions(options, /*use_argnames=*/false)}; !result) {
+              throw std::runtime_error(util::ErrorString(result).original);
+          }
+          return FlattenMiningOptions(std::move(options));
+      }()}
 {
 }
 

src/node/mining_args.cpp

@@ -17,6 +17,7 @@
 
 #include <cstdint>
 #include <optional>
+#include <utility>
 
 using common::AmountErrMsg;
 using util::Error;
@@ -24,22 +25,35 @@ using util::Result;
 
 namespace node {
 
-Result<void> CheckMiningOptions(const BlockCreateOptions& options, bool use_argnames)
+Result<void> CheckMiningOptions(BlockCreateOptions options, bool use_argnames)
 {
-    if (options.block_max_weight && *options.block_max_weight > MAX_BLOCK_WEIGHT) {
-        return Error{Untranslated(strprintf("%s (%d) exceeds consensus maximum block weight (%d)",
-                                            use_argnames ? "-blockmaxweight" : "block_max_weight",
-                                            *options.block_max_weight, MAX_BLOCK_WEIGHT))};
+    options = FlattenMiningOptions(std::move(options));
+    if (*options.block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
+        return Error{Untranslated(strprintf("%s (%d) is lower than minimum safety value of (%d)",
+                                            use_argnames ? "-blockreservedweight" : "block_reserved_weight",
+                                            *options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT))};
     }
-    if (options.block_reserved_weight && *options.block_reserved_weight > MAX_BLOCK_WEIGHT) {
+    if (*options.block_reserved_weight > MAX_BLOCK_WEIGHT) {
         return Error{Untranslated(strprintf("%s (%d) exceeds consensus maximum block weight (%d)",
                                             use_argnames ? "-blockreservedweight" : "block_reserved_weight",
                                             *options.block_reserved_weight, MAX_BLOCK_WEIGHT))};
     }
-    if (options.block_reserved_weight && *options.block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
-        return Error{Untranslated(strprintf("%s (%d) is lower than minimum safety value of (%d)",
+    if (*options.block_max_weight > MAX_BLOCK_WEIGHT) {
+        return Error{Untranslated(strprintf("%s (%d) exceeds consensus maximum block weight (%d)",
+                                            use_argnames ? "-blockmaxweight" : "block_max_weight",
+                                            *options.block_max_weight, MAX_BLOCK_WEIGHT))};
+    }
+    if (*options.block_reserved_weight > *options.block_max_weight) {
+        return Error{Untranslated(strprintf("%s (%d) exceeds %s (%d)",
                                             use_argnames ? "-blockreservedweight" : "block_reserved_weight",
-                                            *options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT))};
+                                            *options.block_reserved_weight,
+                                            use_argnames ? "-blockmaxweight" : "block_max_weight",
+                                            *options.block_max_weight))};
+    }
+    if (options.coinbase_output_max_additional_sigops > MAX_BLOCK_SIGOPS_COST) {
+        return Error{Untranslated(strprintf("%s (%zu) exceeds consensus maximum block sigops cost (%d)",
+                                            "coinbase_output_max_additional_sigops",
+                                            options.coinbase_output_max_additional_sigops, MAX_BLOCK_SIGOPS_COST))};
     }
     return {};
 }

src/node/mining_args.h

@@ -21,7 +21,7 @@ static const bool DEFAULT_PRINT_MODIFIED_FEE = false;
 [[nodiscard]] util::Result<BlockCreateOptions> ReadMiningArgs(const ArgsManager& args);
 
 /** Check option values for validity. Returns an error for invalid values. */
-[[nodiscard]] util::Result<void> CheckMiningOptions(const BlockCreateOptions& options, bool use_argnames);
+[[nodiscard]] util::Result<void> CheckMiningOptions(BlockCreateOptions options, bool use_argnames);
 
 /** Replace null optional values with their hardcoded defaults. */
 [[nodiscard]] BlockCreateOptions FlattenMiningOptions(BlockCreateOptions options);

src/node/mining_types.h

@@ -59,8 +59,8 @@ struct BlockCreateOptions {
     /**
      * Maximum block weight, defaults to -maxblockweight
      *
-     * block_reserved_weight can safely exceed block_max_weight, but the rest of
-     * the block template will be empty.
+     * Must not be lower than block_reserved_weight. Setting this equal to
+     * block_reserved_weight leaves no room for non-coinbase transactions.
      */
     std::optional<uint64_t> block_max_weight{};
     /**

src/test/fuzz/tx_pool.cpp

@@ -6,6 +6,7 @@
 #include <node/context.h>
 #include <node/mempool_args.h>
 #include <node/miner.h>
+#include <policy/policy.h>
 #include <policy/truc_policy.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
@@ -96,7 +97,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
     {
         BlockCreateOptions options{
             .block_min_fee_rate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)},
-            .block_max_weight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT),
+            .block_max_weight = fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(DEFAULT_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT),
         };
         auto assembler = BlockAssembler{chainstate, &tx_pool, options};
         auto block_template = assembler.CreateNewBlock();

src/test/miner_tests.cpp

@@ -764,6 +764,13 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     // Create and check a simple template
     std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options, /*cooldown=*/false);
     BOOST_REQUIRE(block_template);
+
+    BlockCreateOptions invalid_options{options};
+    invalid_options.block_max_weight = DEFAULT_BLOCK_RESERVED_WEIGHT - 1;
+    BOOST_CHECK_EXCEPTION(mining->createNewBlock(invalid_options, /*cooldown=*/false),
+                          std::runtime_error,
+                          HasReason("block_reserved_weight (8000) exceeds block_max_weight (7999)"));
+
     {
         CBlock block{block_template->getBlock()};
         {

test/functional/interface_ipc_mining.py

@@ -17,6 +17,7 @@ from test_framework.messages import (
     CTxInWitness,
     CTxOut,
     DEFAULT_BLOCK_RESERVED_WEIGHT,
+    MAX_BLOCK_SIGOPS_COST,
     MAX_BLOCK_WEIGHT,
     from_hex,
     msg_headers,
@@ -322,9 +323,27 @@ class IPCMiningTest(BitcoinTestFramework):
             self.log.debug("Enforce minimum reserved weight for IPC clients too")
             opts.blockReservedWeight = 0
             await assert_create_new_block_fails(ctx, mining, opts,
-                "block_reserved_weight (0) must be at least 2000 weight units")
+                "block_reserved_weight (0) is lower than minimum safety value of (2000)")
+
+        async def async_routine_check_max_reserved_weight():
+            self.log.debug("Enforce maximum reserved weight for IPC clients too")
+            ctx, mining = await make_mining_ctx(self)
+            opts = self.capnp_modules['mining'].BlockCreateOptions()
+            opts.blockReservedWeight = MAX_BLOCK_WEIGHT + 1
+            await assert_create_new_block_fails(ctx, mining, opts,
+                f"block_reserved_weight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus maximum block weight ({MAX_BLOCK_WEIGHT})")
+
+        async def async_routine_check_sigops_limit():
+            self.log.debug("Enforce sigops limit for IPC clients too")
+            ctx, mining = await make_mining_ctx(self)
+            opts = self.capnp_modules['mining'].BlockCreateOptions()
+            opts.coinbaseOutputMaxAdditionalSigops = MAX_BLOCK_SIGOPS_COST + 1
+            await assert_create_new_block_fails(ctx, mining, opts,
+                f"coinbase_output_max_additional_sigops ({MAX_BLOCK_SIGOPS_COST + 1}) exceeds consensus maximum block sigops cost ({MAX_BLOCK_SIGOPS_COST})")
 
         asyncio.run(capnp.run(async_routine()))
+        asyncio.run(capnp.run(async_routine_check_max_reserved_weight()))
+        asyncio.run(capnp.run(async_routine_check_sigops_limit()))
 
     def run_waitnext_mining_policy_test(self):
         """Verify that waitNext() preserves the mining policy from -blockmintxfee

test/functional/mining_basic.py

@@ -374,6 +374,13 @@ class MiningTest(BitcoinTestFramework):
             expected_msg=f"Error: -blockmaxweight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus maximum block weight ({MAX_BLOCK_WEIGHT})",
         )
 
+        self.log.info("Test that node will fail to start when -blockmaxweight is lower than -blockreservedweight")
+        self.stop_node(0)
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=[f"-blockmaxweight={DEFAULT_BLOCK_RESERVED_WEIGHT - 1}"],
+            expected_msg=f"Error: -blockreservedweight ({DEFAULT_BLOCK_RESERVED_WEIGHT}) exceeds -blockmaxweight ({DEFAULT_BLOCK_RESERVED_WEIGHT - 1})",
+        )
+
     def test_height_in_locktime(self):
         self.log.info("Sanity check generated blocks have their coinbase timelocked to their height.")
         self.generate(self.nodes[0], 1, sync_fun=self.no_op)

test/functional/test_framework/messages.py

@@ -36,6 +36,7 @@ from test_framework.util import (
 
 MAX_LOCATOR_SZ = 101
 MAX_BLOCK_WEIGHT = 4000000
+MAX_BLOCK_SIGOPS_COST = 80000
 DEFAULT_BLOCK_RESERVED_WEIGHT = 8000
 MINIMUM_BLOCK_RESERVED_WEIGHT = 2000
 MAX_BLOOM_FILTER_SIZE = 36000