Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
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: 11 additions & 0 deletions src/node/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
#include <any>
#include <memory>
#include <optional>
#include <stdexcept>
#include <utility>

#include <boost/signals2/signal.hpp>
Expand Down Expand Up @@ -969,6 +970,16 @@ class MinerImpl : public Mining

std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) 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) {

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.

Just a question: Is there a good reason to only enforce the minimum here?

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.

Agreed, since the rationale is to match the startup option checks, it should be verified as well.

@ryanofsky ryanofsky Feb 5, 2026

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)

Agreed, since the rationale is to match the startup option checks, it should be verified as well.

Maybe this check could be expanded but followup #33966 should unify this with startup checks in init.cpp:

bitcoin/src/init.cpp

Lines 1071 to 1077 in 28d8607

const auto block_reserved_weight = args.GetIntArg("-blockreservedweight", DEFAULT_BLOCK_RESERVED_WEIGHT);
if (block_reserved_weight > MAX_BLOCK_WEIGHT) {
return InitError(strprintf(_("Specified -blockreservedweight (%d) exceeds consensus maximum block weight (%d)"), block_reserved_weight, MAX_BLOCK_WEIGHT));
}
if (block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
return InitError(strprintf(_("Specified -blockreservedweight (%d) is lower than minimum safety value of (%d)"), block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT));
}

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.

I'll look into this for the followup.

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.

Added maximum check in #33966.

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: enforce minimum reserved weight for IPC" 8bc1cb7

The interface implementation has a goal of not having implementation code in here, since we are doing this, perhaps it's time to have a wrapper for all this.

which should include 8bc1cb7#r2749269557 suggestion and any other future additions?

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)

The interface implementation has a goal of not having implementation code in here, since we are doing this, perhaps it's time to have a wrapper for all this.

This should be happening in followup #33966 which unifies the startup & ipc checks.

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.

nice

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.
if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};

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
9 changes: 7 additions & 2 deletions src/node/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@ struct BlockCreateOptions {
bool use_mempool{true};
/**
* The default reserved weight for the fixed-size block header,
* transaction count and coinbase transaction.
* transaction count and coinbase transaction. Minimum: 2000 weight units
* (MINIMUM_BLOCK_RESERVED_WEIGHT).
*
* 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
59 changes: 53 additions & 6 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 @@ -160,6 +161,7 @@ async def async_routine():
async with AsyncExitStack() as stack:
self.log.debug("Create a template")
template = await mining_create_block_template(mining, stack, ctx, self.default_block_create_options)
assert template is not None

self.log.debug("Test some inspectors of Template")
header = (await template.getBlockHeader(ctx)).result
Expand All @@ -179,23 +181,26 @@ async def async_routine():
template2 = await wait_and_do(
mining_wait_next_template(template, stack, ctx, waitoptions),
lambda: self.generate(self.nodes[0], 1))
assert template2 is not None
block2 = await mining_get_block(template2, ctx)
assert_equal(len(block2.vtx), 1)

self.log.debug("Wait for another, but time out")
template3 = await template2.waitNext(ctx, waitoptions)
assert_equal(template3._has("result"), False)
template3 = await mining_wait_next_template(template2, stack, ctx, waitoptions)
assert template3 is None

self.log.debug("Wait for another, get one after increase in fees in the mempool")
template4 = await wait_and_do(
mining_wait_next_template(template2, stack, ctx, waitoptions),
lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
assert template4 is not None
block3 = await mining_get_block(template4, ctx)
assert_equal(len(block3.vtx), 2)

self.log.debug("Wait again, this should return the same template, since the fee threshold is zero")
waitoptions.feeThreshold = 0
template5 = await mining_wait_next_template(template4, stack, ctx, waitoptions)
assert template5 is not None
block4 = await mining_get_block(template5, ctx)
assert_equal(len(block4.vtx), 2)
waitoptions.feeThreshold = 1
Expand All @@ -204,24 +209,65 @@ async def async_routine():
template6 = await wait_and_do(
mining_wait_next_template(template5, stack, ctx, waitoptions),
lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
assert template6 is not None
block4 = await mining_get_block(template6, ctx)
assert_equal(len(block4.vtx), 3)

self.log.debug("Wait for another, but time out, since the fee threshold is set now")
template7 = await template6.waitNext(ctx, waitoptions)
assert_equal(template7._has("result"), False)
template7 = await mining_wait_next_template(template6, stack, ctx, waitoptions)
assert template7 is None

self.log.debug("interruptWait should abort the current wait")
async def wait_for_block():
new_waitoptions = self.capnp_modules['mining'].BlockWaitOptions()
new_waitoptions.timeout = timeout * 60 # 1 minute wait
new_waitoptions.feeThreshold = 1
template7 = await template6.waitNext(ctx, new_waitoptions)
assert_equal(template7._has("result"), False)
template7 = await mining_wait_next_template(template6, stack, ctx, new_waitoptions)
assert template7 is None
await wait_and_do(wait_for_block(), template6.interruptWait())

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)

self.log.debug("Enforce minimum reserved weight for IPC clients too")
opts.blockReservedWeight = 0
try:
await mining.createNewBlock(opts)
raise AssertionError("createNewBlock unexpectedly succeeded")
except capnp.lib.capnp.KjException as e:
assert_equal(e.description, "remote exception: std::exception: block_reserved_weight (0) must be at least 2000 weight units")
assert_equal(e.type, "FAILED")

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 @@ -304,6 +350,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
10 changes: 8 additions & 2 deletions test/functional/test_framework/ipc_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,18 @@ async def make_capnp_init_ctx(self):

async def mining_create_block_template(mining, stack, ctx, opts):
"""Call mining.createNewBlock() and return template, then call template.destroy() when stack exits."""
return await stack.enter_async_context(destroying((await mining.createNewBlock(opts)).result, ctx))
response = await mining.createNewBlock(opts)
if not response._has("result"):
return None
return await stack.enter_async_context(destroying(response.result, ctx))


async def mining_wait_next_template(template, stack, ctx, opts):
"""Call template.waitNext() and return template, then call template.destroy() when stack exits."""
return await stack.enter_async_context(destroying((await template.waitNext(ctx, opts)).result, ctx))
response = await template.waitNext(ctx, opts)
if not response._has("result"):
return None
return await stack.enter_async_context(destroying(response.result, ctx))


async def mining_get_block(block_template, ctx):
Expand Down
Loading