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.

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.

3 files changed, 56 insertions, 41 deletions

diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index fffa29a1d3..55766a60b4 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -125,18 +125,12 @@ BasicTestingSetup::~BasicTestingSetup()
     ECC_Stop();
 }
 
-TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
+ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
     : BasicTestingSetup(chainName, extra_args)
 {
-    const CChainParams& chainparams = Params();
-    // Ideally we'd move all the RPC tests to the functional testing framework
-    // instead of unit tests, but for now we need these here.
-    RegisterAllCoreRPCCommands(tableRPC);
-
-    m_node.scheduler = MakeUnique<CScheduler>();
-
     // We have to run a scheduler thread to prevent ActivateBestChain
     // from blocking due to queue overrun.
+    m_node.scheduler = MakeUnique<CScheduler>();
     threadGroup.create_thread([&] { TraceThread("scheduler", [&] { m_node.scheduler->serviceQueue(); }); });
     GetMainSignals().RegisterBackgroundSignalScheduler(*m_node.scheduler);
 
@@ -146,20 +140,6 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
     m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
 
     m_node.chainman = &::g_chainman;
-    m_node.chainman->InitializeChainstate(*m_node.mempool);
-    ::ChainstateActive().InitCoinsDB(
-        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
-    assert(!::ChainstateActive().CanFlushToDisk());
-    ::ChainstateActive().InitCoinsCache(1 << 23);
-    assert(::ChainstateActive().CanFlushToDisk());
-    if (!LoadGenesisBlock(chainparams)) {
-        throw std::runtime_error("LoadGenesisBlock failed.");
-    }
-
-    BlockValidationState state;
-    if (!ActivateBestChain(state, chainparams)) {
-        throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
-    }
 
     // Start script-checking threads. Set g_parallel_script_checks to true so they are used.
     constexpr int script_check_threads = 2;
@@ -167,18 +147,9 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
         threadGroup.create_thread([i]() { return ThreadScriptCheck(i); });
     }
     g_parallel_script_checks = true;
-
-    m_node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    m_node.connman = MakeUnique<CConnman>(0x1337, 0x1337); // Deterministic randomness for tests.
-    m_node.peerman = MakeUnique<PeerManager>(chainparams, *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool);
-    {
-        CConnman::Options options;
-        options.m_msgproc = m_node.peerman.get();
-        m_node.connman->Init(options);
-    }
 }
 
-TestingSetup::~TestingSetup()
+ChainTestingSetup::~ChainTestingSetup()
 {
     if (m_node.scheduler) m_node.scheduler->stop();
     threadGroup.interrupt_all();
@@ -196,6 +167,39 @@ TestingSetup::~TestingSetup()
     pblocktree.reset();
 }
 
+TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
+    : ChainTestingSetup(chainName, extra_args)
+{
+    const CChainParams& chainparams = Params();
+    // Ideally we'd move all the RPC tests to the functional testing framework
+    // instead of unit tests, but for now we need these here.
+    RegisterAllCoreRPCCommands(tableRPC);
+
+    m_node.chainman->InitializeChainstate(*m_node.mempool);
+    ::ChainstateActive().InitCoinsDB(
+        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
+    assert(!::ChainstateActive().CanFlushToDisk());
+    ::ChainstateActive().InitCoinsCache(1 << 23);
+    assert(::ChainstateActive().CanFlushToDisk());
+    if (!LoadGenesisBlock(chainparams)) {
+        throw std::runtime_error("LoadGenesisBlock failed.");
+    }
+
+    BlockValidationState state;
+    if (!ActivateBestChain(state, chainparams)) {
+        throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
+    }
+
+    m_node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    m_node.connman = MakeUnique<CConnman>(0x1337, 0x1337); // Deterministic randomness for tests.
+    m_node.peerman = MakeUnique<PeerManager>(chainparams, *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool);
+    {
+        CConnman::Options options;
+        options.m_msgproc = m_node.peerman.get();
+        m_node.connman->Init(options);
+    }
+}
+
 TestChain100Setup::TestChain100Setup()
 {
     // Generate a 100-block chain:
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 1812ce1666..0498e7d182 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -83,14 +83,21 @@ private:
     const fs::path m_path_root;
 };
 
-/** Testing setup that configures a complete environment.
- * Included are coins database, script check threads setup.
+/** Testing setup that performs all steps up until right before
+ * ChainstateManager gets initialized. Meant for testing ChainstateManager
+ * initialization behaviour.
  */
-struct TestingSetup : public BasicTestingSetup {
+struct ChainTestingSetup : public BasicTestingSetup {
     boost::thread_group threadGroup;
 
+    explicit ChainTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
+    ~ChainTestingSetup();
+};
+
+/** Testing setup that configures a complete environment.
+ */
+struct TestingSetup : public ChainTestingSetup {
     explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
-    ~TestingSetup();
 };
 
 /** Identical to TestingSetup, but chain set to regtest */
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 36badafc4e..75939e0140 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -15,15 +15,16 @@
 
 #include <boost/test/unit_test.hpp>
 
-BOOST_FIXTURE_TEST_SUITE(validation_chainstatemanager_tests, TestingSetup)
+BOOST_FIXTURE_TEST_SUITE(validation_chainstatemanager_tests, ChainTestingSetup)
 
 //! Basic tests for ChainstateManager.
 //!
 //! First create a legacy (IBD) chainstate, then create a snapshot chainstate.
 BOOST_AUTO_TEST_CASE(chainstatemanager)
 {
-    ChainstateManager manager;
-    CTxMemPool mempool;
+    ChainstateManager& manager = *m_node.chainman;
+    CTxMemPool& mempool = *m_node.mempool;
+
     std::vector<CChainState*> chainstates;
     const CChainParams& chainparams = Params();
 
@@ -104,8 +105,9 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
 //! Test rebalancing the caches associated with each chainstate.
 BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
 {
-    ChainstateManager manager;
-    CTxMemPool mempool;
+    ChainstateManager& manager = *m_node.chainman;
+    CTxMemPool& mempool = *m_node.mempool;
+
     size_t max_cache = 10000;
     manager.m_total_coinsdb_cache = max_cache;
     manager.m_total_coinstip_cache = max_cache;
@@ -122,6 +124,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
     {
         LOCK(::cs_main);
         c1.InitCoinsCache(1 << 23);
+        BOOST_REQUIRE(c1.LoadGenesisBlock(Params()));
         c1.CoinsTip().SetBestBlock(InsecureRand256());
         manager.MaybeRebalanceCaches();
     }
@@ -139,6 +142,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
     {
         LOCK(::cs_main);
         c2.InitCoinsCache(1 << 23);
+        BOOST_REQUIRE(c2.LoadGenesisBlock(Params()));
         c2.CoinsTip().SetBestBlock(InsecureRand256());
         manager.MaybeRebalanceCaches();
     }