backport: bitcoin#33212, bitcoin#34897 (don't commit indexes ahead of the flushed chainstate)

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 \

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

@@ -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;
 }
 

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/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);
 }
 

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/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 <chain.h>
+#include <consensus/validation.h>
+#include <index/coinstatsindex.h>
+#include <interfaces/chain.h>
+#include <node/blockstorage.h>
+#include <script/script.h>
+#include <test/util/index.h>
+#include <test/util/setup_common.h>
+#include <util/check.h>
+#include <validation.h>
+#include <validationinterface.h>
+
+#include <boost/test/unit_test.hpp>
+
+namespace {
+//! A minimal index that, like Dash's address, spent and timestamp indexes, keeps one entry per block and erases the
+//! entries of rewound blocks in CustomRewind(). The rewind hooks are exposed so a test can leave the on-disk state
+//! that an unclean shutdown in the middle of a reorg would leave, without having to time an actual crash.
+class ErasingIndex final : public BaseIndex
+{
+    const std::unique_ptr<BaseIndex::DB> m_db;
+
+public:
+    explicit ErasingIndex(std::unique_ptr<interfaces::Chain> chain)
+        : BaseIndex(std::move(chain), "erasing test index"),
+          m_db{std::make_unique<BaseIndex::DB>(gArgs.GetDataDirNet() / "indexes" / "erasingtest", /*n_cache_size=*/1 << 20)}
+    {
+    }
+    ~ErasingIndex() override { BaseIndex::Stop(); }
+
+    bool Indexed(const CBlockIndex* block) const { return m_db->Exists(std::make_pair(uint8_t{'e'}, block->GetBlockHash())); }
+    bool RewindTo(const CBlockIndex* current_tip, const CBlockIndex* new_tip)
+    {
+        return CustomRewind({current_tip->GetBlockHash(), current_tip->nHeight}, {new_tip->GetBlockHash(), new_tip->nHeight});
+    }
+    void CommitLocator(const CBlockIndex* tip)
+    {
+        CDBBatch batch(*m_db);
+        m_db->WriteBestBlock(batch, GetLocator(tip));
+        BOOST_REQUIRE(m_db->WriteBatch(batch));
+    }
+    void RewindLocatorTo(const CBlockIndex* new_tip)
+    {
+        CDBBatch batch(*m_db);
+        WriteRewindLocator(batch, new_tip);
+        BOOST_REQUIRE(m_db->WriteBatch(batch));
+    }
+    uint256 CommittedTip() const
+    {
+        CBlockLocator locator;
+        BOOST_REQUIRE(m_db->ReadBestBlock(locator) && !locator.IsNull());
+        return locator.vHave.front();
+    }
+
+protected:
+    bool CustomAppend(const interfaces::BlockInfo& block) override { return m_db->Write(std::make_pair(uint8_t{'e'}, block.hash), true); }
+    bool CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) override
+    {
+        const CBlockIndex* new_tip_index{WITH_LOCK(::cs_main, return m_chainstate->m_blockman.LookupBlockIndex(new_tip.hash))};
+        for (const CBlockIndex* pindex{WITH_LOCK(::cs_main, return m_chainstate->m_blockman.LookupBlockIndex(current_tip.hash))};
+             pindex != new_tip_index; pindex = pindex->pprev) {
+            if (!m_db->Erase(std::make_pair(uint8_t{'e'}, pindex->GetBlockHash()))) return false;
+        }
+        CDBBatch batch(*m_db);
+        WriteRewindLocator(batch, new_tip_index);
+        return m_db->WriteBatch(batch);
+    }
+    BaseIndex::DB& GetDB() const override { return *m_db; }
+    bool AllowPrune() const override { return true; }
+};
+
+const CBlockIndex* AtHeight(ChainstateManager& chainman, int height)
+{
+    return WITH_LOCK(::cs_main, return chainman.ActiveChain()[height]);
+}
+
+//! Makes the chainstate's last flushed block `height`, below the current tip, by flushing while the blocks above
+//! are disconnected and then connecting them again without a flush.
+void LowerLastFlushedBlock(ChainstateManager& chainman, int height)
+{
+    Chainstate& chainstate{chainman.ActiveChainstate()};
+    CBlockIndex* first_disconnected{WITH_LOCK(::cs_main, return chainman.ActiveChain()[height + 1])};
+    BlockValidationState state;
+    BOOST_REQUIRE(chainstate.InvalidateBlock(state, first_disconnected));
+    chainstate.ForceFlushStateToDisk();
+    WITH_LOCK(::cs_main, chainstate.ResetBlockFailureFlags(first_disconnected));
+    BOOST_REQUIRE(chainstate.ActivateBestChain(state));
+    BOOST_REQUIRE_EQUAL(WITH_LOCK(::cs_main, return chainstate.GetLastFlushedBlock()->nHeight), height);
+}
+} // namespace
+
+// Tests of generic BaseIndex functionality that is independent of which
+// concrete index is being used. CoinStatsIndex is used here merely as a
+// convenient instantiation of BaseIndex.
+BOOST_AUTO_TEST_SUITE(baseindex_tests)
+
+// Test that the index does not commit ahead of the chainstate's last
+// flushed block. If it did, a subsequent unclean shutdown would corrupt
+// the index, because during reverting it would require blocks that were
+// never flushed to disk.
+BOOST_FIXTURE_TEST_CASE(baseindex_no_commit_ahead_of_flush, TestChain100Setup)
+{
+    Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
+    auto sync_index = [&](bool do_flush, int expected_sync_height, int expected_commit_height) {
+        {
+            CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1 << 20};
+            BOOST_REQUIRE(index.Start());
+            IndexWaitSynced(index);
+            if (do_flush) {
+                chainstate.ForceFlushStateToDisk();
+                SyncWithValidationInterfaceQueue();
+            }
+            BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, expected_sync_height);
+            index.Stop();
+        }
+        // Reload index to see which block data was actually committed. Hold back the
+        // sync thread so the loaded best block is not advanced before it is read.
+        node::g_indexes_ready_to_sync = false;
+        {
+            CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1 << 20};
+            BOOST_REQUIRE(index.Start());
+            BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, expected_commit_height);
+            index.Interrupt();
+            index.Stop();
+        }
+        node::g_indexes_ready_to_sync = true;
+    };
+
+    // 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.
+    // The end-of-sync Commit() runs at chain tip (height 100) but the last
+    // flushed block is behind it, 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 the 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
+    // (the last flushed block stays at 100). For a real node this would happen
+    // in parallel with Sync(). Here we do it before Sync() to make the race
+    // state deterministic.
+    CreateAndProcessBlock({}, CScript() << OP_TRUE);
+    sync_index(false, 101, 100);
+}
+
+// An index that erases entries when it rewinds must not leave its committed locator above them, even while the
+// rewound tip is ahead of the last flush and Commit() can't write it.
+BOOST_FIXTURE_TEST_CASE(rewind_locator_follows_erased_entries, TestChain100Setup)
+{
+    ChainstateManager& chainman{*Assert(m_node.chainman)};
+    Chainstate& chainstate{chainman.ActiveChainstate()};
+
+    // A competing block at height 98 that stays off the active chain
+    CBlockIndex* old_98{WITH_LOCK(::cs_main, return chainman.ActiveChain()[98])};
+    BlockValidationState state;
+    BOOST_REQUIRE(chainstate.InvalidateBlock(state, old_98));
+    const uint256 competing_98{CreateAndProcessBlock({}, CScript() << OP_TRUE).GetHash()};
+    WITH_LOCK(::cs_main, chainstate.ResetBlockFailureFlags(old_98));
+    BOOST_REQUIRE(chainstate.ActivateBestChain(state));
+    const CBlockIndex* competing{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(competing_98))};
+    BOOST_REQUIRE_EQUAL(AtHeight(chainman, 100)->nHeight, 100);
+    chainstate.ForceFlushStateToDisk();
+    BOOST_REQUIRE_EQUAL(WITH_LOCK(::cs_main, return chainstate.GetLastFlushedBlock()->nHeight), 100);
+
+    node::g_indexes_ready_to_sync = false;
+    ErasingIndex index{interfaces::MakeChain(m_node)};
+    BOOST_REQUIRE(index.Start());
+
+    // Committed tip above the target on the same branch: the locator moves down to the target
+    index.CommitLocator(AtHeight(chainman, 100));
+    index.RewindLocatorTo(AtHeight(chainman, 97));
+    BOOST_CHECK(index.CommittedTip() == AtHeight(chainman, 97)->GetBlockHash());
+
+    // Committed tip equal to the target: unchanged
+    index.CommitLocator(AtHeight(chainman, 100));
+    index.RewindLocatorTo(AtHeight(chainman, 100));
+    BOOST_CHECK(index.CommittedTip() == AtHeight(chainman, 100)->GetBlockHash());
+
+    // Target on another branch: unchanged, never lowered to a common ancestor below entries that stay
+    index.RewindLocatorTo(competing);
+    BOOST_CHECK(index.CommittedTip() == AtHeight(chainman, 100)->GetBlockHash());
+
+    // Committed tip below the target: unchanged, a restart re-appends from it
+    index.CommitLocator(AtHeight(chainman, 95));
+    index.RewindLocatorTo(AtHeight(chainman, 97));
+    BOOST_CHECK(index.CommittedTip() == AtHeight(chainman, 95)->GetBlockHash());
+
+    index.Interrupt();
+    index.Stop();
+    node::g_indexes_ready_to_sync = true;
+}
+
+// A rewind persists its erasures right away. If the node then stops before the rewound tip is flushed and the
+// rewound blocks are back on the active chain, the next start must index them again.
+BOOST_FIXTURE_TEST_CASE(restart_after_rewind_reindexes_erased_blocks, TestChain100Setup)
+{
+    ChainstateManager& chainman{*Assert(m_node.chainman)};
+    {
+        ErasingIndex index{interfaces::MakeChain(m_node)};
+        BOOST_REQUIRE(index.Start());
+        IndexWaitSynced(index);
+        chainman.ActiveChainstate().ForceFlushStateToDisk();
+        SyncWithValidationInterfaceQueue();
+        index.Stop();
+    }
+    // The rewind target (97) is now ahead of the last flush (95), so Commit() could not persist it
+    LowerLastFlushedBlock(chainman, 95);
+    const CBlockIndex* tip{AtHeight(chainman, 100)};
+
+    node::g_indexes_ready_to_sync = false;
+    {
+        ErasingIndex index{interfaces::MakeChain(m_node)};
+        BOOST_REQUIRE(index.Start());
+        BOOST_CHECK(index.CommittedTip() == tip->GetBlockHash());
+        BOOST_REQUIRE(index.RewindTo(tip, AtHeight(chainman, 97)));
+        BOOST_CHECK(!index.Indexed(tip));
+        index.Interrupt();
+        index.Stop();
+    }
+    node::g_indexes_ready_to_sync = true;
+
+    // The rewound blocks are still the active chain, as after a restart in which their branch wins again
+    ErasingIndex index{interfaces::MakeChain(m_node)};
+    BOOST_REQUIRE(index.Start());
+    IndexWaitSynced(index);
+    for (int height{98}; height <= 100; ++height) {
+        BOOST_CHECK_MESSAGE(index.Indexed(AtHeight(chainman, height)), "block " << height << " was not indexed again");
+    }
+    index.Stop();
+}
+
+BOOST_AUTO_TEST_SUITE_END()