Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
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 <russ@yanofsky.org>
  • Loading branch information
Sjors and ryanofsky committed Feb 7, 2026
commit d3e49528d479613ebe50088d530a621861463fa4
2 changes: 1 addition & 1 deletion src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
11 changes: 7 additions & 4 deletions src/node/miner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In "mining: fix -blockreservedweight shadows IPC option" f69320d

Why make the distinction between RPC and IPC clients?

The narrative of this commit should rather be future proof and changed to: it can be overridded by runtime via the IPC or RPC.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #33965 (comment)

In "mining: fix -blockreservedweight shadows IPC option" f69320d

Why make the distinction between RPC and IPC clients?

The narrative of this commit should rather be future proof and changed to: it can be overridded by runtime via the IPC or RPC.

The commit description is just describing what the commit is changing, which is only IPC clients. The IPC and RPC interfaces are different, with IPC clients currently required to specify block_reserved_weight and RPC clients not able to specify it at all.

RPC behavior is unchanged by this commit. IPC behavior is just being changed so the IPC value is applied instead of ignored when the -blockreservedweight config option is used.

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;
}

Expand All @@ -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
Expand Down
6 changes: 5 additions & 1 deletion src/node/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<size_t> block_reserved_weight{};
/**
* The maximum additional sigops which the pool will add in coinbase
* transaction outputs.
Expand Down
33 changes: 33 additions & 0 deletions test/functional/interface_ipc_mining.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from io import BytesIO
from test_framework.blocktools import NULL_OUTPOINT
from test_framework.messages import (
MAX_BLOCK_WEIGHT,
CTransaction,
CTxIn,
CTxOut,
Expand Down Expand Up @@ -227,6 +228,37 @@ async def wait_for_block():

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "mining: fix -blockreservedweight shadows IPC option" (d3e4952)

I feel like this description would be easier to understand if it began with a sentence saying what this is trying to check like "Confirm that BlockCreateOptions.blockReservedWeight takes precedence over -blockreservedweight. Set an absurdly high -blockreservedweight value that would result in empty blocks to verify this."

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will improve if I need to retouch.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken in #33966.

# 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")
Expand Down Expand Up @@ -309,6 +341,7 @@ def run_test(self):
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__':
Expand Down