g_txindex should be initialized in TestChainSetup's constructor but when backporting bitcoin#19806 (dash#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-frees (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
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, source, source, all three use Start() for their respective indexes afterTestChain100Setup'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 that adopted IndexWaitSynced. This has since been resolved.
In line withotherindexes, 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)
Motivation
g_txindex
should be initialized inTestChainSetup
's constructor but when backporting bitcoin#19806 (dash#5236), portions of the constructor were split intoTestChainSetup::mineBlocks()
,g_txindex
's init was left behind in the latter instead of the former.This meant that every
mineBlocks()
call would re-create aTxIndex
instance, which is not intended behaviour; and was recorded to causeheap-use-after-free
s (comment, also the reason this PR was opened).This PR aims to resolve that.
Additional Information
depends
built with Clang 16, Dash Core built with Clang 16, set of debug-oriented flags, unit tests run using./src/test/test_dash
).configure
'd withCC=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
make check-recursive -j$(( $(nproc --all) - 2 ))
Start()
for their respective indexes afterTestChain100Setup
's constructor runsmineBlocks()
)Start()
earlier (before themineBlocks()
call in the constructor) results in erratic behaviourTestingSetup
(a grandparent ofTestChainSetup
) failedInterrupt()
is supposed to be called beforeStop()
but this was erroneously removed in a commit that adoptedIndexWaitSynced
. This has since been resolved.TxIndex::Start()
is more akin toCChainState::LoadGenesisBlock()
thanCChainState::CanFlushToDisk()
, theassert
has been downgraded to an exception.Checklist: