diff --git a/src/Makefile.test.include b/src/Makefile.test.include index a145cbdec341..bbb90e659864 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 \ 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 42f6f12df59c..977c95561bd6 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -245,12 +245,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) { @@ -260,9 +271,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) @@ -274,18 +295,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/govcollateralindex.cpp b/src/index/govcollateralindex.cpp index cab00c694e1c..eeaf660a7d9b 100644 --- a/src/index/govcollateralindex.cpp +++ b/src/index/govcollateralindex.cpp @@ -117,7 +117,9 @@ bool GovCollateralIndex::CustomRewind(const interfaces::BlockKey& current_tip, c m_db->EraseCollateral(batch, txid); } } - m_db->WriteBestBlock(batch, GetLocator(new_tip_index)); + // Never persist a locator ahead of the chainstate's last flushed block (after an unclean shutdown the block + // index might not know it, and the index would refuse to load), but don't leave it above the erasures either. + WriteRewindLocator(batch, new_tip_index); return m_db->WriteBatch(batch); } 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/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