backport: bitcoin#25193, bitcoin#33212, bitcoin#34897 and index migration fix for v24.0.x

doc/man/dash-qt.1

@@ -451,7 +451,7 @@ This will also rebuild active optional indexes.
 .IP
 Rebuild chain state from the currently indexed blocks. When in pruning
 mode or if blocks on disk might be corrupted, use full \fB\-reindex\fR
-instead. Deactivate all optional indexes before running this.
+instead.
 .HP
 \fB\-spentindex\fR
 .IP

doc/man/dashd.1

@@ -449,7 +449,7 @@ This will also rebuild active optional indexes.
 .IP
 Rebuild chain state from the currently indexed blocks. When in pruning
 mode or if blocks on disk might be corrupted, use full \fB\-reindex\fR
-instead. Deactivate all optional indexes before running this.
+instead.
 .HP
 \fB\-spentindex\fR
 .IP

src/Makefile.test.include

@@ -82,6 +82,7 @@ BITCOIN_TESTS =\
   test/base32_tests.cpp \
   test/base58_tests.cpp \
   test/base64_tests.cpp \
+  test/baseindex_tests.cpp \
   test/bech32_tests.cpp \
   test/bip32_tests.cpp \
   test/bip324_tests.cpp \
@@ -138,6 +139,7 @@ BITCOIN_TESTS =\
   test/hash_tests.cpp \
   test/httpserver_tests.cpp \
   test/i2p_tests.cpp \
+  test/index_migration_tests.cpp \
   test/interfaces_tests.cpp \
   test/key_io_tests.cpp \
   test/key_tests.cpp \

src/index/addressindex.cpp

@@ -398,7 +398,9 @@ bool AddressIndex::CustomRewind(const interfaces::BlockKey& current_tip, const i
         }
     }
 
-    return true;
+    CDBBatch locator_batch(GetDB());
+    WriteRewindLocator(locator_batch, new_tip_index);
+    return GetDB().WriteBatch(locator_batch);
 }
 
 BaseIndex::DB& AddressIndex::GetDB() const { return *m_db; }

src/index/base.cpp

@@ -22,6 +22,7 @@
 
 using node::PruneLockInfo;
 using node::ReadBlockFromDisk;
+using node::g_indexes_ready_to_sync;
 
 constexpr uint8_t DB_BEST_BLOCK{'B'};
 
@@ -81,14 +82,22 @@ bool BaseIndex::Init()
     if (locator.IsNull()) {
         SetBestBlockIndex(nullptr);
     } else {
-        SetBestBlockIndex(m_chainstate->FindForkInGlobalIndex(locator));
+        // Setting the best block to the locator's top block. If it is not part of the
+        // best chain, we will rewind to the fork point during index sync
+        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.at(0))};
+        if (!locator_index) {
+            return InitError(strprintf(Untranslated("%s: best block of the index not found. Please rebuild the index."), GetName()));
+        }
+        SetBestBlockIndex(locator_index);
     }
 
     // Note: this will latch to true immediately if the user starts up with an empty
     // datadir and an index enabled. If this is the case, indexation will happen solely
     // via `BlockConnected` signals until, possibly, the next restart.
     m_synced = m_best_block_index.load() == active_chain.Tip();
-    if (!m_synced) {
+
+    // Skip pruning check if indexes are not ready to sync (because reindex-chainstate has wiped the chain).
+    if (!m_synced && g_indexes_ready_to_sync) {
         bool prune_violation = false;
         if (!m_best_block_index) {
             // index is not built yet
@@ -143,6 +152,12 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 
 void BaseIndex::ThreadSync()
 {
+    // Wait for a possible reindex-chainstate to finish until continuing
+    // with the index sync
+    while (!g_indexes_ready_to_sync) {
+        if (!m_interrupt.sleep_for(std::chrono::milliseconds(500))) return;
+    }
+
     const CBlockIndex* pindex = m_best_block_index.load();
     if (!m_synced) {
         auto& consensus_params = Params().GetConsensus();
@@ -215,12 +230,23 @@ void BaseIndex::ThreadSync()
     }
 }
 
-bool BaseIndex::Commit()
+void BaseIndex::Commit()
 {
     // Don't commit anything if we haven't indexed any block yet
     // (this could happen if init is interrupted).
     bool ok = m_best_block_index != nullptr;
     if (ok) {
+        // Don't commit if the index best block is not an ancestor of the chainstate's last flushed
+        // block. Otherwise, after an unclean shutdown, the index could be
+        // persisted ahead of a chainstate it can no longer roll back to, which
+        // would corrupt indexes with state (e.g. coinstatsindex).
+        const CBlockIndex* index_tip = m_best_block_index.load();
+        const CBlockIndex* last_flushed = WITH_LOCK(::cs_main, return m_chainstate->GetLastFlushedBlock());
+        if (!last_flushed || last_flushed->GetAncestor(index_tip->nHeight) != index_tip) {
+            LogPrint(BCLog::COINDB, "Skipping commit, index is ahead of flushed chainstate (index height %d, last flush at height %d)\n",
+                     index_tip->nHeight, last_flushed ? last_flushed->nHeight : -1);
+            return;
+        }
         CDBBatch batch(GetDB());
         ok = CustomCommit(batch);
         if (ok) {
@@ -230,9 +256,19 @@ bool BaseIndex::Commit()
     }
     if (!ok) {
         LogError("%s: Failed to commit latest %s state\n", __func__, GetName());
-        return false;
     }
-    return true;
+}
+
+void BaseIndex::WriteRewindLocator(CDBBatch& batch, const CBlockIndex* new_tip) const
+{
+    CBlockLocator locator;
+    if (!GetDB().ReadBestBlock(locator) || locator.IsNull()) return;
+    const CBlockIndex* committed = WITH_LOCK(::cs_main, return m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.front()));
+    // The committed tip was an ancestor of a flushed block when it was written, so it and new_tip below it are in
+    // the block index after a restart. A committed tip at or below new_tip, or on another branch, needs no change.
+    if (committed && committed->nHeight > new_tip->nHeight && committed->GetAncestor(new_tip->nHeight) == new_tip) {
+        GetDB().WriteBestBlock(batch, GetLocator(*m_chain, new_tip->GetBlockHash()));
+    }
 }
 
 bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip)
@@ -244,18 +280,13 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti
         return false;
     }
 
-    // In the case of a reorg, ensure persisted block locator is not stale.
+    // Don't commit here - the committed index state must never be ahead of the
+    // flushed chainstate, otherwise unclean restarts would lead to index corruption.
     // Pruning has a minimum of 288 blocks-to-keep and getting the index
     // out of sync may be possible but a users fault.
     // In case we reorg beyond the pruned depth, ReadBlockFromDisk would
     // throw and lead to a graceful shutdown
     SetBestBlockIndex(new_tip);
-    if (!Commit()) {
-        // If commit fails, revert the best block index to avoid corruption.
-        SetBestBlockIndex(current_tip);
-        return false;
-    }
-
     return true;
 }
 

src/index/base.h

@@ -82,14 +82,10 @@ class BaseIndex : public CValidationInterface
     void ThreadSync();
 
     /// Write the current index state (eg. chain block locator and subclass-specific items) to disk.
-    ///
-    /// Recommendations for error handling:
-    /// If called on a successor of the previous committed best block in the index, the index can
-    /// continue processing without risk of corruption, though the index state will need to catch up
-    /// from further behind on reboot. If the new state is not a successor of the previous state (due
-    /// to a chain reorganization), the index must halt until Commit succeeds or else it could end up
-    /// getting corrupted.
-    bool Commit();
+    /// Will skip the commit if no block has been indexed yet or if the index's best block is
+    /// ahead of the chainstate's last flushed block. This avoids persisting state an unclean shutdown
+    /// could not roll back from. A later call commits when the chainstate has flushed far enough.
+    void Commit();
 
     /// Loop over disconnected blocks and call CustomRewind.
     bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip);
@@ -121,6 +117,11 @@ class BaseIndex : public CValidationInterface
     /// be an ancestor of the current best block.
     [[nodiscard]] virtual bool CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) { return true; }
 
+    /// For indexes whose CustomRewind() erases entries: if the committed locator is above new_tip on the same
+    /// branch, move it to new_tip. Commit() may not write new_tip yet (it can be ahead of the last flush), but
+    /// leaving the old locator above erased entries would make an unclean restart resume past them.
+    void WriteRewindLocator(CDBBatch& batch, const CBlockIndex* new_tip) const;
+
     virtual DB& GetDB() const = 0;
 
     /// Get the name of the index for display in logs.

src/index/spentindex.cpp

@@ -158,7 +158,9 @@ bool SpentIndex::CustomRewind(const interfaces::BlockKey& current_tip, const int
         }
     }
 
-    return true;
+    CDBBatch locator_batch(GetDB());
+    WriteRewindLocator(locator_batch, new_tip_index);
+    return GetDB().WriteBatch(locator_batch);
 }
 
 BaseIndex::DB& SpentIndex::GetDB() const { return *m_db; }

src/index/timestampindex.cpp

@@ -85,7 +85,9 @@ bool TimestampIndex::CustomRewind(const interfaces::BlockKey& current_tip, const
         }
     }
 
-    return true;
+    CDBBatch locator_batch(GetDB());
+    WriteRewindLocator(locator_batch, new_tip_index);
+    return GetDB().WriteBatch(locator_batch);
 }
 
 BaseIndex::DB& TimestampIndex::GetDB() const { return *m_db; }

src/init.cpp

@@ -169,6 +169,7 @@ using node::CalculateCacheSizes;
 using node::DEFAULT_PERSIST_MEMPOOL;
 using node::DEFAULT_PRINTPRIORITY;
 using node::DEFAULT_STOPAFTERBLOCKIMPORT;
+using node::g_indexes_ready_to_sync;
 using node::LoadChainstate;
 using node::MempoolPath;
 using node::ShouldPersistMempool;
@@ -639,7 +640,7 @@ void SetupServerArgs(ArgsManager& argsman)
 
     argsman.AddArg("-addressindex", strprintf("Maintain a full address index, used to query for the balance, txids and unspent outputs for addresses (default: %u)", DEFAULT_ADDRESSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk. This will also rebuild active optional indexes.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
-    argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead. Deactivate all optional indexes before running this.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
+    argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-spentindex", strprintf("Maintain a full spent index, used to query the spending txid and input index for an outpoint (default: %u)", DEFAULT_SPENTINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-timestampindex", strprintf("Maintain a timestamp index for block hashes, used to query blocks hashes by a range of timestamps (default: %u)", DEFAULT_TIMESTAMPINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-txindex", strprintf("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)", DEFAULT_TXINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
@@ -1372,19 +1373,6 @@ bool AppInitParameterInteraction(const ArgsManager& args)
     if (args.GetBoolArg("-peerbloomfilters", DEFAULT_PEERBLOOMFILTERS))
         nLocalServices = ServiceFlags(nLocalServices | NODE_BLOOM);
 
-    if (args.GetBoolArg("-reindex-chainstate", false)) {
-        // indexes that must be deactivated to prevent index corruption, see #24630
-        if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) {
-            return InitError(_("-reindex-chainstate option is not compatible with -coinstatsindex. Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
-        }
-        if (g_enabled_filter_types.count(BlockFilterType::BASIC_FILTER)) {
-            return InitError(_("-reindex-chainstate option is not compatible with -blockfilterindex. Please temporarily disable blockfilterindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
-        }
-        if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
-            return InitError(_("-reindex-chainstate option is not compatible with -txindex. Please temporarily disable txindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
-        }
-    }
-
     try {
         const bool fQuorumVvecRequestsEnabled{llmq::GetEnabledQuorumVvecSyncEntries(args).size() > 0};
         if (!args.GetBoolArg("-llmq-data-recovery", llmq::DEFAULT_ENABLE_QUORUM_DATA_RECOVERY) && fQuorumVvecRequestsEnabled) {
@@ -2283,6 +2271,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     node.peerman->AddExtraHandler(std::make_unique<SyncManager>(node.peerman.get(), *node.govman, *node.mn_sync, *node.connman, *node.netfulfilledman));
 
     // ********************************************************* Step 8: start indexers
+
+    // If reindex-chainstate was specified, delay syncing indexes until ThreadImport has reindexed the chain
+    if (!fReindexChainState) g_indexes_ready_to_sync = true;
     if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
         g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex);
         if (!g_txindex->Start()) {

src/node/blockstorage.cpp

@@ -29,6 +29,7 @@ std::atomic_bool fImporting(false);
 std::atomic_bool fReindex(false);
 bool fPruneMode = false;
 uint64_t nPruneTarget = 0;
+std::atomic_bool g_indexes_ready_to_sync{false};
 
 bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const
 {
@@ -961,5 +962,6 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
         }
     } // End scope of CImportingNow
     chainman.ActiveChainstate().LoadMempool(mempool_path);
+    g_indexes_ready_to_sync = true;
 }
 } // namespace node

src/node/blockstorage.h

@@ -50,6 +50,7 @@ extern std::atomic_bool fImporting;
 extern std::atomic_bool fReindex;
 extern bool fPruneMode;
 extern uint64_t nPruneTarget;
+extern std::atomic_bool g_indexes_ready_to_sync;
 
 // Because validation code takes pointers to the map's CBlockIndex objects, if
 // we ever switch to another associative container, we need to either use a