validation: Prune (in)direct g_chainman usage related to ::LookupBlockIndex (bundle 1)

dongcarl commented at 10:25 pm on September 30, 2020: member

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:

…for some functions (LookupBlockIndex being the main culprit), a resolution of their dependency on the g_chainman/::Chain(state|)Active() globals results in an increase in apparent references to g_chainman/::Chain(state|)Active(). However, it is important to note that these are not new dependencies by any means, those dependency were always there – they was just hidden by Qux’s use of the globals. Furthermore, all of these “new” apparent references to g_chainman/::Chain(state|)Active() will be dealt with as we prune the call graph upward.

Please also read the note in the commit message of 407faf2ff364cc778083c145a77b0f150232121d (validation: Move FindForkInGlobalIndex to BlockManager), and let me know your thoughts.

dongcarl added the label Refactoring on Sep 30, 2020

dongcarl added the label Validation on Sep 30, 2020

dongcarl renamed this:
~~validation bundle 1: Prune (in)direct g_chainman usage related to `::LookupBlockIndex`~~
validation: Prune (in)direct g_chainman usage related to `::LookupBlockIndex` (bundle 1)
on Sep 30, 2020

dongcarl renamed this:
~~validation: Prune (in)direct g_chainman usage related to `::LookupBlockIndex` (bundle 1)~~
validation: Prune (in)direct g_chainman usage related to ::LookupBlockIndex (bundle 1)
on Sep 30, 2020

test: Add new ChainTestingSetup and use it

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.

b419519dd2

bench: [FIX] Use existing NodeContext in WalletBalance benchmarks ef1b08c073

wallet/test: [FIX] Use existing NodeContext in wallet_tests a9539e725b

qt/test: [FIX] Add forgotten Context setting in RPCNestedTests b927d6891e

validation: Move LookupBlockIndex to BlockManager

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

8615dbd125

scripted-diff: Use BlockManager::LookupBlockIndex

[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-

7ef17f7a52

validation: Move FindForkInGlobalIndex to BlockManager

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

8dbedcb3bf

validation: Move GetSpendHeight to BlockManager

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

05a9e983b4

validation: Move GetLastCheckpoint to 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.

6d66fca3cf

validation: Pass in blockman to ContextualCheckBlockHeader

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

8f2aadb89f

validation: Make CChainState.m_blockman public

Also guard it with ::cs_main like ChainstateManager does.

867330aedf

validation: Pass in chainstate to TestBlockValidity

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

dfdb9eaa9c

validation: Pass in chainstate to ::NotifyHeaderTip

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

af1e058645

validation: Remove global ::ActivateBestChain

Instead use CChainState::ActivateBestChain, which is what the global one
calls anyway.

7e15a2c88e

validation: Move LoadExternalBlockFile to CChainState

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

2f2e8a2395

validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

18fab989be

validation: Use existing chainman in ChainstateManager::ProcessNewBlock

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

0fc234246f

scripted-diff: Remove review-only assertions

-BEGIN VERIFY SCRIPT-
git grep -lwF 'assert(std::addressof' | xargs sed -i -e '/assert(std::addressof/d'
-END VERIFY SCRIPT-

1ce3af385f

in src/init.cpp:1619 in 41265b2e0f outdated

1587@@ -1588,7 +1588,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1588                 // If the loaded chain has a wrong genesis, bail out immediately
1589                 // (we're likely using a testnet datadir, or the other way around).
1590                 if (!chainman.BlockIndex().empty() &&
1591-                        !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
1592+                        !g_chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {

MarcoFalke commented at 6:51 am on October 1, 2020:

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)

0                        !chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {

Feedback also applies to all other lines in this patch ;)

MarcoFalke commented at 6:53 am on October 1, 2020:

0                        !g_chainman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {

Also, to avoid verbosity, chainman could forward the call to its blockman member.

dongcarl commented at 7:00 pm on October 1, 2020:

Continuing discussion here: #20049 (comment)

dongcarl force-pushed on Oct 15, 2020

dongcarl commented at 8:09 pm on October 15, 2020: member

Closing temporarily for more definite decisions on how to split up #20158

dongcarl closed this on Oct 15, 2020

DrahtBot locked this on Feb 15, 2022

validation: Prune (in)direct g_chainman usage related to ::LookupBlockIndex (bundle 1) #20050