indexes, refactor: Pass Chain interface instead of CChainState class to indexes

ryanofsky · ryanofsky · commit 33b4d48cfcdf · 2022-07-18T13:39:55.000-05:00
Passing abstract Chain interface will let indexes run in separate
processes.

This commit does not change behavior in any way.
diff --git a/src/index/base.cpp b/src/index/base.cpp
@@ -4,7 +4,9 @@
 
 #include <chainparams.h>
 #include <index/base.h>
+#include <interfaces/chain.h>
 #include <node/blockstorage.h>
+#include <node/context.h>
 #include <node/interface_ui.h>
 #include <shutdown.h>
 #include <tinyformat.h>
@@ -49,6 +51,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();
@@ -346,9 +351,11 @@ void BaseIndex::Interrupt()
     m_interrupt();
 }
 
-bool BaseIndex::Start(CChainState& 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);
diff --git a/src/index/base.h b/src/index/base.h
@@ -6,12 +6,16 @@
 #define BITCOIN_INDEX_BASE_H
 
 #include <dbwrapper.h>
+#include <interfaces/chain.h>
 #include <threadinterrupt.h>
 #include <validationinterface.h>
 
 class CBlock;
 class CBlockIndex;
 class CChainState;
+namespace interfaces {
+class Chain;
+} // namespace interfaces
 
 struct IndexSummary {
     std::string name;
@@ -79,6 +83,7 @@ class BaseIndex : public CValidationInterface
     virtual bool AllowPrune() const = 0;
 
 protected:
+    std::unique_ptr<interfaces::Chain> m_chain;
     CChainState* m_chainstate{nullptr};
 
     void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) override;
@@ -110,6 +115,7 @@ class BaseIndex : public CValidationInterface
     void SetBestBlockIndex(const CBlockIndex* block);
 
 public:
+    BaseIndex(std::unique_ptr<interfaces::Chain> chain);
     /// Destructor interrupts sync thread if running and blocks until it exits.
     virtual ~BaseIndex();
 
@@ -124,7 +130,7 @@ class BaseIndex : public CValidationInterface
 
     /// Start initializes the sync state and registers the instance as a
     /// ValidationInterface so that it stays in sync with blockchain updates.
-    [[nodiscard]] bool Start(CChainState& active_chainstate);
+    [[nodiscard]] bool Start();
 
     /// Stops the instance from staying in sync with blockchain updates.
     void Stop();
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
@@ -94,9 +94,9 @@ struct DBHashKey {
 
 static std::map<BlockFilterType, BlockFilterIndex> g_filter_indexes;
 
-BlockFilterIndex::BlockFilterIndex(BlockFilterType filter_type,
+BlockFilterIndex::BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, BlockFilterType filter_type,
                                    size_t n_cache_size, bool f_memory, bool f_wipe)
-    : m_filter_type(filter_type)
+    : BaseIndex(std::move(chain)), m_filter_type(filter_type)
 {
     const std::string& filter_name = BlockFilterTypeName(filter_type);
     if (filter_name.empty()) throw std::invalid_argument("unknown filter_type");
@@ -467,12 +467,12 @@ void ForEachBlockFilterIndex(std::function<void (BlockFilterIndex&)> fn)
     for (auto& entry : g_filter_indexes) fn(entry.second);
 }
 
-bool InitBlockFilterIndex(BlockFilterType filter_type,
+bool InitBlockFilterIndex(std::function<std::unique_ptr<interfaces::Chain>()> make_chain, BlockFilterType filter_type,
                           size_t n_cache_size, bool f_memory, bool f_wipe)
 {
     auto result = g_filter_indexes.emplace(std::piecewise_construct,
                                            std::forward_as_tuple(filter_type),
-                                           std::forward_as_tuple(filter_type,
+                                           std::forward_as_tuple(make_chain(), filter_type,
                                                                  n_cache_size, f_memory, f_wipe));
     return result.second;
 }
diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h
@@ -55,7 +55,7 @@ class BlockFilterIndex final : public BaseIndex
 
 public:
     /** Constructs the index, which becomes available to be queried. */
-    explicit BlockFilterIndex(BlockFilterType filter_type,
+    explicit BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, BlockFilterType filter_type,
                               size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
 
     BlockFilterType GetFilterType() const { return m_filter_type; }
@@ -88,7 +88,7 @@ void ForEachBlockFilterIndex(std::function<void (BlockFilterIndex&)> fn);
  * Initialize a block filter index for the given type if one does not already exist. Returns true if
  * a new index is created and false if one has already been initialized.
  */
-bool InitBlockFilterIndex(BlockFilterType filter_type,
+bool InitBlockFilterIndex(std::function<std::unique_ptr<interfaces::Chain>()> make_chain, BlockFilterType filter_type,
                           size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
 
 /**
diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp
@@ -102,7 +102,8 @@ struct DBHashKey {
 
 std::unique_ptr<CoinStatsIndex> g_coin_stats_index;
 
-CoinStatsIndex::CoinStatsIndex(size_t n_cache_size, bool f_memory, bool f_wipe)
+CoinStatsIndex::CoinStatsIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory, bool f_wipe)
+    : BaseIndex(std::move(chain))
 {
     fs::path path{gArgs.GetDataDirNet() / "indexes" / "coinstats"};
     fs::create_directories(path);
diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h
@@ -53,7 +53,7 @@ class CoinStatsIndex final : public BaseIndex
 
 public:
     // Constructs the index, which becomes available to be queried.
-    explicit CoinStatsIndex(size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
+    explicit CoinStatsIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
 
     // Look up stats for a specific block using CBlockIndex
     std::optional<kernel::CCoinsStats> LookUpStats(const CBlockIndex* block_index) const;
diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
@@ -48,8 +48,8 @@ bool TxIndex::DB::WriteTxs(const std::vector<std::pair<uint256, CDiskTxPos>>& v_
     return WriteBatch(batch);
 }
 
-TxIndex::TxIndex(size_t n_cache_size, bool f_memory, bool f_wipe)
-    : m_db(std::make_unique<TxIndex::DB>(n_cache_size, f_memory, f_wipe))
+TxIndex::TxIndex(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<TxIndex::DB>(n_cache_size, f_memory, f_wipe))
 {}
 
 TxIndex::~TxIndex() = default;
diff --git a/src/index/txindex.h b/src/index/txindex.h
@@ -31,7 +31,7 @@ class TxIndex final : public BaseIndex
 
 public:
     /// Constructs the index, which becomes available to be queried.
-    explicit TxIndex(size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
+    explicit TxIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
 
     // Destructor is declared because this class contains a unique_ptr to an incomplete type.
     virtual ~TxIndex() override;
diff --git a/src/init.cpp b/src/init.cpp
@@ -1594,22 +1594,22 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             return InitError(*error);
         }
 
-        g_txindex = std::make_unique<TxIndex>(cache_sizes.tx_index, false, fReindex);
-        if (!g_txindex->Start(chainman.ActiveChainstate())) {
+        g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex);
+        if (!g_txindex->Start()) {
             return false;
         }
     }
 
     for (const auto& filter_type : g_enabled_filter_types) {
-        InitBlockFilterIndex(filter_type, cache_sizes.filter_index, false, fReindex);
-        if (!GetBlockFilterIndex(filter_type)->Start(chainman.ActiveChainstate())) {
+        InitBlockFilterIndex([&]{ return interfaces::MakeChain(node); }, filter_type, cache_sizes.filter_index, false, fReindex);
+        if (!GetBlockFilterIndex(filter_type)->Start()) {
             return false;
         }
     }
 
     if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) {
-        g_coin_stats_index = std::make_unique<CoinStatsIndex>(/* cache size */ 0, false, fReindex);
-        if (!g_coin_stats_index->Start(chainman.ActiveChainstate())) {
+        g_coin_stats_index = std::make_unique<CoinStatsIndex>(interfaces::MakeChain(node), /* cache size */ 0, false, fReindex);
+        if (!g_coin_stats_index->Start()) {
             return false;
         }
     }
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
@@ -304,6 +304,10 @@ class Chain
 
     //! Return true if an assumed-valid chain is in use.
     virtual bool hasAssumedValidChain() = 0;
+
+    //! Get internal node context. Useful for testing, but not
+    //! accessible across processes.
+    virtual node::NodeContext* context() { return nullptr; }
 };
 
 //! Interface to let node manage chain clients (wallets, or maybe tools for
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -781,6 +781,7 @@ class ChainImpl : public Chain
         return Assert(m_node.chainman)->IsSnapshotActive();
     }
 
+    NodeContext* context() override { return &m_node; }
     NodeContext& m_node;
 };
 } // namespace
diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
@@ -7,6 +7,7 @@
 #include <consensus/merkle.h>
 #include <consensus/validation.h>
 #include <index/blockfilterindex.h>
+#include <interfaces/chain.h>
 #include <node/miner.h>
 #include <pow.h>
 #include <script/standard.h>
@@ -110,7 +111,7 @@ bool BuildChainTestingSetup::BuildChain(const CBlockIndex* pindex,
 
 BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup)
 {
-    BlockFilterIndex filter_index(BlockFilterType::BASIC, 1 << 20, true);
+    BlockFilterIndex filter_index(interfaces::MakeChain(m_node), BlockFilterType::BASIC, 1 << 20, true);
 
     uint256 last_header;
 
@@ -137,7 +138,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup)
     // BlockUntilSyncedToCurrentChain should return false before index is started.
     BOOST_CHECK(!filter_index.BlockUntilSyncedToCurrentChain());
 
-    BOOST_REQUIRE(filter_index.Start(m_node.chainman->ActiveChainstate()));
+    BOOST_REQUIRE(filter_index.Start());
 
     // Allow filter index to catch up with the block index.
     constexpr int64_t timeout_ms = 10 * 1000;
@@ -279,14 +280,14 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_init_destroy, BasicTestingSetup)
     filter_index = GetBlockFilterIndex(BlockFilterType::BASIC);
     BOOST_CHECK(filter_index == nullptr);
 
-    BOOST_CHECK(InitBlockFilterIndex(BlockFilterType::BASIC, 1 << 20, true, false));
+    BOOST_CHECK(InitBlockFilterIndex([&]{ return interfaces::MakeChain(m_node); }, BlockFilterType::BASIC, 1 << 20, true, false));
 
     filter_index = GetBlockFilterIndex(BlockFilterType::BASIC);
     BOOST_CHECK(filter_index != nullptr);
     BOOST_CHECK(filter_index->GetFilterType() == BlockFilterType::BASIC);
 
     // Initialize returns false if index already exists.
-    BOOST_CHECK(!InitBlockFilterIndex(BlockFilterType::BASIC, 1 << 20, true, false));
+    BOOST_CHECK(!InitBlockFilterIndex([&]{ return interfaces::MakeChain(m_node); }, BlockFilterType::BASIC, 1 << 20, true, false));
 
     int iter_count = 0;
     ForEachBlockFilterIndex([&iter_count](BlockFilterIndex& _index) { iter_count++; });
@@ -301,7 +302,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_init_destroy, BasicTestingSetup)
     BOOST_CHECK(filter_index == nullptr);
 
     // Reinitialize index.
-    BOOST_CHECK(InitBlockFilterIndex(BlockFilterType::BASIC, 1 << 20, true, false));
+    BOOST_CHECK(InitBlockFilterIndex([&]{ return interfaces::MakeChain(m_node); }, BlockFilterType::BASIC, 1 << 20, true, false));
 
     DestroyAllBlockFilterIndexes();
 
diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp
@@ -4,6 +4,7 @@
 
 #include <chainparams.h>
 #include <index/coinstatsindex.h>
+#include <interfaces/chain.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
 #include <util/time.h>
@@ -31,7 +32,7 @@ static void IndexWaitSynced(BaseIndex& index)
 
 BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
 {
-    CoinStatsIndex coin_stats_index{1 << 20, true};
+    CoinStatsIndex coin_stats_index{interfaces::MakeChain(m_node), 1 << 20, true};
 
     const CBlockIndex* block_index;
     {
@@ -46,7 +47,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
     // is started.
     BOOST_CHECK(!coin_stats_index.BlockUntilSyncedToCurrentChain());
 
-    BOOST_REQUIRE(coin_stats_index.Start(m_node.chainman->ActiveChainstate()));
+    BOOST_REQUIRE(coin_stats_index.Start());
 
     IndexWaitSynced(coin_stats_index);
 
@@ -90,8 +91,8 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup)
     CChainState& chainstate = Assert(m_node.chainman)->ActiveChainstate();
     const CChainParams& params = Params();
     {
-        CoinStatsIndex index{1 << 20};
-        BOOST_REQUIRE(index.Start(chainstate));
+        CoinStatsIndex index{interfaces::MakeChain(m_node), 1 << 20};
+        BOOST_REQUIRE(index.Start());
         IndexWaitSynced(index);
         std::shared_ptr<const CBlock> new_block;
         CBlockIndex* new_block_index = nullptr;
@@ -116,9 +117,9 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup)
     }
 
     {
-        CoinStatsIndex index{1 << 20};
+        CoinStatsIndex index{interfaces::MakeChain(m_node), 1 << 20};
         // Make sure the index can be loaded.
-        BOOST_REQUIRE(index.Start(chainstate));
+        BOOST_REQUIRE(index.Start());
         index.Stop();
     }
 }
diff --git a/src/test/txindex_tests.cpp b/src/test/txindex_tests.cpp
@@ -4,6 +4,7 @@
 
 #include <chainparams.h>
 #include <index/txindex.h>
+#include <interfaces/chain.h>
 #include <script/standard.h>
 #include <test/util/setup_common.h>
 #include <util/time.h>
@@ -15,7 +16,7 @@ BOOST_AUTO_TEST_SUITE(txindex_tests)
 
 BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
 {
-    TxIndex txindex(1 << 20, true);
+    TxIndex txindex(interfaces::MakeChain(m_node), 1 << 20, true);
 
     CTransactionRef tx_disk;
     uint256 block_hash;
@@ -28,7 +29,7 @@ BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
     // BlockUntilSyncedToCurrentChain should return false before txindex is started.
     BOOST_CHECK(!txindex.BlockUntilSyncedToCurrentChain());
 
-    BOOST_REQUIRE(txindex.Start(m_node.chainman->ActiveChainstate()));
+    BOOST_REQUIRE(txindex.Start());
 
     // Allow tx index to catch up with the block index.
     constexpr int64_t timeout_ms = 10 * 1000;
diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py
@@ -23,6 +23,9 @@
     "wallet/wallet -> wallet/walletdb -> wallet/wallet",
     "kernel/coinstats -> validation -> kernel/coinstats",
     "kernel/mempool_persist -> validation -> kernel/mempool_persist",
+
+    # Temporary, removed in followup https://github.com/bitcoin/bitcoin/pull/24230
+    "index/base -> node/context -> net_processing -> index/blockfilterindex -> index/base",
 )
 
 CODE_DIR = "src"

Original file line number	Diff line number	Diff line change
`@@ -94,9 +94,9 @@ struct DBHashKey {`
`94`	`94`
`95`	`95`	`static std::map<BlockFilterType, BlockFilterIndex> g_filter_indexes;`
`96`	`96`
`97`		`-BlockFilterIndex::BlockFilterIndex(BlockFilterType filter_type,`
	`97`	`+BlockFilterIndex::BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, BlockFilterType filter_type,`
`98`	`98`	`size_t n_cache_size, bool f_memory, bool f_wipe)`
`99`		`- : m_filter_type(filter_type)`
	`99`	`+ : BaseIndex(std::move(chain)), m_filter_type(filter_type)`
`100`	`100`	`{`
`101`	`101`	`const std::string& filter_name = BlockFilterTypeName(filter_type);`
`102`	`102`	`if (filter_name.empty()) throw std::invalid_argument("unknown filter_type");`
`@@ -467,12 +467,12 @@ void ForEachBlockFilterIndex(std::function<void (BlockFilterIndex&)> fn)`
`467`	`467`	`for (auto& entry : g_filter_indexes) fn(entry.second);`
`468`	`468`	`}`
`469`	`469`
`470`		`-bool InitBlockFilterIndex(BlockFilterType filter_type,`
	`470`	`+bool InitBlockFilterIndex(std::function<std::unique_ptr<interfaces::Chain>()> make_chain, BlockFilterType filter_type,`
`471`	`471`	`size_t n_cache_size, bool f_memory, bool f_wipe)`
`472`	`472`	`{`
`473`	`473`	`auto result = g_filter_indexes.emplace(std::piecewise_construct,`
`474`	`474`	`std::forward_as_tuple(filter_type),`
`475`		`- std::forward_as_tuple(filter_type,`
	`475`	`+ std::forward_as_tuple(make_chain(), filter_type,`
`476`	`476`	`n_cache_size, f_memory, f_wipe));`
`477`	`477`	`return result.second;`
`478`	`478`	`}`
Original file line number	Diff line number	Diff line change
`@@ -102,7 +102,8 @@ struct DBHashKey {`
`102`	`102`
`103`	`103`	`std::unique_ptr<CoinStatsIndex> g_coin_stats_index;`
`104`	`104`
`105`		`-CoinStatsIndex::CoinStatsIndex(size_t n_cache_size, bool f_memory, bool f_wipe)`
	`105`	`+CoinStatsIndex::CoinStatsIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory, bool f_wipe)`
	`106`	`+ : BaseIndex(std::move(chain))`
`106`	`107`	`{`
`107`	`108`	`fs::path path{gArgs.GetDataDirNet() / "indexes" / "coinstats"};`
`108`	`109`	`fs::create_directories(path);`
Original file line number	Diff line number	Diff line change
`@@ -48,8 +48,8 @@ bool TxIndex::DB::WriteTxs(const std::vector<std::pair<uint256, CDiskTxPos>>& v_`
`48`	`48`	`return WriteBatch(batch);`
`49`	`49`	`}`
`50`	`50`
`51`		`-TxIndex::TxIndex(size_t n_cache_size, bool f_memory, bool f_wipe)`
`52`		`- : m_db(std::make_unique<TxIndex::DB>(n_cache_size, f_memory, f_wipe))`
	`51`	`+TxIndex::TxIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory, bool f_wipe)`
	`52`	`+ : BaseIndex(std::move(chain)), m_db(std::make_unique<TxIndex::DB>(n_cache_size, f_memory, f_wipe))`
`53`	`53`	`{}`
`54`	`54`
`55`	`55`	`TxIndex::~TxIndex() = default;`
Original file line number	Diff line number	Diff line change
`@@ -1594,22 +1594,22 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)`
`1594`	`1594`	`return InitError(*error);`
`1595`	`1595`	`}`
`1596`	`1596`
`1597`		`- g_txindex = std::make_unique<TxIndex>(cache_sizes.tx_index, false, fReindex);`
`1598`		`- if (!g_txindex->Start(chainman.ActiveChainstate())) {`
	`1597`	`+ g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex);`
	`1598`	`+ if (!g_txindex->Start()) {`
`1599`	`1599`	`return false;`
`1600`	`1600`	`}`
`1601`	`1601`	`}`
`1602`	`1602`
`1603`	`1603`	`for (const auto& filter_type : g_enabled_filter_types) {`
`1604`		`- InitBlockFilterIndex(filter_type, cache_sizes.filter_index, false, fReindex);`
`1605`		`- if (!GetBlockFilterIndex(filter_type)->Start(chainman.ActiveChainstate())) {`
	`1604`	`+ InitBlockFilterIndex([&]{ return interfaces::MakeChain(node); }, filter_type, cache_sizes.filter_index, false, fReindex);`
	`1605`	`+ if (!GetBlockFilterIndex(filter_type)->Start()) {`
`1606`	`1606`	`return false;`
`1607`	`1607`	`}`
`1608`	`1608`	`}`
`1609`	`1609`
`1610`	`1610`	`if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) {`
`1611`		`- g_coin_stats_index = std::make_unique<CoinStatsIndex>(/* cache size */ 0, false, fReindex);`
`1612`		`- if (!g_coin_stats_index->Start(chainman.ActiveChainstate())) {`
	`1611`	`+ g_coin_stats_index = std::make_unique<CoinStatsIndex>(interfaces::MakeChain(node), /* cache size */ 0, false, fReindex);`
	`1612`	`+ if (!g_coin_stats_index->Start()) {`
`1613`	`1613`	`return false;`
`1614`	`1614`	`}`
`1615`	`1615`	`}`