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
https://github.com/bitcoin/bitcoin/pull/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.

8 files changed, 31 insertions, 20 deletions

diff --git a/src/index/base.cpp b/src/index/base.cpp
index af11923206..4f399620e4 100644
--- a/src/index/base.cpp
+++ b/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;
 }
 
diff --git a/src/index/base.h b/src/index/base.h
index 0aee0db054..77c46dd428 100644
--- a/src/index/base.h
+++ b/src/index/base.h
@@ -104,7 +104,7 @@ protected:
 
     /// 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.
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index e223869843..8232567230 100644
--- a/src/index/blockfilterindex.cpp
+++ b/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
diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h
index 75ae97d11a..968eccb6b3 100644
--- a/src/index/blockfilterindex.h
+++ b/src/index/blockfilterindex.h
@@ -43,7 +43,7 @@ private:
 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;
 
diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp
index c8290887dc..b722118b2e 100644
--- a/src/index/coinstatsindex.cpp
+++ b/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
diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h
index 221dd21a20..c4af223388 100644
--- a/src/index/coinstatsindex.h
+++ b/src/index/coinstatsindex.h
@@ -41,7 +41,7 @@ private:
 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;
 
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index ed8df2256c..5fc0e540a9 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -57,6 +57,8 @@ public:
     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 @@ public:
     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;
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 414eff7125..56b5a928ad 100644
--- a/src/node/interfaces.cpp
+++ b/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);