refactor: disentangle miner startup defaults from runtime options #33966
Conversation
|
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. |
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. |
e03c083 to
517a9b2
Compare
|
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 |
517a9b2 to
4b35dcc
Compare
|
Rebased on the latest #33965 which allowed for some simplifications:
We could trivially make the new Note that I brought back I haven't addressed this yet:
What would be a good way to organize these things? E.g. having |
4b35dcc to
5f3e67e
Compare
Oh I see. You mean you want the mining interface options separate from the |
975c912 to
e0d0a57
Compare
|
I moved
Yes, I think it's easier for Mining IPC client developers to understand how things work that way. |
e0d0a57 to
055c3d4
Compare
|
Rebased and added a commit to move the minimum |
Refactor the create_block_template and wait_next_template helpers in interface_ipc.py to return None if they time out or fail. It makes the test easier to read and provides a more clear error message in case of a regression.
The -blockreservedweight startup option should only affect RPC code, because IPC clients (currently) do not have a way to signal their intent to use the node default (the BlockCreateOptions struct defaults merely document a recommendation for client software). Before this commit however, if the user set -blockreservedweight then ApplyArgsManOptions would cause the block_reserved_weight option passed by IPC clients to be ignored. Users who don't set this value were not affected. Fix this by making BlockCreateOptions::block_reserved_weight an std::optional. Internal interface users, such as the RPC call sites, don't set a value so -blockreservedweight is used. Whereas IPC clients do set a value which is no longer ignored. Test coverage is added. mining_basic.py already ensured -blockreservedweight is enforced by mining RPC methods. This commit adds coverage for Mining interface IPC clients. It also verifies that -blockreservedweight has no effect on them. Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>
Previously a lower value was silently clamped to MINIMUM_BLOCK_RESERVED_WEIGHT.
Have most tests, benchmarks and fuzzers go through the mining interface. This avoids the use node::BlockAssembler::Options, which makes it easier to drop in a later commit. Two exceptions which use BlockAssembler directly: - one check in test/miner_tests.cpp needs m_package_feerates - fuzz/tx_pool.cpp Finish() doesn't have access to a NodeContext Move test_block_validity from BlockAssembler::Options to BlockCreateOptions so bench/block_assemble.cpp can continue to set it. Just like coinbase_output_script, this is not exposed to IPC clients. Inline options variable in places where it's only needed once. The remaining PrepareBlock() constructor takes a const NodeContext parameter, but internally const_cast it. This should be harmless and avoids a larger refactor around constness in the tests. We also drop one unused PrepareBlock declaration and one unused implementation. TestChain100Setup::CreateBlock no longer needs a chainstate argument which in turn means it can be dropped from CreateAndProcessBlock.
Move mining related structs there. This simplifies includes in later commits and makes the code easier to understand for Mining IPC client developers.
Move the argument parsing for -blockmaxweight, -blockreservedweight, -blockmintxfee and -blockversion out of init.cpp to a dedicated mining_args.cpp.
This new optional replaces nBlockMaxWeight. The option is not exposed to IPC clients.
Instead of parsing arguments like -blockmaxweight each time a block template is generated, do it once in 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.
Currently the exception can only be triggered by IPC (and test) code, because for the RPC path the minimum -blockreservedweight is enforced during node startup. If in the future RPC methods like getblocktemplate allow a custom value per request, they would trigger this exception and the RPC call would return an error - consistent with IPC behavior.
055c3d4 to
40020bc
Compare
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(block_max_weight) matches-maxblockheightand is left alone. The one onBlockCreateOptions(block_max_weight) is an optional, it's set byApplyMiningDefaultsonly if empty.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.