diff --git a/doc/man/dash-qt.1 b/doc/man/dash-qt.1 index ec4b1a40b8cf..b5d118c05a14 100644 --- a/doc/man/dash-qt.1 +++ b/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 diff --git a/doc/man/dashd.1 b/doc/man/dashd.1 index 5d4ae91221f1..32fd5dcb4238 100644 --- a/doc/man/dashd.1 +++ b/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 diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 90a0ed75ed64..b43918ed708b 100644 --- a/src/Makefile.test.include +++ b/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 \ diff --git a/src/index/addressindex.cpp b/src/index/addressindex.cpp index 342de4e359d6..86886a9272f6 100644 --- a/src/index/addressindex.cpp +++ b/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; } diff --git a/src/index/base.cpp b/src/index/base.cpp index 61b7e7ea768f..24f97f0e0019 100644 --- a/src/index/base.cpp +++ b/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; } diff --git a/src/index/base.h b/src/index/base.h index db67924d9e75..92bb7df9914c 100644 --- a/src/index/base.h +++ b/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. diff --git a/src/index/spentindex.cpp b/src/index/spentindex.cpp index 64c373a151ff..afeb4191f655 100644 --- a/src/index/spentindex.cpp +++ b/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; } diff --git a/src/index/timestampindex.cpp b/src/index/timestampindex.cpp index 6f1d9af18952..69573472b5ae 100644 --- a/src/index/timestampindex.cpp +++ b/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; } diff --git a/src/init.cpp b/src/init.cpp index 27f73133230f..cb6620cbcf7c 100644 --- a/src/init.cpp +++ b/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(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(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex); if (!g_txindex->Start()) { diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ec84092af90c..339a3133e227 100644 --- a/src/node/blockstorage.cpp +++ b/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 vImportFile } } // End scope of CImportingNow chainman.ActiveChainstate().LoadMempool(mempool_path); + g_indexes_ready_to_sync = true; } } // namespace node diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ec7ba9c76e81..4318ae57478f 100644 --- a/src/node/blockstorage.h +++ b/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 diff --git a/src/test/baseindex_tests.cpp b/src/test/baseindex_tests.cpp new file mode 100644 index 000000000000..710011a5857b --- /dev/null +++ b/src/test/baseindex_tests.cpp @@ -0,0 +1,238 @@ +// Copyright (c) 2020-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include