Seems confusing to pass chainparams to each method individually, when the params can’t change anyway for the whole lifetime of the block manager, and also must be equal to the ones used by the chainstate manager.
Fix this issue by removing them from the methods and instead storing a reference once in a member field.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | dergoegge, TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
Concept ACK
Should we do the same for Consensus::Params?
Should we do the same for Consensus::Params?
Yes, this is what this pull is doing.
147@@ -147,6 +148,9 @@ class BlockManager
148 : m_prune_mode{opts.prune_target > 0},
149 m_opts{std::move(opts)} {};
150
151+ const CChainParams& GetParams() const { return m_opts.chainparams; }
152+ const Consensus::Params& GetConsensus() const { return m_opts.chainparams.GetConsensus(); }
Yes, but why?
I feel like in the long run, only having one way of getting the params would be the consistent thing to do. Storing a public reference to the params on various components seems weird given that the params are already a global (might as well use the global directly then?).
Also, why should these getters be part of the public interface for the block storage? Doesn’t seem right to me.
Yes, this is what this pull is doing.
Sorry, I was confused because Consensus::Params are stil being passed to ReadBlockFromDisk.
and use it in blockstorage.cpp
pindex can not be nullptr, so document that, and clear it up in the next
commit.
Also, replace pointer with reference while touching the signature.
class BlockManager, so fixing them is non-trivial and should happen in a follow-up.
20@@ -21,23 +21,26 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
21 BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
22 {
23 const auto params {CreateChainParams(ArgsManager{}, CBaseChainParams::MAIN)};
24- BlockManager blockman{{}};
25+ node::BlockManager::Options blockman_opts{
const (here and elsewhere) and skip the node:: namespacing, since it is imported above. If this is merged before #27125 I will follow-up there.
I left it mutable to make it less code-churn to run ApplyArgsManOptions in the future, which you called in 27125 at some point, but no longer in the current state.
I’ll remove node:: in the next push or maybe a follow-up.
ACK fa5d7c39eb992467e43b12db213b27913374fb83
Thank you for following up so quickly!
Is this some kind of dependency for libbitcoinkernel or for something other than general tidy-up impulses? If so, feel free to merge - assumeutxo is fresh in my brain and handling the rebase won’t be so painful as a result.
Otherwise, wouldn’t mind this getting shelved for later. But really no big deal either way. I’d say if you’re going to merge do it soon so that I can manage the rebase with fresh context.
Is this some kind of dependency for libbitcoinkernel or for something other than general tidy-up impulses?
It spawned out of here: #27125 (review). If you’re happy to rebase over then I’m just going to go-ahead and merge this now. @TheCharlatan can then address any of his own comments as part of #27125 (which is also going to conflict with 15606, so probably worth having a look there as well.)