From d3e49528d479613ebe50088d530a621861463fa4 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 19 Dec 2025 15:18:33 -0600 Subject: [PATCH] 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 --- src/init.cpp | 2 +- src/node/miner.cpp | 11 ++++++--- src/node/types.h | 6 ++++- test/functional/interface_ipc_mining.py | 33 +++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index a44cdf807e0..e6cc2045b4a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -685,7 +685,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-blockmaxweight=", strprintf("Set maximum BIP141 block weight (default: %d)", DEFAULT_BLOCK_MAX_WEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::BLOCK_CREATION); - argsman.AddArg("-blockreservedweight=", 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=", 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=", 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=", "Override block version to test forking scenarios", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::BLOCK_CREATION); diff --git a/src/node/miner.cpp b/src/node/miner.cpp index b5073054eb2..7bdd8b690d7 100644 --- a/src/node/miner.cpp +++ b/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(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(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(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(options.nBlockMaxWeight, options.block_reserved_weight, MAX_BLOCK_WEIGHT); + options.nBlockMaxWeight = std::clamp(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 diff --git a/src/node/types.h b/src/node/types.h index deab1faacb8..1eea9460535 100644 --- a/src/node/types.h +++ b/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 block_reserved_weight{}; /** * The maximum additional sigops which the pool will add in coinbase * transaction outputs. diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py index 876a9231a0f..d1bdf609ad1 100755 --- a/test/functional/interface_ipc_mining.py +++ b/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__':