indexes, refactor: Remove CChainState use in index CommitInternal method

Replace CommitInternal method with CustomCommit and use interfaces::Chain
instead of CChainState to generate block locator.

This commit does not change behavior in any way, except in the
(m_best_block_index == nullptr) case, which was added recently in
bitcoin/bitcoin#24117 as part of an ongoing attempt to
prevent index corruption if bitcoind is interrupted during startup. New
behavior in that case should be slightly better than the old behavior (skipping
the entire custom+base commit now vs only skipping the base commit previously)
and this might avoid more cases of corruption.

Author: ryanofsky

src/index/base.cpp

@@ -34,6 +34,15 @@ static void FatalError(const char* fmt, const Args&... args)
     StartShutdown();
 }
 
+CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)
+{
+    CBlockLocator locator;
+    bool found = chain.findBlock(block_hash, interfaces::FoundBlock().locator(locator));
+    assert(found);
+    assert(!locator.IsNull());
+    return locator;
+}
+
 BaseIndex::DB::DB(const fs::path& path, size_t n_cache_size, bool f_memory, bool f_wipe, bool f_obfuscate) :
     CDBWrapper(path, n_cache_size, f_memory, f_wipe, f_obfuscate)
 {}
@@ -206,22 +215,20 @@ void BaseIndex::ThreadSync()
 
 bool BaseIndex::Commit()
 {
-    CDBBatch batch(GetDB());
-    if (!CommitInternal(batch) || !GetDB().WriteBatch(batch)) {
-        return error("%s: Failed to commit latest %s state", __func__, GetName());
-    }
-    return true;
-}
-
-bool BaseIndex::CommitInternal(CDBBatch& batch)
-{
-    LOCK(cs_main);
     // Don't commit anything if we haven't indexed any block yet
     // (this could happen if init is interrupted).
-    if (m_best_block_index == nullptr) {
-        return false;
+    bool ok = m_best_block_index != nullptr;
+    if (ok) {
+        CDBBatch batch(GetDB());
+        ok = CustomCommit(batch);
+        if (ok) {
+            GetDB().WriteBestBlock(batch, GetLocator(*m_chain, m_best_block_index.load()->GetBlockHash()));
+            ok = GetDB().WriteBatch(batch);
+        }
+    }
+    if (!ok) {
+        return error("%s: Failed to commit latest %s state", __func__, GetName());
     }
-    GetDB().WriteBestBlock(batch, m_chainstate->m_chain.GetLocator(m_best_block_index));
     return true;
 }
 

src/index/base.h

@@ -104,7 +104,7 @@ class BaseIndex : public CValidationInterface
 
     /// Virtual method called internally by Commit that can be overridden to atomically
     /// commit more index state.
-    virtual bool CommitInternal(CDBBatch& batch);
+    virtual bool CustomCommit(CDBBatch& batch) { return true; }
 
     /// Rewind index to an earlier chain tip during a chain reorg. The tip must
     /// be an ancestor of the current best block.

src/index/blockfilterindex.cpp

@@ -128,7 +128,7 @@ bool BlockFilterIndex::CustomInit(const std::optional<interfaces::BlockKey>& blo
     return true;
 }
 
-bool BlockFilterIndex::CommitInternal(CDBBatch& batch)
+bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
 {
     const FlatFilePos& pos = m_next_filter_pos;
 
@@ -142,7 +142,7 @@ bool BlockFilterIndex::CommitInternal(CDBBatch& batch)
     }
 
     batch.Write(DB_FILTER_POS, pos);
-    return BaseIndex::CommitInternal(batch);
+    return true;
 }
 
 bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256& hash, BlockFilter& filter) const

src/index/blockfilterindex.h

@@ -43,7 +43,7 @@ class BlockFilterIndex final : public BaseIndex
 protected:
     bool CustomInit(const std::optional<interfaces::BlockKey>& block) override;
 
-    bool CommitInternal(CDBBatch& batch) override;
+    bool CustomCommit(CDBBatch& batch) override;
 
     bool CustomAppend(const interfaces::BlockInfo& block) override;
 

src/index/coinstatsindex.cpp

@@ -391,12 +391,12 @@ bool CoinStatsIndex::CustomInit(const std::optional<interfaces::BlockKey>& block
     return true;
 }
 
-bool CoinStatsIndex::CommitInternal(CDBBatch& batch)
+bool CoinStatsIndex::CustomCommit(CDBBatch& batch)
 {
     // DB_MUHASH should always be committed in a batch together with DB_BEST_BLOCK
     // to prevent an inconsistent state of the DB.
     batch.Write(DB_MUHASH, m_muhash);
-    return BaseIndex::CommitInternal(batch);
+    return true;
 }
 
 // Reverse a single block as part of a reorg

src/index/coinstatsindex.h

@@ -41,7 +41,7 @@ class CoinStatsIndex final : public BaseIndex
 protected:
     bool CustomInit(const std::optional<interfaces::BlockKey>& block) override;
 
-    bool CommitInternal(CDBBatch& batch) override;
+    bool CustomCommit(CDBBatch& batch) override;
 
     bool CustomAppend(const interfaces::BlockInfo& block) override;
 

src/interfaces/chain.h

@@ -57,6 +57,8 @@ class FoundBlock
     FoundBlock& mtpTime(int64_t& mtp_time) { m_mtp_time = &mtp_time; return *this; }
     //! Return whether block is in the active (most-work) chain.
     FoundBlock& inActiveChain(bool& in_active_chain) { m_in_active_chain = &in_active_chain; return *this; }
+    //! Return locator if block is in the active chain.
+    FoundBlock& locator(CBlockLocator& locator) { m_locator = &locator; return *this; }
     //! Return next block in the active chain if current block is in the active chain.
     FoundBlock& nextBlock(const FoundBlock& next_block) { m_next_block = &next_block; return *this; }
     //! Read block data from disk. If the block exists but doesn't have data
@@ -69,6 +71,7 @@ class FoundBlock
     int64_t* m_max_time = nullptr;
     int64_t* m_mtp_time = nullptr;
     bool* m_in_active_chain = nullptr;
+    CBlockLocator* m_locator = nullptr;
     const FoundBlock* m_next_block = nullptr;
     CBlock* m_data = nullptr;
     mutable bool found = false;

src/node/interfaces.cpp

@@ -402,6 +402,7 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
     if (block.m_max_time) *block.m_max_time = index->GetBlockTimeMax();
     if (block.m_mtp_time) *block.m_mtp_time = index->GetMedianTimePast();
     if (block.m_in_active_chain) *block.m_in_active_chain = active[index->nHeight] == index;
+    if (block.m_locator) { *block.m_locator = active.GetLocator(index); }
     if (block.m_next_block) FillBlock(active[index->nHeight] == index ? active[index->nHeight + 1] : nullptr, *block.m_next_block, lock, active);
     if (block.m_data) {
         REVERSE_LOCK(lock);