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

stickies-v commented at 1:38 PM on January 12, 2023: contributor

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, 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.

refactor: rename local gArgs to args

Avoid confusion with the global gArgs

cba749a9b7

DrahtBot commented at 1:38 PM on January 12, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27018 (mempool / miner: regularly flush below-minrelayfeerate entries, mine everything in the mempool by glozow)

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.

DrahtBot added the label Refactoring on Jan 12, 2023

maflcko renamed this:
~~refactor: follow-ups for #26695~~
refactor: src/node/miner cleanups, follow-ups for #26695
on Jan 12, 2023

glozow commented at 2:42 PM on January 12, 2023: member

thanks for the followup, light review ACK c14cf65ecde432c15c1e5334cfa4e5e6bdbeec92

in src/node/miner.h:170 in c14cf65ecd outdated

 166 | @@ -172,6 +167,8 @@ class BlockAssembler
 167 |      inline static std::optional<int64_t> m_last_block_weight{};
 168 |  
 169 |  private:
 170 | +    Options m_options;

ajtowns commented at 3:05 PM on January 12, 2023:

could make that const by also adding:

static BlockAssembler::Options clamp_options(const BlockAssembler::Options& opt)
{
    Options clamped{opt};
    clamped.nBlockMaxWeight = std::clamp<size_t>(clamped.nBlockMaxWeight, 4000, DEFAULT_BLOCK_MAX_WEIGHT);
    return clamped;
}

and setting m_options{clamped(options)} in the constructor.

stickies-v commented at 7:03 PM on January 16, 2023:

Thanks - I've adopted your suggestion, although I simplified it a bit by passing Options by value, I think that's equivalent to what you suggested?

in src/node/miner.cpp:77 in c14cf65ecd outdated

  87 | -    if (args.IsArgSet("-blockmintxfee")) {
  88 | -        std::optional<CAmount> parsed = ParseMoney(args.GetArg("-blockmintxfee", ""));
  89 | -        options.blockMinFeeRate = CFeeRate{parsed.value_or(DEFAULT_BLOCK_MIN_TX_FEE)};
  90 | -    } else {
  91 | -        options.blockMinFeeRate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
  92 | +    options.nBlockMaxWeight = args.GetIntArg("-blockmaxweight", options.nBlockMaxWeight);

ajtowns commented at 3:13 PM on January 12, 2023:

This (and below) is a potential behaviour change: if you call ApplyArgsManOptions, then change the arguments, and call ApplyArgsManOptions again on the same options object it won't reset values back to the defaults any more. Probably no big deal, but not really a refactor.

glozow commented at 3:44 PM on January 12, 2023:

True that there is a difference. I think this is better as it matches other ApplyArgsManOptions around the codebase - convention seems to be to default to the value passed in and not look at default constants. For example https://github.com/bitcoin/bitcoin/blob/7386da7a0b08cd2df8ba88dae1fab9d36424b15c/src/node/mempool_args.cpp#L29-L38 https://github.com/bitcoin/bitcoin/blob/7386da7a0b08cd2df8ba88dae1fab9d36424b15c/src/node/validation_cache_args.cpp#L20-L32

stickies-v commented at 4:22 PM on January 12, 2023:

That's a good observation, I hadn't considered that, thanks @ajtowns. I agree with @glozow that I'd prefer this behaviour (I intentionally brought it in line with existing ApplyArgsManOptions() functions and Options objects). In practice, there is no actual behaviour change in the current code as we never use node::ApplyArgsManOptions() in the way you describe here. However, I'm not quite sure if it would be inappropriate to label this commit and PR with refactor: - what do you think?

ajtowns commented at 4:22 PM on January 12, 2023:

Doesn't it make more sense for ApplyArgsManOptions(args, foo) to consistently reset foo to the same thing you'd get if you'd just created foo as a new object? TBH, I think just having an explicit Options(const ArgsManager& args); constructor and writing Options cache_sizes{gArgs} would probably be a better api...

glozow commented at 5:36 PM on January 12, 2023:

TBH, I think just having an explicit Options(const ArgsManager& args); constructor and writing Options cache_sizes{gArgs} would probably be a better api...

I'm mostly going with the idea from this commit message (https://github.com/bitcoin/bitcoin/commit/f1941e8bfd2eecc478c7660434b1ebf6a64095a0):

With this patch, most CTxMemPool construction are along the lines of:

MemPoolOptions mempool_opts{...default overrides...}; ApplyArgsManOptions(argsman, mempool_opts); ...hard overrides... CTxMemPool pool{mempool_opts};

This "compositional" style of building options means that we can omit unnecessary/irrelevant steps wherever we want but also maintain full customizability.

For example:

For users of libbitcoinkernel, where we eventually want to remove ArgsManager, they simply won't call (or even know about) ApplyArgsManOptions.

As in, {mempool, block assembler, cache, etc.} ::Options struct living in kernel/ shouldn't know about the ArgsManager which lives in node/. Otherwise I agree a ctor using argsman makes sense.

stickies-v commented at 7:11 PM on January 16, 2023:

As @glozow commented, my understanding is that we're moving in the direction of separating kernel/ and node/ and the proposed (admittedly more elegant) constructor would go against that.

Since I think the new behaviour of ApplyArgsManOptions() is preferable but I appreciate your concern about this not being a pure refactor commit, I've carved out this interface change into a separate non-refactor commit to make that more clear. I've also updated the PR description to reflect this. @ajtowns happy to reopen this if it doesn't properly address your concerns?

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

ea72c3d9d5

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.

6a5e88e5cf

stickies-v force-pushed on Jan 16, 2023

stickies-v renamed this:
~~refactor: src/node/miner cleanups, follow-ups for #26695~~
src/node/miner cleanups, follow-ups for #26695
on Jan 16, 2023

stickies-v commented at 7:13 PM on January 16, 2023: contributor

Force pushed to address review comments (thank you @ajtowns):

made m_options const by adding a ClampOptions helper function
carved out the ApplyArgsManOptions() (potential/unobserved) behaviour change into a separate commit

fanquake requested review from glozow on Jan 25, 2023

in src/node/miner.h:9 in 6a5e88e5cf

   5 | @@ -6,6 +6,7 @@
   6 |  #ifndef BITCOIN_NODE_MINER_H
   7 |  #define BITCOIN_NODE_MINER_H
   8 |  
   9 | +#include <policy/policy.h>

TheCharlatan commented at 10:19 AM on February 20, 2023:

Nit: IWYU says you can now remove this include from miner.cpp

TheCharlatan commented at 10:37 AM on February 20, 2023: contributor

Light code review ACK 6a5e88e5cf06a6b410486cc36aba7afece0d9da9

glozow commented at 11:27 AM on February 20, 2023: member

ACK 6a5e88e5cf

glozow merged this on Feb 20, 2023

glozow closed this on Feb 20, 2023

sidhujag referenced this in commit 40eec9149b on Feb 25, 2023

stickies-v deleted the branch on Mar 14, 2023

bitcoin locked this on Mar 13, 2024