src/node/miner cleanups, follow-ups for #26695 #26883
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Avoid confusion with the global gArgs
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
maflcko
changed the title
ajtowns
reviewed
Jan 12, 2023
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
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.
stickies-v
force-pushed
the
w/n26695-followups
branch
from
c14cf65 to
6a5e88e
Compare
stickies-v
changed the title
|
Force pushed to address review comments (thank you @ajtowns):
|
| #ifndef BITCOIN_NODE_MINER_H | ||
| #define BITCOIN_NODE_MINER_H | ||
|
|
||
| #include <policy/policy.h> |
There was a problem hiding this comment.
Nit: IWYU says you can now remove this include from miner.cpp
|
ACK 6a5e88e |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two follow-ups for #26695, both refactoring and no observed (*) behaviour change:
gArgstoargsbecause it's not actually a globalBlockAssembler::Optionsas a (private) member toBlockAssemblerto avoid having to assign all the options individually, essentially duplicating themReduces 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.