indexes, refactor: Remove remaining CBlockIndex* uses in index Rewind methods

Move ReadBlock code from CoinStatsIndex::CustomRewind to BaseIndex::Rewind

Move ReadUndo code from CoinStatsIndex::ReverseBlock to BaseIndex::Rewind

This commit does change behavior slightly. Since the new CustomRemove
methods only take a single block at a time instead of a range of
disconnected blocks, when they call CopyHeightIndexToHashIndex they will
now do an index seek for each removed block instead of only seeking once
to the height of the earliest removed block. Seeking instead of scanning
is a little worse for performance if there is a >1 block reorg, but
probably not noticeable unless the reorg is very deep.

Author: ryanofsky

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>
@@ -254,8 +255,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.

src/index/base.h

@@ -90,7 +90,7 @@ 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);
 
     virtual bool AllowPrune() const = 0;
@@ -107,6 +107,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 +120,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

@@ -316,15 +316,15 @@ bool BlockFilterIndex::Write(const BlockFilter& filter, uint32_t block_height, c
     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)) {
+    if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, block.height - 1, block.height)) {
         return false;
     }
 
@@ -334,8 +334,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

@@ -58,7 +58,7 @@ class BlockFilterIndex final : public BaseIndex
 
     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; }
 

src/index/coinstatsindex.cpp

@@ -265,40 +265,22 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
     return true;
 }
 
-bool CoinStatsIndex::CustomRewind(const interfaces::BlockRef& current_tip, const interfaces::BlockRef& new_tip)
+bool CoinStatsIndex::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 hash digests 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)) {
+    if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, block.height - 1, block.height)) {
         return false;
     }
 
     if (!m_db->WriteBatch(batch)) return false;
 
-    {
-        LOCK(cs_main);
-        const CBlockIndex* iter_tip{m_chainstate->m_blockman.LookupBlockIndex(current_tip.hash)};
-        const CBlockIndex* new_tip_index{m_chainstate->m_blockman.LookupBlockIndex(new_tip.hash)};
-
-        do {
-            CBlock block;
-
-            if (!m_chainstate->m_blockman.ReadBlock(block, *iter_tip)) {
-                LogError("%s: Failed to read block %s from disk\n",
-                             __func__, iter_tip->GetBlockHash().ToString());
-                return false;
-            }
-
-            if (!ReverseBlock(block, iter_tip)) {
-                return false; // failure cause logged internally
-            }
-
-            iter_tip = iter_tip->GetAncestor(iter_tip->nHeight - 1);
-        } while (new_tip_index != iter_tip);
+    if (!ReverseBlock(block)) {
+        return false; // failure cause logged internally
     }
 
     return true;
@@ -404,26 +386,29 @@ bool CoinStatsIndex::CustomCommit(CDBBatch& batch)
     return true;
 }
 
+interfaces::Chain::NotifyOptions CoinStatsIndex::CustomOptions()
+{
+    interfaces::Chain::NotifyOptions options;
+    options.disconnect_data = true;
+    options.disconnect_undo_data = true;
+    return options;
+}
+
 // Reverse a single block as part of a reorg
-bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex)
+bool CoinStatsIndex::ReverseBlock(const interfaces::BlockInfo& block)
 {
-    CBlockUndo block_undo;
     std::pair<uint256, DBVal> read_out;
 
-    const CAmount block_subsidy{GetBlockSubsidy(pindex->nHeight, Params().GetConsensus())};
+    const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};
     m_total_subsidy -= block_subsidy;
 
     // Ignore genesis block
-    if (pindex->nHeight > 0) {
-        if (!m_chainstate->m_blockman.ReadBlockUndo(block_undo, *pindex)) {
-            return false;
-        }
-
-        if (!m_db->Read(DBHeightKey(pindex->nHeight - 1), read_out)) {
+    if (block.height > 0) {
+        if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) {
             return false;
         }
 
-        uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
+        uint256 expected_block_hash{*block.prev_hash};
         if (read_out.first != expected_block_hash) {
             LogPrintf("WARNING: previous block header belongs to unexpected block %s; expected %s\n",
                       read_out.first.ToString(), expected_block_hash.ToString());
@@ -437,13 +422,15 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
     }
 
     // Remove the new UTXOs that were created from the block
-    for (size_t i = 0; i < block.vtx.size(); ++i) {
-        const auto& tx{block.vtx.at(i)};
+    assert(block.data);
+    assert(block.undo_data);
+    for (size_t i = 0; i < block.data->vtx.size(); ++i) {
+        const auto& tx{block.data->vtx.at(i)};
 
         for (uint32_t j = 0; j < tx->vout.size(); ++j) {
             const CTxOut& out{tx->vout[j]};
             COutPoint outpoint{tx->GetHash(), j};
-            Coin coin{out, pindex->nHeight, tx->IsCoinBase()};
+            Coin coin{out, block.height, tx->IsCoinBase()};
 
             // Skip unspendable coins
             if (coin.out.scriptPubKey.IsUnspendable()) {
@@ -467,7 +454,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
 
         // The coinbase tx has no undo data since no former output is spent
         if (!tx->IsCoinBase()) {
-            const auto& tx_undo{block_undo.vtxundo.at(i - 1)};
+            const auto& tx_undo{block.undo_data->vtxundo.at(i - 1)};
 
             for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) {
                 Coin coin{tx_undo.vprevout[j]};

src/index/coinstatsindex.h

@@ -38,18 +38,20 @@ class CoinStatsIndex final : public BaseIndex
     CAmount m_total_unspendables_scripts{0};
     CAmount m_total_unspendables_unclaimed_rewards{0};
 
-    [[nodiscard]] bool ReverseBlock(const CBlock& block, const CBlockIndex* pindex);
+    [[nodiscard]] bool ReverseBlock(const interfaces::BlockInfo& block);
 
     bool AllowPrune() const override { return true; }
 
 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 override { return *m_db; }
 

src/interfaces/chain.h

@@ -327,6 +327,15 @@ class Chain
         virtual void chainStateFlushed(ChainstateRole role, const CBlockLocator& locator) {}
     };
 
+    //! Options specifying which chain notifications are required.
+    struct NotifyOptions
+    {
+        //! Include block data with block disconnected notifications.
+        bool disconnect_data = false;
+        //! Include undo data with block disconnected notifications.
+        bool disconnect_undo_data = false;
+    };
+
     //! Register handler for notifications.
     virtual std::unique_ptr<Handler> handleNotifications(std::shared_ptr<Notifications> notifications) = 0;