fix: keep an unclean shutdown from blocking index startup through the uncommitted tip

src/index/base.cpp

@@ -118,8 +118,8 @@ bool BaseIndex::Init()
     const auto uncommitted_tip{WritesEagerly() ? GetDB().ReadUncommittedTip() : std::nullopt};
     if (uncommitted_tip && resume_block == locator_index) {
         // The entries of the blocks up to it are on disk, but without the blocks the index can't tell whether they
-        // are still on the active chain, nor rewind them.
-        if (!m_chainstate->m_blockman.LookupBlockIndex(*uncommitted_tip)) {
+        // are still on the active chain, nor rewind them. That only matters if readers trust them.
+        if (!m_chainstate->m_blockman.LookupBlockIndex(*uncommitted_tip) && !IgnoresEntriesOffActiveChain()) {
             return InitError(strprintf(Untranslated("%s: the block the index was last synced to is not found. Please "
                                                     "rebuild the index."),
                                        GetName()));
@@ -401,6 +401,11 @@ void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const
                       best_block_index->GetBlockHash().ToString());
             return;
         }
+        // Init() can resume from an uncommitted tip above the active chain. The blocks below it are indexed already,
+        // and after an unclean stop their data may not be on disk again yet, so they can't be rewound.
+        if (best_block_index->GetAncestor(pindex->nHeight) == pindex) {
+            return;
+        }
         if (best_block_index != pindex->pprev && !Rewind(best_block_index, pindex->pprev)) {
             FatalError("%s: Failed to rewind index %s to a previous chain tip",
                        __func__, GetName());

src/index/base.h

@@ -110,6 +110,11 @@ class BaseIndex : public CValidationInterface
     /// entries of blocks above it that are no longer on the active chain.
     virtual bool WritesEagerly() const { return false; }
 
+    /// Whether readers ignore entries of blocks that aren't on the active chain, so leftover entries of blocks that
+    /// left it are harmless. Init() then resumes from the locator when the block index lost the uncommitted tip,
+    /// instead of failing.
+    virtual bool IgnoresEntriesOffActiveChain() const { return false; }
+
 protected:
     std::unique_ptr<interfaces::Chain> m_chain;
     Chainstate* m_chainstate{nullptr};

src/index/govcollateralindex.cpp

@@ -107,6 +107,9 @@ bool GovCollateralIndex::CustomRewind(const interfaces::BlockKey& current_tip, c
     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) {
+        // A block without data, like one an unclean stop left with only its header, keeps its collaterals: they
+        // only confirm while it is on the active chain
+        if (!WITH_LOCK(::cs_main, return pindex->nStatus & BLOCK_HAVE_DATA)) continue;
         CBlock block;
         if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *pindex)) {
             LogError("%s: Failed to read block %s from disk\n", __func__, pindex->GetBlockHash().ToString());

src/index/govcollateralindex.h

@@ -33,6 +33,9 @@ class GovCollateralIndex final : public BaseIndex, public governance::Collateral
 protected:
     bool WritesEagerly() const override { return true; }
 
+    // A collateral only confirms while its block is on the active chain
+    bool IgnoresEntriesOffActiveChain() const override { return true; }
+
     bool CustomInit(const std::optional<interfaces::BlockKey>& block) override;
 
     bool CustomAppend(const interfaces::BlockInfo& block) override;

src/test/baseindex_tests.cpp

@@ -3,12 +3,15 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chain.h>
+#include <chainparams.h>
 #include <common/system.h>
 #include <consensus/validation.h>
 #include <index/coinstatsindex.h>
 #include <interfaces/chain.h>
 #include <node/blockstorage.h>
+#include <pow.h>
 #include <script/script.h>
+#include <shutdown.h>
 #include <test/util/index.h>
 #include <test/util/setup_common.h>
 #include <util/check.h>
@@ -23,9 +26,9 @@
 
 namespace {
 //! A minimal index that, like Dash's address and spent indexes, keeps one entry per block and erases the entries of
-//! rewound blocks in CustomRewind() block by block. The rewind is exposed, and can stop after a given block, 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.
+//! rewound blocks in CustomRewind() block by block, reading each one from disk. The rewind is exposed, and can stop
+//! after a given block, 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;
@@ -45,7 +48,7 @@ class ErasingIndex final : public BaseIndex
 
     bool Indexed(const CBlockIndex* block) const { return Indexed(block->GetBlockHash()); }
     bool Indexed(const uint256& block_hash) const { return m_db->Exists(std::make_pair(uint8_t{'e'}, block_hash)); }
-    //! Write the entry CustomAppend() writes, for a block the block index doesn't have
+    //! Write the entry CustomAppend() writes, for a block whose data the node doesn't have
     void WriteEntry(const uint256& block_hash)
     {
         BOOST_REQUIRE(m_db->Write(std::make_pair(uint8_t{'e'}, block_hash), true));
@@ -80,6 +83,8 @@ class ErasingIndex final : public BaseIndex
         const CBlockIndex* new_tip_index{WITH_LOCK(::cs_main, return m_chainstate->m_blockman.LookupBlockIndex(new_tip.hash))};
         return RewindBlockByBlock(current_tip_index, new_tip_index, [&](const CBlockIndex* pindex, CDBBatch& batch) {
             if (m_stop_after && pindex == m_stop_after->pprev) return false;
+            CBlock block;
+            if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *pindex)) return false;
             batch.Erase(std::make_pair(uint8_t{'e'}, pindex->GetBlockHash()));
             return true;
         });
@@ -323,6 +328,36 @@ BOOST_FIXTURE_TEST_CASE(restart_rewinds_blocks_synced_above_flush, TestChain100S
         BOOST_CHECK(index.Indexed(lost_block.GetHash()));
     }
     node::g_indexes_ready_to_sync = true;
+
+    // After an unclean stop the blocks above the flush can also be left with only their headers, as headers are
+    // stored before the blocks. The node connects them again as it downloads them, before the ones above them are
+    // on disk again, so the index must keep their entries rather than rewind blocks it can't read.
+    const CBlock reconnected{CreateBlock({}, CScript() << OP_2, chainman.ActiveChainstate())};
+    BOOST_REQUIRE(chainman.ProcessNewBlockHeaders({reconnected}, state));
+    const CBlockIndex* reconnected_index{
+        WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(reconnected.GetHash()))};
+    CBlockHeader header_only{reconnected};
+    header_only.hashPrevBlock = reconnected.GetHash();
+    header_only.nTime = reconnected.nTime + 1;
+    header_only.nBits = GetNextWorkRequired(reconnected_index, &header_only, Params().GetConsensus());
+    while (!CheckProofOfWork(header_only.GetHash(), header_only.nBits, Params().GetConsensus())) ++header_only.nNonce;
+    BOOST_REQUIRE(chainman.ProcessNewBlockHeaders({header_only}, state));
+    {
+        ErasingIndex index{interfaces::MakeChain(m_node)};
+        index.WriteEntry(reconnected.GetHash());
+        index.WriteEntry(header_only.GetHash());
+        index.SetUncommittedTip(header_only.GetHash());
+        BOOST_REQUIRE(index.Start());
+        IndexWaitSynced(index);
+        BOOST_REQUIRE(chainman.ProcessNewBlock(std::make_shared<const CBlock>(reconnected), /*force_processing=*/true,
+                                               /*new_block=*/nullptr));
+        SyncWithValidationInterfaceQueue();
+        BOOST_CHECK(!ShutdownRequested());
+        BOOST_CHECK(index.Indexed(reconnected_index));
+        BOOST_CHECK(index.Indexed(header_only.GetHash()));
+        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, reconnected_index->nHeight + 1);
+        index.Stop();
+    }
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/test/governance_collateral_tests.cpp

@@ -232,6 +232,42 @@ BOOST_FIXTURE_TEST_CASE(sync_check_needs_only_blocks_above_the_resume_point, Tes
     BOOST_CHECK(can_sync_without_data_at(50));
     BOOST_CHECK(!can_sync_without_data_at(102));
 
+    // After an unclean stop the uncommitted tip can be missing from the block index, or have only its header. A
+    // collateral only confirms while its block is on the active chain, so the index then resumes from the locator,
+    // or keeps the entries of the blocks a reorg rewinds that it can't read, instead of failing.
+    const auto write_uncommitted_tip = [&](const uint256& block_hash) {
+        CDBWrapper db{DBParams{.path = m_args.GetDataDirNet() / "indexes" / "govcollateralindex", .cache_bytes = 1 << 20}};
+        CDBBatch batch{db};
+        batch.Write(uint8_t{'U'}, block_hash);
+        BOOST_REQUIRE(db.WriteBatch(batch));
+    };
+    write_uncommitted_tip(uint256::ONE);
+    BOOST_CHECK(!can_sync_without_data_at(50));
+    {
+        GovCollateralIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1 << 20};
+        BOOST_REQUIRE(index.Start());
+        IndexWaitSynced(index);
+        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, 103);
+        index.Stop();
+    }
+    const CBlock header_only{CreateBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()), chainstate)};
+    BlockValidationState state;
+    BOOST_REQUIRE(m_node.chainman->ProcessNewBlockHeaders({header_only}, state));
+    write_uncommitted_tip(header_only.GetHash());
+    BOOST_CHECK(can_sync_without_data_at(50));
+    {
+        GovCollateralIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1 << 20};
+        BOOST_REQUIRE(index.Start());
+        IndexWaitSynced(index);
+        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, 104);
+        BOOST_REQUIRE(chainstate.InvalidateBlock(state, WITH_LOCK(::cs_main, return chainstate.m_chain.Tip())));
+        const CBlock replacement{CreateAndProcessBlock({}, CScript() << OP_TRUE)};
+        SyncWithValidationInterfaceQueue();
+        BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return chainstate.m_chain.Tip()->GetBlockHash()), replacement.GetHash());
+        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, 103);
+        index.Stop();
+    }
+
     // Startup fails when the block index lacks the locator's block, so the sync check must not rely on the
     // uncommitted tip then
     {