This is part 1/n of the effort to de-globalize ChainstateManager
Reviewers: Looking for tested/Code-Review/plain-ACKs
Context
In many of our tests, we manually instantiate NodeContexts or ChainstateManagers in the test code, which is error prone. Instead, we should create or use existing references because:
- Before we de-globalize
ChainstateManager, much of our code still acts ong_chainman(our globalChainstateManager), sometimes even when you're calling a method on a specific instance ofChainstateManager! This means that we may act on two instances ofChainstateManager, which is most likely not what we want. - Using existing references (initialized by the
{Basic,}TestingSetupconstructors) means that you're acting on objects which are properly initialized, instead of "just initialized enough for this dang test to pass". Also, they're already there! It's free! - By acting on the right object, we also allow the review-only assertions in future commits of de-globalize
ChainstateManagerto work and demonstrate correctness.
Some more detailed debugging notes can be found in the first commit, reproduced below:
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.
This change adds 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.
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.
In the test chainstatemanager_rebalance_caches, an additional
LoadGenesisBlock call is added as MaybeReblanaceCaches eventually calls
FlushBlockFile, which tries to access vinfoBlockFile[nLastBlockFile],
which is out of bounds when LoadGenesisBlock hasn't been called yet.
-----
Note for the future:
In a previous version of this change, I put ChainTestingSetup between
BasicTestingSetup and TestingSetup such that TestingSetup inherited from
ChainTestingSetup.
This was suboptimal, and showed how the class con/destructor inheritance
structure we have for these TestingSetup classes is probably not the
most suitable abstraction. In particular, for both TestingSetup and
ChainTestingSetup, we need to stop the scheduler first before anything
else. Otherwise classes depending on the scheduler may be referenced
by the scheduler after said classes are freed. This means that there's
no clear parallel between our teardown code and C++'s destructuring
order for class hierarchies.
Future work should strive to coalesce (as much as possible) test and
non-test init codepaths and perhaps structure it in a more fail-proof
way.
