index: Don't commit ahead of the flushed chainstate

Otherwise, if the node has an unclean restart,
indexes with state (coinstatsindex) couldn't reorg to the
last flushed tip and would be corrupted.

Also updates documentation of Commit() -
the locator functionality isn't used, so the previous text was wrong:
We must have the best block in our block index after a restart.

Co-authored-by: Fabian Jahr <fjahr@protonmail.com>

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) {

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.

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()