refactor: disentangle miner startup defaults from runtime options #33966

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

Although this PR is primarily a refactor, there are behavior changes documented in the release note:

the IPC mining interface now rejects out-of-range block template options instead of silently clamping them;
startup now rejects -blockmaxweight values lower than -blockreservedweight, instead of allowing them to be clamped later.

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

BlockCreateOptions is used by interface clients. As before, IPC clients have access to a safe / sane subset, whereas RPC and test code can use all fields. The same type is also used to store mining defaults parsed once during node startup in NodeContext.

The maximum block weight setting (block_max_weight) is optional. When read from startup options it matches -blockmaxweight; when provided by callers it is a runtime override. Merge() fills unset fields from startup defaults while preserving caller-provided values.

This all happens in commits mining: add block create option helpers and mining: store block create options in NodeContext, 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 mining: use interface for tests, bench and fuzzers 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 structs from both the miner and node initialization code. To avoid having to pull in all of BlockAssembler for the latter, the move-only: add node/mining_types.h commit introduces node/mining_types.h and moves BlockCreateOptions, BlockWaitOptions and BlockCheckOptions there from src/node/types.h.

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.

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

Commit summary

Tests and test cleanup:

test: misc interface_ipc_mining.py improvements
test: add assert_create_fails helper
test: regression test for waitNext mining policy
test: cover IPC blockmaxweight policy

Refactoring test/bench/fuzz callers:

interfaces: make Mining use const NodeContext
mining: use interface for tests, bench and fuzzers

Moving mining interface types:

move-only: add node/mining_types.h

Separating startup defaults from runtime options:

mining: parse block creation args in mining_args: adds node/mining_args.{h,cpp} and moves mining option parsing out of init.cpp, without storing the parsed values yet.
miner: add block_max_weight to BlockCreateOptions: moves the runtime maximum block weight setting into BlockCreateOptions as an optional value, so it can later be defaulted from startup args when unset.
mining: add block create option helpers: centralizes block template option defaulting and merging, removes BlockAssembler::Options, and preserves behavior except for dropping the Specified prefix from startup option error messages.
mining: reject invalid block create options: checks typed BlockCreateOptions before block template creation, so invalid runtime options are rejected instead of silently clamped. Startup validation also rejects -blockmaxweight values lower than -blockreservedweight.
mining: store block create options in NodeContext: stores the startup mining options in NodeContext as BlockCreateOptions, so startup defaults and runtime overrides can be merged with the same option type.

Include hygiene, CI and release note:

refactor: have mining files include what they use
ci: enforce iwyu for touched files
doc: add release note for mining option validation

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
ACK	ryanofsky, w0xlt
Concept ACK	sedited
Stale ACK	enirox001

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35318 (fuzz: Bound package eval tx fanout by AgusR7)
#35182 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
#35081 (consensus: soft fork on testnet4 that fixes the min difficulty blocks exploit by batmanbytes)
#35043 (refactor: Properly return from ThreadSafeQuestion signal + btcsignals follow-ups by maflcko)
#35037 (ipc: support per-address max-connections options on -ipcbind by enirox001)
#35011 (ci, iwyu: Fix warnings in src/script and treat them as errors by BrandonOdiwuor)
#35003 (validation: improve block data I/O error handling in P2P paths by furszy)
#34995 (ci, iwyu: Fix warnings in src/common and treat them as errors by hebasto)
#34806 (refactor: logging: Various API improvements by ajtowns)
#34644 (mining: add submitBlock to IPC Mining interface by w0xlt)
#34411 ([POC] Full Libevent removal by fanquake)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
#34020 (mining: add getTransactions(ByWitnessID) IPC methods by Sjors)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)
#33324 (blocks: add -reobfuscate-blocks argument to enable (de)obfuscating existing blocks by l0rinc)
#32427 (kernel: Replace leveldb-based BlockTreeDB with flat-file based store by sedited)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)
#31117 (miner: Reorg Testnet4 minimum difficulty blocks by fjahr)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/interface_ipc_mining.py] assert template is not None / assert template_next is not None -> use assert_not_equal(template, None) and assert_not_equal(template_next, None) instead.
[test/functional/interface_ipc_mining.py] assert initial_included < NUM_TXS -> use assert_greater_than(NUM_TXS, initial_included).
[test/functional/interface_ipc_mining.py] assert len(block_next.vtx) - 1 < NUM_TXS + 1 -> use assert_greater_than(NUM_TXS + 1, len(block_next.vtx) - 1).

2026-05-18 17:12:28

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.

<details><summary>Hints</summary>

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.

</details>

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.

<details><summary>Hints</summary>

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.

</details>

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.

Sjors commented at 7:36 AM on May 14, 2026:

I added a comment to this effect.

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

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

DrahtBot added the label Needs rebase on Feb 24, 2026

Sjors force-pushed on Mar 25, 2026

Sjors commented at 4:06 PM on March 25, 2026: member

Rebased after #34422, #34727, #34661, #34184 and #33676, but I'm still ironing out a few issues.

Sjors force-pushed on Mar 25, 2026

Sjors commented at 5:19 PM on March 25, 2026: member

Needed to set /*cooldown=*/false in a few more tests.

Also, less obvious, I had to set m_node.notifications->setChainstateLoaded(true); in src/test/util/setup_common.cpp after #34661.

I'm not introducing the assert_create_fails helper in a separate commit 51ac192a3712c939b6a1cdfd62a667aa8d947055. That makes the test change in baf4954087ffcfd552c1f1d9fd6f48fc140ed14b mining: add helper for block constraint checks a little more calm.

DrahtBot removed the label Needs rebase on Mar 25, 2026

Sjors force-pushed on Mar 25, 2026

Sjors commented at 6:14 PM on March 25, 2026: member

Forgot to use the new assert_capnp_failed from #34727 in assert_create_fails.

DrahtBot removed the label CI failed on Mar 25, 2026

Sjors marked this as ready for review on Mar 25, 2026

DrahtBot added the label Needs rebase on Apr 9, 2026

Sjors force-pushed on Apr 12, 2026

Sjors commented at 11:59 PM on April 12, 2026: member

Rebased after #34858.

DrahtBot added the label CI failed on Apr 13, 2026

DrahtBot removed the label Needs rebase on Apr 13, 2026

DrahtBot removed the label CI failed on Apr 13, 2026

in src/node/miner.h:11 in e9ce8e00a2 outdated

   7 | @@ -8,7 +8,7 @@
   8 |  
   9 |  #include <interfaces/types.h>
  10 |  #include <node/types.h>
  11 | -#include <policy/policy.h>
  12 | +#include <node/mining_types.h>

sedited commented at 7:52 AM on April 27, 2026:

Nit (clang-format): Can you run clang format over the commits to order the includes?

Sjors commented at 11:56 AM on May 6, 2026:

Done, I think.

in src/node/mining_args.h:8 in 92c74acee8 outdated

   0 | @@ -0,0 +1,18 @@
   1 | +// Copyright (c) 2026 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#ifndef BITCOIN_NODE_MINING_ARGS_H
   6 | +#define BITCOIN_NODE_MINING_ARGS_H
   7 | +
   8 | +#include <util/result.h>

sedited commented at 10:19 AM on April 27, 2026:

Nit (iwyu): Can you correct the includes for this new module?

2026-04-13T00:04:55.6999286Z /home/admin/actions-runner/_work/_temp/src/node/mining_args.h should add these lines:
2026-04-13T00:04:55.6999724Z #include <cstddef>           // for size_t
2026-04-13T00:04:55.6999927Z 
2026-04-13T00:04:55.7000171Z /home/admin/actions-runner/_work/_temp/src/node/mining_args.h should remove these lines:
2026-04-13T00:04:55.7000472Z 
2026-04-13T00:04:55.7000699Z The full include-list for /home/admin/actions-runner/_work/_temp/src/node/mining_args.h:
2026-04-13T00:04:55.7001156Z #include <policy/feerate.h>  // for CFeeRate
2026-04-13T00:04:55.7001547Z #include <policy/policy.h>   // for DEFAULT_BLOCK_MAX_WEIGHT, DEFAULT_BLOCK_RESERVED_WEIGHT, DEFAULT_BLOCK_MIN_TX_FEE
2026-04-13T00:04:55.7001932Z #include <util/result.h>     // for Result
2026-04-13T00:04:55.7002196Z #include <cstddef>           // for size_t
2026-04-13T00:04:55.7002441Z class ArgsManager;  // lines 12-12
2026-04-13T00:04:55.7002684Z ---
2026-04-13T00:04:55.7002838Z 
2026-04-13T00:04:55.7003104Z (/home/admin/actions-runner/_work/_temp/src/primitives/transaction.h has correct #includes/fwd-decls)
2026-04-13T00:04:55.7003424Z 
2026-04-13T00:04:55.7003715Z (/home/admin/actions-runner/_work/_temp/src/primitives/transaction_identifier.h has correct #includes/fwd-decls)
2026-04-13T00:04:55.7004054Z 
2026-04-13T00:04:55.7004292Z (/home/admin/actions-runner/_work/_temp/src/primitives/block.h has correct #includes/fwd-decls)
2026-04-13T00:04:55.7004791Z 
2026-04-13T00:04:55.7005023Z /home/admin/actions-runner/_work/_temp/src/node/mining_args.cpp should add these lines:
2026-04-13T00:04:55.7005367Z #include <consensus/amount.h>   // for CAmount
2026-04-13T00:04:55.7005636Z #include <optional>             // for optional
2026-04-13T00:04:55.7005923Z #include <string>               // for basic_string, operator+
2026-04-13T00:04:55.7006154Z 
2026-04-13T00:04:55.7006385Z /home/admin/actions-runner/_work/_temp/src/node/mining_args.cpp should remove these lines:
2026-04-13T00:04:55.7006672Z 
2026-04-13T00:04:55.7006903Z The full include-list for /home/admin/actions-runner/_work/_temp/src/node/mining_args.cpp:
2026-04-13T00:04:55.7007235Z #include <node/mining_args.h>
2026-04-13T00:04:55.7007589Z #include <common/args.h>        // for ArgsManager
2026-04-13T00:04:55.7007873Z #include <common/messages.h>    // for AmountErrMsg
2026-04-13T00:04:55.7008145Z #include <consensus/amount.h>   // for CAmount
2026-04-13T00:04:55.7008478Z #include <node/mining_types.h>  // for CheckBlockMaxWeight, CheckBlockReservedWeight
2026-04-13T00:04:55.7008814Z #include <util/moneystr.h>      // for ParseMoney
2026-04-13T00:04:55.7009125Z #include <util/translation.h>   // for bilingual_str, Untranslated
2026-04-13T00:04:55.7009428Z #include <optional>             // for optional
2026-04-13T00:04:55.7009706Z #include <string>               // for basic_string, operator+
2026-04-13T00:04:55.7009978Z ---

Sjors commented at 9:36 AM on May 6, 2026:

@sedited why didn't the iwyu job fail?

maflcko commented at 9:48 AM on May 6, 2026:

src/node is not yet covered/enforced?

Sjors commented at 11:56 AM on May 6, 2026:

Should be better now, but have to wait for the IWYU job.

in src/node/interfaces.cpp:926 in 2ea21540fb outdated

 923 | +                                                  m_node.mempool.get(),
 924 | +                                                  m_block_template,
 925 | +                                                  options,
 926 | +                                                  m_mining_args,
 927 | +                                                  m_create_options,
 928 | +                                                  m_interrupt_wait);

sedited commented at 10:52 AM on April 27, 2026:

Nit (in commit 2ea21540fbfc4251a8982ff563120b73d2890447): This is starting to get a bit hard to read. Can you add parameter name comments, and maybe also rename them to create_options and assemble_options, since you are touching those lines anyway?

Sjors commented at 11:55 AM on May 6, 2026:

I renamed the remaining assemble_options to create_options for consistency. Also renamed options to wait_options.

in src/node/miner.h:83 in 2ea21540fb outdated

  84 | -
  85 | -    explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options);
  86 | +    explicit BlockAssembler(Chainstate& chainstate,
  87 | +                            const CTxMemPool* mempool,
  88 | +                            MiningArgs mining_args,
  89 | +                            BlockCreateOptions create_options);

sedited commented at 11:01 AM on April 27, 2026:

Just a question: This struct might be expensive to copy, if the output script is large, but the expectation is that it practically won't be?

Sjors commented at 10:07 AM on May 6, 2026:

The coinbase output is only customized by tests (not exposed to IPC clients), so this shouldn't get large.

in src/node/miner.cpp:80 in 5f4092a682 outdated

  76 | @@ -76,16 +77,26 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
  77 |      block.hashMerkleRoot = BlockMerkleRoot(block);
  78 |  }
  79 |  
  80 | -static BlockCreateOptions ClampOptions(BlockCreateOptions options)
  81 | +static BlockCreateOptions ApplyBlockCreateOptions(BlockCreateOptions options)

sedited commented at 11:20 AM on April 27, 2026:

Another question: It seems wasteful to me that we are rechecking these every time, even if in all likelihood nothing has changed in the meantime. How about introducing a CheckedBlockCreateOptions that can only be created through these checks and making the IPC code call that?

Sjors commented at 10:07 AM on May 6, 2026:

Yes it's wasteful, but probably not worth adding yet another confusing struct.

Another way to reduce the number of these calls is to call ApplyBlockCreateOptions only in the createNewBlock interface implementation. But even after 7cda5d638c6bbf3db6346ec21973258510e7b433 there's still multiple call sites for the BlockAssembler constructor, so we'd have to verify for each of these call sites whether the Apply method is necessary, and/or call it there too.

sedited commented at 11:39 AM on April 27, 2026: contributor

I have a more conceptual question here: Might this be showing us that exposing these raw option structs over IPC opens a can of worms, and we might be better off making them interfaces that are internally consistent by construction?

Sjors commented at 1:44 PM on April 27, 2026: member

@sedited I'm not sure what you mean by "internally consistent by construction".

In master @ f40da7afc03033695a5a4f1fb19e679535348c3c we have the following options exposed to IPC clients:

struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") {
    useMempool [@0](/github-metadata-backup-bitcoin-bitcoin/contributor/0/) :Bool = true $Proxy.name("use_mempool");
    blockReservedWeight [@1](/github-metadata-backup-bitcoin-bitcoin/contributor/1/) :UInt64 = .defaultBlockReservedWeight $Proxy.name("block_reserved_weight");
    coinbaseOutputMaxAdditionalSigops [@2](/github-metadata-backup-bitcoin-bitcoin/contributor/2/) :UInt64 = .defaultCoinbaseOutputMaxAdditionalSigops $Proxy.name("coinbase_output_max_additional_sigops");
}

struct BlockWaitOptions $Proxy.wrap("node::BlockWaitOptions") {
    timeout [@0](/github-metadata-backup-bitcoin-bitcoin/contributor/0/) : Float64 = .maxDouble $Proxy.name("timeout");
    feeThreshold [@1](/github-metadata-backup-bitcoin-bitcoin/contributor/1/) : Int64 = .maxMoney $Proxy.name("fee_threshold");
}

struct BlockCheckOptions $Proxy.wrap("node::BlockCheckOptions") {
    checkMerkleRoot [@0](/github-metadata-backup-bitcoin-bitcoin/contributor/0/) :Bool = true $Proxy.name("check_merkle_root");
    checkPow [@1](/github-metadata-backup-bitcoin-bitcoin/contributor/1/) :Bool = true $Proxy.name("check_pow");
}

The only ones relevant for this PR are BlockCreateOptions. Within that, useMempool probably isn't strictly needed and coinbaseOutputMaxAdditionalSigops could be initialized during a handshake. But blockReservedWeight really does need to be set per block, because it depends on the size of the coinbase, which in some pool designs is not constant.

sedited commented at 1:49 PM on April 27, 2026: contributor

@sedited I'm not sure what you mean by "internally consistent by construction".

What I mean is we don't expose them as structs, but as interfaces with getters and setters that are always made to be consistent. Then we can check any limits or inconsistencies immediately, without having to defer these runtime checks to every time the structs get used internally when creating the templates.

DrahtBot added the label Needs rebase on Apr 30, 2026

Sjors force-pushed on May 1, 2026

Sjors commented at 7:28 AM on May 1, 2026: member

Rebased after #34858 and #34669.

DrahtBot removed the label Needs rebase on May 1, 2026

sedited commented at 9:19 PM on May 2, 2026: contributor

Concept ACK

Still curious to hear your opinion here #33966 (comment) , but thought I'd post my pending review comments in the meantime.

Sjors commented at 10:09 AM on May 6, 2026: member

It might be better to merge #34860 first since it makes the diff here slightly smaller by getting rid of include_dummy_extranonce.

Sjors force-pushed on May 6, 2026

Sjors marked this as a draft on May 6, 2026

Sjors commented at 12:41 PM on May 6, 2026: member

Silent merge conflict with #33300. Rebasing.

Sjors force-pushed on May 6, 2026

DrahtBot added the label CI failed on May 6, 2026

DrahtBot removed the label CI failed on May 6, 2026

Sjors force-pushed on May 6, 2026

Sjors commented at 2:38 PM on May 6, 2026: member

I took the IWYU suggestions from all touched files, with the exception of all the boost/preprocessor and boost/test internals, which 8b920f06a980e6a65051b77d26763d460c96ea2d adds exceptions for (@maflcko does that look ok?).

Sjors force-pushed on May 6, 2026

DrahtBot added the label CI failed on May 6, 2026

DrahtBot commented at 2:53 PM on May 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task Windows-cross to x86_64, msvcrt: https://github.com/bitcoin/bitcoin/actions/runs/25441994126/job/74635553888 LLM reason (✨ experimental): CI failed due to a C++ build compilation error in the fuzz target (package_eval.cpp): MineBlock/COutPoint constructor call signature mismatch (no matching function).

<details><summary>Hints</summary>

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.

</details>

Sjors force-pushed on May 6, 2026

Sjors commented at 4:04 PM on May 6, 2026: member

Trying to please both IWYU and the per-commit check wasn't the brightest idea...

DrahtBot removed the label CI failed on May 6, 2026

Sjors commented at 7:27 PM on May 6, 2026: member

But I might as well enforce IWYU now on the touched files. This nit #33966 (review) got a little out of hand :-)

Sjors force-pushed on May 6, 2026

Sjors marked this as ready for review on May 6, 2026

Sjors marked this as a draft on May 6, 2026

Sjors force-pushed on May 6, 2026

DrahtBot added the label CI failed on May 6, 2026

DrahtBot commented at 8:01 PM on May 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/25456449411/job/74687136598 LLM reason (✨ experimental): CI failed due to a C++ build error in the fuzz target (package_eval.cpp) where MineBlock(...) is called with a mismatched argument type (initializer list can’t convert to node::BlockCreateOptions).

<details><summary>Hints</summary>

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.

</details>

Sjors force-pushed on May 6, 2026

Sjors marked this as ready for review on May 6, 2026

DrahtBot removed the label CI failed on May 6, 2026

in ci/test/03_test_script.sh:212 in 54c0d3b8d6

 208 | @@ -209,7 +209,7 @@ fi
 209 |  
 210 |  if [[ "${RUN_IWYU}" == true ]]; then
 211 |    # TODO: Consider enforcing IWYU across the entire codebase.
 212 | -  FILES_WITH_ENFORCED_IWYU="/src/(((crypto|index|kernel|primitives|univalue/(lib|test)|util|zmq)/.*|common/license_info|node/blockstorage|node/utxo_snapshot|clientversion|core_io|signet)\\.cpp)"
 213 | +  FILES_WITH_ENFORCED_IWYU="/src/(((crypto|index|kernel|primitives|univalue/(lib|test)|util|zmq)/.*|bench/(block_assemble|connectblock)|common/license_info|node/(blockstorage|interfaces|miner|mining_args|utxo_snapshot)|rpc/mining|clientversion|core_io|signet|test/(blockfilter_index_tests|coinstatsindex_tests|miner_tests|peerman_tests|testnet4_miner_tests|validation_block_tests|validation_chainstate_tests)|test/fuzz/(cmpctblock|mini_miner|package_eval|process_message|process_messages|tx_pool|utxo_total_supply)|test/util/(mining|setup_common))\\.cpp)"

hebasto commented at 8:31 AM on May 7, 2026:

I suggest to postpone enforcing IWYU warnings for test/* subdirectories until later and do this in a dedicated PR.

in contrib/devtools/iwyu/bitcoin.core.imp:29 in 54c0d3b8d6

  24 | +  { "include": [ "<boost/test/tools/assertion_result.hpp>", "private", "<boost/test/unit_test.hpp>", "public" ] },
  25 | +  { "include": [ "<boost/test/tools/old/interface.hpp>", "private", "<boost/test/unit_test.hpp>", "public" ] },
  26 | +  { "include": [ "<boost/test/tree/auto_registration.hpp>", "private", "<boost/test/unit_test.hpp>", "public" ] },
  27 | +  { "include": [ "<boost/test/unit_test_suite.hpp>", "private", "<boost/test/unit_test.hpp>", "public" ] },
  28 | +  { "include": [ "<boost/test/utils/basic_cstring/basic_cstring.hpp>", "private", "<boost/test/unit_test.hpp>", "public" ] },
  29 | +  { "include": [ "<boost/test/utils/lazy_ostream.hpp>", "private", "<boost/test/unit_test.hpp>", "public" ] },

hebasto commented at 8:33 AM on May 7, 2026:

Perhaps, boost-all.imp could be used instead.

Sjors commented at 8:55 AM on May 7, 2026:

Thanks, I'll drop this commit and remove enforcement in test.

Sjors commented at 9:12 AM on May 7, 2026:

@hebasto I tested that file locally, but it doesn't seem to cover everything we use.

My LLM generated this much smaller patch and it seems to work:

diff --git a/contrib/devtools/iwyu/bitcoin.core.imp b/contrib/devtools/iwyu/bitcoin.core.imp
index 9b471cb82f..fa4e5ecd98 100644
--- a/contrib/devtools/iwyu/bitcoin.core.imp
+++ b/contrib/devtools/iwyu/bitcoin.core.imp
@@ -5,4 +5,9 @@
   { "include": [ "<mmintrin.h>", "private", "<immintrin.h>", "public" ] },
   { "include": [ "<smmintrin.h>", "private", "<immintrin.h>", "public" ] },
   { "include": [ "<tmmintrin.h>", "private", "<immintrin.h>", "public" ] },
+
+  # Boost.Test exposes its API through <boost/test/unit_test.hpp>, but IWYU
+  # otherwise suggests internal Boost.Test and Boost.Preprocessor headers.
+  { "include": [ "@<boost/test/.*>", "private", "<boost/test/unit_test.hpp>", "public" ] },
+  { "include": [ "@<boost/preprocessor/.*>", "private", "<boost/test/unit_test.hpp>", "public" ] },
 ]

(in any case, this is better left for a followup)

Sjors force-pushed on May 7, 2026

Sjors commented at 9:02 AM on May 7, 2026: member

I removed enforcement for tests, fuzz and util and dropped the commit that changes bitcoin.core.imp. As @hebasto points out, this is better left to a followup, e.g. using #33966 (review).

I kept the include changes.

DrahtBot added the label CI failed on May 7, 2026

DrahtBot removed the label CI failed on May 7, 2026

in src/node/interfaces.cpp:924 in df8190119d outdated

 921 | +        auto new_template = WaitAndCreateNewBlock(chainman(),
 922 | +                                                  notifications(),
 923 | +                                                  m_node.mempool.get(),
 924 | +                                                  m_block_template,
 925 | +                                                  /*wait_options=*/options,
 926 | +                                                  /*mining_args=*/m_mining_args,

enirox001 commented at 11:29 AM on May 12, 2026:

In commit "mining: store mining args in NodeContext" (https://github.com/bitcoin/bitcoin/pull/33966/changes/df8190119ddcb2884f0928562e76acbd792a9dc2):

I think this commit accidentally makes waitNext() use default MiningArgs.

createNewBlock() passes context()->mining_args to BlockAssembler, so the first template respects settings like -blockmintxfee. But waitNext() now passes BlockTemplateImpl::m_mining_args, and that member is never initialized in the constructor, so it stays default constructed.

I think this means IPC mining clients can get the correct policy on the first template, then switch back to default mining policy after waitNext() refreshes the template.

I verified it locally with the following functional test. I believe it fails on this commit because the below -blockmintxfee tx appears in the template returned by `waitNext()

index 26c13320a9..bbcabeb2e5 100755
--- a/test/functional/interface_ipc_mining.py
+++ b/test/functional/interface_ipc_mining.py
@@ -7,6 +7,7 @@ import asyncio
 import re
 import time
 from contextlib import AsyncExitStack
+from decimal import Decimal
 from io import BytesIO
 from test_framework.blocktools import NULL_OUTPOINT
 from test_framework.messages import (
@@ -369,6 +370,54 @@ class IPCMiningTest(BitcoinTestFramework):
         asyncio.run(capnp.run(async_routine_check_max_reserved_weight()))
         asyncio.run(capnp.run(async_routine_check_sigops_limit()))

+    def run_waitnext_mining_policy_test(self):
+        self.log.info("Running waitNext mining policy test")
+        block_min_tx_fee = Decimal("0.00002000")
+        below_block_min_tx_fee = Decimal("0.00001000")
+        above_block_min_tx_fee = Decimal("0.00003000")
+
+        self.restart_node(0, extra_args=[
+            f"-blockmintxfee={block_min_tx_fee:.8f}",
+            "-minrelaytxfee=0",
+            "-persistmempool=0",
+        ])
+
+        async def async_routine():
+            ctx, mining = await make_mining_ctx(self)
+
+            self.log.debug("Create a below -blockmintxfee transaction")
+            low_fee_tx = self.miniwallet.send_self_transfer(
+                fee_rate=below_block_min_tx_fee,
+                from_node=self.nodes[0],
+                confirmed_only=True,
+            )
+
+            async with AsyncExitStack() as stack:
+                self.log.debug("createNewBlock should respect -blockmintxfee")
+                template = await mining_create_block_template(mining, stack, ctx, self.default_block_create_options)
+                assert template is not None
+                block = await mining_get_block(template, ctx)
+                assert low_fee_tx["txid"] not in {tx.txid_hex for tx in block.vtx[1:]}
+
+                self.log.debug("waitNext should preserve the same mining policy")
+                high_fee_tx = self.miniwallet.send_self_transfer(
+                    fee_rate=above_block_min_tx_fee,
+                    from_node=self.nodes[0],
+                    confirmed_only=True,
+                )
+                waitoptions = self.capnp_modules['mining'].BlockWaitOptions()
+                waitoptions.timeout = 1000.0 * self.options.timeout_factor
+                waitoptions.feeThreshold = 1
+                template_next = await mining_wait_next_template(template, stack, ctx, waitoptions)
+                assert template_next is not None
+
+                block_next = await mining_get_block(template_next, ctx)
+                block_next_txids = {tx.txid_hex for tx in block_next.vtx[1:]}
+                assert high_fee_tx["txid"] in block_next_txids
+                assert low_fee_tx["txid"] not in block_next_txids
+
+        asyncio.run(capnp.run(async_routine()))
+
     def run_coinbase_and_submission_test(self):
         """Test coinbase construction (getCoinbaseTx) and block submission (submitSolution)."""
         self.log.info("Running coinbase construction and submission test")
@@ -454,6 +503,7 @@ class IPCMiningTest(BitcoinTestFramework):
         self.run_early_startup_test()
         self.run_block_template_test()
         self.run_coinbase_and_submission_test()
+        self.run_waitnext_mining_policy_test()
         self.run_ipc_option_override_test()

Sjors commented at 2:03 PM on May 12, 2026:

Great catch. It's fixed now and I added your test.

I'm still investigating how this regression crept in. In particular, if it was there from the first time I opened this PR.

Sjors commented at 2:19 PM on May 12, 2026:

It looks like this PR introduced this regression from the get go.

I went for a more rigorous fix, namely dropping m_mining_args.

enirox001 commented at 12:13 PM on May 12, 2026: contributor

Concept ACK.

IIUC this PR moves mining template options and args into shared node types/state, with NodeContext holding the configured mining policy. I like this approach because it should make the mining RPC/IPC paths easier to keep consistent and could make follow-up cleanups and refactor easier #34644 (review)

Also, the PR description still mentions

To avoid having to pull in all of BlockAssembler for the latter, the second commit introduces node/mining.h

I think this should be changed to node/mining_types.h

Sjors commented at 1:40 PM on May 12, 2026: member

@enirox001 thanks, I updatd the PR description to point to node/mining_types.h (in the fourth commit)

Also pushed fix and test coverage for #33966 (review)

Sjors force-pushed on May 12, 2026

DrahtBot added the label CI failed on May 12, 2026

DrahtBot removed the label CI failed on May 12, 2026

in src/node/miner.cpp:405 in a059dc1d84 outdated

 402 | @@ -373,7 +403,7 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
 403 |      // Alternate waiting for a new tip and checking if fees have risen.
 404 |      // The latter check is expensive so we only run it once per second.
 405 |      auto now{NodeClock::now()};

w0xlt commented at 11:48 PM on May 12, 2026:

WaitAndCreateNewBlock() can use SteadyClock for timeout/deadline handling, matching the pattern already used by WaitTipChanged().

diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index 7a6a104e48..248c218dca 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -402,8 +402,12 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
 
     // Alternate waiting for a new tip and checking if fees have risen.
     // The latter check is expensive so we only run it once per second.
-    auto now{NodeClock::now()};
-    const auto deadline = now + wait_options.timeout;
+    auto timeout{wait_options.timeout};
+    Assume(timeout >= 0ms); // No internal callers should use a negative timeout.
+    if (timeout < 0ms) timeout = 0ms;
+    if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono.
+    auto now{SteadyClock::now()};
+    const auto deadline = now + timeout;
     const MillisecondsDouble tick{1000};
     const bool allow_min_difficulty{chainman.GetParams().GetConsensus().fPowAllowMinDifficultyBlocks};
 
@@ -437,7 +441,7 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
         // On test networks return a minimum difficulty block after 20 minutes
         if (!tip_changed && allow_min_difficulty) {
             const NodeClock::time_point tip_time{std::chrono::seconds{chainman.ActiveChain().Tip()->GetBlockTime()}};
-            if (now > tip_time + 20min) {
+            if (NodeClock::now() > tip_time + 20min) {
                 tip_changed = true;
             }
         }
@@ -472,7 +476,7 @@ std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainma
             if (new_fees >= current_fees + wait_options.fee_threshold) return new_tmpl;
         }
 
-        now = NodeClock::now();
+        now = SteadyClock::now();
     } while (now < deadline);
 
     return nullptr;

Sjors commented at 6:53 AM on May 13, 2026:

@w0xlt that seems like an unrelated refactor?

w0xlt commented at 5:27 PM on May 13, 2026:

Yes, makes sense. It can be done in a new PR.

in src/node/mining_args.h:24 in a059dc1d84

  19 | +
  20 | +/**
  21 | + * Block template creation defaults and limits configured for the node.
  22 | + */
  23 | +struct MiningArgs {
  24 | +    CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};

w0xlt commented at 5:26 PM on May 13, 2026:

nit:

    CFeeRate block_min_fee_rate{DEFAULT_BLOCK_MIN_TX_FEE};

Sjors commented at 5:49 PM on May 13, 2026:

Taken, will push after I've studied your patch below.

w0xlt commented at 5:45 PM on May 13, 2026: contributor

With the patch below, IPC public header stays free of formatting/translation deps. That removes dependencies like consensus/consensus.h, tinyformat.h, util/result.h, and util/translation.h from mining_types.h.

diff --git a/src/node/mining_args.cpp b/src/node/mining_args.cpp
index bcf4f9f429..bfa6a9e115 100644
--- a/src/node/mining_args.cpp
+++ b/src/node/mining_args.cpp
@@ -7,7 +7,8 @@
 #include <common/args.h>
 #include <common/messages.h>
 #include <consensus/amount.h>
-#include <node/mining_types.h>
+#include <consensus/consensus.h>
+#include <tinyformat.h>
 #include <util/moneystr.h>
 #include <util/translation.h>
 
@@ -18,6 +19,37 @@ using common::AmountErrMsg;
 
 namespace node {
 
+util::Result<void> CheckBlockMaxWeight(size_t block_max_weight)
+{
+    if (block_max_weight > MAX_BLOCK_WEIGHT) {
+        return util::Error{Untranslated(strprintf("(%zu) exceeds consensus maximum block weight (%u)",
+                                                  block_max_weight, MAX_BLOCK_WEIGHT))};
+    }
+    return {};
+}
+
+util::Result<void> CheckBlockReservedWeight(size_t block_reserved_weight)
+{
+    if (block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
+        return util::Error{Untranslated(strprintf("(%zu) is lower than minimum safety value of (%u)",
+                                                  block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT))};
+    }
+    if (block_reserved_weight > MAX_BLOCK_WEIGHT) {
+        return util::Error{Untranslated(strprintf("(%zu) exceeds consensus maximum block weight (%u)",
+                                                  block_reserved_weight, MAX_BLOCK_WEIGHT))};
+    }
+    return {};
+}
+
+util::Result<void> CheckCoinbaseOutputMaxAdditionalSigops(size_t sigops)
+{
+    if (sigops > MAX_BLOCK_SIGOPS_COST) {
+        return util::Error{Untranslated(strprintf("(%zu) exceeds consensus maximum block sigops cost (%d)",
+                                                  sigops, MAX_BLOCK_SIGOPS_COST))};
+    }
+    return {};
+}
+
 util::Result<void> ReadMiningArgs(const ArgsManager& args, MiningArgs& mining_args)
 {
     if (const auto arg{args.GetArg("-blockmintxfee")}) {
diff --git a/src/node/mining_args.h b/src/node/mining_args.h
index 0522ec6b93..f71272f325 100644
--- a/src/node/mining_args.h
+++ b/src/node/mining_args.h
@@ -43,6 +43,15 @@ struct MiningArgs {
  */
 [[nodiscard]] util::Result<void> ReadMiningArgs(const ArgsManager& args, MiningArgs& mining_args);
 
+/** Check that block_max_weight does not exceed consensus limits. */
+[[nodiscard]] util::Result<void> CheckBlockMaxWeight(size_t block_max_weight);
+
+/** Check that block_reserved_weight is within allowed bounds. */
+[[nodiscard]] util::Result<void> CheckBlockReservedWeight(size_t block_reserved_weight);
+
+/** Check that coinbase_output_max_additional_sigops does not exceed consensus limits. */
+[[nodiscard]] util::Result<void> CheckCoinbaseOutputMaxAdditionalSigops(size_t sigops);
+
 } // namespace node
 
 #endif // BITCOIN_NODE_MINING_ARGS_H
diff --git a/src/node/mining_types.h b/src/node/mining_types.h
index dbff077eee..84d1995333 100644
--- a/src/node/mining_types.h
+++ b/src/node/mining_types.h
@@ -12,15 +12,11 @@
 #define BITCOIN_NODE_MINING_TYPES_H
 
 #include <consensus/amount.h>
-#include <consensus/consensus.h>
 #include <policy/policy.h>
 #include <primitives/transaction.h>
 #include <script/script.h>
-#include <tinyformat.h>
 #include <uint256.h>
-#include <util/result.h>
 #include <util/time.h>
-#include <util/translation.h>
 
 #include <cstddef>
 #include <optional>
@@ -166,40 +162,6 @@ struct CoinbaseTx {
     uint32_t lock_time;
 };
 
-/** Check that block_max_weight does not exceed consensus limits. */
-[[nodiscard]] inline util::Result<void> CheckBlockMaxWeight(size_t block_max_weight)
-{
-    if (block_max_weight > MAX_BLOCK_WEIGHT) {
-        return util::Error{Untranslated(strprintf("(%zu) exceeds consensus maximum block weight (%u)",
-                                                  block_max_weight, MAX_BLOCK_WEIGHT))};
-    }
-    return {};
-}
-
-/** Check that block_reserved_weight is within allowed bounds. */
-[[nodiscard]] inline util::Result<void> CheckBlockReservedWeight(size_t block_reserved_weight)
-{
-    if (block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
-        return util::Error{Untranslated(strprintf("(%zu) is lower than minimum safety value of (%u)",
-                                                  block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT))};
-    }
-    if (block_reserved_weight > MAX_BLOCK_WEIGHT) {
-        return util::Error{Untranslated(strprintf("(%zu) exceeds consensus maximum block weight (%u)",
-                                                  block_reserved_weight, MAX_BLOCK_WEIGHT))};
-    }
-    return {};
-}
-
-/** Check that coinbase_output_max_additional_sigops does not exceed consensus limits. */
-[[nodiscard]] inline util::Result<void> CheckCoinbaseOutputMaxAdditionalSigops(size_t sigops)
-{
-    if (sigops > MAX_BLOCK_SIGOPS_COST) {
-        return util::Error{Untranslated(strprintf("(%zu) exceeds consensus maximum block sigops cost (%d)",
-                                                  sigops, MAX_BLOCK_SIGOPS_COST))};
-    }
-    return {};
-}
-
 } // namespace node
 
 #endif // BITCOIN_NODE_MINING_TYPES_H

</details>

w0xlt commented at 8:13 PM on May 13, 2026: contributor

The test below adds two validations: the first half verifies that -blockmaxweight is honored on a freshly created IPC template. The second half checks that the cap survives across waitNext-triggered template refreshes.

diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py
index 23fb6dbbb8..074464bed7 100755
--- a/test/functional/interface_ipc_mining.py
+++ b/test/functional/interface_ipc_mining.py
@@ -17,6 +17,7 @@ from test_framework.messages import (
     CTxOut,
     CTxInWitness,
     COIN,
+    DEFAULT_BLOCK_RESERVED_WEIGHT,
     MAX_BLOCK_SIGOPS_COST,
     MAX_BLOCK_WEIGHT,
     from_hex,
@@ -420,6 +421,67 @@ class IPCMiningTest(BitcoinTestFramework):
 
         asyncio.run(capnp.run(async_routine()))
 
+    def run_block_max_weight_test(self):
+        """Verify that -blockmaxweight is honored on the IPC createNewBlock() path.
+        The startup arg flows MiningArgs.default_block_max_weight ->
+        ApplyMiningDefaults() -> BlockCreateOptions.block_max_weight, and
+        BlockAssembler enforces the cap."""
+        self.log.info("Running block_max_weight test")
+
+        # Cap that leaves room for only a handful of mempool transactions
+        # above DEFAULT_BLOCK_RESERVED_WEIGHT (8000). Well below MAX_BLOCK_WEIGHT
+        # (4_000_000), so any truncation observed here is attributable to the
+        # cap, not to consensus limits or wallet chain limits.
+        small_cap = DEFAULT_BLOCK_RESERVED_WEIGHT + 4000
+        NUM_TXS = 20
+
+        self.restart_node(0, extra_args=[
+            f"-blockmaxweight={small_cap}",
+            "-minrelaytxfee=0",
+            "-persistmempool=0",
+        ])
+        # Refresh miniwallet's UTXO view from the chain after restart.
+        self.miniwallet.rescan_utxos()
+
+        for _ in range(NUM_TXS):
+            self.miniwallet.send_self_transfer(from_node=self.nodes[0], confirmed_only=True)
+        assert_equal(self.nodes[0].getmempoolinfo()["size"], NUM_TXS)
+
+        async def async_routine():
+            ctx, mining = await make_mining_ctx(self)
+            async with AsyncExitStack() as stack:
+                template = await mining_create_block_template(mining, stack, ctx, self.default_block_create_options)
+                assert template is not None
+                block = await mining_get_block(template, ctx)
+                assert_greater_than_or_equal(small_cap, block.get_weight())
+                # Exclude the coinbase; the cap must have forced truncation.
+                initial_included = len(block.vtx) - 1
+                assert initial_included < NUM_TXS, (
+                    f"Expected -blockmaxweight={small_cap} to truncate; "
+                    f"included {initial_included}/{NUM_TXS} mempool txs"
+                )
+
+                self.log.debug("waitNext should preserve -blockmaxweight")
+                high_fee_tx = self.miniwallet.send_self_transfer(
+                    from_node=self.nodes[0],
+                    confirmed_only=True,
+                    fee_rate=10,
+                )
+                waitoptions = self.capnp_modules['mining'].BlockWaitOptions()
+                waitoptions.timeout = 1000.0 * self.options.timeout_factor
+                waitoptions.feeThreshold = 1
+                template_next = await mining_wait_next_template(template, stack, ctx, waitoptions)
+                assert template_next is not None
+
+                block_next = await mining_get_block(template_next, ctx)
+                assert_greater_than_or_equal(small_cap, block_next.get_weight())
+                assert high_fee_tx["txid"] in {tx.txid_hex for tx in block_next.vtx[1:]}
+                # If waitNext() lost the capped create options, it would be able
+                # to include the whole small mempool under the default 4M limit.
+                assert len(block_next.vtx) - 1 < NUM_TXS + 1
+
+        asyncio.run(capnp.run(async_routine()))
+
     def run_coinbase_and_submission_test(self):
         """Test coinbase construction (getCoinbaseTx) and block submission (submitSolution)."""
         self.log.info("Running coinbase construction and submission test")
@@ -506,6 +568,7 @@ class IPCMiningTest(BitcoinTestFramework):
         self.run_block_template_test()
         self.run_coinbase_and_submission_test()
         self.run_waitnext_mining_policy_test()
+        self.run_block_max_weight_test()
         self.run_ipc_option_override_test()

</details>

w0xlt commented at 8:14 PM on May 13, 2026: contributor

ACK a059dc1d8405eba15ff4f92bd1ac6b3d79d6b063 with above suggestions. Happy to reACK if taken.

DrahtBot requested review from ryanofsky on May 13, 2026

DrahtBot requested review from sedited on May 13, 2026

DrahtBot requested review from enirox001 on May 13, 2026

in test/functional/interface_ipc_mining.py:122 in 5829fb539d

 117 | +                # The remote exception isn't caught currently and leads to a
 118 | +                # std::terminate call. In that case, verify the expected message
 119 | +                # via bitcoind stderr before restarting.
 120 | +                # This bug is fixed with
 121 | +                # https://github.com/bitcoin-core/libmultiprocess/pull/218
 122 | +                assert_equal(e.description, "Peer disconnected.")

ryanofsky commented at 11:10 PM on May 13, 2026:

In commit "test: add assert_create_fails helper" (5829fb539d140281d9f14496aecc1be84fabd768)

I think after bitcoin-core/libmultiprocess#240 / 8fe91f37194edcca1b7dfdd06bd0d4f5b2154e9b the node should no longer crash when it throws an uncaught exception, and the previous comments about needing bitcoin-core/libmultiprocess#218 to prevent this were not accurate since bitcoin-core/libmultiprocess#240 is sufficient.

So you can probably delete the if branch here and just keep the else code

Sjors commented at 7:00 AM on May 14, 2026:

That seems to behave; taken.

in src/node/mining_args.cpp:31 in 311b15901e

  26 | @@ -25,17 +27,14 @@ util::Result<void> ReadMiningArgs(const ArgsManager& args, MiningArgs& mining_ar
  27 |      }
  28 |  
  29 |      const size_t max_block_weight = args.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT);
  30 | -    if (max_block_weight > MAX_BLOCK_WEIGHT) {
  31 | -        return util::Error{strprintf(_("Specified -blockmaxweight (%d) exceeds consensus maximum block weight (%d)"), max_block_weight, MAX_BLOCK_WEIGHT)};
  32 | +    if (auto result{CheckBlockMaxWeight(max_block_weight)}; !result) {
  33 | +        return util::Error{Untranslated("Specified -blockmaxweight " + util::ErrorString(result).original)};

ryanofsky commented at 11:34 PM on May 13, 2026:

In commit "mining: add helper for block constraint checks" (311b15901e7e831a43599d6d1cc471b57ebcc8eb)

Would suggest passing an optional extra parameter to Check functions so they can be responsible for generating error strings, and callers do not need to awkwardly concatenate string values. E.g.

util::Result<void> CheckBlockMaxWeight(size_t block_max_weight, const std::string& arg_name = "")

where CheckBlockMaxWeight(max_block_weight, "-blockmaxweight") returns "Specified -blockmaxweight..." errors and CheckBlockMaxWeight(max_block_weight) returns "block_max_weight..." errors.

This would make calling code simpler and also make it straightforward to support string translation later if desired.

Sjors commented at 7:26 AM on May 14, 2026:

Done, but I also simplified it by dropping "Specified".

ryanofsky approved

ryanofsky commented at 12:13 AM on May 14, 2026: contributor

Light code review ACK a059dc1d8405eba15ff4f92bd1ac6b3d79d6b063, and I plan to review more. The changes are nice and get rid of a lot of awkwardness. Some high level comments:

I think it would be good to qualify the word "refactor" in the title and be clear about behaviors that are changing in the PR description, like previous clamping behavior now being replaced by exceptions , and anything else that might be changing. It would also be good to distinguish which commits change behavior since there a lot of commits, and it might be good to add release notes.
I think my previous comment #33966#pullrequestreview-3791721113 still applies, but it's not too important. I don't think it's good to have separate MiningArgs and BlockCreateOptions structs. It seems like it would be better to drop MiningArgs and use BlockCreateOptions everywhere, and have Check/Flatten/Merge functions that can operate on it, where Check checks for valid option values and returns errors, Flatten replaces null optional values with defaults, and Merge(x, y) merges values from two structs, replacing null values in x with nonnull values from y. I think it would probably simplify the PR to implement this, but not necessary, and it could also be done as a followup.
Previous comment #33966 (review) still applies too, but not very important. Node context struct is only meant to contain pointers and not depend on headers from other parts of the code (otherwise it risks becoming a huge struct that depends on everything in the codebase). But #33421 seems close to being ready, and mining args could be moved into the BlockTemplateCache it introduces solving this issue

Sjors force-pushed on May 14, 2026

Sjors commented at 7:40 AM on May 14, 2026: member

The new push adds test coverage, and applies review feedback, except:

second bullet point in #33966#pullrequestreview-4285956996 (for now I think it's better as a followup)

I improved the PR description a bit, mainly adding a summary of the various kinds of commits at the bottom. I also mention at the top the only (afaik) actual behavior change.

DrahtBot added the label Needs rebase on May 14, 2026

Sjors force-pushed on May 14, 2026

DrahtBot removed the label Needs rebase on May 14, 2026

Sjors commented at 4:42 PM on May 14, 2026: member

Another rebase, after include conflict with #35284.

in src/node/mining_args.cpp:68 in 742e0d7cec

  63 | +        std::optional<CAmount> block_min_tx_fee{ParseMoney(*arg)};
  64 | +        if (!block_min_tx_fee) return util::Error{AmountErrMsg("blockmintxfee", *arg)};
  65 | +        mining_args.block_min_fee_rate = CFeeRate{*block_min_tx_fee};
  66 | +    }
  67 | +
  68 | +    const size_t max_block_weight = args.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT);

w0xlt commented at 6:41 PM on May 14, 2026:

nit: Can this cause issues on 32-bit systems if the -blockmaxweight is greater than std::numeric_limits<uint32_t>::max()? GetIntArg() returns an int64_t, but assigning it to size_t before a validation could wrap the value.

Sjors commented at 8:04 AM on May 15, 2026:

Yes. Previously we assigned it to const auto before doing the size comparison.

But fixing this gets ugly fast, and mining gigabyte blocks on a 32-bit system is not a good idea :-) So I'm not sure if it's worth it.

A thorough fix would be to have GetIntArg take a minimum and maximum argument. That would be a whole different PR though, here's a sketch (up for grabs): 5d349106f6cf325f5253a9d534816ae39232e628

ryanofsky commented at 10:47 PM on May 17, 2026:

re: #33966 (review)

Would GetArg<size_t> help here? Template method was added in https://github.com/bitcoin/bitcoin/pull/34582

Sjors commented at 9:56 AM on May 18, 2026:

Yes, that's quite nice.

w0xlt commented at 6:42 PM on May 14, 2026: contributor

ACK 742e0d7cec0d3342a1ddbf434f0cef647ec5f271

DrahtBot requested review from ryanofsky on May 14, 2026

DrahtBot added the label Needs rebase on May 17, 2026

in src/test/util/mining.cpp:136 in 8675767baa

 136 |  {
 137 | -    auto block = std::make_shared<CBlock>(
 138 | -        BlockAssembler{Assert(node.chainman)->ActiveChainstate(), Assert(node.mempool.get()), assembler_options}
 139 | -            .CreateNewBlock()
 140 | -            ->block);
 141 | +    auto mining = interfaces::MakeMining(const_cast<NodeContext&>(node));

ryanofsky commented at 10:29 PM on May 17, 2026:

In commit "mining: use interface for tests, bench and fuzzers" (8675767baa362fc19ac12bb65cfbf08b729099a9)

Can there be a followup to remove the const-cast? It seems MakeMining, and the mining class should be actually using a const-reference not a mutable one because they shouldn't be changing the NodeContext struct.

Sjors commented at 11:26 AM on May 18, 2026:

Added a commit to do that.

ryanofsky approved

ryanofsky commented at 11:00 PM on May 17, 2026: contributor

Light code review ACK 742e0d7cec0d3342a1ddbf434f0cef647ec5f271. There are parts of this I would still like to review in more detail but the changes all seem good, and thanks for implementing the previous suggestions.

re: #33966 (comment)

second bullet point in refactor: disentangle miner startup defaults from runtime options [#33966](/github-metadata-backup-bitcoin-bitcoin/33966/) (review) (for now I think it's better as a followup)

I gave the suggestion to an llm, and it produced 1f91dd2ad8cfe105ee12c42ce9a559326fac762a, which I think shows how it is a simplification that would make the PR a little smaller, and make it more straightforward to add options in the future. It's not very important though, and could be a followup

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
- reuse the shared miniwallet in the low block height test: https://github.com/bitcoin/bitcoin/pull/34860#discussion_r3255163123

63ac212285

test: add assert_create_fails helper 3864344620

Sjors commented at 11:29 AM on May 18, 2026: member

Rebased after #34860 which removed .include_dummy_extranonce.

Included the suggestion from #34860 (review) in the initial misc test improvements commit. Also #34860 (review).

I switched to using GetArg<uint64_t> which #34582 made possible, see #33966 (review).

Sjors force-pushed on May 18, 2026

in src/node/types.h:131 in 08f5887a9b outdated

 117 | -     * Prefix which needs to be placed at the beginning of the scriptSig.
 118 | -     * Clients may append extra data to this as long as the overall scriptSig
 119 | -     * size is 100 bytes or less, to avoid the block being rejected with
 120 | -     * "bad-cb-length" error. At heights <= 16 the BIP 34 height push is only
 121 | -     * one byte long, so clients must append at least one additional byte to
 122 | -     * meet the consensus minimum scriptSig length of two bytes.

ryanofsky commented at 12:47 PM on May 18, 2026:

In commit "move-only: add node/mining_types.h" (08f5887a9b7b22fb386e4fcf5ab938f2c6f1db0a)

The new "At heights <= 16" comment is being dropped by this commit, looks like a bad conflict resolution

Sjors commented at 3:26 PM on May 18, 2026:

Thanks, will be fixed in the next push.

in src/node/mining_args.cpp:30 in 03dad459e2 outdated

  25 | +        if (!ParseMoney(*arg)) {
  26 | +            return util::Error{AmountErrMsg("blockmintxfee", *arg)};
  27 | +        }
  28 | +    }
  29 | +
  30 | +    const uint64_t max_block_weight{args.GetArg<uint64_t>("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT)};

ryanofsky commented at 1:04 PM on May 18, 2026:

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

Note: It looks like the changes to moved code here don't affect behavior. Changing GetIntArg (which returned in64_t) to GetArg<uint64_t> doesn't change anything because the latter clamps negative values to 0, and the MAX_BLOCK_WEIGHT / MINIMUM_BLOCK_RESERVED_WEIGHT are 32 bit unsigned int so the results of the comparisons are the same.

I believe %s also behaves identically to %d in tinyformat.

Might be good to mention the changes in the commit message so it's clear they are intentional, but they seem ok.

Sjors commented at 3:43 PM on May 18, 2026:

I'm switching %s back to %d as it was unneeded churn. I'm also getting rid of util::ToString in a later commit, which seems like leftover from when we had a more elaborate helper (that tried to preserve the exact original error message).

in src/node/mining_types.h:76 in 1dd417e223 outdated

  71 | +     * Maximum block weight, defaults to -maxblockweight
  72 | +     *
  73 | +     * block_reserved_weight can safely exceed block_max_weight, but the rest of
  74 | +     * the block template will be empty.
  75 | +     */
  76 | +    std::optional<size_t> block_max_weight{};

ryanofsky commented at 1:09 PM on May 18, 2026:

In commit "miner: add block_max_weight to BlockCreateOptions" (1dd417e22308b40bdfea6cff1a8ef64a907c0a1b)

Might be good to migrate away from size_t towards uint64_t for these fields since uint64_t is what the BlockAssembler::nBlockWeight field this is compared against uses. Also the related block_reserved_weight field is size_t in c++ but UInt64 in capnproto and that discrepancy could be good to fix, maybe in a followup

Sjors commented at 3:56 PM on May 18, 2026:

Done both.

Sjors force-pushed on May 18, 2026

Sjors commented at 1:15 PM on May 18, 2026: member

@ryanofsky I implemented your suggestion: #33966#pullrequestreview-4306390355

I also moved all the IWYU header churn to a single commit, as it was getting too noisy.

I grouped all four test commits toghether at the start of the PR.

Sjors force-pushed on May 18, 2026

DrahtBot added the label CI failed on May 18, 2026

DrahtBot commented at 1:25 PM on May 18, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/26035851181/job/76533659843 LLM reason (✨ experimental): CI failed due to the lint check rpc_assert detecting a fatal assert(...) used in RPC code (src/rpc/mining.cpp).

<details><summary>Hints</summary>

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.

</details>

in test/functional/interface_ipc_mining.py:337 in 9424f9173f outdated

 331 | @@ -331,6 +332,56 @@ async def async_routine():
 332 |  
 333 |          asyncio.run(capnp.run(async_routine()))
 334 |  
 335 | +    def run_waitnext_mining_policy_test(self):
 336 | +        """Verify that waitNext() preserves the mining policy from -blockmintxfee
 337 | +        instead of falling back to defaults."""

ryanofsky commented at 1:41 PM on May 18, 2026:

In commit "test: regression test for waitNext mining policy" (9424f9173f289d5f6f2893cd2a681432a57b7f25)

I found this description hard to understand because it sounds generic and doesn't really say what's being checked. Would maybe describe what the test does like "Creates a transaction with a fee less than -blockmintxfee and ensures it is not included in templates returned by createNewBlock and waitNext methods"

in test/functional/interface_ipc_mining.py:364 in 9424f9173f outdated

 359 | +            async with AsyncExitStack() as stack:
 360 | +                self.log.debug("createNewBlock should respect -blockmintxfee")
 361 | +                template = await mining_create_block_template(mining, stack, ctx, self.default_block_create_options)
 362 | +                assert template is not None
 363 | +                block = await mining_get_block(template, ctx)
 364 | +                assert low_fee_tx["txid"] not in {tx.txid_hex for tx in block.vtx[1:]}

ryanofsky commented at 1:43 PM on May 18, 2026:

In commit "test: regression test for waitNext mining policy" (9424f9173f289d5f6f2893cd2a681432a57b7f25)

Here and below, would it be good to assert that the low fee tx is actually in the mempool? (to be more confident that -blockmintxfee is what excludes it from the block).

DrahtBot removed the label Needs rebase on May 18, 2026

in test/functional/interface_ipc_mining.py:390 in 27427d3e57 outdated

 382 | @@ -382,6 +383,67 @@ async def async_routine():
 383 |  
 384 |          asyncio.run(capnp.run(async_routine()))
 385 |  
 386 | +    def run_block_max_weight_test(self):
 387 | +        """Verify that -blockmaxweight is honored on the IPC createNewBlock() path.
 388 | +        The startup arg flows MiningArgs.default_block_max_weight ->
 389 | +        ApplyMiningDefaults() -> BlockCreateOptions.block_max_weight, and
 390 | +        BlockAssembler enforces the cap."""

ryanofsky commented at 1:56 PM on May 18, 2026:

I feel like a short description of what the test actually does would make it a easier to understand. LLM suggests "Tests that -blockmaxweight is respected both by createBlockTemplate() and waitNext(). It sets a small cap (DEFAULT_BLOCK_RESERVED_WEIGHT + 4000 WU), floods the mempool with 20 txs, verifies the first template is truncated by the cap, then sends a high-fee tx and verifies the waitNext() template is also truncated and includes the new tx."

Also think describing the flow of values inside c++ code is probably not good since those details could change and this is a supposed to be a test of external behavior.

in src/test/peerman_tests.cpp:20 in aaed101654 outdated

  16 | @@ -16,12 +17,14 @@ BOOST_FIXTURE_TEST_SUITE(peerman_tests, RegTestingSetup)
  17 |  /** Window, in blocks, for connecting to NODE_NETWORK_LIMITED peers */
  18 |  static constexpr int64_t NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS = 144;
  19 |  
  20 | -static void mineBlock(const node::NodeContext& node, std::chrono::seconds block_time)
  21 | +static void mineBlock(node::NodeContext& node, std::chrono::seconds block_time)

ryanofsky commented at 2:07 PM on May 18, 2026:

In commit "mining: use interface for tests, bench and fuzzers" (aaed101654c1c1a7b4c54bec3a394b49e4b5bef2)

const can be kept here I think

Sjors commented at 3:27 PM on May 18, 2026:

Indeed, the original signature will be restored in the next push.

in src/node/mining_args.h:31 in e96e626e20 outdated

  27 | + * @param[in,out] options The BlockCreateOptions to modify according to \p args.
  28 | + */
  29 | +[[nodiscard]] util::Result<void> ReadMiningArgs(const ArgsManager& args, BlockCreateOptions& options);
  30 | +
  31 | +/** Check option values for validity. Returns an error for invalid values. */
  32 | +[[nodiscard]] util::Result<void> Check(const BlockCreateOptions& options);

ryanofsky commented at 2:18 PM on May 18, 2026:

In commit "mining: add block create option helpers" (e96e626e20a2813868b189315f38d05902605557)

Would maybe give Check/Flatten/Merge more descriptive names like CheckMiningOptions / FlattingMiningOptions / MergeMiningOptions

DrahtBot removed the label CI failed on May 18, 2026

in src/node/mining_args.h:49 in e96e626e20 outdated

  45 | +
  46 | +/** Check that block_reserved_weight is within allowed bounds. */
  47 | +[[nodiscard]] util::Result<void> CheckBlockReservedWeight(uint64_t block_reserved_weight, const std::string& arg_name = "");
  48 | +
  49 | +/** Check that coinbase_output_max_additional_sigops does not exceed consensus limits. */
  50 | +[[nodiscard]] util::Result<void> CheckCoinbaseOutputMaxAdditionalSigops(size_t sigops, const std::string& arg_name = "");

ryanofsky commented at 2:31 PM on May 18, 2026:

In commit "mining: add block create option helpers" (e96e626e20a2813868b189315f38d05902605557)

IMO it would be good to drop all these separate check methods and just have one check method:

util::Result<void> CheckMiningOptions(const BlockCreateOptions& options, bool use_argnames)
{
    if (options.block_max_weight) {
        if (*options.block_max_weight > MAX_BLOCK_WEIGHT) {
            return util::Error{Untranslated(strprintf("%s (%s) exceeds consensus maximum block weight (%u)",
                                                      use_argnames ? "-blockmaxweight" : "block_max_weight",
                                                      util::ToString(*options.block_max_weight), MAX_BLOCK_WEIGHT))};
        }
    }
    if (options.block_reserved_weight) {
        if (*options.block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
            return util::Error{Untranslated(strprintf("%s (%s) is lower than minimum safety value of (%u)",
                                                      use_argnames ? "-blockreservedweight" : "block_reserved_weight",
                                                      util::ToString(*options.block_reserved_weight), MINIMUM_BLOCK_RESERVED_WEIGHT))};
        }
        if (*options.block_reserved_weight > MAX_BLOCK_WEIGHT) {
            return util::Error{Untranslated(strprintf("%s (%s) exceeds consensus maximum block weight (%u)",
                                                      use_argnames ? "-blockreservedweight" : "block_reserved_weight",
                                                      util::ToString(*options.block_reserved_weight), MAX_BLOCK_WEIGHT))};
        }
    }
    if (options.coinbase_output_max_additional_sigops > MAX_BLOCK_SIGOPS_COST) {
        return util::Error{Untranslated(strprintf("%s (%zu) exceeds consensus maximum block sigops cost (%d)",
                                                  "coinbase_output_max_additional_sigops",
                                                  options.coinbase_output_max_additional_sigops, MAX_BLOCK_SIGOPS_COST))};
    }
    return {};
}

Sjors force-pushed on May 18, 2026

Sjors commented at 2:45 PM on May 18, 2026: member

25adaedf725405937f5fd07d2e7480f1032786e1 -> be3080be1eccd7105fc62927ad898207a4c9f457: I moved some things around between commits for easier review. No net code change, except for restoring the script_sig_prefix comment in mining_types.h that was broken in the rebase.

I split mining: add block create option helpers to make it easier to review: e96e626e20a2813868b189315f38d05902605557 -> 2662c9131c2e024f96b5d247937ea55d6409471a + e92b9541eaa32f0872855300cc151ce2f65ef021.

I also moved interfaces: make Mining use const NodeContext to immediately before the commit that needs it, and moved move-only: add node/mining_types.h before interfaces: make Mining use const NodeContext.

in src/node/miner.cpp:84 in e96e626e20 outdated

  76 | @@ -76,40 +77,21 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
  77 |      block.hashMerkleRoot = BlockMerkleRoot(block);
  78 |  }
  79 |  
  80 | -static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
  81 | -{
  82 | -    // Apply DEFAULT_BLOCK_RESERVED_WEIGHT and DEFAULT_BLOCK_MAX_WEIGHT when the caller left it unset.
  83 | -    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);
  84 | -    options.coinbase_output_max_additional_sigops = std::clamp<size_t>(options.coinbase_output_max_additional_sigops, 0, MAX_BLOCK_SIGOPS_COST);
  85 | -    // Limit weight to between block_reserved_weight and MAX_BLOCK_WEIGHT for sanity:

ryanofsky commented at 3:05 PM on May 18, 2026:

In commit "mining: add block create option helpers" (e96e626e20a2813868b189315f38d05902605557)

The lower-bound clamp below which increases block_max_weight to be at least as high as block_reserved_weight "for sanity" is now gone. This seems like the only clamp here not replaced by an error.

This seems ok, but would be good to say in commit message we are intentionally getting rid of this. (It seems ilke it was never necessary, but maybe a cosmetic thing)

Sjors commented at 3:56 PM on May 18, 2026:

I'm making it an error.

in src/node/mining_args.cpp:31 in e96e626e20

  27 |  
  28 | -util::Result<void> ReadMiningArgs(const ArgsManager& args)
  29 | +util::Result<void> CheckBlockMaxWeight(uint64_t block_max_weight, const std::string& arg_name)
  30 | +{
  31 | +    if (block_max_weight > MAX_BLOCK_WEIGHT) {
  32 | +        return util::Error{Untranslated(strprintf("%s (%s) exceeds consensus maximum block weight (%u)",

ryanofsky commented at 3:08 PM on May 18, 2026:

In commit "mining: add block create option helpers" (e96e626e20a2813868b189315f38d05902605557)

Commit message describes code changes without describing what effect code changes actually have. Would be good to be clear this is not a refactoring and that invalid option values now trigger errors instead of being clamped.

Sjors commented at 3:28 PM on May 18, 2026:

I've been splitting this, should be better in the next push.

ryanofsky commented at 3:13 PM on May 18, 2026: contributor

Code review 25adaedf725405937f5fd07d2e7480f1032786e1. Not finished reviewing, but seems like this is actively being developed so wanted to post my comments so far.

Sjors marked this as a draft on May 18, 2026

test: regression test for waitNext mining policy

Demonstrates that BlockTemplateImpl::waitNext() must respect the
mining policy supplied via -blockmintxfee, not silently fall back
to defaults.

Co-authored-by: Enoch Azariah <enirox001@gmail.com>

268fd0891b

test: cover IPC blockmaxweight policy

Verify that -blockmaxweight is honored both when an IPC block template is first created and after waitNext() refreshes the template.

Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>

c1505acb29

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.

c1a316819a

interfaces: make Mining use const NodeContext

The next commit switches test helpers that take a const NodeContext& to
create blocks through the Mining interface, so MakeMining needs to
accept a const NodeContext too.

b4d794ef51

mining: use interface for tests, bench and fuzzers

Have most tests, benchmarks and fuzzers go through the mining interface.
This avoids most direct test, benchmark and fuzzer use of
node::BlockAssembler::Options, making 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.

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. Using
the Mining interface here also requires marking the test
KernelNotifications chainstate as loaded after LoadVerifyActivateChainstate().

e8e95ebf0c

mining: parse block creation args in mining_args

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

This also switches the weight arguments to use GetArg<uint64_t>,
introduced in bitcoin/bitcoin#34582.

569f14ddac

miner: add block_max_weight to BlockCreateOptions

This new optional replaces nBlockMaxWeight.

Use uint64_t for the block weight options to match BlockAssembler's
nBlockWeight accounting and the IPC schema's blockReservedWeight type.

The new block_max_weight option is not exposed to IPC clients.

b1f7fc191e

mining: add block create option helpers

Move block template defaulting into helper functions for
BlockCreateOptions. Flatten() fills hardcoded defaults and Merge()
overlays defaults without replacing caller-provided values.

Use the shared option type in BlockAssembler so IPC callers and internal
callers can pass through the same options path. This commit does not
change behavior, except for dropping the "Specified " prefix from
startup option error messages.

Keep the -blockmintxfee ParseMoney check in ReadMiningArgs() instead of
CheckMiningOptions(), because CheckMiningOptions() only sees the parsed
CFeeRate value and not the original string.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

dc18e22c64

Sjors force-pushed on May 18, 2026

Sjors commented at 4:37 PM on May 18, 2026: member

A couple of cosmetic changes:

IWYU enforce init.cpp
avoid touching createNewBlock signature
dd012afc50 -> 1a1ebe7e13: stale commit message
mining: add block create option helpers temporarily adds an Assert; although it goes away in mining: store block create options in NodeContext, I changed it to an Assume for peace of mind.
avoid introducing ErrorOptionName only to delete it a few commits later
changes suggested by @ryanofsky above: #33966#pullrequestreview-4310258511

Behavior change:

#33966 (review) (replace clamp with an error)

Other:

#33966 (review) (change block_reserved_weight field from size_t to uint64_t to match UInt64 in capnproto.

mining: reject invalid block create options

Check BlockCreateOptions before block template creation instead of
clamping runtime values. This makes invalid runtime block creation
options, including those passed by IPC mining clients, fail explicitly
instead of silently mining with different values than the caller
requested.

Runtime option validation now uses the same error wording as startup
option validation. Startup validation also rejects -blockmaxweight
values lower than -blockreservedweight instead of allowing them to be
clamped later.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

4670b8e256

mining: store block create options in NodeContext

Read configured mining defaults once during startup instead of parsing
options like -blockmaxweight each time a block template is generated.

Store the parsed defaults as BlockCreateOptions so IPC overrides and node
defaults can be merged with the same option type used by BlockAssembler.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

82c1165403

refactor: have mining files include what they use 7193dff50c

ci: enforce iwyu for touched files

Except tests, fuzz and util which are left to a followup.

e5fe74ef39

doc: add release note for mining option validation e0cbbe2458

Sjors commented at 5:11 PM on May 18, 2026: member

e7aedce88360ce9fb794d3a6bc64bc2004997934 -> e0cbbe2 (compare): adjust src/test/fuzz/tx_pool.cpp because we no longer clamp, update stale comment

Unless CI fails again, I'll leave this alone for today, review welcome.

Sjors force-pushed on May 18, 2026

Sjors marked this as ready for review on May 18, 2026

DrahtBot added the label CI failed on May 18, 2026

DrahtBot commented at 5:13 PM on May 18, 2026: contributor

🚧 At least one of the CI tasks failed. Task Windows native, fuzz, VS: https://github.com/bitcoin/bitcoin/actions/runs/26046855439/job/76573199077 LLM reason (✨ experimental): Fuzz test failure: the tx_pool fuzz target exited with code 1 due to “Error processing input” for a qa-assets/fuzz_corpora/tx_pool corpus file.

<details><summary>Hints</summary>

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.

</details>

DrahtBot removed the label CI failed on May 18, 2026

in test/functional/interface_ipc_mining.py:105 in 3864344620

 101 | @@ -102,6 +102,14 @@ async def build_coinbase_test(self, template, ctx, miniwallet, extra_nonce=b""):
 102 |          coinbase_tx.nLockTime = coinbase_res.lockTime
 103 |          return coinbase_tx
 104 |  
 105 | +    async def assert_create_fails(self, ctx, mining, opts, expected_msg):

enirox001 commented at 2:09 PM on May 19, 2026:

In commit "test: add assert_create_fails helper" (3864344)

This method seems like a utility, so I think it can be moved to ipc_util.py. I had done this refactor, and it seems good to have consistency.

index d0b0193c6e..6fdb1987b7 100755
--- a/test/functional/interface_ipc_mining.py
+++ b/test/functional/interface_ipc_mining.py
@@ -41,6 +41,7 @@ from test_framework.ipc_util import (
     mining_get_coinbase_tx,
     mining_wait_next_template,
     wait_and_do,
+    assert_create_fails,
 )
 
 # Test may be skipped and not have capnp installed
@@ -69,10 +70,15 @@ class IPCMiningTest(BitcoinTestFramework):
     async def build_coinbase_test(self, template, ctx, miniwallet, extra_nonce=b""):
         self.log.debug("Build coinbase transaction using getCoinbaseTx()")
         assert template is not None
+
         coinbase_res = await mining_get_coinbase_tx(template, ctx)
+
         coinbase_tx = CTransaction()
+        #simple basic coinbase
         coinbase_tx.version = coinbase_res.version
+        # one vin
         coinbase_tx.vin = [CTxIn()]
+        # no prevout becasue it is the coinbase transaction
         coinbase_tx.vin[0].prevout = NULL_OUTPOINT
         coinbase_tx.vin[0].nSequence = coinbase_res.sequence
 
@@ -105,14 +111,6 @@ class IPCMiningTest(BitcoinTestFramework):
         coinbase_tx.nLockTime = coinbase_res.lockTime
         return coinbase_tx
 
-    async def assert_create_fails(self, ctx, mining, opts, expected_msg):
-        """Assert that createNewBlock fails with the expected remote exception."""
-        try:
-            await mining.createNewBlock(ctx, opts)
-            raise AssertionError("createNewBlock unexpectedly succeeded")
-        except capnp.lib.capnp.KjException as e:
-            assert_capnp_failed(e, f"remote exception: std::exception: {expected_msg}")
-
     def run_mining_interface_test(self):
         """Test Mining interface methods."""
         self.log.info("Running Mining interface test")
@@ -329,7 +327,7 @@ class IPCMiningTest(BitcoinTestFramework):
 
             self.log.debug("Enforce minimum reserved weight for IPC clients too")
             opts.blockReservedWeight = 0
-            await self.assert_create_fails(ctx, mining, opts,
+            await assert_create_fails(ctx, mining, opts,
                 "block_reserved_weight (0) is lower than minimum safety value of (2000)")
 
         async def async_routine_check_max_reserved_weight():
@@ -337,7 +335,7 @@ class IPCMiningTest(BitcoinTestFramework):
             ctx, mining = await make_mining_ctx(self)
             opts = self.capnp_modules['mining'].BlockCreateOptions()
             opts.blockReservedWeight = MAX_BLOCK_WEIGHT + 1
-            await self.assert_create_fails(ctx, mining, opts,
+            await assert_create_fails(ctx, mining, opts,
                 f"block_reserved_weight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus maximum block weight ({MAX_BLOCK_WEI
GHT})")
 
         async def async_routine_check_sigops_limit():
@@ -345,7 +343,7 @@ class IPCMiningTest(BitcoinTestFramework):
             ctx, mining = await make_mining_ctx(self)
             ctx, mining = await make_mining_ctx(self)
             ctx, mining = await make_mining_ctx(self)
             ctx, mining = await make_mining_ctx(self)
             opts = self.capnp_modules['mining'].BlockCreateOptions()
             opts.coinbaseOutputMaxAdditionalSigops = MAX_BLOCK_SIGOPS_COST + 1
-            await self.assert_create_fails(ctx, mining, opts,
+            await assert_create_fails(ctx, mining, opts,
                 f"coinbase_output_max_additional_sigops ({MAX_BLOCK_SIGOPS_COST + 1}) exceeds consensus maximum block s
igops cost ({MAX_BLOCK_SIGOPS_COST})")
 
         asyncio.run(capnp.run(async_routine()))
diff --git a/test/functional/test_framework/ipc_util.py b/test/functional/test_framework/ipc_util.py
index 340cd15c61..b86c766eab 100644
--- a/test/functional/test_framework/ipc_util.py
+++ b/test/functional/test_framework/ipc_util.py
@@ -162,3 +162,11 @@ async def make_mining_ctx(self):
 def assert_capnp_failed(e, description_prefix):
     assert e.description.startswith(description_prefix), f"Expected description starting with '{description_prefix}', g
ot '{e.description}'"
     assert_equal(e.type, "FAILED")
+
+async def assert_create_fails(ctx, mining, opts, expected_msg):
+        """Assert that createNewBlock fails with the expected remote exception."""
+        try:
+            await mining.createNewBlock(ctx, opts)
+            raise AssertionError("createNewBlock unexpectedly succeeded")
+        except capnp.lib.capnp.KjException as e:
+            assert_capnp_failed(e, f"remote exception: std::exception: {expected_msg}")

Also, could we use a better name for this function instead of assert_create_fails? This is only for mining, but the name isn't very clear on that.

I am also wondering if this commit could have a description for why it was added. I can see its use in the code, but it would be beneficial to include a commit description that explains why this helper was introduced.

in test/functional/interface_ipc_mining.py:376 in 268fd0891b

 371 | +                    confirmed_only=True,
 372 | +                )
 373 | +                mempool_txids = self.nodes[0].getrawmempool()
 374 | +                assert high_fee_tx["txid"] in mempool_txids
 375 | +                assert low_fee_tx["txid"] in mempool_txids
 376 | +                waitoptions = self.capnp_modules['mining'].BlockWaitOptions()

enirox001 commented at 2:38 PM on May 19, 2026:

In commit "test: regression test for waitNext mining policy" (268fd08)

This suggestion is more of a nit, feel free to ignore, but I think for this waitoptions we could introduce a new variable in run_test self.default_block_wait_options = self.capnp_modules['mining'].BlockWaitOptions() and use it in this test.

I find it a bit inconsistent that some tests in the interface_ipc_mining redeclare the self.capnp_modules['mining'].BlockCreateOptions() as well. Instead of using the default variable already declared in run_test.

This change is a bit out of scope for this PR, but cleanup would be beneficial.

in test/functional/interface_ipc_mining.py:391 in c1505acb29

 386 | @@ -386,6 +387,62 @@ async def async_routine():
 387 |  
 388 |          asyncio.run(capnp.run(async_routine()))
 389 |  
 390 | +    def run_block_max_weight_test(self):
 391 | +        """Verify that -blockmaxweight is honored on the IPC createNewBlock() path."""

enirox001 commented at 3:08 PM on May 19, 2026:

In commit "test: cover IPC blockmaxweight policy" (c1505ac)

This docstring seems a bit narrow, as the test also checks the waitNext() refresh path. Maybe say something along the lines of:

"Verify IPC createNewBlock() and waitNext() preserve the -blockmaxweight policy."

in test/functional/interface_ipc_mining.py:409 in c1505acb29

 404 | +            "-persistmempool=0",
 405 | +        ])
 406 | +        # Refresh miniwallet's UTXO view from the chain after restart.
 407 | +        self.miniwallet.rescan_utxos()
 408 | +
 409 | +        for _ in range(NUM_TXS):

enirox001 commented at 3:10 PM on May 19, 2026:

In commit "test: cover IPC blockmaxweight policy" (c1505ac)

I think a short comment could be added here explaining that these transactions are used to fill the mempool enough that the configured block weight cap forces the template to truncate. That would make the later initial_included < NUM_TXS assertion easier to follow.

in test/functional/interface_ipc_mining.py:442 in c1505acb29

 437 | +                assert template_next is not None
 438 | +
 439 | +                block_next = await mining_get_block(template_next, ctx)
 440 | +                assert_greater_than_or_equal(small_cap, block_next.get_weight())
 441 | +                assert high_fee_tx["txid"] in {tx.txid_hex for tx in block_next.vtx[1:]}
 442 | +                assert len(block_next.vtx) - 1 < NUM_TXS + 1

enirox001 commented at 3:15 PM on May 19, 2026:

In commit "test: cover IPC blockmaxweight policy" (c1505ac)

This assertion is correct, but it is a little indirect.

Maybe assign this to a named value like next_included = len(block_next.vtx) - 1 and use an assertion message similar to the initial template check above.

That would make it clearer that waitNext() is expected to stay capped after adding the high fee tx

in src/node/mining_args.cpp:17 in 569f14ddac

  12 | +#include <util/moneystr.h>
  13 | +#include <util/translation.h>
  14 | +
  15 | +#include <cstdint>
  16 | +
  17 | +using common::AmountErrMsg;

ryanofsky commented at 5:28 PM on May 19, 2026:

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

using util::Error; and using util::Result could cut down noise in this file

in src/node/mining_args.cpp:1 in 569f14ddac

   0 | @@ -0,0 +1,45 @@
   1 | +// Copyright (c) 2026 The Bitcoin Core developers

ryanofsky commented at 5:30 PM on May 19, 2026:

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

I think we are not adding years anymore

in src/test/util/mining.cpp:127 in e8e95ebf0c

 127 | -    auto block = std::make_shared<CBlock>(
 128 | -        BlockAssembler{Assert(node.chainman)->ActiveChainstate(), Assert(node.mempool.get()), assembler_options}
 129 | -            .CreateNewBlock()
 130 | -            ->block);
 131 | +    auto mining = interfaces::MakeMining(node);
 132 | +    auto block_template = mining->createNewBlock(assembler_options, /*cooldown=*/false);

ryanofsky commented at 5:36 PM on May 19, 2026:

In commit "mining: use interface for tests, bench and fuzzers" (e8e95ebf0cdc62592e33ddd78a0f4c5d6f9b7bb4)

Could be good to say explicitly that this commit is a refactoring and doesn't change what blocks are created, just how the create calls are made.

in src/node/mining_args.cpp:39 in 569f14ddac

  34 | +    const uint64_t block_reserved_weight{args.GetArg<uint64_t>("-blockreservedweight", DEFAULT_BLOCK_RESERVED_WEIGHT)};
  35 | +    if (block_reserved_weight > MAX_BLOCK_WEIGHT) {
  36 | +        return util::Error{strprintf(_("Specified -blockreservedweight (%d) exceeds consensus maximum block weight (%d)"), block_reserved_weight, MAX_BLOCK_WEIGHT)};
  37 | +    }
  38 | +    if (block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
  39 | +        return util::Error{strprintf(_("Specified -blockreservedweight (%d) is lower than minimum safety value of (%d)"), block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT)};

ryanofsky commented at 5:44 PM on May 19, 2026:

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

Would be good to say in commit message this commit mostly does not change behavior, except two weight arguments are printed as unsigned instead of signed. So if large or negative values are passed the error messages can be printed differently, but the checks are still the same.

in src/node/miner.cpp:102 in b1f7fc191e

  97 | @@ -98,13 +98,15 @@ BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool
  98 |  void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& options)
  99 |  {
 100 |      // Block resource limits
 101 | -    options.nBlockMaxWeight = args.GetIntArg("-blockmaxweight", options.nBlockMaxWeight);
 102 | +    if (!options.block_max_weight) {
 103 | +        options.block_max_weight = args.GetArg<uint64_t>("-blockmaxweight");

ryanofsky commented at 5:47 PM on May 19, 2026:

In commit "miner: add block_max_weight to BlockCreateOptions" (b1f7fc1) Would be good to note small change in behavior in commit message: negative values treated as 0 instead of large overflowed values

in src/node/interfaces.cpp:1002 in dc18e22c64

 997 | @@ -990,10 +998,16 @@ class MinerImpl : public Mining
 998 |              // Also wait during the final catch-up moments after IBD.
 999 |              if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip, m_interrupt_mining)) return {};
1000 |          }
1001 | -
1002 | -        BlockAssembler::Options assemble_options{options};
1003 | -        ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
1004 | -        return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), m_node.mempool.get(), assemble_options}.CreateNewBlock(), m_node);
1005 | +        BlockCreateOptions args_options;
1006 | +        Assume(ReadMiningArgs(*Assert(m_node.args), args_options));

ryanofsky commented at 5:54 PM on May 19, 2026:

In commit "mining: add block create option helpers" (dc18e22c64211b0f795fc70776d6049db746f7c5)

I think unchecked Assumes usually do not make sense in high level code and it would be good to either an throw an exception if this fails or change the Assume to an Assert. A failure to apply mining options seems like it could be potentially bad or costly, and not good to ignore with no error in release builds.

in src/rpc/mining.cpp:478 in dc18e22c64

 477 | -    BlockAssembler::Options assembler_options;
 478 | -    ApplyArgsManOptions(*node.args, assembler_options);
 479 | -    obj.pushKV("blockmintxfee", ValueFromAmount(assembler_options.blockMinFeeRate.GetFeePerK()));
 480 | +    node::BlockCreateOptions options;
 481 | +    Assert(ReadMiningArgs(*node.args, options));
 482 | +    obj.pushKV("blockmintxfee", ValueFromAmount(Assert(options.block_min_fee_rate)->GetFeePerK()));

ryanofsky commented at 5:59 PM on May 19, 2026:

In commit "mining: add block create option helpers" (dc18e22c64211b0f795fc70776d6049db746f7c5)

This Assert is changed to CHECK_NONFATAL later, but probably would make more sense to change here

in src/node/mining_args.cpp:66 in dc18e22c64

  77 | +    return CheckMiningOptions(options, /*use_argnames=*/true);
  78 | +}
  79 | +
  80 | +BlockCreateOptions FlattenMiningOptions(BlockCreateOptions options)
  81 | +{
  82 | +    if (!options.block_min_fee_rate) options.block_min_fee_rate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};

ryanofsky commented at 6:06 PM on May 19, 2026:

In commit "mining: add block create option helpers" (dc18e22c64211b0f795fc70776d6049db746f7c5)

It would be good if BlockCreateOptions fields were handled in the same order in these different functions. This function handles the fields in the same order as the struct, which is good, but the other functions are inconsistent

in src/node/mining_types.h:76 in b1f7fc191e

  71 | +     * Maximum block weight, defaults to -maxblockweight
  72 | +     *
  73 | +     * block_reserved_weight can safely exceed block_max_weight, but the rest of
  74 | +     * the block template will be empty.
  75 | +     */
  76 | +    std::optional<uint64_t> block_max_weight{};

ryanofsky commented at 6:08 PM on May 19, 2026:

In commit "miner: add block_max_weight to BlockCreateOptions" (b1f7fc191effa7b1f2c466639f769dfae72a2905)

Might make sense to move this field above next to block_reserved_weight since the fields are related.

in src/node/mining_args.h:24 in dc18e22c64

  20 | + * \p options. Returns an error if one was encountered.
  21 | + *
  22 | + * @param[in]  args The ArgsManager in which to check set options.
  23 | + * @param[in,out] options The BlockCreateOptions to modify according to \p args.
  24 | + */
  25 | +[[nodiscard]] util::Result<void> ReadMiningArgs(const ArgsManager& args, BlockCreateOptions& options);

ryanofsky commented at 6:10 PM on May 19, 2026:

In commit "mining: add block create option helpers" (dc18e22c64211b0f795fc70776d6049db746f7c5)

It doesn't seem good to use an output argument here because semantics could be ambiguous if options are already set. Is this able to use Result<BlockCreateOptions> instead?

in src/node/mining_args.cpp:52 in dc18e22c64

  62 | +    if (const auto arg{args.GetArg("-blockmintxfee")}) {
  63 | +        std::optional<CAmount> block_min_tx_fee{ParseMoney(*arg)};
  64 | +        if (!block_min_tx_fee) return util::Error{AmountErrMsg("blockmintxfee", *arg)};
  65 | +        options.block_min_fee_rate = CFeeRate{*block_min_tx_fee};
  66 | +    } else {
  67 | +        options.block_min_fee_rate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};

ryanofsky commented at 6:18 PM on May 19, 2026:

In commit "mining: add block create option helpers" (dc18e22c64211b0f795fc70776d6049db746f7c5)

It would seem better to drop this else branch and leave also omit DEFAULT_BLOCK_MAX_WEIGHT, DEFAULT_BLOCK_RESERVED_WEIGHT, DEFAULT_PRINT_MODIFIED_FEE arguments below. Two reasons:

dropping defaults here would lets callers distinguish between unset options and explicitly specified ones
dropping defaults here could catch bugs if code was failing to flatten the struct before using

It also seems cleaner to reference the defaults one place instead of multiple

in src/node/mining_args.cpp:43 in 4670b8e256

  38 | @@ -39,6 +39,20 @@ util::Result<void> CheckMiningOptions(const BlockCreateOptions& options, bool us
  39 |                                                    use_argnames ? "-blockreservedweight" : "block_reserved_weight",
  40 |                                                    *options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT))};
  41 |      }
  42 | +    const uint64_t block_max_weight{options.block_max_weight.value_or(DEFAULT_BLOCK_MAX_WEIGHT)};
  43 | +    const uint64_t block_reserved_weight{options.block_reserved_weight.value_or(DEFAULT_BLOCK_RESERVED_WEIGHT)};

ryanofsky commented at 6:38 PM on May 19, 2026:

In commit "mining: reject invalid block create options" (4670b8e256f12e036e2ba96a25de249362619da1)

Could simplify this function just to do what ClampOptions did, and take a value parameter BlockCreateOptions option, then do options = FlattenMiningOptions(std::move(options)); and stop needing to check for null values throughout

in src/node/mining_types.h:89 in 4670b8e256

  84 | @@ -85,8 +85,7 @@ struct BlockCreateOptions {
  85 |      /**
  86 |       * Maximum block weight, defaults to -maxblockweight
  87 |       *
  88 | -     * block_reserved_weight can safely exceed block_max_weight, but the rest of
  89 | -     * the block template will be empty.
  90 | +     * Must not be lower than block_reserved_weight.
  91 |       */

ryanofsky commented at 6:40 PM on May 19, 2026:

In commit "mining: reject invalid block create options" (4670b8e256f12e036e2ba96a25de249362619da1)

Think it would be nice still be nice to say if this is set to the reserved weight the block template will be empty. It gives more context for how the options work.

in src/node/mining_args.cpp:45 in 4670b8e256

  38 | @@ -39,6 +39,20 @@ util::Result<void> CheckMiningOptions(const BlockCreateOptions& options, bool us
  39 |                                                    use_argnames ? "-blockreservedweight" : "block_reserved_weight",
  40 |                                                    *options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT))};
  41 |      }
  42 | +    const uint64_t block_max_weight{options.block_max_weight.value_or(DEFAULT_BLOCK_MAX_WEIGHT)};
  43 | +    const uint64_t block_reserved_weight{options.block_reserved_weight.value_or(DEFAULT_BLOCK_RESERVED_WEIGHT)};
  44 | +    if (block_max_weight < block_reserved_weight) {
  45 | +        return util::Error{Untranslated(strprintf("%s (%d) is lower than %s (%d)",

ryanofsky commented at 6:44 PM on May 19, 2026:

In commit "mining: reject invalid block create options" (4670b8e256f12e036e2ba96a25de249362619da1)

This is fine, but "reserved weight exceeds max weight" seems clearer to me than "max weight is lower than reserved weight"

ryanofsky approved

ryanofsky commented at 6:53 PM on May 19, 2026: contributor

Code review ACK e0cbbe2458adf4ba1e391569d85209b0d98c193a. Rereviewed the PR and everything looks very good. This makes it much more straightforward to combine node settings with mining client options, and gives mining clients better feedback when invalid options are specified. All the review suggestions I left below and minor and not important.

DrahtBot requested review from w0xlt on May 19, 2026

enirox001 commented at 6:57 PM on May 19, 2026: contributor

Code Review ACK c1a3168

Going through the changes again. The PR is in a good state as it is. These are minor suggestions and nits

DrahtBot requested review from enirox001 on May 19, 2026

w0xlt commented at 12:11 AM on May 20, 2026: contributor

reACK e0cbbe2458adf4ba1e391569d85209b0d98c193a