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
Approach 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:

#34568 (mining: Break compatibility with existing IPC mining clients by ryanofsky)
#34184 (mining: add cooldown to createNewBlock() immediately after IBD by Sjors)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)

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.

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

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

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

Sjors force-pushed on Jan 8, 2026

Sjors commented at 3:55 am on January 8, 2026: member

Rebased and added a commit to move the minimum block_reserved_weight check from interface to mining code, as suggested here: #33965 (review)

DrahtBot removed the label Needs rebase on Jan 8, 2026

DrahtBot added the label Needs rebase on Jan 13, 2026

Sjors force-pushed on Jan 14, 2026

DrahtBot removed the label Needs rebase on Jan 14, 2026

DrahtBot added the label Needs rebase on Feb 2, 2026

Sjors force-pushed on Feb 7, 2026

Sjors commented at 1:40 pm on February 7, 2026: member

Minimalistic rebase after #32420 and #34452, to stay on top of the current #33965. I’ll address feedback later, probably after the latter is merged.

Sjors force-pushed on Feb 7, 2026

DrahtBot added the label CI failed on Feb 7, 2026

DrahtBot commented at 2:06 pm on February 7, 2026: contributor

🚧 At least one of the CI tasks failed. Task No wallet: https://github.com/bitcoin/bitcoin/actions/runs/21780978538/job/62844896556 LLM reason (✨ experimental): Compilation failed in fuzz tests: missing node::BlockAssembler (Options) in process_message.cpp causing undefined identifier errors.

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 Feb 7, 2026

DrahtBot removed the label Needs rebase on Feb 7, 2026

DrahtBot removed the label CI failed on Feb 7, 2026

Sjors force-pushed on Feb 12, 2026

Sjors commented at 11:47 am on February 12, 2026: member

#33965 landed and I rebased, so this is ready for review.

I added a commit with minor test fixups suggested in earlier pull requests.

I implemented the check for maximum block_reserved_weight, as suggested in #33965 (review)

I think with that I’ve addressed all actionable feedback, but let me know if I missed something.

Sjors marked this as ready for review on Feb 12, 2026

in src/node/mining_types.h:1 in 705fd23780 outdated

ryanofsky commented at 3:28 pm on February 12, 2026:

In commit “move-only: add node/mining.h” (705fd237806c80d70d9079326410962d39891abe)

Since this file is included by non-node code it would be good to add a @file comment similar to the one in types.h that this include files is used externally by IPC clients and should only declare simple data definitions. It shouldn’t declare or use functions or classes with methods unless they are header-only or provided by the util library. (Some other projects have public include directories to keep internal & external headers separate, but for now we only mark the distinction with comments.)

MIght also be good to rename this file mining_types.h or similar to reduce temptation to declare functions and classes not available over IPC here and distinguish from miner.h more

Sjors commented at 7:58 pm on February 12, 2026:

Done both.

in src/node/mining_args.cpp:19 in a3bced6737

14+
15+using common::AmountErrMsg;
16+
17+namespace node {
18+
19+util::Result<void> ApplyArgsManOptions(const ArgsManager& args)

ryanofsky commented at 3:37 pm on February 12, 2026:

In commit “mining: parse block creation args in mining_args” (a3bced673787696ca5824b43866fa4c84780a742)

Could consider calling this ReadMiningArgs instead of ApplyArgsManOptions. (We have two conventions for functions of this type Read[Something]Args and ApplyArgsManOptions and I dislike latter because it’s not descriptive and difficult to grep because it is overloaded.)

Sjors commented at 7:58 pm on February 12, 2026:

Renamed

in src/node/context.h:84 in bfa6e3f0f2 outdated

80@@ -79,6 +81,7 @@ struct NodeContext {
81     //! Reference to chain client that should used to load or create wallets
82     //! opened by the gui.
83     std::unique_ptr<interfaces::Mining> mining;
84+    MiningArgs mining_args;

ryanofsky commented at 3:49 pm on February 12, 2026:

In commit “mining: store mining args in NodeContext” (bfa6e3f0f257d6d03ed03249305f218e5a85f216)

Note: It seems ok to include node/mining_args.h and declare this as a value member, even this goes against the comment above that this struct “should just be a collection of references that can be used without pulling in unwanted dependencies or functionality” because (1) this is a pretty minimal dependency, and (2) if we have a block template manager class (#33421) mining_args could be dropped here and moved into that class.

in src/test/miner_tests.cpp:185 in bfa6e3f0f2

181@@ -182,6 +182,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
182     const auto block_package_feerates = BlockAssembler{
183         m_node.chainman->ActiveChainstate(),
184         &tx_mempool,
185+        {},

ryanofsky commented at 3:55 pm on February 12, 2026:

In commit “mining: store mining args in NodeContext” (bfa6e3f0f257d6d03ed03249305f218e5a85f216)

Would seem good to pass m_node.mining_args so if a test happens to set those args they are applied, also to make it a little more obvious what is being passed here. (Could also replace {} with BlockCreateOptions{} on the line below to clarify that parameter.)

Sjors commented at 7:58 pm on February 12, 2026:

Done

ryanofsky approved

ryanofsky commented at 4:52 pm on February 12, 2026: contributor

Approach ACK b466b32f4f0b5cfcdcf5536cd3f40a104f3a4927. PR seems to make a lot of good changes and move in the right direction. Am wondering if we actually need two overlapping structs and two very similar error checking functions or can get away with one of each. Also if we still need to keep the clamping behavior or could just have errors raised if values are out of bounds? There are still std::clamp calls but I’m not sure if they are dead code.

I’m thinking maybe we could have a single BlockCreateOptions struct with mostly std::optional members and one ReadMiningArgs(const ArgsManager& args, BlockCreateOptions& options) function that applies command line settings, and a ApplyMiningDefaults(BlockCreateOptions&) that applies default settings and returns errors and that would be sufficient?

No problems with current approach though, and additional simplifications could be made on top of this.

Sjors force-pushed on Feb 12, 2026

Sjors commented at 7:58 pm on February 12, 2026: member

Applied the inline suggestions.

Also if we still need to keep the clamping behavior or could just have errors raised if values are out of bounds? There are still std::clamp calls but I’m not sure if they are dead code.

Since I added a check for the maximum reserved weight in my previous push, I might as well add the same checks for sigops and max weight. Then we no longer need to clamp, so I renamed the method to ApplyBlockCreateOptions. In the last commit.

Am wondering if we actually need two overlapping structs and two very similar error checking functions or can get away with one of each.

Since getblocktemplate is based on values set at node launch (MiningArgs) while IPC is dynamic per request (BlockCreateOptions) I think it makes sense to separate these. This even holds after #34568 introduces defaults for IPC clients.

But I did add helper functions for max weight, reserved weight (and sigop count, to be future proof) to address the overlap in the checking functions ReadMiningArgs and ClampOptions. I put them inline in mining_types.h, if you don’t mind, it seems ok because these limits are relevant to clients.

DrahtBot added the label Needs rebase on Feb 19, 2026

Sjors force-pushed on Feb 19, 2026

Sjors commented at 9:59 am on February 19, 2026: member

Trivial rebase after #28792.

Sjors force-pushed on Feb 19, 2026

DrahtBot added the label CI failed on Feb 19, 2026

DrahtBot removed the label Needs rebase on Feb 19, 2026

DrahtBot removed the label CI failed on Feb 19, 2026

DrahtBot added the label Needs rebase on Feb 20, 2026

test: misc interface_ipc_mining.py improvements

- clarify run_ipc_option_override_test description: https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2784515535
- trailing comma after includes: https://github.com/bitcoin/bitcoin/pull/34452#discussion_r2775183035
- include order: https://github.com/bitcoin/bitcoin/pull/34452#discussion_r2774730966

4933afe4ed

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.

13cf3bd177

move-only: add node/mining_types.h

Move mining related structs there.

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

efca496865

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.

3460c76a7f

miner: add block_max_weight to BlockCreateOptions

This new optional replaces nBlockMaxWeight.

The option is not exposed to IPC clients.

fbc2c507ac

mining: store mining args in NodeContext

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

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.

abe5ce11ef

mining: add helper for block constraint checks

The maximum block size, reserved weight and additional sigops
constraints can be provided as startup arguments and/or IPC
options. Avoid duplication of these checks by adding helpers.

Have IPC check all of these, instead of only block_reserved_weight.
This removes the need for clamping options, so ClampOptions is
renamed to ApplyBlockCreateOptions.

Currently these exceptions can only be triggered by IPC (and test)
code, because for the RPC path the minimum -blockreservedweight is
enforced during node startup.

If in the future RPC methods like getblocktemplate allow a custom
value per request, they would trigger this exception and the RPC
call would return an error - consistent with IPC behavior.

4010cf223e

Sjors force-pushed on Feb 20, 2026

Sjors commented at 3:35 pm on February 20, 2026: member

Rebased after:

#34165
- touched validation_chainstate_tests.cpp
#34568
- added DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS, which we now move)
- ipc changes, in particular createNewBlock takes a context argument
- required changes to run_ipc_option_override_test

DrahtBot removed the label Needs rebase on Feb 20, 2026

Sjors commented at 4:52 pm on February 20, 2026: member

Although it might be a slightly different issue, I’m inclined to wait for #34422, rebase and then see if Alpine behaves. See #34284 (comment).

Sjors marked this as a draft on Feb 20, 2026

DrahtBot added the label CI failed on Feb 20, 2026

refactor: disentangle miner startup defaults from runtime options #33966

Code Coverage & Benchmarks

Reviews

Conflicts