From 8daac1d6eba79991a1d2758ec24d314f3874cf66 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 18 May 2026 15:58:39 +0200 Subject: [PATCH] mining: add block create option helpers Move block template defaulting into helper functions for BlockCreateOptions. FlattenMiningOptions() fills hardcoded defaults and MergeMiningOptions() 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 --- src/node/interfaces.cpp | 31 ++++++++++----- src/node/miner.cpp | 50 +++++++++-------------- src/node/miner.h | 20 +++------- src/node/mining_args.cpp | 70 +++++++++++++++++++++++++-------- src/node/mining_args.h | 21 +++++++++- src/node/mining_types.h | 17 +++++++- src/rpc/mining.cpp | 9 +++-- src/test/fuzz/cmpctblock.cpp | 2 +- src/test/fuzz/tx_pool.cpp | 8 ++-- src/test/miner_tests.cpp | 2 +- test/functional/mining_basic.py | 6 +-- 11 files changed, 151 insertions(+), 85 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 7abd4b75b2c..7a078c81cb2 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -86,6 +87,7 @@ using interfaces::Rpc; using interfaces::WalletLoader; using kernel::ChainstateRole; using node::BlockAssembler; +using node::BlockCreateOptions; using node::BlockWaitOptions; using node::CoinbaseTx; using util::Join; @@ -867,9 +869,9 @@ public: class BlockTemplateImpl : public BlockTemplate { public: - explicit BlockTemplateImpl(BlockAssembler::Options assemble_options, + explicit BlockTemplateImpl(BlockCreateOptions create_options, std::unique_ptr block_template, - const NodeContext& node) : m_assemble_options(std::move(assemble_options)), + const NodeContext& node) : m_create_options(std::move(create_options)), m_block_template(std::move(block_template)), m_node(node) { @@ -914,8 +916,14 @@ public: std::unique_ptr waitNext(BlockWaitOptions options) override { - auto new_template = WaitAndCreateNewBlock(chainman(), notifications(), m_node.mempool.get(), m_block_template, options, m_assemble_options, m_interrupt_wait); - if (new_template) return std::make_unique(m_assemble_options, std::move(new_template), m_node); + auto new_template = WaitAndCreateNewBlock(chainman(), + notifications(), + m_node.mempool.get(), + m_block_template, + /*wait_options=*/options, + /*create_options=*/m_create_options, + /*interrupt_wait=*/m_interrupt_wait); + if (new_template) return std::make_unique(m_create_options, std::move(new_template), m_node); return nullptr; } @@ -924,7 +932,7 @@ public: InterruptWait(notifications(), m_interrupt_wait); } - const BlockAssembler::Options m_assemble_options; + const BlockCreateOptions m_create_options; const std::unique_ptr m_block_template; @@ -990,10 +998,15 @@ public: // Also wait during the final catch-up moments after IBD. if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip, m_interrupt_mining)) return {}; } - - BlockAssembler::Options assemble_options{options}; - ApplyArgsManOptions(*Assert(m_node.args), assemble_options); - return std::make_unique(assemble_options, BlockAssembler{chainman().ActiveChainstate(), m_node.mempool.get(), assemble_options}.CreateNewBlock(), m_node); + const auto args_options{*Assert(ReadMiningArgs(*Assert(m_node.args)))}; + const BlockCreateOptions create_options{MergeMiningOptions(options, args_options)}; + return std::make_unique(create_options, + BlockAssembler{ + chainman().ActiveChainstate(), + m_node.mempool.get(), + create_options, + }.CreateNewBlock(), + m_node); } void interrupt() override diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 41c1d997128..e1b6a480fbd 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -76,40 +77,27 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman) block.hashMerkleRoot = BlockMerkleRoot(block); } -static BlockAssembler::Options ClampOptions(BlockAssembler::Options options) +static BlockCreateOptions ClampOptions(BlockCreateOptions options) { - // Apply DEFAULT_BLOCK_RESERVED_WEIGHT and DEFAULT_BLOCK_MAX_WEIGHT when the caller left it unset. - options.block_reserved_weight = std::clamp(options.block_reserved_weight.value_or(DEFAULT_BLOCK_RESERVED_WEIGHT), MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT); + options = FlattenMiningOptions(std::move(options)); + options.block_reserved_weight = std::clamp(*options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT); options.coinbase_output_max_additional_sigops = std::clamp(options.coinbase_output_max_additional_sigops, 0, MAX_BLOCK_SIGOPS_COST); // Limit weight to between block_reserved_weight and MAX_BLOCK_WEIGHT for sanity: // block_reserved_weight can safely exceed -blockmaxweight, but the rest of the block template will be empty. - options.block_max_weight = std::clamp(options.block_max_weight.value_or(DEFAULT_BLOCK_MAX_WEIGHT), *options.block_reserved_weight, MAX_BLOCK_WEIGHT); + options.block_max_weight = std::clamp(*options.block_max_weight, *options.block_reserved_weight, MAX_BLOCK_WEIGHT); return options; } -BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options) +BlockAssembler::BlockAssembler(Chainstate& chainstate, + const CTxMemPool* mempool, + BlockCreateOptions options) : chainparams{chainstate.m_chainman.GetParams()}, m_mempool{options.use_mempool ? mempool : nullptr}, m_chainstate{chainstate}, - m_options{ClampOptions(options)} + m_options{ClampOptions(std::move(options))} { } -void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& options) -{ - // Block resource limits - if (!options.block_max_weight) { - options.block_max_weight = args.GetArg("-blockmaxweight"); - } - if (const auto blockmintxfee{args.GetArg("-blockmintxfee")}) { - if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed}; - } - options.print_modified_fee = args.GetBoolArg("-printpriority", options.print_modified_fee); - if (!options.block_reserved_weight) { - options.block_reserved_weight = args.GetArg("-blockreservedweight"); - } -} - void BlockAssembler::resetBlock() { // Reserve space for fixed-size block header, txs count, and coinbase tx. @@ -274,7 +262,7 @@ void BlockAssembler::AddToBlock(const CTxMemPoolEntry& entry) nBlockSigOpsCost += entry.GetSigOpCost(); nFees += entry.GetFee(); - if (m_options.print_modified_fee) { + if (*m_options.print_modified_fee) { LogInfo("fee rate %s txid %s\n", CFeeRate(entry.GetModifiedFee(), entry.GetTxSize()).ToString(), entry.GetTx().GetHash().ToString()); @@ -300,7 +288,7 @@ void BlockAssembler::addChunks() while (selected_transactions.size() > 0) { // Check to see if min fee rate is still respected. - if (ByRatio{chunk_feerate_vsize} < ByRatio{m_options.blockMinFeeRate.GetFeePerVSize()}) { + if (ByRatio{chunk_feerate_vsize} < ByRatio{m_options.block_min_fee_rate->GetFeePerVSize()}) { // Everything else we might consider has a lower feerate return; } @@ -367,8 +355,8 @@ std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainma KernelNotifications& kernel_notifications, CTxMemPool* mempool, const std::unique_ptr& block_template, - const BlockWaitOptions& options, - const BlockAssembler::Options& assemble_options, + const BlockWaitOptions& wait_options, + const BlockCreateOptions& create_options, bool& interrupt_wait) { // Delay calculating the current template fees, just in case a new block @@ -378,7 +366,7 @@ std::unique_ptr 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 + options.timeout; + const auto deadline = now + wait_options.timeout; const MillisecondsDouble tick{1000}; const bool allow_min_difficulty{chainman.GetParams().GetConsensus().fPowAllowMinDifficultyBlocks}; @@ -425,12 +413,12 @@ std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainma * * We'll also create a new template if the tip changed during this iteration. */ - if (options.fee_threshold < MAX_MONEY || tip_changed) { + if (wait_options.fee_threshold < MAX_MONEY || tip_changed) { auto new_tmpl{BlockAssembler{ chainman.ActiveChainstate(), mempool, - assemble_options} - .CreateNewBlock()}; + create_options + }.CreateNewBlock()}; // If the tip changed, return the new template regardless of its fees. if (tip_changed) return new_tmpl; @@ -442,8 +430,8 @@ std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainma // Check if fees increased enough to return the new template const CAmount new_fees = std::accumulate(new_tmpl->vTxFees.begin(), new_tmpl->vTxFees.end(), CAmount{0}); - Assume(options.fee_threshold != MAX_MONEY); - if (new_fees >= current_fees + options.fee_threshold) return new_tmpl; + Assume(wait_options.fee_threshold != MAX_MONEY); + if (new_fees >= current_fees + wait_options.fee_threshold) return new_tmpl; } now = NodeClock::now(); diff --git a/src/node/miner.h b/src/node/miner.h index b579dc68893..fcb5649f63a 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -38,8 +38,6 @@ using interfaces::BlockRef; namespace node { class KernelNotifications; -static const bool DEFAULT_PRINT_MODIFIED_FEE = false; - struct CBlockTemplate { CBlock block; @@ -79,12 +77,9 @@ private: Chainstate& m_chainstate; public: - struct Options : BlockCreateOptions { - CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE}; - bool print_modified_fee{DEFAULT_PRINT_MODIFIED_FEE}; - }; - - explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options); + explicit BlockAssembler(Chainstate& chainstate, + const CTxMemPool* mempool, + BlockCreateOptions create_options); /** Construct a new block template */ std::unique_ptr CreateNewBlock(); @@ -95,7 +90,7 @@ public: inline static std::optional m_last_block_weight{}; private: - const Options m_options; + const BlockCreateOptions m_options; // utility functions /** Clear the block's state and prepare for assembling a new block */ @@ -131,9 +126,6 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman); -/** Apply -blockmintxfee and -blockmaxweight options from ArgsManager to BlockAssembler options. */ -void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options); - /* Compute the block's merkle root, insert or replace the coinbase transaction and the merkle root into the block */ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce); @@ -148,8 +140,8 @@ std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainma KernelNotifications& kernel_notifications, CTxMemPool* mempool, const std::unique_ptr& block_template, - const BlockWaitOptions& options, - const BlockAssembler::Options& assemble_options, + const BlockWaitOptions& wait_options, + const BlockCreateOptions& create_options, bool& interrupt_wait); /* Locks cs_main and returns the block hash and block height of the active chain if it exists; otherwise, returns nullopt.*/ diff --git a/src/node/mining_args.cpp b/src/node/mining_args.cpp index ece422a808e..ef859655cae 100644 --- a/src/node/mining_args.cpp +++ b/src/node/mining_args.cpp @@ -6,13 +6,17 @@ #include #include +#include #include #include +#include +#include #include #include #include #include +#include using common::AmountErrMsg; using util::Error; @@ -20,28 +24,60 @@ using util::Result; namespace node { -Result ReadMiningArgs(const ArgsManager& args) +Result CheckMiningOptions(const BlockCreateOptions& options, bool use_argnames) { - if (const auto arg{args.GetArg("-blockmintxfee")}) { - if (!ParseMoney(*arg)) { - return Error{AmountErrMsg("blockmintxfee", *arg)}; - } + if (options.block_max_weight && *options.block_max_weight > MAX_BLOCK_WEIGHT) { + return Error{Untranslated(strprintf("%s (%d) exceeds consensus maximum block weight (%d)", + use_argnames ? "-blockmaxweight" : "block_max_weight", + *options.block_max_weight, MAX_BLOCK_WEIGHT))}; } - - const uint64_t max_block_weight{args.GetArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT)}; - if (max_block_weight > MAX_BLOCK_WEIGHT) { - return Error{strprintf(_("Specified -blockmaxweight (%d) exceeds consensus maximum block weight (%d)"), max_block_weight, MAX_BLOCK_WEIGHT)}; + if (options.block_reserved_weight && *options.block_reserved_weight > MAX_BLOCK_WEIGHT) { + return Error{Untranslated(strprintf("%s (%d) exceeds consensus maximum block weight (%d)", + use_argnames ? "-blockreservedweight" : "block_reserved_weight", + *options.block_reserved_weight, MAX_BLOCK_WEIGHT))}; } - - const uint64_t block_reserved_weight{args.GetArg("-blockreservedweight", DEFAULT_BLOCK_RESERVED_WEIGHT)}; - if (block_reserved_weight > MAX_BLOCK_WEIGHT) { - return Error{strprintf(_("Specified -blockreservedweight (%d) exceeds consensus maximum block weight (%d)"), block_reserved_weight, MAX_BLOCK_WEIGHT)}; + if (options.block_reserved_weight && *options.block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) { + return Error{Untranslated(strprintf("%s (%d) is lower than minimum safety value of (%d)", + use_argnames ? "-blockreservedweight" : "block_reserved_weight", + *options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT))}; } - if (block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) { - return Error{strprintf(_("Specified -blockreservedweight (%d) is lower than minimum safety value of (%d)"), block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT)}; - } - return {}; } +Result ReadMiningArgs(const ArgsManager& args) +{ + BlockCreateOptions options; + if (const auto arg{args.GetArg("-blockmintxfee")}) { + std::optional block_min_tx_fee{ParseMoney(*arg)}; + if (!block_min_tx_fee) return Error{AmountErrMsg("blockmintxfee", *arg)}; + options.block_min_fee_rate = CFeeRate{*block_min_tx_fee}; + } + + if (const auto arg{args.GetBoolArg("-printpriority")}) options.print_modified_fee = *arg; + + options.block_reserved_weight = args.GetArg("-blockreservedweight"); + options.block_max_weight = args.GetArg("-blockmaxweight"); + + if (auto result{CheckMiningOptions(options, /*use_argnames=*/true)}; !result) return Error{util::ErrorString(result)}; + return options; +} + +BlockCreateOptions FlattenMiningOptions(BlockCreateOptions options) +{ + if (!options.block_min_fee_rate) options.block_min_fee_rate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE}; + if (!options.print_modified_fee) options.print_modified_fee = DEFAULT_PRINT_MODIFIED_FEE; + if (!options.block_reserved_weight) options.block_reserved_weight = DEFAULT_BLOCK_RESERVED_WEIGHT; + if (!options.block_max_weight) options.block_max_weight = DEFAULT_BLOCK_MAX_WEIGHT; + return options; +} + +BlockCreateOptions MergeMiningOptions(BlockCreateOptions x, const BlockCreateOptions& y) +{ + if (!x.block_min_fee_rate) x.block_min_fee_rate = y.block_min_fee_rate; + if (!x.print_modified_fee) x.print_modified_fee = y.print_modified_fee; + if (!x.block_reserved_weight) x.block_reserved_weight = y.block_reserved_weight; + if (!x.block_max_weight) x.block_max_weight = y.block_max_weight; + return x; +} + } // namespace node diff --git a/src/node/mining_args.h b/src/node/mining_args.h index 01e695ee82c..e3ebf959c0e 100644 --- a/src/node/mining_args.h +++ b/src/node/mining_args.h @@ -5,13 +5,32 @@ #ifndef BITCOIN_NODE_MINING_ARGS_H #define BITCOIN_NODE_MINING_ARGS_H +#include #include class ArgsManager; namespace node { -[[nodiscard]] util::Result ReadMiningArgs(const ArgsManager& args); +static const bool DEFAULT_PRINT_MODIFIED_FEE = false; + +/** + * Read the mining options set in \p args. Returns an error if one was + * encountered. + */ +[[nodiscard]] util::Result ReadMiningArgs(const ArgsManager& args); + +/** Check option values for validity. Returns an error for invalid values. */ +[[nodiscard]] util::Result CheckMiningOptions(const BlockCreateOptions& options, bool use_argnames); + +/** Replace null optional values with their hardcoded defaults. */ +[[nodiscard]] BlockCreateOptions FlattenMiningOptions(BlockCreateOptions options); + +/** + * Merge two BlockCreateOptions structs, replacing null values in \p x with + * non-null values from \p y. + */ +[[nodiscard]] BlockCreateOptions MergeMiningOptions(BlockCreateOptions x, const BlockCreateOptions& y); } // namespace node diff --git a/src/node/mining_types.h b/src/node/mining_types.h index 30206c2e87c..6e3ee0f4afb 100644 --- a/src/node/mining_types.h +++ b/src/node/mining_types.h @@ -12,7 +12,7 @@ #define BITCOIN_NODE_MINING_TYPES_H #include -#include +#include #include #include #include