From 4637cd157da836ef377fb35e40956a0226d0baac Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 18 May 2026 15:59:57 +0200 Subject: [PATCH] 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 --- src/node/interfaces.cpp | 10 ------- src/node/miner.cpp | 21 ++++++-------- src/node/mining_args.cpp | 32 ++++++++++++++++------ src/node/mining_args.h | 2 +- src/node/mining_types.h | 4 +-- src/test/fuzz/tx_pool.cpp | 3 +- src/test/miner_tests.cpp | 7 +++++ test/functional/interface_ipc_mining.py | 21 +++++++++++++- test/functional/mining_basic.py | 7 +++++ test/functional/test_framework/messages.py | 1 + 10 files changed, 71 insertions(+), 37 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 7a078c81cb2..5317af9ff97 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -969,16 +969,6 @@ public: std::unique_ptr 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 maybe_tip{waitTipChanged(uint256::ZERO, MillisecondsDouble::max())}; diff --git a/src/node/miner.cpp b/src/node/miner.cpp index e1b6a480fbd..97a316ce217 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -29,8 +29,9 @@ #include #include -#include #include +#include +#include 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(*options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT); - options.coinbase_output_max_additional_sigops = std::clamp(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(*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)); + }()} { } diff --git a/src/node/mining_args.cpp b/src/node/mining_args.cpp index ef859655cae..33e623a14be 100644 --- a/src/node/mining_args.cpp +++ b/src/node/mining_args.cpp @@ -17,6 +17,7 @@ #include #include +#include using common::AmountErrMsg; using util::Error; @@ -24,22 +25,35 @@ using util::Result; namespace node { -Result CheckMiningOptions(const BlockCreateOptions& options, bool use_argnames) +Result 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 {}; } diff --git a/src/node/mining_args.h b/src/node/mining_args.h index e3ebf959c0e..8baa7395285 100644 --- a/src/node/mining_args.h +++ b/src/node/mining_args.h @@ -21,7 +21,7 @@ static const bool DEFAULT_PRINT_MODIFIED_FEE = false; [[nodiscard]] util::Result ReadMiningArgs(const ArgsManager& args); /** Check option values for validity. Returns an error for invalid values. */ -[[nodiscard]] util::Result CheckMiningOptions(const BlockCreateOptions& options, bool use_argnames); +[[nodiscard]] util::Result CheckMiningOptions(BlockCreateOptions options, bool use_argnames); /** Replace null optional values with their hardcoded defaults. */ [[nodiscard]] BlockCreateOptions FlattenMiningOptions(BlockCreateOptions options); diff --git a/src/node/mining_types.h b/src/node/mining_types.h index 6e3ee0f4afb..9c4fbcbbafe 100644 --- a/src/node/mining_types.h +++ b/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 block_max_weight{}; /** diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index b7279990a1d..825b32bc0c2 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -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(DEFAULT_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT), }; auto assembler = BlockAssembler{chainstate, &tx_pool, options}; auto block_template = assembler.CreateNewBlock(); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 6f2855c819a..be2b5e60f6a 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -764,6 +764,13 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) // Create and check a simple template std::unique_ptr 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()}; { diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py index 710a9a2d82c..b59ffaab653 100755 --- a/test/functional/interface_ipc_mining.py +++ b/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 diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py index 2f6f1146c19..7060a3cba62 100755 --- a/test/functional/mining_basic.py +++ b/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) diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 012049c257c..a0f2a174023 100755 --- a/test/functional/test_framework/messages.py +++ b/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