sikkienl/dash
1
0
Fork 0
mirror of https://github.com/dashpay/dash.git synced 2024-12-25 12:02:48 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity

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

Browse Source 
77915de40a test: run `Interrupt()` before `Stop()`, add additional sanity check (Kittywhiskers Van Gogh)
a376e9357a 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](6bf39d7632 (diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R289)) ([dash#5236](https://github.com/dashpay/dash/pull/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](https://github.com/dashpay/dash/pull/6085#issuecomment-2228109300), 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](09239a17c7/src/test/blockfilter_index_tests.cpp (L141)), [source](09239a17c7/src/test/coinstatsindex_tests.cpp (L33)), [source](09239a17c7/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](cc9dcdd0e0 (diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6L413-L419)) that adopted `IndexWaitSynced`. This has since been resolved.
  * In line [with](09239a17c7/src/test/blockfilter_index_tests.cpp (L138-L139)) [other](09239a17c7/src/test/coinstatsindex_tests.cpp (L29-L31)) [indexes](09239a17c7/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 77915de40a
  UdjinM6:
    utACK 77915de40a
  PastaPastaPasta:
    utACK 77915de40a

Tree-SHA512: 5051dcf62b6cad4597044b4d27dc54c5bafa8692ba31e03c8afc0f7ef80b790a3873cf0275bb2cf6260939a11f95625da79fc7987951e66114900a86276c037c
This commit is contained in:
pasta 2024-07-19 11:15:59 -05:00
commit 5f9f05edbc
No known key found for this signature in database
GPG Key ID: 52527BEDABE87984
1 changed files with 12 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
src/test/util/setup_common.cpp
View File

@ -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();