Repository navigation
fix: don't adopt an empty or rebuilt async index as migrated legacy data #8031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| // Copyright (c) 2026 The Dash Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include <coins.h> | ||
| #include <index/timestampindex.h> | ||
| #include <interfaces/chain.h> | ||
| #include <node/blockstorage.h> | ||
| #include <test/util/setup_common.h> | ||
| #include <txdb.h> | ||
| #include <util/system.h> | ||
| #include <validation.h> | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| namespace { | ||
| struct IndexMigrationSetup : public TestChainSetup { | ||
| IndexMigrationSetup() : | ||
| TestChainSetup{100, CBaseChainParams::REGTEST, {"-timestampindex=1"}} | ||
|
PastaPastaPasta marked this conversation as resolved.
Resolved
|
||
| { | ||
| } | ||
|
|
||
| //! Give MigrateOldIndexData() the on-disk chainstate best block it reads on a real node. | ||
| void WriteChainstateBestBlock() | ||
| { | ||
| const uint256 tip{WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip()->GetBlockHash())}; | ||
| CCoinsViewDB coins_db{DBParams{.path = gArgs.GetDataDirNet() / "chainstate", .cache_bytes = 1 << 20}, | ||
| CoinsViewOptions{}}; | ||
| CCoinsMapMemoryResource resource; | ||
| CCoinsMap no_coins{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; | ||
| BOOST_REQUIRE(coins_db.BatchWrite(no_coins, tip)); | ||
| } | ||
|
|
||
| //! Height the timestamp index resumes from, read before its sync thread can advance it. | ||
| int LoadedTimestampIndexHeight() | ||
| { | ||
| node::g_indexes_ready_to_sync = false; | ||
| TimestampIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1 << 20}; | ||
| BOOST_REQUIRE(index.Start()); | ||
| const int height{index.GetSummary().best_block_height}; | ||
| index.Interrupt(); | ||
| index.Stop(); | ||
| node::g_indexes_ready_to_sync = true; | ||
| return height; | ||
| } | ||
| }; | ||
| } // namespace | ||
|
|
||
| BOOST_AUTO_TEST_SUITE(index_migration_tests) | ||
|
|
||
| // A datadir upgraded from v23 keeps its legacy "timestampindex" flag. If the index's own | ||
| // database is then missing (deleted to rebuild it), there is nothing left to migrate, and | ||
| // the empty database must not be stamped with the chain tip as if it were synced. | ||
| BOOST_FIXTURE_TEST_CASE(empty_index_is_not_adopted_as_migrated, IndexMigrationSetup) | ||
| { | ||
| WriteChainstateBestBlock(); | ||
| CBlockTreeDB& block_tree_db{*WITH_LOCK(cs_main, return m_node.chainman->m_blockman.m_block_tree_db.get())}; | ||
| BOOST_REQUIRE(block_tree_db.WriteFlag("timestampindex", true)); | ||
|
|
||
| BOOST_REQUIRE(WITH_LOCK(cs_main, return block_tree_db.MigrateOldIndexData())); | ||
|
|
||
| BOOST_CHECK_EQUAL(LoadedTimestampIndexHeight(), 0); | ||
| // The legacy entries are gone, so older versions must not trust their synchronous index either. | ||
| bool legacy_current{true}; | ||
| BOOST_REQUIRE(block_tree_db.ReadFlag("timestampindex", legacy_current)); | ||
| BOOST_CHECK(!legacy_current); | ||
|
PastaPastaPasta marked this conversation as resolved.
Resolved
|
||
| } | ||
|
|
||
| BOOST_AUTO_TEST_SUITE_END() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -475,14 +475,17 @@ static bool FinalizeMigration(CDBWrapper& target_db, const uint256& best_block_h | |
| return true; | ||
| } | ||
|
|
||
| // Returns true if the target index database has no best-block locator yet — | ||
| // i.e. the new async index has never been finalized on this datadir. Used to | ||
| // guard FinalizeMigration so we don't regress an already-advanced locator | ||
| // (e.g. left by ThreadSync after a previous successful migration). | ||
| // Returns true if the target index database holds migrated entries but no best-block | ||
| // locator yet, i.e. a migration copied data that was never finalized. Used to guard | ||
| // FinalizeMigration so we don't regress an already-advanced locator (e.g. left by | ||
| // ThreadSync after a previous successful migration), and so we never stamp the chain | ||
| // tip onto an empty target (e.g. index directories deleted after an earlier migration | ||
| // already moved the legacy entries), which would report an empty index as synced. | ||
| static bool TargetNeedsLocator(CDBWrapper& target_db) | ||
| { | ||
| CBlockLocator existing; | ||
| return !target_db.Read(DB_BEST_BLOCK, existing) || existing.IsNull(); | ||
| if (target_db.Read(DB_BEST_BLOCK, existing) && !existing.IsNull()) return false; | ||
| return !target_db.IsEmpty(); | ||
| } | ||
|
|
||
| bool CBlockTreeDB::MigrateOldIndexData() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This migration is the one-shot v23 -> v24 shim. With the flags cleared here it leaves no state behind after the first v24 start, so there is nothing left to harden further. Please mark it for removal in v25 instead of extending it:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed: after the first v24 start this shim leaves nothing behind, so it should be removed rather than extended. To keep your utACK on a0e35d3 valid, I'll add the 🤖 Posted autonomously by Claude on behalf of pasta. |
||
|
|
@@ -519,7 +522,11 @@ bool CBlockTreeDB::MigrateOldIndexData() | |
| // the legacy entries don't cover the suffix of the chain that ran with the index | ||
| // disabled, and copying them while stamping the chainstate tip as the locator would | ||
| // mark an incomplete index as fully synced. In that case we discard the source entries | ||
| // instead and let the new async index resync from genesis. | ||
| // instead and let the new async index resync from genesis. Once an enabled index has been | ||
| // processed its legacy entries are gone, so its flag is cleared: a later start must never | ||
| // adopt whatever the new database then holds (for example a partial rebuild after the | ||
| // index directory was deleted) as migrated data, and older versions see that their | ||
| // synchronous index needs a reindex. | ||
| bool legacy_flag = false; | ||
| const bool addressindex_was_current = ReadFlag("addressindex", legacy_flag) && legacy_flag; | ||
| const bool spentindex_was_current = ReadFlag("spentindex", legacy_flag) && legacy_flag; | ||
|
|
@@ -575,12 +582,14 @@ bool CBlockTreeDB::MigrateOldIndexData() | |
| return false; | ||
| } | ||
| // Finalize even when count==0 to recover from a previous run that crashed | ||
| // after writing target data but before writing the locator. | ||
| // after writing target data but before writing the locator. An empty target is | ||
| // left without a locator, so the index rebuilds from genesis. | ||
| if (timestampindex_was_current && TargetNeedsLocator(timestamp_db)) { | ||
| if (!FinalizeMigration(timestamp_db, best_block_hash, "timestamp index")) { | ||
| return false; | ||
| } | ||
| } | ||
| WriteFlag("timestampindex", false); | ||
| } | ||
|
|
||
| // Migrate spent index (only if enabled) | ||
|
|
@@ -599,6 +608,7 @@ bool CBlockTreeDB::MigrateOldIndexData() | |
| return false; | ||
| } | ||
| } | ||
| WriteFlag("spentindex", false); | ||
| } | ||
|
|
||
| // Migrate address index (includes both address and unspent indexes) (only if enabled) | ||
|
|
@@ -627,6 +637,7 @@ bool CBlockTreeDB::MigrateOldIndexData() | |
| return false; | ||
| } | ||
| } | ||
| WriteFlag("addressindex", false); | ||
| } | ||
|
|
||
| if (total_count > 0) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.