Merge bitcoin/bitcoin#32694: index: move disk read lookups to base class

029ba1a index: remove CBlockIndex access from CustomAppend() (furszy)
91b7ab6 refactor: index, simplify CopyHeightIndexToHashIndex to process single block (furszy)
6f1392c indexes, refactor: Remove remaining CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
0a24870 indexes, refactor: Stop requiring CBlockIndex type to call IsBIP30Unspendable (Ryan Ofsky)
331a25c test: indexes, avoid creating threads when sync runs synchronously (furszy)

Pull request description:

  Combining common refactors from #24230 and #26966, aiming to move both efforts forward while reducing their size and review burden.

  Broadly, #24230 focuses on enabling indexes to run in a separate process, and #26966 aims to parallelize the indexes initial synchronization process. A shared prerequisite for both is ensuring that only the base index class interacts with the node’s chain internals - child index classes should instead operate solely through chain events.

  This PR moves disk read lookups from child index classes to the base index class. It also includes a few documentation improvements and a test-only code cleanup.

ACKs for top commit:
  maflcko:
    review ACK 029ba1a 👡
  achow101:
    ACK 029ba1a
  TheCharlatan:
    Re-ACK 029ba1a
  davidgumberg:
    ACK 029ba1a
  mzumsande:
    Code Review ACK 029ba1a

Tree-SHA512: f073af407fc86f228cb47a32c7bcf2241551cc89ff32059317eb81d5b86fd5fda35f228d2567e0aedbc9fd6826291f5fee05619db35ba44108421ae04d11e6fb

Author: achow101

src/index/base.cpp

@@ -14,6 +14,7 @@
 #include <node/database_args.h>
 #include <node/interface_ui.h>
 #include <tinyformat.h>
+#include <undo.h>
 #include <util/string.h>
 #include <util/thread.h>
 #include <util/translation.h>
@@ -143,9 +144,44 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
         return pindex;
     }
 
+    // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
+    // Caller will be responsible for rewinding back to the common ancestor.
     return chain.Next(chain.FindFork(pindex_prev));
 }
 
+bool BaseIndex::ProcessBlock(const CBlockIndex* pindex, const CBlock* block_data)
+{
+    interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex, block_data);
+
+    CBlock block;
+    if (!block_data) { // disk lookup if block data wasn't provided
+        if (!m_chainstate->m_blockman.ReadBlock(block, *pindex)) {
+            FatalErrorf("%s: Failed to read block %s from disk",
+                        __func__, pindex->GetBlockHash().ToString());
+            return false;
+        }
+        block_info.data = &block;
+    }
+
+    CBlockUndo block_undo;
+    if (CustomOptions().connect_undo_data) {
+        if (pindex->nHeight > 0 && !m_chainstate->m_blockman.ReadBlockUndo(block_undo, *pindex)) {
+            FatalErrorf("%s: Failed to read undo block data %s from disk",
+                        __func__, pindex->GetBlockHash().ToString());
+            return false;
+        }
+        block_info.undo_data = &block_undo;
+    }
+
+    if (!CustomAppend(block_info)) {
+        FatalErrorf("%s: Failed to write block %s to index database",
+                    __func__, pindex->GetBlockHash().ToString());
+        return false;
+    }
+
+    return true;
+}
+
 void BaseIndex::Sync()
 {
     const CBlockIndex* pindex = m_best_block_index.load();
@@ -191,20 +227,7 @@ void BaseIndex::Sync()
             pindex = pindex_next;
 
 
-            CBlock block;
-            interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex);
-            if (!m_chainstate->m_blockman.ReadBlock(block, *pindex)) {
-                FatalErrorf("%s: Failed to read block %s from disk",
-                           __func__, pindex->GetBlockHash().ToString());
-                return;
-            } else {
-                block_info.data = &block;
-            }
-            if (!CustomAppend(block_info)) {
-                FatalErrorf("%s: Failed to write block %s to index database",
-                           __func__, pindex->GetBlockHash().ToString());
-                return;
-            }
+            if (!ProcessBlock(pindex)) return; // error logged internally
 
             auto current_time{std::chrono::steady_clock::now()};
             if (last_log_time + SYNC_LOG_INTERVAL < current_time) {
@@ -254,8 +277,28 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti
     assert(current_tip == m_best_block_index);
     assert(current_tip->GetAncestor(new_tip->nHeight) == new_tip);
 
-    if (!CustomRewind({current_tip->GetBlockHash(), current_tip->nHeight}, {new_tip->GetBlockHash(), new_tip->nHeight})) {
-        return false;
+    CBlock block;
+    CBlockUndo block_undo;
+
+    for (const CBlockIndex* iter_tip = current_tip; iter_tip != new_tip; iter_tip = iter_tip->pprev) {
+        interfaces::BlockInfo block_info = kernel::MakeBlockInfo(iter_tip);
+        if (CustomOptions().disconnect_data) {
+            if (!m_chainstate->m_blockman.ReadBlock(block, *iter_tip)) {
+                LogError("%s: Failed to read block %s from disk",
+                             __func__, iter_tip->GetBlockHash().ToString());
+                return false;
+            }
+            block_info.data = &block;
+        }
+        if (CustomOptions().disconnect_undo_data && iter_tip->nHeight > 0) {
+            if (!m_chainstate->m_blockman.ReadBlockUndo(block_undo, *iter_tip)) {
+                return false;
+            }
+            block_info.undo_data = &block_undo;
+        }
+        if (!CustomRemove(block_info)) {
+            return false;
+        }
     }
 
     // In the case of a reorg, ensure persisted block locator is not stale.
@@ -316,17 +359,14 @@ void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptr<const
             return;
         }
     }
-    interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex, block.get());
-    if (CustomAppend(block_info)) {
+
+    // Dispatch block to child class; errors are logged internally and abort the node.
+    if (ProcessBlock(pindex, block.get())) {
         // Setting the best block index is intentionally the last step of this
         // function, so BlockUntilSyncedToCurrentChain callers waiting for the
         // best block index to be updated can rely on the block being fully
         // processed, and the index object being safe to delete.
         SetBestBlockIndex(pindex);
-    } else {
-        FatalErrorf("%s: Failed to write block %s to index",
-                   __func__, pindex->GetBlockHash().ToString());
-        return;
     }
 }
 

src/index/base.h

@@ -90,9 +90,11 @@ class BaseIndex : public CValidationInterface
     /// getting corrupted.
     bool Commit();
 
-    /// Loop over disconnected blocks and call CustomRewind.
+    /// Loop over disconnected blocks and call CustomRemove.
     bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip);
 
+    bool ProcessBlock(const CBlockIndex* pindex, const CBlock* block_data = nullptr);
+
     virtual bool AllowPrune() const = 0;
 
     template <typename... Args>
@@ -107,6 +109,9 @@ class BaseIndex : public CValidationInterface
 
     void ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) override;
 
+    /// Return custom notification options for index.
+    [[nodiscard]] virtual interfaces::Chain::NotifyOptions CustomOptions() { return {}; }
+
     /// Initialize internal state from the database and block index.
     [[nodiscard]] virtual bool CustomInit(const std::optional<interfaces::BlockRef>& block) { return true; }
 
@@ -117,9 +122,8 @@ class BaseIndex : public CValidationInterface
     /// commit more index state.
     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.
-    [[nodiscard]] virtual bool CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip) { return true; }
+    /// Rewind index by one block during a chain reorg.
+    [[nodiscard]] virtual bool CustomRemove(const interfaces::BlockInfo& block) { return true; }
 
     virtual DB& GetDB() const = 0;
 

src/index/blockfilterindex.cpp

@@ -13,7 +13,6 @@
 #include <node/blockstorage.h>
 #include <undo.h>
 #include <util/fs_helpers.h>
-#include <validation.h>
 
 /* The index database stores three items for each block: the disk location of the encoded filter,
  * its dSHA256 hash, and the header. Those belonging to blocks on the active chain are indexed by
@@ -112,6 +111,13 @@ BlockFilterIndex::BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, Blo
     m_filter_fileseq = std::make_unique<FlatFileSeq>(std::move(path), "fltr", FLTR_FILE_CHUNK_SIZE);
 }
 
+interfaces::Chain::NotifyOptions BlockFilterIndex::CustomOptions()
+{
+    interfaces::Chain::NotifyOptions options;
+    options.connect_undo_data = true;
+    return options;
+}
+
 bool BlockFilterIndex::CustomInit(const std::optional<interfaces::BlockRef>& block)
 {
     if (!m_db->Read(DB_FILTER_POS, m_next_filter_pos)) {
@@ -250,19 +256,7 @@ std::optional<uint256> BlockFilterIndex::ReadFilterHeader(int height, const uint
 
 bool BlockFilterIndex::CustomAppend(const interfaces::BlockInfo& block)
 {
-    CBlockUndo block_undo;
-
-    if (block.height > 0) {
-        // pindex variable gives indexing code access to node internals. It
-        // will be removed in upcoming commit
-        const CBlockIndex* pindex = WITH_LOCK(cs_main, return m_chainstate->m_blockman.LookupBlockIndex(block.hash));
-        if (!m_chainstate->m_blockman.ReadBlockUndo(block_undo, *pindex)) {
-            return false;
-        }
-    }
-
-    BlockFilter filter(m_filter_type, *Assert(block.data), block_undo);
-
+    BlockFilter filter(m_filter_type, *Assert(block.data), *Assert(block.undo_data));
     const uint256& header = filter.ComputeHeader(m_last_header);
     bool res = Write(filter, block.height, header);
     if (res) m_last_header = header; // update last header
@@ -289,42 +283,37 @@ bool BlockFilterIndex::Write(const BlockFilter& filter, uint32_t block_height, c
 }
 
 [[nodiscard]] static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch,
-                                       const std::string& index_name,
-                                       int start_height, int stop_height)
+                                                     const std::string& index_name, int height)
 {
-    DBHeightKey key(start_height);
+    DBHeightKey key(height);
     db_it.Seek(key);
 
-    for (int height = start_height; height <= stop_height; ++height) {
-        if (!db_it.GetKey(key) || key.height != height) {
-            LogError("%s: unexpected key in %s: expected (%c, %d)\n",
-                         __func__, index_name, DB_BLOCK_HEIGHT, height);
-            return false;
-        }
-
-        std::pair<uint256, DBVal> value;
-        if (!db_it.GetValue(value)) {
-            LogError("%s: unable to read value in %s at key (%c, %d)\n",
-                         __func__, index_name, DB_BLOCK_HEIGHT, height);
-            return false;
-        }
-
-        batch.Write(DBHashKey(value.first), std::move(value.second));
+    if (!db_it.GetKey(key) || key.height != height) {
+        LogError("%s: unexpected key in %s: expected (%c, %d)\n",
+                 __func__, index_name, DB_BLOCK_HEIGHT, height);
+        return false;
+    }
 
-        db_it.Next();
+    std::pair<uint256, DBVal> value;
+    if (!db_it.GetValue(value)) {
+        LogError("%s: unable to read value in %s at key (%c, %d)\n",
+                 __func__, index_name, DB_BLOCK_HEIGHT, height);
+        return false;
     }
+
+    batch.Write(DBHashKey(value.first), std::move(value.second));
     return true;
 }
 
-bool BlockFilterIndex::CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip)
+bool BlockFilterIndex::CustomRemove(const interfaces::BlockInfo& block)
 {
     CDBBatch batch(*m_db);
     std::unique_ptr<CDBIterator> db_it(m_db->NewIterator());
 
-    // During a reorg, we need to copy all filters for blocks that are getting disconnected from the
-    // height index to the hash index so we can still find them when the height index entries are
-    // overwritten.
-    if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, new_tip.height, current_tip.height)) {
+    // During a reorg, we need to copy block filter that is getting disconnected from the
+    // height index to the hash index so we can still find it when the height index entry
+    // is overwritten.
+    if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, block.height)) {
         return false;
     }
 
@@ -334,8 +323,8 @@ bool BlockFilterIndex::CustomRewind(const interfaces::BlockRef& current_tip, con
     batch.Write(DB_FILTER_POS, m_next_filter_pos);
     if (!m_db->WriteBatch(batch)) return false;
 
-    // Update cached header
-    m_last_header = *Assert(ReadFilterHeader(new_tip.height, new_tip.hash));
+    // Update cached header to the previous block hash
+    m_last_header = *Assert(ReadFilterHeader(block.height - 1, *Assert(block.prev_hash)));
     return true;
 }
 

src/index/blockfilterindex.h

@@ -52,13 +52,15 @@ class BlockFilterIndex final : public BaseIndex
     std::optional<uint256> ReadFilterHeader(int height, const uint256& expected_block_hash);
 
 protected:
+    interfaces::Chain::NotifyOptions CustomOptions() override;
+
     bool CustomInit(const std::optional<interfaces::BlockRef>& block) override;
 
     bool CustomCommit(CDBBatch& batch) override;
 
     bool CustomAppend(const interfaces::BlockInfo& block) override;
 
-    bool CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip) override;
+    bool CustomRemove(const interfaces::BlockInfo& block) override;
 
     BaseIndex::DB& GetDB() const LIFETIMEBOUND override { return *m_db; }