diff --git a/src/index/base.cpp b/src/index/base.cpp index bcd7c434b2b..68b712bbb12 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -279,6 +279,17 @@ void BaseIndex::Commit() // (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) { + LogDebug(BCLog::COINDB, "Skipping commit, index is ahead of flushed chainstate (index height %d, last flush at height %d)", + index_tip->nHeight, last_flushed ? last_flushed->nHeight : -1); + return; + } CDBBatch batch(GetDB()); ok = CustomCommit(batch); if (ok) { diff --git a/src/index/base.h b/src/index/base.h index 00ce800d69c..86ee9029c85 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -94,13 +94,9 @@ private: CThreadInterrupt m_interrupt; /// 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. + /// 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 CustomRemove. diff --git a/src/test/baseindex_tests.cpp b/src/test/baseindex_tests.cpp index 525309bf3a2..16cbab19738 100644 --- a/src/test/baseindex_tests.cpp +++ b/src/test/baseindex_tests.cpp @@ -42,10 +42,13 @@ BOOST_FIXTURE_TEST_CASE(baseindex_no_commit_ahead_of_flush, TestChain100Setup) // Part 1: Sync, then "crash" (stop without flushing). Models a node that // started up, had its index catch up, but never flushed before going down. - sync_index(false, 100, 100); + // The end-of-sync Commit() runs at chain tip (height 100) but + // m_last_flushed_block is null, so it is skipped. + sync_index(false, 100, 0); // Part 2: Restart cleanly. Sync, force a chainstate flush, and drain the // validation queue so the index's ChainStateFlushed callback runs. + // Now m_last_flushed_block == tip == 100 and the index can commit. sync_index(true, 100, 100); // Part 3: Connect a new block on the chain without flushing @@ -53,7 +56,7 @@ BOOST_FIXTURE_TEST_CASE(baseindex_no_commit_ahead_of_flush, TestChain100Setup) // in parallel with Sync(). Here we do it before Sync() to make the race // state deterministic. CreateAndProcessBlock({}, CScript() << OP_TRUE); - sync_index(false, 101, 101); + sync_index(false, 101, 100); } BOOST_AUTO_TEST_SUITE_END()