Merge #6119: fix: move g_txindex initialization out of erroneous location and into constructor

77915de test: run `Interrupt()` before `Stop()`, add additional sanity check (Kittywhiskers Van Gogh)
a376e93 fix: init `g_txindex` only once, downgrade from assert to exception (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  `g_txindex` should be initialized in `TestChainSetup`'s constructor but when backporting [bitcoin#19806](6bf39d7#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R289) ([dash#5236](#5236)), portions of the constructor were split into `TestChainSetup::mineBlocks()`, `g_txindex`'s init was left behind in the latter instead of the former.

  This meant that every `mineBlocks()` call would re-create a `TxIndex` instance, which is not intended behaviour; and was recorded to cause `heap-use-after-free`s ([comment](#6085 (comment)), also the reason this PR was opened).

  This PR aims to resolve that.

  ## Additional Information

  * Crashes stemming from previous attempts (except for one attempt) were not reproducible with my regular local setup (`depends` built with Clang 16, Dash Core built with Clang 16, set of debug-oriented flags, unit tests run using `./src/test/test_dash`).
    * Attempting to rebuild Dash Core with GCC 9 was insufficient, required to rebuild depends with GCC 9 as well
    * `configure`'d with `CC=gcc CXX=g++ CPPFLAGS="-DDEBUG_LOCKORDER -DARENA_DEBUG" ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --enable-crash-hooks --enable-c++20 --disable-ccache`
    * Unit tests must be run with `make check-recursive -j$(( $(nproc --all) - 2 ))`
  * An index must be initialized **after** the chain is constructed, this seems to be corroborated by all other index usage ([source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/blockfilter_index_tests.cpp#L141), [source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/coinstatsindex_tests.cpp#L33), [source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/txindex_tests.cpp#L31), all three use `Start()` for their respective indexes _after_ `TestChain100Setup`'s constructor runs `mineBlocks()`)
    * Attempting to run `Start()` earlier (_before_ the `mineBlocks()` call in the constructor) results in erratic behaviour
    * This also explains why my attempt at moving it back to `TestingSetup` (a grandparent of `TestChainSetup`) failed
  * `Interrupt()` is supposed to be called before `Stop()` but this was erroneously removed in a [commit](cc9dcdd#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6L413-L419) that adopted `IndexWaitSynced`. This has since been resolved.
  * In line [with](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/blockfilter_index_tests.cpp#L138-L139) [other](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/coinstatsindex_tests.cpp#L29-L31) [indexes](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/txindex_tests.cpp#L28-L29), an sanity check has been added. Additionally, as `TxIndex::Start()` is more akin to `CChainState::LoadGenesisBlock()` than `CChainState::CanFlushToDisk()`, the `assert` has been downgraded to an exception.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK 77915de
  UdjinM6:
    utACK 77915de
  PastaPastaPasta:
    utACK 77915de

Tree-SHA512: 5051dcf62b6cad4597044b4d27dc54c5bafa8692ba31e03c8afc0f7ef80b790a3873cf0275bb2cf6260939a11f95625da79fc7987951e66114900a86276c037c

Author: PastaPastaPasta

src/test/util/setup_common.cpp

@@ -323,6 +323,14 @@ TestChainSetup::TestChainSetup(int num_blocks, const std::vector<const char*>& e
     // Generate a num_blocks length chain:
     this->mineBlocks(num_blocks);
 
+    // Initialize transaction index *after* chain has been constructed
+    g_txindex = std::make_unique<TxIndex>(1 << 20, true);
+    assert(!g_txindex->BlockUntilSyncedToCurrentChain());
+    if (!g_txindex->Start(m_node.chainman->ActiveChainstate())) {
+        throw std::runtime_error("TxIndex::Start() failed.");
+    }
+    IndexWaitSynced(*g_txindex);
+
     CCheckpointData checkpoints{
         {
             /* TestChainDATSetup */
@@ -361,11 +369,10 @@ void TestChainSetup::mineBlocks(int num_blocks)
         m_coinbase_txns.push_back(b.vtx[0]);
     }
 
-    g_txindex = std::make_unique<TxIndex>(1 << 20, true);
-    assert(g_txindex->Start(m_node.chainman->ActiveChainstate()));
-
     // Allow tx index to catch up with the block index.
-    IndexWaitSynced(*g_txindex);
+    if (g_txindex) {
+        IndexWaitSynced(*g_txindex);
+    }
 }
 
 CBlock TestChainSetup::CreateAndProcessBlock(const std::vector<CMutableTransaction>& txns, const CScript& scriptPubKey)
@@ -500,6 +507,7 @@ TestChainSetup::~TestChainSetup()
     // we might be destroying it while scheduler still has some work for it
     // e.g. via BlockConnected signal
     IndexWaitSynced(*g_txindex);
+    g_txindex->Interrupt();
     g_txindex->Stop();
     SyncWithValidationInterfaceQueue();
     g_txindex.reset();