From 331a25cb16632042dd6782a9b62fcc5c8aa6da3b Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 6 Jun 2025 14:24:46 -0400 Subject: [PATCH 1/5] test: indexes, avoid creating threads when sync runs synchronously The indexes test call StartBackgroundSync(), which spawns a thread to run Sync(), only for the test thread to wait for it to complete by calling IndexWaitSynced(). So, since the sync is performed synchronously, we can skip the extra thread creation entirely and call Sync() directly. --- src/test/blockfilter_index_tests.cpp | 6 +----- src/test/coinstatsindex_tests.cpp | 8 ++------ src/test/txindex_tests.cpp | 6 +----- src/test/util/CMakeLists.txt | 1 - src/test/util/index.cpp | 22 ---------------------- src/test/util/index.h | 16 ---------------- 6 files changed, 4 insertions(+), 55 deletions(-) delete mode 100644 src/test/util/index.cpp delete mode 100644 src/test/util/index.h diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index 81119f52b1..1fc5bbdf3f 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -12,7 +12,6 @@ #include #include #include -#include #include #include @@ -143,10 +142,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.StartBackgroundSync()); - - // Allow filter index to catch up with the block index. - IndexWaitSynced(filter_index, *Assert(m_node.shutdown_signal)); + filter_index.Sync(); // Check that filter index has all blocks that were in the chain before it started. { diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index e09aad05e9..7d54f0fdf1 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -33,9 +32,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) // is started. BOOST_CHECK(!coin_stats_index.BlockUntilSyncedToCurrentChain()); - BOOST_REQUIRE(coin_stats_index.StartBackgroundSync()); - - IndexWaitSynced(coin_stats_index, *Assert(m_node.shutdown_signal)); + coin_stats_index.Sync(); // Check that CoinStatsIndex works for genesis block. const CBlockIndex* genesis_block_index; @@ -85,8 +82,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup) { CoinStatsIndex index{interfaces::MakeChain(m_node), 1 << 20}; BOOST_REQUIRE(index.Init()); - BOOST_REQUIRE(index.StartBackgroundSync()); - IndexWaitSynced(index, *Assert(m_node.shutdown_signal)); + index.Sync(); std::shared_ptr new_block; CBlockIndex* new_block_index = nullptr; { diff --git a/src/test/txindex_tests.cpp b/src/test/txindex_tests.cpp index 9ee5387830..e06735e186 100644 --- a/src/test/txindex_tests.cpp +++ b/src/test/txindex_tests.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include @@ -30,10 +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.StartBackgroundSync()); - - // Allow tx index to catch up with the block index. - IndexWaitSynced(txindex, *Assert(m_node.shutdown_signal)); + txindex.Sync(); // Check that txindex excludes genesis block transactions. const CBlock& genesis_block = Params().GenesisBlock(); diff --git a/src/test/util/CMakeLists.txt b/src/test/util/CMakeLists.txt index 3a6e31c720..32396c41c3 100644 --- a/src/test/util/CMakeLists.txt +++ b/src/test/util/CMakeLists.txt @@ -6,7 +6,6 @@ add_library(test_util STATIC EXCLUDE_FROM_ALL blockfilter.cpp coins.cpp coverage.cpp - index.cpp json.cpp logging.cpp mining.cpp diff --git a/src/test/util/index.cpp b/src/test/util/index.cpp deleted file mode 100644 index cfeba35756..0000000000 --- a/src/test/util/index.cpp +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) 2020-2022 The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#include - -#include -#include -#include -#include - -void IndexWaitSynced(const BaseIndex& index, const util::SignalInterrupt& interrupt) -{ - while (!index.BlockUntilSyncedToCurrentChain()) { - // Assert shutdown was not requested to abort the test, instead of looping forever, in case - // there was an unexpected error in the index that caused it to stop syncing and request a shutdown. - Assert(!interrupt); - - UninterruptibleSleep(100ms); - } - assert(index.GetSummary().synced); -} diff --git a/src/test/util/index.h b/src/test/util/index.h deleted file mode 100644 index a3bd1dddc3..0000000000 --- a/src/test/util/index.h +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright (c) 2020-2022 The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#ifndef BITCOIN_TEST_UTIL_INDEX_H -#define BITCOIN_TEST_UTIL_INDEX_H - -class BaseIndex; -namespace util { -class SignalInterrupt; -} // namespace util - -/** Block until the index is synced to the current chain */ -void IndexWaitSynced(const BaseIndex& index, const util::SignalInterrupt& interrupt); - -#endif // BITCOIN_TEST_UTIL_INDEX_H From 0a248708dc9d465db09168c39b3f12cb4c9465b7 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 24 Jan 2022 15:02:09 -0500 Subject: [PATCH 2/5] indexes, refactor: Stop requiring CBlockIndex type to call IsBIP30Unspendable This commit does not change behavior in any way. --- src/index/coinstatsindex.cpp | 2 +- src/validation.cpp | 6 +++--- src/validation.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index b5869416b9..9ec034ce3c 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -150,7 +150,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) const auto& tx{block.data->vtx.at(i)}; // Skip duplicate txid coinbase transactions (BIP30). - if (IsBIP30Unspendable(*pindex) && tx->IsCoinBase()) { + if (IsBIP30Unspendable(block.hash, block.height) && tx->IsCoinBase()) { m_total_unspendable_amount += block_subsidy; m_total_unspendables_bip30 += block_subsidy; continue; diff --git a/src/validation.cpp b/src/validation.cpp index 97190dc248..b87de9c456 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6341,10 +6341,10 @@ bool IsBIP30Repeat(const CBlockIndex& block_index) (block_index.nHeight==91880 && block_index.GetBlockHash() == uint256{"00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721"}); } -bool IsBIP30Unspendable(const CBlockIndex& block_index) +bool IsBIP30Unspendable(const uint256& block_hash, int block_height) { - return (block_index.nHeight==91722 && block_index.GetBlockHash() == uint256{"00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e"}) || - (block_index.nHeight==91812 && block_index.GetBlockHash() == uint256{"00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f"}); + return (block_height==91722 && block_hash == uint256{"00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e"}) || + (block_height==91812 && block_hash == uint256{"00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f"}); } static fs::path GetSnapshotCoinsDBPath(Chainstate& cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) diff --git a/src/validation.h b/src/validation.h index 0a8a94db12..b2ac117698 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1350,6 +1350,6 @@ bool DeploymentEnabled(const ChainstateManager& chainman, DEP dep) bool IsBIP30Repeat(const CBlockIndex& block_index); /** Identifies blocks which coinbase output was subsequently overwritten in the UTXO set (see BIP30) */ -bool IsBIP30Unspendable(const CBlockIndex& block_index); +bool IsBIP30Unspendable(const uint256& block_hash, int block_height); #endif // BITCOIN_VALIDATION_H From 6f1392cc42cde638773f2b697d7d2c58abcdc860 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Sun, 23 Jan 2022 23:20:41 -0500 Subject: [PATCH 3/5] 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. --- src/index/base.cpp | 25 ++++++++++++-- src/index/base.h | 10 +++--- src/index/blockfilterindex.cpp | 8 ++--- src/index/blockfilterindex.h | 2 +- src/index/coinstatsindex.cpp | 61 +++++++++++++--------------------- src/index/coinstatsindex.h | 6 ++-- src/interfaces/chain.h | 9 +++++ 7 files changed, 71 insertions(+), 50 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 5767116ce5..0e76bafeca 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -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 = █ + } + 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. diff --git a/src/index/base.h b/src/index/base.h index fbd9069a51..841bfcb00e 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -90,7 +90,7 @@ private: /// 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 @@ protected: 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& block) { return true; } @@ -117,9 +120,8 @@ protected: /// 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; diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 5ce85e1f84..460d5d7c41 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -316,7 +316,7 @@ 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 db_it(m_db->NewIterator()); @@ -324,7 +324,7 @@ bool BlockFilterIndex::CustomRewind(const interfaces::BlockRef& current_tip, con // 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; } diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h index ccb4845ef5..13543b961c 100644 --- a/src/index/blockfilterindex.h +++ b/src/index/blockfilterindex.h @@ -58,7 +58,7 @@ protected: 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; } diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 9ec034ce3c..5f82cf93b7 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -265,7 +265,7 @@ 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 db_it(m_db->NewIterator()); @@ -273,32 +273,14 @@ bool CoinStatsIndex::CustomRewind(const interfaces::BlockRef& current_tip, const // 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; } -// Reverse a single block as part of a reorg -bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex) +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 interfaces::BlockInfo& block) { - CBlockUndo block_undo; std::pair 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)) { + if (block.height > 0) { + if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) { return false; } - if (!m_db->Read(DBHeightKey(pindex->nHeight - 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]}; diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index 885b9e0a86..6e2743688a 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -38,18 +38,20 @@ private: 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& 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; } diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index de99262e98..f98eeeed8b 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -327,6 +327,15 @@ public: 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 handleNotifications(std::shared_ptr notifications) = 0; From 91b7ab6c69264a46f70825546a1574478d9e824a Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 10 Jun 2025 20:21:16 -0400 Subject: [PATCH 4/5] refactor: index, simplify CopyHeightIndexToHashIndex to process single block The function previously handled a range of heights but now copies only a single entry. This change simplifies the code to match its current usage. --- src/index/blockfilterindex.cpp | 43 +++++++++++++++------------------- src/index/coinstatsindex.cpp | 42 ++++++++++++++------------------- 2 files changed, 37 insertions(+), 48 deletions(-) diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 460d5d7c41..de55170daa 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -289,30 +289,25 @@ 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 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)); - - db_it.Next(); + 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 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; } @@ -321,10 +316,10 @@ bool BlockFilterIndex::CustomRemove(const interfaces::BlockInfo& block) CDBBatch batch(*m_db); std::unique_ptr 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, block.height - 1, block.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; } diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 5f82cf93b7..28e7d66e8f 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -238,30 +238,25 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) } [[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 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)); - - db_it.Next(); + 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 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; } @@ -270,10 +265,9 @@ bool CoinStatsIndex::CustomRemove(const interfaces::BlockInfo& block) CDBBatch batch(*m_db); std::unique_ptr 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, block.height - 1, block.height)) { + // During a reorg, copy the block's hash digest from the height index to the hash index, + // ensuring it's still accessible after the height index entry is overwritten. + if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, block.height)) { return false; } From 029ba1a21d570f7db6c4366ec9a30a381b56d6fb Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 18 Feb 2023 10:39:24 -0300 Subject: [PATCH 5/5] index: remove CBlockIndex access from CustomAppend() Moved CBlockUndo disk read lookups from child index classes to the base index class. The goal is for child index classes to synchronize only through events, without directly accessing the chain database. This change will enable future parallel synchronization mechanisms, reduce database access (when batched), and contribute toward the goal of running indexes in a separate process (with no chain database access). Besides that, this commit also documents how NextSyncBlock() behaves. It is not immediately clear this function could return the first block after the fork point during a reorg. --- src/index/base.cpp | 59 ++++++++++++++++++++++------------ src/index/base.h | 2 ++ src/index/blockfilterindex.cpp | 22 +++++-------- src/index/blockfilterindex.h | 2 ++ src/index/coinstatsindex.cpp | 11 ++----- src/interfaces/chain.h | 2 ++ 6 files changed, 55 insertions(+), 43 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 0e76bafeca..a5c0cc29c9 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -144,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 = █ + } + + 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(); @@ -192,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 = █ - } - 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) { @@ -337,17 +359,14 @@ void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptrGetBlockHash().ToString()); - return; } } diff --git a/src/index/base.h b/src/index/base.h index 841bfcb00e..4131b06cad 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -93,6 +93,8 @@ private: /// 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 diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index de55170daa..de43e8e271 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -13,7 +13,6 @@ #include #include #include -#include /* 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 chain, Blo m_filter_fileseq = std::make_unique(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& block) { if (!m_db->Read(DB_FILTER_POS, m_next_filter_pos)) { @@ -250,19 +256,7 @@ std::optional 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 diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h index 13543b961c..983cf32206 100644 --- a/src/index/blockfilterindex.h +++ b/src/index/blockfilterindex.h @@ -52,6 +52,8 @@ private: std::optional ReadFilterHeader(int height, const uint256& expected_block_hash); protected: + interfaces::Chain::NotifyOptions CustomOptions() override; + bool CustomInit(const std::optional& block) override; bool CustomCommit(CDBBatch& batch) override; diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 28e7d66e8f..ef2af8a567 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -114,19 +114,11 @@ CoinStatsIndex::CoinStatsIndex(std::unique_ptr chain, size_t bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) { - CBlockUndo block_undo; const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())}; m_total_subsidy += block_subsidy; // Ignore genesis block 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; - } - std::pair read_out; if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) { return false; @@ -183,7 +175,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) // 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{Assert(block.undo_data)->vtxundo.at(i - 1)}; for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) { Coin coin{tx_undo.vprevout[j]}; @@ -383,6 +375,7 @@ bool CoinStatsIndex::CustomCommit(CDBBatch& batch) interfaces::Chain::NotifyOptions CoinStatsIndex::CustomOptions() { interfaces::Chain::NotifyOptions options; + options.connect_undo_data = true; options.disconnect_data = true; options.disconnect_undo_data = true; return options; diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index f98eeeed8b..f6e831624d 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -330,6 +330,8 @@ public: //! Options specifying which chain notifications are required. struct NotifyOptions { + //! Include undo data with block connected notifications. + bool connect_undo_data = false; //! Include block data with block disconnected notifications. bool disconnect_data = false; //! Include undo data with block disconnected notifications.