foster/bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-03-06 14:19:59 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#26883: src/node/miner cleanups, follow-ups for #26695

Browse source 
6a5e88e5cf miner: don't re-apply default Options value if argument is unset (stickies-v)
ea72c3d9d5 refactor: avoid duplicating BlockAssembler::Options members (stickies-v)
cba749a9b7 refactor: rename local gArgs to args (stickies-v)

Pull request description:

  Two follow-ups for #26695, both refactoring and no observed (*) behaviour change:
  - Rename `gArgs` to `args` because it's not actually a global
  - Add `BlockAssembler::Options` as a (private) member to `BlockAssembler` to avoid having to assign all the options individually, essentially duplicating them

  Reduces LoC and makes the code more readable, in my opinion.

  ---

  (*) as [pointed out by ajtowns](https://github.com/bitcoin/bitcoin/pull/26883#discussion_r1068247937), this PR changes the interface of `ApplyArgsManOptions()`, making this not a pure refactoring PR. In practice, `ApplyArgsManOptions()` is never called in such a way that this leads to observed behaviour change. Regardless, I've carved out the potential behaviour change into a separate commit and would be okay with dropping it, should it turn out to be controversial.

ACKs for top commit:
  glozow:
    ACK 6a5e88e5cf
  TheCharlatan:
    Light code review ACK 6a5e88e5cf

Tree-SHA512: 15c30442ff0e070b1a58dc4c9615550d619ce35b4a2596b2c0a9d790259bbf987cab708f7cbb1057a8cf8b4c3226f3ad981282d3499ac442094806492a5f68ce
This commit is contained in:
glozow 2023-02-20 11:31:13 +00:00
commit 08b65df1bb
No known key found for this signature in database
GPG key ID: BA03F4DBE0C63FB4
2 changed files with 24 additions and 34 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
src/node/miner.cpp
View file

@ -56,34 +56,27 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
block.hashMerkleRoot = BlockMerkleRoot(block); block.hashMerkleRoot = BlockMerkleRoot(block);
} }
BlockAssembler::Options::Options() static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
{ {
blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE); // Limit weight to between 4K and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT; options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, 4000, DEFAULT_BLOCK_MAX_WEIGHT);
test_block_validity = true; return options;
} }
BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options) BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options)
: test_block_validity{options.test_block_validity}, : chainparams{chainstate.m_chainman.GetParams()},
chainparams{chainstate.m_chainman.GetParams()}, m_mempool{mempool},
m_mempool(mempool), m_chainstate{chainstate},
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<size_t>(4000, std::min<size_t>(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 // Block resource limits
// If -blockmaxweight is not given, limit to DEFAULT_BLOCK_MAX_WEIGHT options.nBlockMaxWeight = args.GetIntArg("-blockmaxweight", options.nBlockMaxWeight);
options.nBlockMaxWeight = gArgs.GetIntArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT); if (const auto blockmintxfee{args.GetArg("-blockmintxfee")}) {
if (gArgs.IsArgSet("-blockmintxfee")) { if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed};
std::optional<CAmount> parsed = ParseMoney(gArgs.GetArg("-blockmintxfee", ""));
options.blockMinFeeRate = CFeeRate{parsed.value_or(DEFAULT_BLOCK_MIN_TX_FEE)};
} else {
options.blockMinFeeRate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
} }
} }
static BlockAssembler::Options ConfiguredOptions() static BlockAssembler::Options ConfiguredOptions()
@ -176,7 +169,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]); pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]);
BlockValidationState state; 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)) { GetAdjustedTime, /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString())); throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString()));
} }
@ -205,7 +198,7 @@ void BlockAssembler::onlyUnconfirmed(CTxMemPool::setEntries& testSet)
bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost) const bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost) const
{ {
// TODO: switch to weight-based accounting for packages instead of vsize-based accounting. // 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; return false;
} }
if (nBlockSigOpsCost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) { if (nBlockSigOpsCost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) {
@ -377,7 +370,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
packageSigOpsCost = modit->nSigOpCostWithAncestors; 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 // Everything else we might consider has a lower fee rate
return; return;
} }
@ -394,7 +387,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
++nConsecutiveFailed; ++nConsecutiveFailed;
if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight > 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 // Give up if we're close to full and haven't succeeded in a while
break; break;
} }

19
src/node/miner.h
View file

@ -6,6 +6,7 @@
#ifndef BITCOIN_NODE_MINER_H #ifndef BITCOIN_NODE_MINER_H
#define BITCOIN_NODE_MINER_H #define BITCOIN_NODE_MINER_H
#include <policy/policy.h>
#include <primitives/block.h> #include <primitives/block.h>
#include <txmempool.h> #include <txmempool.h>
@ -132,13 +133,6 @@ private:
// The constructed block template // The constructed block template
std::unique_ptr<CBlockTemplate> pblocktemplate; std::unique_ptr<CBlockTemplate> 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 // Information on the current status of the block
uint64_t nBlockWeight; uint64_t nBlockWeight;
uint64_t nBlockTx; uint64_t nBlockTx;
@ -156,10 +150,11 @@ private:
public: public:
struct Options { struct Options {
Options(); // Configuration parameters for the block size
size_t nBlockMaxWeight; size_t nBlockMaxWeight{DEFAULT_BLOCK_MAX_WEIGHT};
CFeeRate blockMinFeeRate; CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
bool test_block_validity; // Whether to call TestBlockValidity() at the end of CreateNewBlock().
bool test_block_validity{true};
}; };
explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool); explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool);
@ -172,6 +167,8 @@ public:
inline static std::optional<int64_t> m_last_block_weight{}; inline static std::optional<int64_t> m_last_block_weight{};
private: private:
const Options m_options;
// utility functions // utility functions
/** Clear the block's state and prepare for assembling a new block */ /** Clear the block's state and prepare for assembling a new block */
void resetBlock(); void resetBlock();