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 (block_max_weight) matches -maxblockheight and is left alone. The one on BlockCreateOptions (block_max_weight) is an optional, it’s set by ApplyMiningDefaults only if empty.

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.

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:

#34020 (mining: add getTransactions(ByWitnessID) IPC methods by Sjors)
#34003 (test: interface_ipc.py minor fixes and cleanup by ryanofsky)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33819 (mining: getCoinbase() returns struct instead of raw tx by Sjors)
#33795 (test: Ignore error message give from python because of PYTHON_GIL by kevkevinpal)
#32420 (miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC by Sjors)
#29641 (scripted-diff: Use LogInfo over LogPrintf 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.

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.

DrahtBot added the label Needs rebase on Dec 2, 2025

Sjors force-pushed on Dec 2, 2025

Sjors commented at 2:14 pm on December 2, 2025: member

Rebased on the latest #33965 which allowed for some simplifications:

ClampOptions no longer takes a MiningArgs argument
no modifications to RPC code
the new block_max_weight option on BlockCreateOptions will no longer be ignored if a caller sets it, only the tx_pool fuzzer does.

We could trivially make the new block_max_weight option available to IPC clients, but I don’t think we should encourage its use.

Note that I brought back ApplyArgsManOptions in miner.cpp, but it’s been renamed to ApplyMiningDefaults and it now only deals with the two optionals.

I haven’t addressed this yet:

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.

What would be a good way to organize these things? E.g. having BlockCreateOptions, BlockWaitOptions and BlockCheckOptions defined in node/types.h wasn’t great either, because Mining interface clients don’t care the other structs in that file.

Sjors force-pushed on Dec 2, 2025

DrahtBot removed the label Needs rebase on Dec 2, 2025

ryanofsky commented at 5:43 pm on December 2, 2025: contributor

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.

What would be a good way to organize these things? E.g. having BlockCreateOptions, BlockWaitOptions and BlockCheckOptions defined in node/types.h wasn’t great either, because Mining interface clients don’t care the other structs in that file.

Oh I see. You mean you want the mining interface options separate from the TransactionError TxBroadcast enum types? I guess I put all these types in the same category because they are all used by src/interfaces/ code and by cap’n proto interfaces in later multiprocess PRs. But it is does seem reasonable to move these to a separate header like mining.h. I would just want to the structs not to have any fields that get ignored, and would want to move MiningArgs to different file, since it’s an internal type.

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>

54d3054db1

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.

f8fd7bd20c

Sjors force-pushed on Dec 4, 2025

move-only: add node/mining.h

Move mining related structs there.

This simplifies includes in later commits and makes the code easier to
understand for Mining IPC client developers.

97ca0dc727

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.

af196a3ec0

miner: add block_max_weight to BlockCreateOptions

This new optional replaces nBlockMaxWeight.

The option is not exposed to IPC clients.

9abeaa9e37

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.

e0d0a57583

Sjors force-pushed on Dec 4, 2025

Sjors commented at 1:28 pm on December 4, 2025: member

I moved MiningArgs and DEFAULT_PRINT_MODIFIED_FEE from node/mining.h to node/mining_args.h.

You mean you want the mining interface options separate from the TransactionError TxBroadcast enum types?

Yes, I think it’s easier for Mining IPC client developers to understand how things work that way.

DrahtBot added the label CI failed on Dec 4, 2025

DrahtBot removed the label CI failed on Dec 4, 2025

DrahtBot added the label Needs rebase on Dec 6, 2025

DrahtBot commented at 2:41 pm on December 6, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Sjors commented at 10:17 am on December 8, 2025: member

Holding off on trivial #29641 rebase until #33965 is merged or needs changes.

refactor: disentangle miner startup defaults from runtime options #33966

Code Coverage & Benchmarks

Reviews

Conflicts