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:
0Previously, the validation_chainstatemanager_tests test suite
1instantiated its own duplicate ChainstateManager on which tests were
2performed.
3
4This wasn't a problem for the specific actions performed in
5that suite. However, the existence of this duplicate ChainstateManager
6and the fact that many of our validation static functions reach for
7g_chainman, ::Chain(state|)Active means we may end up acting on two
8different CChainStates should we write more extensive tests in the
9future.
10
11This change adds a new ChainTestingSetup which performs all
12initialization previously done by TestingSetup except:
13
141. Mempool sanity check frequency setting
152. ChainState initialization
163. Genesis Activation
174. {Ban,Conn,Peer}Man initialization
18
19Means that we will no longer need to initialize a duplicate
20ChainstateManger in order to test the initialization codepaths of
21CChainState and ChainstateManager.
22
23Lastly, this change has the additional benefit of allowing for
24review-only assertions meant to show correctness to work in future work
25de-globalizing g_chainman.
26
27In the test chainstatemanager_rebalance_caches, an additional
28LoadGenesisBlock call is added as MaybeReblanaceCaches eventually calls
29FlushBlockFile, which tries to access vinfoBlockFile[nLastBlockFile],
30which is out of bounds when LoadGenesisBlock hasn't been called yet.
31
32-----
33
34Note for the future:
35
36In a previous version of this change, I put ChainTestingSetup between
37BasicTestingSetup and TestingSetup such that TestingSetup inherited from
38ChainTestingSetup.
39
40This was suboptimal, and showed how the class con/destructor inheritance
41structure we have for these TestingSetup classes is probably not the
42most suitable abstraction. In particular, for both TestingSetup and
43ChainTestingSetup, we need to stop the scheduler first before anything
44else. Otherwise classes depending on the scheduler may be referenced
45by the scheduler after said classes are freed. This means that there's
46no clear parallel between our teardown code and C++'s destructuring
47order for class hierarchies.
48
49Future work should strive to coalesce (as much as possible) test and
50non-test init codepaths and perhaps structure it in a more fail-proof
51way.
