validation: Prune (in)direct g_chainman usage related to ::LookupBlockIndex (bundle 1) #20050
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
::LookupBlockIndex::LookupBlockIndex (bundle 1)
::LookupBlockIndex (bundle 1)
maflcko
reviewed
Oct 1, 2020
| @@ -1588,7 +1588,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA | |||
| // If the loaded chain has a wrong genesis, bail out immediately | |||
| // (we're likely using a testnet datadir, or the other way around). | |||
| if (!chainman.BlockIndex().empty() && | |||
| !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) { | |||
| !g_chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) { | |||
There was a problem hiding this comment.
What is the point of moving a helper that accesses the global to be a member method of the class of the global, which is then called from the global again?
This should use the local reference (like the line immediately above it)
Suggested change
| !g_chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) { | |
| !chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) { |
Feedback also applies to all other lines in this patch ;)
There was a problem hiding this comment.
Suggested change
| !g_chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) { | |
| !g_chainman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) { |
Also, to avoid verbosity, chainman could forward the call to its blockman member.
There was a problem hiding this comment.
Continuing discussion here: #20049 (comment)
Previously, the validation_chainstatemanager_tests test suite
instantiated its own duplicate ChainstateManager on which tests were
performed.
This wasn't a problem for the specific actions performed in
that suite. However, the existence of this duplicate ChainstateManager
and the fact that many of our validation static functions reach for
g_chainman, ::Chain(state|)Active means we may end up acting on two
different CChainStates should we write more extensive tests in the
future.
Adding a new ChainTestingSetup which performs all initialization
previously done by TestingSetup except:
1. Mempool sanity check frequency setting
2. ChainState initialization
3. Genesis Activation
4. {Ban,Conn,Peer}Man initialization
Means that we will no longer need to initialize a duplicate
ChainstateManger in order to test the initialization codepaths of
CChainState and ChainstateManager. TestingSetup can also cleanly
inherits from this new ChainTestingSetup.
Lastly, this change has the additional benefit of allowing for
review-only assertions meant to show correctness to work in future work
de-globalizing g_chainman.
[META] This commit should be followed up by a scripted-diff commit which
fixes calls to LookupBlockIndex tree-wide.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
LookupBlockIndex only acts on BlockManager.
[META] In a previous commit, we moved ::LookupBlockIndex to become a
member function of BlockManager. This commit is split out from
that one since it can be expressed nicely as a scripted-diff.
-BEGIN VERIFY SCRIPT-
find_regex='LookupBlockIndex' \
&& git grep -l -E "$find_regex" -- src \
| grep -v '^src/validation\.\(cpp\|h\)$' \
| xargs sed -i -E "s@${find_regex}@g_chainman.m_blockman.LookupBlockIndex@g"
-END VERIFY SCRIPT-
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
FindForkInGlobalIndex only acts on BlockManager.
Note to reviewers: Since FindForkInGlobalIndex is always called with
::ChainActive() as its first parameter, it is possible to move
FindForkInGlobalIndex to CChainState and remove this const CChain&
parameter to instead use m_chain. However, it seems like the original
intention was for FindForkInGlobalIndex to work with _any_ chain, not
just the current active chain. Let me know if this should be changed.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
GetSpendHeight only acts on BlockManager.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
GetLastCheckPoint mainly acts on BlockManager.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
Also guard it with ::cs_main like ChainstateManager does.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
Instead use CChainState::ActivateBestChain, which is what the global one calls anyway.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
LoadExternalBlockFile mainly acts on CChainState.
…lockHeaders
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
-BEGIN VERIFY SCRIPT- git grep -lwF 'assert(std::addressof' | xargs sed -i -e '/assert(std::addressof/d' -END VERIFY SCRIPT-
41265b2 to
1ce3af3
Compare
|
Closing temporarily for more definite decisions on how to split up #20158 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR builds on the work of #19927 towards #20049
Please see #20049 for the broader context, especially this section which applies specifically to this PR:
Please also read the note in the commit message of 407faf2 (validation: Move FindForkInGlobalIndex to BlockManager), and let me know your thoughts.