Merge #13033: Build txindex in parallel with validation #4178
Conversation
This file contains hidden or 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
9b27047 [doc] Include txindex changes in the release notes. (Jim Posen) ed77dd6 [test] Simple unit test for TxIndex. (Jim Posen) 6d772a3 [rpc] Public interfaces to GetTransaction block until synced. (Jim Posen) a03f804 [index] Move disk IO logic from GetTransaction to TxIndex::FindTx. (Jim Posen) e0a3b80 [validation] Replace tx index code in validation code with TxIndex. (Jim Posen) 8181db8 [init] Initialize and start TxIndex in init code. (Jim Posen) f90c3a6 [index] TxIndex method to wait until caught up. (Jim Posen) 70d510d [index] Allow TxIndex sync thread to be interrupted. (Jim Posen) 94b4f8b [index] TxIndex initial sync thread. (Jim Posen) 34d68bf [index] Create new TxIndex class. (Jim Posen) c88bcec [db] Migration for txindex data to new, separate database. (Jim Posen) 0cb8303 [db] Create separate database for txindex. (Jim Posen) Pull request description: I'm re-opening bitcoin#11857 as a new pull request because the last one stopped loading for people ------------------------------- This refactors the tx index code to be in it's own class and get built concurrently with validation code. The main benefit is decoupling and moving the txindex into a separate DB. The primary motivation is to lay the groundwork for other indexers that might be desired (such as the [compact filters](bitcoin/bips#636)). The basic idea is that the TxIndex spins up its own thread, which first syncs the txindex to the current block index, then once in sync the BlockConnected ValidationInterface hook writes new blocks. ### DB changes At the suggestion of some other developers, the txindex has been split out into a separate database. A data migration runs at startup on any nodes with a legacy txindex. Currently the migration blocks node initialization until complete. ### Open questions - Should the migration of txindex data from the old DB to the new DB block in init or should it happen in a background thread? The downside to backgrounding it is that `getrawtransaction` would return an error message saying the txindex is syncing while the migration is running. ### Impact In a sample size n=1 test where I synced nodes from scratch, the average time [Index writing](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L1903) was 3.36ms in master and 1.72ms in this branch. The average time between `UpdateTip` log lines for sequential blocks between 400,000 and IBD end on mainnet was 0.297204s in master and 0.286134s in this branch. Most likely this is just variance in IBD times, but I can try with some more trials if people want. Tree-SHA512: 451fd7d95df89dfafceaa723cdf0f7b137615b531cf5c5035cfb54e9ccc2026cec5ac85edbcf71b7f4e2f102e36e9202b8b3a667e1504a9e1a9976ab1f0079c4
PastaPastaPasta
approved these changes
Jun 3, 2021
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Oct 21, 2024
, bitcoin#24626, bitcoin#21726, bitcoin#25123, bitcoin#25074, bitcoin#24832, bitcoin#26215, bitcoin#24858, bitcoin#26417, bitcoin#16981 (index backports) 7d9ff96 merge bitcoin#16981: Improve runtime performance of --reindex (Kittywhiskers Van Gogh) e531dff merge bitcoin#26417: fix intermittent failure in feature_index_prune.py (Kittywhiskers Van Gogh) b04b71a merge bitcoin#24858: incorrect blk file size calculation during reindex results in recoverable blk file corruption (Kittywhiskers Van Gogh) 9e75b99 merge bitcoin#26215: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Kittywhiskers Van Gogh) 3bd584c merge bitcoin#24832: Verify the block filter hash when reading the filter from disk (Kittywhiskers Van Gogh) e507a51 fix: avoid `mandatory-script-verify-flag-failed` crash in bench test (Kittywhiskers Van Gogh) a86109a merge bitcoin#25074: During sync, commit best block after indexing (Kittywhiskers Van Gogh) e6867a3 merge bitcoin#25123: Fix race condition in index prune test (Kittywhiskers Van Gogh) baf6e26 merge bitcoin#21726: Improve Indices on pruned nodes via prune blockers (Kittywhiskers Van Gogh) c65ec19 merge bitcoin#24626: disallow reindex-chainstate when pruning (Kittywhiskers Van Gogh) bcd24a2 fix: push activation height for forks ahead, fix `feature_pruning.py` (Kittywhiskers Van Gogh) 1020356 merge bitcoin#24812: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro (Kittywhiskers Van Gogh) 1caaa85 merge bitcoin#24138: Commit MuHash and best block together for coinstatsindex (Kittywhiskers Van Gogh) b218f12 merge bitcoin#23046: Add txindex migration test (Kittywhiskers Van Gogh) ebae59e fix: make sure we flush our committed best block in no-upgrade cases (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * When backporting [bitcoin#23046](bitcoin#23046), it was discovered that there has been a longstanding bug in `CDeterministicMNManager::MigrateDBIfNeeded`(`2`)`()` that flagged a database taken from an older version for failing its "previous migration attempt", requiring the database to be fully rebuilt through a reindex. This occurred because the older database would be read pre-DIP3 in `MigrateDBIfNeeded()`, which then caused the migration logic to write the new best block ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1236-L1241)) (the legacy best block is erased before the DIP3 condition is checked, [source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1233)) but while it completed the transaction ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1240)), critically, it didn't write it to disk (example of writing to disk, [here](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1288-L1292)). This meant that when it was read again by `MigrateDBIfNeeded2()`, it saw three things a) there is no new best block (because it didn't get written), b) there is no legacy best block (because it gets erased before the new best block is written) and c) that the chain height is greater than 1 (since this isn't a new datadir and the chain has already advanced), it concludes that it was a botched migration attempt and fails ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1337-L1343)). This bug affects v19 to `develop` (`3f0c2ff3` as of this writing) and prevents `feature_txindex_compatibility.py` from working as expected as it would migrate legacy databases to newer versions to test txindex migration code and get stuck at unhappy EvoDB migration logic, to allow the test to function properly when testing against the latest version of the client, this bug has been fixed as part of this PR. * In [bitcoin#23046](bitcoin#23046), version v0.17 was used as the last version to support legacy txindex as the updated txindex format was introduced in [dash#4178](#4178) (i.e. after v0.17) and the version selected for having migration code in it (note, migration code was removed in [dash#6296](#6296), so far not included as part of any release) was v18.2.2 despite the range being v18.x to v21.x was a) due to the bug mentioned above affecting v19.x onwards and b) v18.2.2 being the latest release in the v18.x lifecycle. * The specific version number used for v0.17 is `170003` as the binaries corresponding to `170000` are not populated in `releases/`, which causes a CI failure ([source](https://gitlab.com/dashpay/dash/-/jobs/8073041955#L380)) * As of `develop` (`3f0c2ff3` as of this writing), `feature_pruning.py` was broken due to changes in Core that were not adjusted for, namely: * The enforcement of `MAX_STANDARD_TX_SIZE` ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/policy/policy.h#L23-L24)) from DIP1 onwards ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/validation.cpp#L299-L301)) resulting in `bad-txns-oversize` errors in blocks generated for the test as the transactions generated are ~9.5x larger than the now-enforced limit ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/test/functional/feature_pruning.py#L48C51-L48C57)), this is resolved by pushing the DIP1 activation height upwards to `2000` (the same activation height used for DIP3 and DIP8). * Change in subsidy logic in v20 ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/validation.cpp#L1073-L1082)) that results in `bad-cb-amount` errors, this is resolved by pushing the v20 activation height upwards. Additionally, an inopportune implicit post-`generate` sync ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/test/functional/feature_pruning.py#L215)) also causes the test to fail. Alongside the above, they have been resolved in this PR. * As of `develop` (`3f0c2ff3` as of this writing), `bench_dash` crashes when running the `AssembleBlock` benchmark. The regression was traced back to [bitcoin#21840](bitcoin#21840) (5d10b41) in [dash#6152](#6152) due to the switch to `P2SH_OP_TRUE`. This has been resolved by reverting this particular change. <details> <summary>Pre-fix test failure:</summary> ``` $ ./src/bench/bench_dash Warning, results might be unstable: * CPU governor is '' but should be 'performance' * Turbo is enabled, CPU frequency will fluctuate Recommendations * Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf | ns/op | op/s | err% | ins/op | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|---------------:|--------:|----------:|:---------- | 17,647,657.00 | 56.66 | 0.1% | 231,718,349.00 | 42,246,265.00 | 0.1% | 0.20 | `AddrManAdd` | 42,201,861.00 | 23.70 | 0.1% | 544,366,811.00 | 102,569,244.00 | 0.0% | 0.46 | `AddrManAddThenGood` | 189,697.83 | 5,271.54 | 0.1% | 1,763,991.40 | 356,189.40 | 0.3% | 0.01 | `AddrManGetAddr` | 454.38 | 2,200,808.04 | 0.6% | 6,229.11 | 1,343.92 | 0.1% | 0.01 | `AddrManSelect` | 1,066,471.00 | 937.67 | 67.6% | 13,350,463.00 | 3,150,465.00 | 0.4% | 0.01 | 〰️ `AddrManSelectByNetwork` (Unstable with ~1.0 iters. Increase `minEpochIterations` to e.g. 10) | 1,181,774.50 | 846.19 | 49.0% | 18,358,489.50 | 4,224,642.50 | 0.0% | 0.02 | 〰️ `AddrManSelectFromAlmostEmpty` (Unstable with ~1.1 iters. Increase `minEpochIterations` to e.g. 11) bench_dash: bench/block_assemble.cpp:46: void AssembleBlock(benchmark::Bench &): Assertion `res.m_result_type == MempoolAcceptResult::ResultType::VALID' failed. [1] 2343746 IOT instruction (core dumped) ./src/bench/bench_dash ``` </details> ## 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 - [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 7d9ff96 PastaPastaPasta: utACK 7d9ff96 Tree-SHA512: e2f1e58abb0a0368c4f1d5e488520957e042e6207b7d2d68a15eb18662405a3cdac91c5ff8e93c8a94c0fdab9b1412bd608d055f196230506c1640439939c25d
Merged
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Oct 13, 2025
… `MIN_PEER_PROTO_VERSION` to `70221`) 3562045 chore: drop `GOVSCRIPT_PROTO_VERSION` checks (Kittywhiskers Van Gogh) c5bfb57 chore: drop `ISDLOCK_PROTO_VERSION` checks (Kittywhiskers Van Gogh) 4096d77 chore: drop `LLMQ_DATA_MESSAGES_VERSION` checks (Kittywhiskers Van Gogh) 8c141f8 chore: drop `MNAUTH_NODE_VER_VERSION` checks (Kittywhiskers Van Gogh) 5d9435d chore: bump `MIN_PEER_PROTO_VERSION` to `70221` (Kittywhiskers Van Gogh) 1d39829 merge bitcoin#28753: Remove feature_txindex_compatibility.py (Kittywhiskers Van Gogh) 8d13227 partial bitcoin#28195: Drop legacy -txindex check (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6838 * `feature_txindex_compatibility.py` uses Dash Core v0.17.0.3, which has `PROTOCOL_VERSION` `70219` ([source](https://github.com/dashpay/dash/blob/eaca69b22c5a5b1ec0a01e003c2d487542062b44/src/version.h#L14)). Due to the minimum protocol version bump, this test will no longer pass (spotted by UdjinM6!) * Legacy txindex checks were introduced in [bitcoin#22626](bitcoin#22626) (see [dash#6296](#6296), part of Dash Core v22) and the test was introduced in [bitcoin#23046](bitcoin#23046) (see [dash#6327](#6327), part of Dash Core v22). * The migration logic _itself_ was added in [bitcoin#13033](bitcoin#13033) (see [dash#4178](#4178), part of Dash Core v18). This means that it _should_ be safe to drop the failing test altogether as we migrated databases 5 major versions ago, the databases are not funds-sensitive and we dropped support for migration a major version ago. ## Breaking Changes Clients below `70221` (`GOVSCRIPT_PROTO_VERSION`) will not be able to connect to Dash Core ## 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 3562045 PastaPastaPasta: utACK 3562045 Tree-SHA512: e1d3f3cde66e99ed93ad339d4f9aab49fda536bfaefb2102b535a25144fb7d660b77239d6125d95e40350b81fd40a38d99e855066e61280b928baef9ded247f4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This is effectively a
backports-0.17-pr30PR but changes in 13033 are too complex and too critical to mix them with anything else.NOTE: Pls test this one with caution! There is no easy way of going back once txindex migration is done, you'll have to reindex!