Merge bitcoin/bitcoin#22564: refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager`

7ab07e033237d6ea179a6a2c76575ed6bd01a670 validation: Prune UnloadBlockIndex and callees (Carl Dong)
7d99d725cdb5428ed25dc07c2d7fddf420da7786 validation: No mempool clearing in UnloadBlockIndex (Carl Dong)
572d8319272ae84a81d6bfd53dd9685585697f65 Clear {versionbits,warning}cache in ~Chainstatemanager (Carl Dong)
eca4ca4d60599c9dbdd4e03a73beb33e9b44655a style-only: Use std::clamp for check_ratio, rename (Carl Dong)
fe96a2e4bd87768df8001eb4117926a0977d876e style-only: Use for instead of when loading Chainstate (Carl Dong)
5921b863e39e5c3997895ffee1c87159e37a5d6f init: Reset mempool and chainman via reconstruction (Carl Dong)
6e747e80e7094df0b5bee1eed57e57e82015d0ee validation: default initialize and guard chainman members (Anthony Towns)
98f4bdae81804de17f125bd7c2cd8a48e850a6d2 refactor: Convert warningcache to std::array (Carl Dong)

Pull request description:

  Fixes #22964

  -----

  This is a small part of the work to accomplish what I described in 972c5166ee685447a6d4bf5e501b07a0871fba85:
  ```
  Over time, we should probably move these mutable global state variables
  into ChainstateManager or CChainState so it's easier to reason about
  their lifecycles.
  ```

  `::UnloadBlockIndex` manually resets a subset of our mutable globals in addition to unloading the `ChainstateManager` and clearing the mempool. The need for this manual reset (AFAICT) arises out of the fact that many of these globals are closely related to the block index (hence `::UnloadBlockIndex`), and need to be reset with it.

  I've shot this "manual reset" gun at my foot several times while doing the de-globalize chainman work.

  Thankfully, now that we have a `BlockManager` class that owns the block index, these globals should be moved under that class so that they can live and die with the block index. These moves, along with making the block index non-heap-based, eliminates:
  1. https://github.com/bitcoin/bitcoin/commit/3585b521392c5b2c855c3ba6dc9b7d2a171b3710 The need to reason about when we need to manually call `::UnloadBlockIndex` (this decision can at times seem almost arbitrary)
  2. https://github.com/bitcoin/bitcoin/commit/f741623c25455c20bff9eb1ddd10a4ac84dc5655 The need to have an `::UnloadBlockIndex` or explicit `~ChainstateManager` at all

ACKs for top commit:
  MarcoFalke:
    ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670 👘
  ajtowns:
    ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670
  ryanofsky:
    Code review ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670. This all looks good and simplifies things nicely. I left some minor suggestions below but feel free to ignore.

Tree-SHA512: a36ee3fc122ce0b4e8d1c432662d7009df06264b724b793252978a1e409dde7a7ef1f78b9ade3f8bfb5388213f10ae2d058d57a7a46ae563e9034d7d33a52b69

3 files changed, 3 insertions, 4 deletions

diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 1830ec05af..2fc71c2a6e 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -182,7 +182,6 @@ ChainTestingSetup::~ChainTestingSetup()
     m_node.addrman.reset();
     m_node.netgroupman.reset();
     m_node.args = nullptr;
-    WITH_LOCK(::cs_main, UnloadBlockIndex(m_node.mempool.get(), *m_node.chainman));
     m_node.mempool.reset();
     m_node.scheduler.reset();
     m_node.chainman.reset();
diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
index b0d7389d39..2a3990bb7c 100644
--- a/src/test/validation_chainstate_tests.cpp
+++ b/src/test/validation_chainstate_tests.cpp
@@ -93,7 +93,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup)
     BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(m_node, m_path_root));
 
     // Ensure our active chain is the snapshot chainstate.
-    BOOST_CHECK(chainman.IsSnapshotActive());
+    BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.IsSnapshotActive()));
 
     curr_tip = ::g_best_block;
 
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 5d0ec593e3..6dc522b421 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -45,7 +45,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
     WITH_LOCK(::cs_main, c1.InitCoinsCache(1 << 23));
 
     BOOST_CHECK(!manager.IsSnapshotActive());
-    BOOST_CHECK(!manager.IsSnapshotValidated());
+    BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated()));
     auto all = manager.GetAll();
     BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end());
 
@@ -78,7 +78,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
     BOOST_CHECK(c2.ActivateBestChain(_, nullptr));
 
     BOOST_CHECK(manager.IsSnapshotActive());
-    BOOST_CHECK(!manager.IsSnapshotValidated());
+    BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated()));
     BOOST_CHECK_EQUAL(&c2, &manager.ActiveChainstate());
     BOOST_CHECK(&c1 != &manager.ActiveChainstate());
     auto all2 = manager.GetAll();