refactor: disentangle miner startup defaults from runtime options

Sjors commented at 3:42 pm on November 28, 2025: member

The interaction between node startup options like -blockreservedweight and runtime options, especially those passed via IPC, is confusing.

They’re combined in BlockAssembler::Options, which this PR gets rid of in favour of two distinct structs:

BlockCreateOptions: used by interface clients. As before, IPC clients have access to a safe / sane subset, whereas RPC and test code can use all fields.
MiningArgs: these are set once during node startup

Both structs have a member for the maximum block height, which is not a problem since they’re different structs. The one on MiningArgs (nBlockMaxWeight) matches -maxblockheight and is left alone. The one on BlockCreateOptions (block_max_weight) is (exclusively) set by ClampOptions.

This all happens in the last commit and requires some preparation to keep things easy to review.

We get rid of BlockAssembler::Options but this is used in many tests. Since large churn is inevitable, we might as well switch all tests, bench and fuzzers over to the Mining interface. The first (non-base) commit does that, dramatically reducing direct use of BlockAssembler. Two exceptions are documented in the commit message. Because test_block_validity wasn’t available via the interface and the block_assemble benchmark needs it, it’s moved from BlockAssembler::Options to BlockCreateOptions (still not exposed via IPC).

We need access to mining related defaults and structs from both the miner and node initialization code. To avoid having to pull in all of BlockAssembler for the latter, the second commit introduces node/mining.h and moves constants and structs there from src/node/types.h (BlockCreateOptions, BlockWaitOptions, BlockCheckOptions) and src/node/miner.h (DEFAULT_PRINT_MODIFIED_FEE).

I considered also moving DEFAULT_BLOCK_MAX_WEIGHT, DEFAULT_BLOCK_RESERVED_WEIGHT, MINIMUM_BLOCK_RESERVED_WEIGHT and DEFAULT_BLOCK_MIN_TX_FEE there from policy.h, since they are distinct from relay policy and not needed by the kernel. But this seems more appropriate for a follow-up and requires additional discussion.

The meat and potatoes of this PR is split between the three last commits for easier review:

Adds node/mining_args.{h,cpp} and move the options checking out of init.cpp, without adding storage
Add block_max_weight to BlockCreateOptions as explained above
Introduce the MiningArgs struct, add it to the NodeContext, expand mining_args.cpp to store it, pass it to BlockAssembler, etc.

I kept variable renaming and other formatting changes to a minimum to ease review with --color-moved=dimmed-zebra.

This PR builds on the bug fix in:

#33965

Once that is merged, this PR should not change behaviour.

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 having ApplyArgsManOptions not touch block_reserved_weight.
Update RPC callers, in particular getblocktemplate, to explicitly set
this option since it's no longer automatic.

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.

46d901c253

DrahtBot renamed this:
~~refactor: disentangle miner startup defaults from runtime options~~
refactor: disentangle miner startup defaults from runtime options
on Nov 28, 2025

DrahtBot added the label Refactoring on Nov 28, 2025

DrahtBot commented at 3:42 pm on November 28, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33966.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33819 (mining: add getCoinbase() by Sjors)
#33795 (test: Ignore error message give from python because of PYTHON_GIL by kevkevinpal)
#33591 (Cluster mempool followups by sdaftuar)
#32468 (rpc: generateblock to allow multiple outputs by polespinasa)
#32420 (miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC by Sjors)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#28792 (build: Embedded ASMap [3/3]: Build binary dump header file by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors force-pushed on Nov 28, 2025

DrahtBot added the label CI failed on Nov 28, 2025

DrahtBot commented at 4:00 pm on November 28, 2025: contributor

🚧 At least one of the CI tasks failed. Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/19768365687/job/56646627852 LLM reason (✨ experimental): Compilation failed due to incorrect initialization order (designated initializers) in setup_common.cpp, causing test_util build to fail.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

mining: use interface for tests, bench and fuzzers

Have most tests, benchmarks and fuzzers go through the mining interface.
This avoids the use node::BlockAssembler::Options, which makes it easier
to drop in a later commit.

Two exceptions which use BlockAssembler directly:
- one check in test/miner_tests.cpp needs m_package_feerates
- fuzz/tx_pool.cpp Finish() doesn't have access to a NodeContext

Move test_block_validity from BlockAssembler::Options to
BlockCreateOptions so bench/block_assemble.cpp can continue to set it.
Just like coinbase_output_script, this is not exposed to IPC clients.

Inline options variable in places where it's only needed once.

The remaining PrepareBlock() constructor takes a const NodeContext
parameter, but internally const_cast it. This should be harmless and
avoids a larger refactor around constness in the tests.

We also drop one unused PrepareBlock declaration and one unused
implementation.

TestChain100Setup::CreateBlock no longer needs a chainstate argument
which in turn means it can be dropped from CreateAndProcessBlock.

29051db767

move-only: add node/mining.h

Move mining related defaults and structs there.

This simplifies includes in later commits.

eecbd41748

mining: parse block creation args in mining_args

Move the argument parsing for -blockmaxweight, -blockreservedweight,
-blockmintxfee and -blockversion out of init.cpp to a dedicated
mining_args.cpp.

ee2738cb38

miner: add block_max_weight to BlockCreateOptions

After the next commit nBlockMaxWeight is set once at startup and not
mutated after.

In preparation this commit introduces BlockCreateOptions::block_max_weight
which is set by ClampOptions, leaving nBlockMaxWeight unmodified.

This option is not exposed to IPC clients.

3f7d885f15

mining: store mining args in NodeContext

Instead of parsing arguments like -blockmaxweight each time a block
template is generated, do it once in ApplyArgsManOptions().

These arguments are stored in a new struct MiningArgs.

The BlockAssembler::Options struct is removed in favor of passing
both MiningArgs and BlockCreateOptions.

This disentangles node configuration from client (or test) options.

517a9b2328

Sjors force-pushed on Nov 28, 2025

DrahtBot removed the label CI failed on Nov 28, 2025

ryanofsky commented at 8:52 pm on December 1, 2025: contributor

Concept ACK 517a9b23283fee0a861578b2686a71c48a4b67b4, but I had some questions on #33965 which this builds on, and this PR will be affected by what happens to that.

Overall most changes here seem very good: it’s nice to introduce a MiningArgs struct and move handling of mining options out of init.cpp. Also nice to port more code to use interfaces::Mining. Other changes seem less positive. Before, there was a clear separation of which options were controllable by interfaces/mining.h and which options were internal and not part of the public interface. But now all options are mixed together in node/mining.h. For example the BlockCreateOptions struct passed to createNewBlock has a block_max_weight member, but setting that member will not do anything, because it will always be overridden by the MiningArgs::nBlockMaxWeight in ClampOptions. Also, as long as the -blockreservedweight option exists, maybe it would be nice if miners could use this value instead of having to provide their own value, so it could be nice to use std::optional for settings that can come from 2 places instead of the more ad-hoc approach here.

refactor: disentangle miner startup defaults from runtime options #33966

Code Coverage & Benchmarks

Reviews

Conflicts