tests: Create or use existing properly initialized NodeContexts #20323

dongcarl · 2020-11-05T18:56:53Z

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 on g_chainman (our global ChainstateManager), sometimes even when you're calling a method on a specific instance of ChainstateManager! This means that we may act on two instances of ChainstateManager, which is most likely not what we want.
Using existing references (initialized by the {Basic,}TestingSetup constructors) 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 ChainstateManager to 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.

jnewbery · 2020-11-05T19:38:36Z

Concept ACK. Thanks for moving ChainTestingSetup into validation_chainstatemanager_tests.cpp!

practicalswift · 2020-11-05T20:05:48Z

Concept ACK

DrahtBot · 2020-11-05T21:12:49Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

addrman: Make addrman a top-level component #20228 (addrman: Make addrman a top-level component by jnewbery)
net: Remove g_relay_txes #20217 (net: Remove g_relay_txes by jnewbery)
refactor: Cleanup thread ctor calls #19064 (refactor: Cleanup thread ctor calls by hebasto)
refactor: Make CCheckQueue RAII-styled #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)

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.

ryanofsky

Code review ACK 3938763. Obvious ACK. This is just test changes and the first 4 commits from #20158 with commit descriptions tweaked and improved

jnewbery

utACK 3938763

Some suggestions to remove unneeded code inline.

src/test/validation_chainstatemanager_tests.cpp

promag

Concept ACK.

dongcarl · 2020-11-17T17:35:19Z

Addressed a few comments.

fuzz CI failing because of #20188 (comment).

ryanofsky

Code review ACK 1319b17. Only changes since last review were suggested simplifications to ChainTestingSetup constructor

ryanofsky · 2020-12-01T16:38:22Z

Hopefully as a test-only cleanup PR with a lot of other changes building on top it (#20158), this can be merged soon

maflcko · 2020-12-01T16:46:26Z

Reason I didn't merge this is that this overlaps with the test changes here: #19425 (review). Though there have been outstanding questions and those questions also apply to this pull (in the extent that they overlap).

ryanofsky

Reason I didn't merge this is that this overlaps with the test changes here: #19425 (review). Though there have been outstanding questions and those questions also apply to this pull (in the extent that they overlap).

Thanks. I guess I need to rebase #19425. I'll check out the test changes too there and make updates if needed to make sure they are compatible with this PR.

This PR just takes approach of having fewer duplicate disconnected state objects inside tests, so I don't think there are real questions about it or major drawbacks. But if anyone has concerns or questions (even vague ones), it'd be good to post something so Carl can reply and this work doesn't languish

ryanofsky · 2020-12-01T17:17:07Z

src/test/validation_chainstatemanager_tests.cpp

@@ -122,6 +167,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
    {
        LOCK(::cs_main);
        c1.InitCoinsCache(1 << 23);
+        assert(c1.LoadGenesisBlock(Params()));


In commit "test: Add new ChainTestingSetup and use it" (43c880a)

Would suggest using BOOST_REQUIRE instead of assert or splitting as bool loaded = c1.LoadGenesisBlock(Params()); assert(loaded);. Writing code with side effects in an assert adds some ambiguity because behavior will depend on value of NDEBUG

Oh! Didn't know about NDEBUG... So when NDEBUG is set, the code inside assert(...) may be optimized out? Or is it the case that only the result isn't checked/asserted?

jnewbery · 2020-12-04T11:31:10Z

utACK 1319b17

dongcarl · 2020-12-04T21:40:50Z

Pushed 1319b17 -> 11fac8b:

Rebased over master for refactor: CTxMempool constructor clean up #20222
Changed asserts in tests to BOOST_REQUIRE as recommended: tests: Create or use existing properly initialized NodeContexts #20323 (comment)
Extracted out testing app initialization and destruction logic into TestingAppInitSequence and TestingAppDestructionSequence to improve sync and decrease maintenance burden between ChainTestingSetup and TestingSetup to address: tests: Create or use existing properly initialized NodeContexts #20323 (comment)

In (3), ordering-wise, the script-checking thread initialization was moved up to before the chainstate gets initialized and RPC command registration was moved down. Neither of which should have any tangible effect on the correctness of initialization.

I can also remove the bool chain_testing_only argument to TestingAppDestructionSequence as it's a mostly idempotent sequence, somehow I like the self-documentation and symmetry. Let me know what seems better!

jnewbery · 2020-12-07T10:34:43Z

I've had a quick look through the new branch and find the TestingAppInitSequence() and TestingAppDestructionSequence() less clear than the previous version. In general functions that take a bool argument which substantially changes the logic inside that function (in this case chain_testing_only), suggest that the abstraction may be wrong.

Was there an actual problem with the previous branch, or are you just trying to reduce code duplication?

maflcko

left some comments

src/test/util/setup_common.cpp

src/test/validation_chainstatemanager_tests.cpp

src/test/util/setup_common.cpp

ryanofsky

Code review ACK 11fac8b. Changes since last review are deduping test init functions and replacing asserts (thanks!).

Other review suggestions for splitting up functions instead of using a bool arguments seem reasonable.

Another idea (not sure whether it is good or applicable): an alternative to calling pairs of functions in constructors and destructors is to just add class members. Member constructors will run in containing class's default constructor, and same with destructors. Seems like this would allow getting rid of the 4 one-line constructor/destructors.

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. RPC command registration 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: 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.

dongcarl · 2020-12-08T20:10:41Z

Thanks everyone for the reviews. The latest push 81137c6 is now rebased over #19485.

This PR is intended to just move things around a bit for the rest of the ChainstateManager de-globalization to work properly. I believe we've gotten a bit stuck on the intricacies of these TestingSetup abstractions.

Personally, I don't particularly care how the abstraction works here. The TestingSetup classes and their inheritance hierarchy is a somewhat poor abstraction of how our initialization and de-initialization logic work. Therefore, I'd like to just fix this particular problem for now and move on.

I do fully intend on coming back to the initialization codepaths later, most likely after having moved a couple of mutable statics into classes they logically belong in, and having a larger PR that coalesces and cleans all of this up.

In this latest push I:

Reverted back to my original solution of having this inheritance chain: TestingSetup -> ChainTestingSetup -> BasicTestingSetup. This has the advantage of not having to introduce any new functions.
Compared to my last try at making this work, I now take advantage of the idempotency of our teardown sequence and have ~ChainTestingSetup teardown all application logic while ~TestingSetup inherits from it.
Put ChainTestingSetup back in setup_common so that future tests can use it as requested here. (Sorry John, I knew you weren't the biggest fan of this 😕)

jnewbery: Was there an actual problem with the previous branch, or are you just trying to reduce code duplication?

There wasn't an actual problem with the previous branch, MarcoFalke brought up the concern that it was harder to maintain duplicate codepaths, so this previous push was a (perhaps poorly conceived) attempt at doing so. I agree w/re the bool argument and have since reverted to something similar to how I originally attempted this which avoids using a bool argument.

ryanofsky

Code review ACK 81137c6. This change is simpler after the rebase because wallet & bench commits are dropped.

Along with the rebase, new test init functions were dropped again in response to #20323 (comment), which is fine. I think in the longer run more init code can be moved into RAII classes, and we won't need these intricate initialization functions and class hierarchies (tests could just state their resource requirements as variable declarations).

maflcko · 2020-12-09T06:57:38Z

Thanks for shuffling this around for over a month

ACK 81137c6 looking excellent now 🐩

Show signature and timestamp

Signature:

Timestamp of file with hash 1fd492318f7f8e7f0f9f62fce5042d824a2c5c3fcb368e2027f02582ea8ed27e -

maflcko · 2020-12-09T07:02:09Z

Put ChainTestingSetup back in setup_common so that future tests can use it as requested here. (Sorry John, I knew you weren't the biggest fan of this confused)

Ok, let's wait for @jnewbery 's green light before merge

jnewbery · 2020-12-09T08:50:48Z

ACK 81137c6

…ed Node…

dongcarl mentioned this pull request Nov 5, 2020

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

DrahtBot added GUI Wallet labels Nov 5, 2020

dongcarl added the Tests label Nov 5, 2020

This was referenced Nov 5, 2020

validation: UTXO snapshot activation #19806

Merged

refactor: Get rid of more redundant chain methods #19425

Merged

Use shared pointers only in validation interface #18354

Closed

ryanofsky approved these changes Nov 10, 2020

View reviewed changes

jnewbery reviewed Nov 10, 2020

View reviewed changes

maflcko removed GUI Wallet labels Nov 10, 2020

promag reviewed Nov 14, 2020

View reviewed changes

dongcarl force-pushed the 2020-10-chainman-fixes branch from 3938763 to 12399f0 Compare November 16, 2020 22:07

dongcarl force-pushed the 2020-10-chainman-fixes branch from 12399f0 to 1319b17 Compare November 24, 2020 15:52

ryanofsky approved these changes Dec 1, 2020

View reviewed changes

ryanofsky reviewed Dec 1, 2020

View reviewed changes

dongcarl force-pushed the 2020-10-chainman-fixes branch from 1319b17 to 11fac8b Compare December 4, 2020 21:25

DrahtBot mentioned this pull request Dec 5, 2020

Add local thread pool to CCheckQueue #18710

Merged

maflcko reviewed Dec 7, 2020

View reviewed changes

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

src/test/validation_chainstatemanager_tests.cpp Outdated Show resolved Hide resolved

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Dec 7, 2020

ryanofsky approved these changes Dec 8, 2020

View reviewed changes

dongcarl added 2 commits December 8, 2020 14:22

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

7e9e7fe

dongcarl force-pushed the 2020-10-chainman-fixes branch from 11fac8b to 81137c6 Compare December 8, 2020 20:01

DrahtBot removed the Needs rebase label Dec 8, 2020

ryanofsky approved these changes Dec 8, 2020

View reviewed changes

maflcko merged commit a3586d5 into bitcoin:master Dec 9, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 9, 2020

Merge bitcoin#20323: tests: Create or use existing properly initializ…

d771bbd

…ed Node…

dongcarl mentioned this pull request Jan 12, 2021

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

tests: Create or use existing properly initialized NodeContexts #20323

tests: Create or use existing properly initialized NodeContexts #20323

Uh oh!

Conversation

dongcarl commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Uh oh!

jnewbery commented Nov 5, 2020

Uh oh!

practicalswift commented Nov 5, 2020

Uh oh!

DrahtBot commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Nov 17, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Dec 1, 2020

Uh oh!

maflcko commented Dec 1, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

dongcarl Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 4, 2020

Uh oh!

dongcarl commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Dec 7, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 9, 2020

Uh oh!

maflcko commented Dec 9, 2020

Uh oh!

jnewbery commented Dec 9, 2020

Uh oh!

Uh oh!

dongcarl commented Nov 5, 2020 •

edited

Loading

DrahtBot commented Nov 5, 2020 •

edited

Loading

dongcarl commented Dec 4, 2020 •

edited

Loading

dongcarl commented Dec 8, 2020 •

edited

Loading