fix: move g_txindex initialization out of erroneous location and into constructor
#6119
+12
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Special thanks to @knst for spotting the |
kwvg
changed the title
g_txindex initialization out of erroneous location and into constructor, initialize earlierg_txindex initialization out of erroneous location and into constructor
kwvg
force-pushed
the
txindex_fix
branch
4 times, most recently
from
5f819c8
to
9316b61
Compare
knst
reviewed
Jul 17, 2024
`g_txindex` should be initialized in `TestChainSetup`'s constructor but in bitcoin#19806 (dash#5236), when portions of the constructor were split into `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. Also, a runtime exception is more appropriate and closer to the usage of `BOOST_REQUIRE` in other index `Start()` calls than the harsher `assert`
5 tasks
knst
approved these changes
Jul 18, 2024
UdjinM6
approved these changes
Jul 18, 2024
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 23, 2024
, bitcoin#23174, bitcoin#23785, bitcoin#23581, bitcoin#23974, bitcoin#22932, bitcoin#24050, bitcoin#24515 (blockstorage backports) 1bf0bf4 merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh) 5c1eb67 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh) c440304 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh) e303a4e merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh) 301163c merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh) 732e871 merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh) b402fd5 merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh) a08f2f4 merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh) 472caa0 merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh) d69ca83 merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh) 6df927f chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6078 * Dependent on #6074 * Dependent on #6083 * Dependent on #6119 * Dependency for #6138 * In [bitcoin#24050](bitcoin#24050), `BlockMap` is given ownership of the `CBlockIndex` instance contained within the `unordered_map`. The same has not been done for `PrevBlockMap` as `PrevBlockMap` is populated with `pprev` pointers and doing so seems to break validation logic. * Dash has a specific linter for all Dash-specific code present in Core. The introduction of `util/translation.h` into `validation.h` has caused the linter to trigger shadowing warnings due to a conflict between the common use of `_` as a placeholder/throwaway name ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/spork.cpp#L44)) and upstream's usage of it to process translatable strings ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/util/translation.h#L55-L62)). Neither C++17 nor C++20 have an _official_ placeholder/throwaway term or annotation for structured bindings (which cannot use `[[maybe_unused]` or `std::ignore`) but [P2169](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2169r4.pdf) is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore. ## Breaking Changes None expected ## 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 - [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: UdjinM6: utACK 1bf0bf4 (with one nit) knst: utACK 1bf0bf4 PastaPastaPasta: utACK 1bf0bf4 Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
g_txindexshould 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 aTxIndexinstance, which is not intended behaviour; and was recorded to causeheap-use-after-frees (comment, also the reason this PR was opened).This PR aims to resolve that.
Additional Information
dependsbuilt 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-ccachemake 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(), theasserthas been downgraded to an exception.Checklist: