refactor: disentangle miner startup defaults from runtime options by Sjors · Pull Request #33966 · bitcoin/bitcoin

Sjors · 2025-11-28T15:42:31Z

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:

mining: fix -blockreservedweight shadows IPC option #33965

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

DrahtBot · 2025-11-28T15:42:41Z

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  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34422 (Update libmultiprocess subtree to be more stable with rust IPC client 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.

DrahtBot · 2025-11-28T16:00:17Z

🚧 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.}

Hints

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.

ryanofsky · 2025-12-01T20:52:51Z

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.

Sjors · 2025-12-02T14:14:11Z

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.

ryanofsky · 2025-12-02T17:43:01Z

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 · 2025-12-04T13:28:16Z

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.

ryanofsky

Approach ACK b466b32. 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.

ryanofsky · 2026-02-12T15:28:41Z

src/node/mining_types.h

In commit "move-only: add node/mining.h" (705fd23)

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

ryanofsky · 2026-02-12T15:37:01Z

src/node/mining_args.cpp

+
+namespace node {
+
+util::Result<void> ApplyArgsManOptions(const ArgsManager& args)


In commit "mining: parse block creation args in mining_args" (a3bced6)

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.)

ryanofsky · 2026-02-12T15:49:02Z

src/node/context.h

    //! Reference to chain client that should used to load or create wallets
    //! opened by the gui.
    std::unique_ptr<interfaces::Mining> mining;
+    MiningArgs mining_args;


In commit "mining: store mining args in NodeContext" (bfa6e3f)

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.

ryanofsky · 2026-02-12T15:55:00Z

src/test/miner_tests.cpp

    const auto block_package_feerates = BlockAssembler{
        m_node.chainman->ActiveChainstate(),
        &tx_mempool,
+        {},


In commit "mining: store mining args in NodeContext" (bfa6e3f)

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 · 2026-02-12T19:58:09Z

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.

Sjors · 2026-02-19T09:59:59Z

Trivial rebase after #28792.

- clarify run_ipc_option_override_test description: bitcoin#33965 (comment) - trailing comma after includes: bitcoin#34452 (comment) - include order: bitcoin#34452 (comment)

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.

Move mining related structs there. This simplifies includes in later commits and makes the code easier to understand for Mining IPC client developers.

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

This new optional replaces nBlockMaxWeight. The option is not exposed to IPC clients.

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.

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.

Sjors · 2026-02-20T15:35:34Z

Rebased after:

coins: don't mutate main cache when connecting block #34165
- touched validation_chainstate_tests.cpp
mining: Break compatibility with existing IPC mining clients #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

Sjors · 2026-02-20T16:52:48Z

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).

DrahtBot · 2026-02-24T12:38:29Z

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

DrahtBot changed the title ~~refactor: disentangle miner startup defaults from runtime options~~ refactor: disentangle miner startup defaults from runtime options Nov 28, 2025

DrahtBot added the Refactoring label Nov 28, 2025

Sjors mentioned this pull request Nov 28, 2025

mining: fix -blockreservedweight shadows IPC option #33965

Merged

Sjors force-pushed the 2025/11/miner-options branch from 37d9740 to e03c083 Compare November 28, 2025 15:59

DrahtBot added the CI failed label Nov 28, 2025

Sjors force-pushed the 2025/11/miner-options branch from e03c083 to 517a9b2 Compare November 28, 2025 16:21

DrahtBot removed the CI failed label Nov 28, 2025

DrahtBot added the Needs rebase label Dec 2, 2025

Sjors force-pushed the 2025/11/miner-options branch from 517a9b2 to 4b35dcc Compare December 2, 2025 14:14

Sjors force-pushed the 2025/11/miner-options branch from 4b35dcc to 5f3e67e Compare December 2, 2025 14:30

DrahtBot removed the Needs rebase label Dec 2, 2025

DrahtBot mentioned this pull request Dec 4, 2025

test: interface_ipc.py minor fixes and cleanup #34003

Merged

Sjors force-pushed the 2025/11/miner-options branch 2 times, most recently from 975c912 to e0d0a57 Compare December 4, 2025 13:28

ryanofsky approved these changes Feb 12, 2026

View reviewed changes

Sjors force-pushed the 2025/11/miner-options branch from b466b32 to c22002a Compare February 12, 2026 19:58

DrahtBot mentioned this pull request Feb 14, 2026

mining: Break compatibility with existing IPC mining clients #34568

Merged

DrahtBot added the Needs rebase label Feb 19, 2026

Sjors force-pushed the 2025/11/miner-options branch from c22002a to 9060c76 Compare February 19, 2026 09:59

Sjors force-pushed the 2025/11/miner-options branch from 9060c76 to 5a0c5e2 Compare February 19, 2026 11:12

DrahtBot added CI failed Needs rebase and removed Needs rebase CI failed labels Feb 19, 2026

Sjors added 7 commits February 20, 2026 15:03

test: misc interface_ipc_mining.py improvements

4933afe

- clarify run_ipc_option_override_test description: bitcoin#33965 (comment) - trailing comma after includes: bitcoin#34452 (comment) - include order: bitcoin#34452 (comment)

move-only: add node/mining_types.h

efca496

Move mining related structs there. This simplifies includes in later commits and makes the code easier to understand for Mining IPC client developers.

mining: parse block creation args in mining_args

3460c76

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

miner: add block_max_weight to BlockCreateOptions

fbc2c50

This new optional replaces nBlockMaxWeight. The option is not exposed to IPC clients.

Sjors force-pushed the 2025/11/miner-options branch from 5a0c5e2 to 4010cf2 Compare February 20, 2026 15:35

DrahtBot removed the Needs rebase label Feb 20, 2026

Sjors mentioned this pull request Feb 20, 2026

ipc, test: Add tests for unclean disconnect and thread busy behavior #34284

Closed

Sjors marked this pull request as draft February 20, 2026 16:52

DrahtBot added the CI failed label Feb 20, 2026

DrahtBot mentioned this pull request Feb 23, 2026

Update libmultiprocess subtree to be more stable with rust IPC client #34422

Merged

DrahtBot added the Needs rebase label Feb 24, 2026

ryanofsky mentioned this pull request Feb 25, 2026

mpgen: support primitive std::optional struct fields bitcoin-core/libmultiprocess#243

Open


		namespace node {

		util::Result<void> ApplyArgsManOptions(const ArgsManager& args)

Conversation

Sjors commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Nov 28, 2025

Uh oh!

ryanofsky commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Dec 2, 2025

Uh oh!

Sjors commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 12, 2026

Uh oh!

Sjors commented Feb 19, 2026

Uh oh!

Sjors commented Feb 20, 2026

Uh oh!

Sjors commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sjors commented Nov 28, 2025 •

edited

Loading

DrahtBot commented Nov 28, 2025 •

edited

Loading

ryanofsky commented Dec 1, 2025 •

edited

Loading

Sjors commented Dec 2, 2025 •

edited

Loading

Sjors commented Dec 4, 2025 •

edited

Loading

Sjors commented Feb 20, 2026 •

edited

Loading