From cba749a9b7a6cd24e8887bddeb0430a1ebc783da Mon Sep 17 00:00:00 2001 From: stickies-v Date: Thu, 12 Jan 2023 11:16:17 +0000 Subject: [PATCH 1/3] refactor: rename local gArgs to args Avoid confusion with the global gArgs --- src/node/miner.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index c2b6fd1dc3..9a6af5f936 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -74,13 +74,13 @@ BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool nBlockMaxWeight = std::max(4000, std::min(MAX_BLOCK_WEIGHT - 4000, options.nBlockMaxWeight)); } -void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options) +void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& options) { // Block resource limits // If -blockmaxweight is not given, limit to DEFAULT_BLOCK_MAX_WEIGHT - options.nBlockMaxWeight = gArgs.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT); - if (gArgs.IsArgSet("-blockmintxfee")) { - std::optional parsed = ParseMoney(gArgs.GetArg("-blockmintxfee", "")); + options.nBlockMaxWeight = args.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT); + if (args.IsArgSet("-blockmintxfee")) { + std::optional parsed = ParseMoney(args.GetArg("-blockmintxfee", "")); options.blockMinFeeRate = CFeeRate{parsed.value_or(DEFAULT_BLOCK_MIN_TX_FEE)}; } else { options.blockMinFeeRate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE}; From ea72c3d9d594b2ea9b3397e64efd08f8563cb400 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Thu, 12 Jan 2023 13:14:19 +0000 Subject: [PATCH 2/3] refactor: avoid duplicating BlockAssembler::Options members Add Options as a member to BlockAssembler to avoid having to assign all the options individually. Additionally brings the struct more in line with how we typically define default and ArgManager values, as e.g. with ChainstateManager::Options and and CTxMemPool::Options --- src/node/miner.cpp | 27 ++++++++++++--------------- src/node/miner.h | 19 ++++++++----------- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 9a6af5f936..a337771f53 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -56,22 +56,19 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman) block.hashMerkleRoot = BlockMerkleRoot(block); } -BlockAssembler::Options::Options() +static BlockAssembler::Options ClampOptions(BlockAssembler::Options options) { - blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE); - nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT; - test_block_validity = true; + // Limit weight to between 4K and DEFAULT_BLOCK_MAX_WEIGHT for sanity: + options.nBlockMaxWeight = std::clamp(options.nBlockMaxWeight, 4000, DEFAULT_BLOCK_MAX_WEIGHT); + return options; } BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options) - : test_block_validity{options.test_block_validity}, - chainparams{chainstate.m_chainman.GetParams()}, - m_mempool(mempool), - m_chainstate(chainstate) + : chainparams{chainstate.m_chainman.GetParams()}, + m_mempool{mempool}, + m_chainstate{chainstate}, + m_options{ClampOptions(options)} { - blockMinFeeRate = options.blockMinFeeRate; - // Limit weight to between 4K and MAX_BLOCK_WEIGHT-4K for sanity: - nBlockMaxWeight = std::max(4000, std::min(MAX_BLOCK_WEIGHT - 4000, options.nBlockMaxWeight)); } void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& options) @@ -176,7 +173,7 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]); BlockValidationState state; - if (test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, + if (m_options.test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, GetAdjustedTime, /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) { throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString())); } @@ -205,7 +202,7 @@ void BlockAssembler::onlyUnconfirmed(CTxMemPool::setEntries& testSet) bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost) const { // TODO: switch to weight-based accounting for packages instead of vsize-based accounting. - if (nBlockWeight + WITNESS_SCALE_FACTOR * packageSize >= nBlockMaxWeight) { + if (nBlockWeight + WITNESS_SCALE_FACTOR * packageSize >= m_options.nBlockMaxWeight) { return false; } if (nBlockSigOpsCost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) { @@ -377,7 +374,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele packageSigOpsCost = modit->nSigOpCostWithAncestors; } - if (packageFees < blockMinFeeRate.GetFee(packageSize)) { + if (packageFees < m_options.blockMinFeeRate.GetFee(packageSize)) { // Everything else we might consider has a lower fee rate return; } @@ -394,7 +391,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele ++nConsecutiveFailed; if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight > - nBlockMaxWeight - 4000) { + m_options.nBlockMaxWeight - 4000) { // Give up if we're close to full and haven't succeeded in a while break; } diff --git a/src/node/miner.h b/src/node/miner.h index ea9e470a64..f1ccffff55 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -6,6 +6,7 @@ #ifndef BITCOIN_NODE_MINER_H #define BITCOIN_NODE_MINER_H +#include #include #include @@ -132,13 +133,6 @@ private: // The constructed block template std::unique_ptr pblocktemplate; - // Configuration parameters for the block size - unsigned int nBlockMaxWeight; - CFeeRate blockMinFeeRate; - - // Whether to call TestBlockValidity() at the end of CreateNewBlock(). - const bool test_block_validity; - // Information on the current status of the block uint64_t nBlockWeight; uint64_t nBlockTx; @@ -156,10 +150,11 @@ private: public: struct Options { - Options(); - size_t nBlockMaxWeight; - CFeeRate blockMinFeeRate; - bool test_block_validity; + // Configuration parameters for the block size + size_t nBlockMaxWeight{DEFAULT_BLOCK_MAX_WEIGHT}; + CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE}; + // Whether to call TestBlockValidity() at the end of CreateNewBlock(). + bool test_block_validity{true}; }; explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool); @@ -172,6 +167,8 @@ public: inline static std::optional m_last_block_weight{}; private: + const Options m_options; + // utility functions /** Clear the block's state and prepare for assembling a new block */ void resetBlock(); From 6a5e88e5cf06a6b410486cc36aba7afece0d9da9 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Mon, 16 Jan 2023 18:04:09 +0000 Subject: [PATCH 3/3] miner: don't re-apply default Options value if argument is unset ApplyArgsManOptions does not need to set default values for missing arguments, these are already defined in the BlockAssembler::Options. This commit changes the interface of ApplyArgsManOptions(). If ApplyArgsManOptions() is called again after a option is changed, this option will no longer be reset to the default value. There is no observed behaviour change due to how ApplyArgsManOptions() is currently used, and the new interface is consistent with e.g. ValidationCacheSizes and MemPoolLimits. --- src/node/miner.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index a337771f53..c7bc9a9a3d 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -74,13 +74,9 @@ BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& options) { // Block resource limits - // If -blockmaxweight is not given, limit to DEFAULT_BLOCK_MAX_WEIGHT - options.nBlockMaxWeight = args.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT); - if (args.IsArgSet("-blockmintxfee")) { - std::optional parsed = ParseMoney(args.GetArg("-blockmintxfee", "")); - options.blockMinFeeRate = CFeeRate{parsed.value_or(DEFAULT_BLOCK_MIN_TX_FEE)}; - } else { - options.blockMinFeeRate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE}; + options.nBlockMaxWeight = args.GetIntArg("-blockmaxweight", options.nBlockMaxWeight); + if (const auto blockmintxfee{args.GetArg("-blockmintxfee")}) { + if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed}; } } static BlockAssembler::Options ConfiguredOptions()