refactor: disentangle miner startup defaults from runtime options #33966
Conversation
The -blockreservedweight startup option should only affect RPC code, because IPC clients currently do not have a way to signal their intent to use the node default (the BlockCreateOptions struct defaults merely document a recommendation for client software). Before this commit however, if the user set -blockreservedweight then ApplyArgsManOptions would cause the block_reserved_weight option passed by IPC clients to be ignored. Users who don't set this value were not affected. Fix this by having ApplyArgsManOptions not touch block_reserved_weight. Update RPC callers, in particular getblocktemplate, to explicitly set this option since it's no longer automatic. Test coverage is added. mining_basic.py already ensured -blockreservedweight is enforced by mining RPC methods. This commit adds coverage for Mining interface IPC clients. It also verifies that -blockreservedweight has no effect on them.
DrahtBot
changed the title
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33966. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Sjors
force-pushed
the
2025/11/miner-options
branch
from
37d9740 to
e03c083
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Have most tests, benchmarks and fuzzers go through the mining interface. This avoids the use node::BlockAssembler::Options, which makes it easier to drop in a later commit. Two exceptions which use BlockAssembler directly: - one check in test/miner_tests.cpp needs m_package_feerates - fuzz/tx_pool.cpp Finish() doesn't have access to a NodeContext Move test_block_validity from BlockAssembler::Options to BlockCreateOptions so bench/block_assemble.cpp can continue to set it. Just like coinbase_output_script, this is not exposed to IPC clients. Inline options variable in places where it's only needed once. The remaining PrepareBlock() constructor takes a const NodeContext parameter, but internally const_cast it. This should be harmless and avoids a larger refactor around constness in the tests. We also drop one unused PrepareBlock declaration and one unused implementation. TestChain100Setup::CreateBlock no longer needs a chainstate argument which in turn means it can be dropped from CreateAndProcessBlock.
Move mining related defaults and structs there. This simplifies includes in later commits.
Move the argument parsing for -blockmaxweight, -blockreservedweight, -blockmintxfee and -blockversion out of init.cpp to a dedicated mining_args.cpp.
After the next commit nBlockMaxWeight is set once at startup and not mutated after. In preparation this commit introduces BlockCreateOptions::block_max_weight which is set by ClampOptions, leaving nBlockMaxWeight unmodified. This option is not exposed to IPC clients.
Instead of parsing arguments like -blockmaxweight each time a block template is generated, do it once in ApplyArgsManOptions(). These arguments are stored in a new struct MiningArgs. The BlockAssembler::Options struct is removed in favor of passing both MiningArgs and BlockCreateOptions. This disentangles node configuration from client (or test) options.
Sjors
force-pushed
the
2025/11/miner-options
branch
from
e03c083 to
517a9b2
Compare
|
Concept ACK 517a9b2, 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 |
3 participants
The interaction between node startup options like
-blockreservedweightand runtime options, especially those passed via IPC, is confusing.They're combined in
BlockAssembler::Options, which this PR gets rid of in favour of two distinct structs:BlockCreateOptions: used by interface clients. As before, IPC clients have access to a safe / sane subset, whereas RPC and test code can use all fields.MiningArgs: these are set once during node startupBoth structs have a member for the maximum block height, which is not a problem since they're different structs. The one on
MiningArgs(nBlockMaxWeight) matches-maxblockheightand is left alone. The one onBlockCreateOptions(block_max_weight) is (exclusively) set byClampOptions.This all happens in the last commit and requires some preparation to keep things easy to review.
We get rid of
BlockAssembler::Optionsbut this is used in many tests. Since large churn is inevitable, we might as well switch all tests, bench and fuzzers over to the Mining interface. The first (non-base) commit does that, dramatically reducing direct use ofBlockAssembler. Two exceptions are documented in the commit message. Becausetest_block_validitywasn't available via the interface and the block_assemble benchmark needs it, it's moved fromBlockAssembler::OptionstoBlockCreateOptions(still not exposed via IPC).We need access to mining related defaults and structs from both the miner and node initialization code. To avoid having to pull in all of
BlockAssemblerfor the latter, the second commit introducesnode/mining.hand moves constants and structs there fromsrc/node/types.h(BlockCreateOptions,BlockWaitOptions,BlockCheckOptions) andsrc/node/miner.h(DEFAULT_PRINT_MODIFIED_FEE).I considered also moving
DEFAULT_BLOCK_MAX_WEIGHT,DEFAULT_BLOCK_RESERVED_WEIGHT,MINIMUM_BLOCK_RESERVED_WEIGHTandDEFAULT_BLOCK_MIN_TX_FEEthere frompolicy.h, since they are distinct from relay policy and not needed by the kernel. But this seems more appropriate for a follow-up and requires additional discussion.The meat and potatoes of this PR is split between the three last commits for easier review:
node/mining_args.{h,cpp}and move the options checking out ofinit.cpp, without adding storageblock_max_weighttoBlockCreateOptionsas explained aboveMiningArgsstruct, add it to theNodeContext, expandmining_args.cppto store it, pass it toBlockAssembler, etc.I kept variable renaming and other formatting changes to a minimum to ease review with
--color-moved=dimmed-zebra.This PR builds on the bug fix in:
Once that is merged, this PR should not change behaviour.