Merge bitcoin#25494: indexes: Stop using node internal types

7878f97 indexes, refactor: Remove CChainState use in index CommitInternal method (Ryan Ofsky)
ee3a079 indexes, refactor: Remove CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
dc971be indexes, refactor: Remove CBlockIndex* uses in index WriteBlock methods (Ryan Ofsky)
bef4e40 indexes, refactor: Remove CBlockIndex* uses in index Init methods (Ryan Ofsky)
addb4f2 indexes, refactor: Remove CBlockIndex* uses in coinstatsindex LookUpOne function (Ryan Ofsky)
33b4d48 indexes, refactor: Pass Chain interface instead of CChainState class to indexes (Ryan Ofsky)
a0b5b4a interfaces, refactor: Add more block information to block connected notifications (Ryan Ofsky)

Pull request description:

  Start transitioning index code away from using internal node types like `CBlockIndex` and `CChain` so index code is less coupled to node code and index code will later be able to stop locking cs_main and sync without having to deal with validationinterface race conditions, and so new indexes are easier to write and can run as plugins or separate processes.

  This PR contains the first 7 commits from bitcoin#24230 (comment) which have been split off for easier review. Previous review comments can be found in bitcoin#24230

ACKs for top commit:
  MarcoFalke:
    ACK 7878f97 though did not review the last commit 🤼
  mzumsande:
    Code Review ACK 7878f97

Tree-SHA512: f84ac2eb6dca2c305566ddeb35ea14d0b71c00860c0fd752bbcf1a0188be833d8c2a6ac9d3ef6ab5b46fbd02d7a24cbb8f60cf12464cb8ba208e22287f709989

Co-authored-by: fanquake <fanquake@gmail.com>

Author: knst

src/Makefile.am

@@ -279,6 +279,7 @@ BITCOIN_CORE_H = \
   interfaces/node.h \
   interfaces/wallet.h \
   kernel/blockmanager_opts.h \
+  kernel/chain.h \
   kernel/chainstatemanager_opts.h \
   kernel/checks.h \
   kernel/coinstats.h \
@@ -562,6 +563,7 @@ libbitcoin_node_a_SOURCES = \
   instantsend/lock.cpp \
   instantsend/net_instantsend.cpp \
   instantsend/signing.cpp \
+  kernel/chain.cpp \
   kernel/checks.cpp \
   kernel/coinstats.cpp \
   kernel/context.cpp \
@@ -1285,6 +1287,7 @@ libdashkernel_la_SOURCES = \
   init/common.cpp \
   instantsend/db.cpp \
   instantsend/instantsend.cpp \
+  kernel/chain.cpp \
   kernel/checks.cpp \
   kernel/coinstats.cpp \
   kernel/context.cpp \

src/core_write.cpp

@@ -19,7 +19,7 @@
 #include <util/strencodings.h>
 #include <util/system.h>
 
-#include <index/spentindex.h>
+#include <index/spentindex_types.h>
 
 #include <evo/assetlocktx.h>
 #include <evo/cbtx.h>

src/index/addressindex.cpp

@@ -13,6 +13,7 @@
 #include <tinyformat.h>
 #include <undo.h>
 #include <util/system.h>
+#include <validation.h>
 
 constexpr uint8_t DB_ADDRESSINDEX{'a'};
 constexpr uint8_t DB_ADDRESSUNSPENTINDEX{'u'};
@@ -159,39 +160,44 @@ bool AddressIndex::DB::RewindBatch(const std::vector<CAddressIndexEntry>& addres
     return CDBWrapper::WriteBatch(batch);
 }
 
-AddressIndex::AddressIndex(size_t n_cache_size, bool f_memory, bool f_wipe) :
+AddressIndex::AddressIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory, bool f_wipe) :
+    BaseIndex(std::move(chain)),
     m_db(std::make_unique<AddressIndex::DB>(n_cache_size, f_memory, f_wipe))
 {
 }
 
 AddressIndex::~AddressIndex() = default;
 
-bool AddressIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
+bool AddressIndex::CustomAppend(const interfaces::BlockInfo& block)
 {
     // Skip genesis block (no inputs to index)
-    if (pindex->nHeight == 0) {
+    if (block.height == 0) {
         return true;
     }
 
+    // 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));
+
     // Read undo data for this block to get information about spent outputs
     CBlockUndo blockundo;
     if (!node::UndoReadFromDisk(blockundo, pindex)) {
         return error("%s: Failed to read undo data for block %s at height %d", __func__,
-                     pindex->GetBlockHash().ToString(), pindex->nHeight);
+                     block.hash.ToString(), block.height);
     }
 
     std::vector<CAddressIndexEntry> addressIndex;
     std::vector<CAddressUnspentIndexEntry> addressUnspentIndex;
 
     // Process each non-coinbase transaction
     // blockundo.vtxundo[i] corresponds to block.vtx[i+1] (coinbase is skipped in undo data)
-    if (blockundo.vtxundo.size() != block.vtx.size() - 1) {
+    if (blockundo.vtxundo.size() != block.data->vtx.size() - 1) {
         return error("%s: Undo data size mismatch for block %s (expected %zu, got %zu)", __func__,
-                     pindex->GetBlockHash().ToString(), block.vtx.size() - 1, blockundo.vtxundo.size());
+                     block.hash.ToString(), block.data->vtx.size() - 1, blockundo.vtxundo.size());
     }
 
     for (size_t i = 0; i < blockundo.vtxundo.size(); i++) {
-        const CTransactionRef& tx = block.vtx[i + 1]; // +1 to skip coinbase
+        const CTransactionRef& tx = block.data->vtx[i + 1]; // +1 to skip coinbase
         const CTxUndo& txundo = blockundo.vtxundo[i];
         const uint256 txhash = tx->GetHash();
 
@@ -213,7 +219,7 @@ bool AddressIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
             }
 
             // Record spending activity
-            addressIndex.emplace_back(CAddressIndexKey(address_type, address_bytes, pindex->nHeight, i + 1, txhash, j, true),
+            addressIndex.emplace_back(CAddressIndexKey(address_type, address_bytes, block.height, i + 1, txhash, j, true),
                                       prevout.nValue * -1);
 
             // Remove from unspent index
@@ -234,17 +240,17 @@ bool AddressIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
             }
 
             // Record receiving activity
-            addressIndex.emplace_back(CAddressIndexKey(address_type, address_bytes, pindex->nHeight, i + 1, txhash, k, false),
+            addressIndex.emplace_back(CAddressIndexKey(address_type, address_bytes, block.height, i + 1, txhash, k, false),
                                       out.nValue);
 
             // Add to unspent index
             addressUnspentIndex.emplace_back(CAddressUnspentKey(address_type, address_bytes, txhash, k),
-                                             CAddressUnspentValue(out.nValue, out.scriptPubKey, pindex->nHeight));
+                                             CAddressUnspentValue(out.nValue, out.scriptPubKey, block.height));
         }
     }
 
     // Also process coinbase outputs (receiving activity only)
-    const CTransactionRef& coinbase = block.vtx[0];
+    const CTransactionRef& coinbase = block.data->vtx[0];
     const uint256 coinbase_hash = coinbase->GetHash();
     for (size_t k = 0; k < coinbase->vout.size(); k++) {
         const CTxOut& out = coinbase->vout[k];
@@ -256,24 +262,26 @@ bool AddressIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
         }
 
         // Record receiving activity for coinbase
-        addressIndex.emplace_back(CAddressIndexKey(address_type, address_bytes, pindex->nHeight, 0, coinbase_hash, k, false),
+        addressIndex.emplace_back(CAddressIndexKey(address_type, address_bytes, block.height, 0, coinbase_hash, k, false),
                                   out.nValue);
 
         // Add coinbase outputs to unspent index
         addressUnspentIndex.emplace_back(CAddressUnspentKey(address_type, address_bytes, coinbase_hash, k),
-                                         CAddressUnspentValue(out.nValue, out.scriptPubKey, pindex->nHeight));
+                                         CAddressUnspentValue(out.nValue, out.scriptPubKey, block.height));
     }
 
     return m_db->WriteBatch(addressIndex, addressUnspentIndex);
 }
 
-bool AddressIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip)
+bool AddressIndex::CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip)
 {
-    assert(current_tip->GetAncestor(new_tip->nHeight) == new_tip);
+    const CBlockIndex* current_tip_index = WITH_LOCK(cs_main, return m_chainstate->m_blockman.LookupBlockIndex(current_tip.hash));
+    const CBlockIndex* new_tip_index = WITH_LOCK(cs_main, return m_chainstate->m_blockman.LookupBlockIndex(new_tip.hash));
+    assert(current_tip_index->GetAncestor(new_tip_index->nHeight) == new_tip_index);
 
     // Rewind the unspent index by processing blocks in reverse
     // We need to undo all operations from current_tip back to (but not including) new_tip
-    for (const CBlockIndex* pindex = current_tip; pindex != new_tip; pindex = pindex->pprev) {
+    for (const CBlockIndex* pindex = current_tip_index; pindex != new_tip_index; pindex = pindex->pprev) {
         CBlock block;
         if (!node::ReadBlockFromDisk(block, pindex, Params().GetConsensus())) {
             return error("%s: Failed to read block %s from disk during rewind", __func__,
@@ -387,8 +395,7 @@ bool AddressIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new
         }
     }
 
-    // Call base class Rewind to update the best block pointer
-    return BaseIndex::Rewind(current_tip, new_tip);
+    return true;
 }
 
 BaseIndex::DB& AddressIndex::GetDB() const { return *m_db; }

src/index/addressindex.h

@@ -62,18 +62,18 @@ class AddressIndex final : public BaseIndex
     bool AllowPrune() const override { return false; }
 
     /// Write block data to the index databases
-    bool WriteBlock(const CBlock& block, const CBlockIndex* pindex) override;
+    bool CustomAppend(const interfaces::BlockInfo& block) override;
 
     /// Custom rewind to handle both transaction history and unspent index
-    bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip) override;
+    bool CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) override;
 
     BaseIndex::DB& GetDB() const override;
 
     const char* GetName() const override { return "addressindex"; }
 
 public:
     /// Constructs the index, which becomes available to be queried
-    explicit AddressIndex(size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
+    explicit AddressIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
 
     /// Destructor
     virtual ~AddressIndex() override;

src/index/base.cpp

@@ -4,7 +4,10 @@
 
 #include <chainparams.h>
 #include <index/base.h>
+#include <interfaces/chain.h>
+#include <kernel/chain.h>
 #include <node/blockstorage.h>
+#include <node/context.h>
 #include <node/interface_ui.h>
 #include <shutdown.h>
 #include <tinyformat.h>
@@ -31,6 +34,15 @@ void BaseIndex::FatalErrorImpl(const std::string& message)
     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)
 {}
@@ -49,6 +61,9 @@ void BaseIndex::DB::WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator
     batch.Write(DB_BEST_BLOCK, locator);
 }
 
+BaseIndex::BaseIndex(std::unique_ptr<interfaces::Chain> chain)
+    : m_chain{std::move(chain)} {}
+
 BaseIndex::~BaseIndex()
 {
     Interrupt();
@@ -164,12 +179,15 @@ void BaseIndex::ThreadSync()
             }
 
             CBlock block;
+            interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex);
             if (!ReadBlockFromDisk(block, pindex, consensus_params)) {
                 FatalError("%s: Failed to read block %s from disk",
                            __func__, pindex->GetBlockHash().ToString());
                 return;
+            } else {
+                block_info.data = &block;
             }
-            if (!WriteBlock(block, pindex)) {
+            if (!CustomAppend(block_info)) {
                 FatalError("%s: Failed to write block %s to index database",
                            __func__, pindex->GetBlockHash().ToString());
                 return;
@@ -200,22 +218,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;
 }
 
@@ -224,6 +240,10 @@ 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;
+    }
+
     // In the case of a reorg, ensure persisted block locator is not stale.
     // Pruning has a minimum of 288 blocks-to-keep and getting the index
     // out of sync may be possible but a users fault.
@@ -271,8 +291,8 @@ void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const
             return;
         }
     }
-
-    if (WriteBlock(*block, pindex)) {
+    interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex, block.get());
+    if (CustomAppend(block_info)) {
         // 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
@@ -377,13 +397,18 @@ void BaseIndex::Interrupt()
     m_interrupt();
 }
 
-bool BaseIndex::Start(Chainstate& active_chainstate)
+bool BaseIndex::Start()
 {
-    m_chainstate = &active_chainstate;
+    // m_chainstate member gives indexing code access to node internals. It is
+    // removed in followup https://github.com/bitcoin/bitcoin/pull/24230
+    m_chainstate = &m_chain->context()->chainman->ActiveChainstate();
     // Need to register this ValidationInterface before running Init(), so that
     // callbacks are not missed if Init sets m_synced to true.
     RegisterValidationInterface(this);
-    if (!Init()) {
+    if (!Init()) return false;
+
+    const CBlockIndex* index = m_best_block_index.load();
+    if (!CustomInit(index ? std::make_optional(interfaces::BlockKey{index->GetBlockHash(), index->nHeight}) : std::nullopt)) {
         return false;
     }