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