From 749637da89221c9623e25270c2980e830e35b31b Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 17 May 2023 13:26:44 -0400 Subject: [PATCH 1/8] Merge bitcoin/bitcoin#25193: indexes: Read the locator's top block during init, allow interaction with reindex-chainstate BACKPORT NOTE: tests that delete blocks/ and chainstate/ must delete indexes/ too: txindex is on by default in Dash and BaseIndex::Init now refuses to start when the index's best block is unknown ("txindex: best block of the index not found. Please rebuild the index.") ------- 97844d9268b87b5d09b1091bfd0326ed18ce5b91 index: Enable reindex-chainstate with active indexes (Martin Zumsande) 60bec3c82d09ec208bf0028ad32499e251ad205e index: Use first block from locator instead of looking for fork point (Martin Zumsande) Pull request description: This makes two improvements to the index init phase: **1) Prevent index corruption in case a reorg happens when the index was switched off**: This is done by reading in the top block stored in the locator instead of looking for a fork point already in `BaseIndex::Init()`. Before, we'd just go back to the fork point by calling `FindForkInGlobalIndex()`, which would have corrupted the coinstatsindex because its saved muhash needs to be reverted step by step by un-applying all blocks in between, which wasn't done before. This is now being done a bit later in `ThreadSync()`, which has existing logic to call the custom `Rewind()` method when going back along the chain to the forking point (thanks ryanofsky for pointing this out to me!). **2) Allow using the `-reindex-chainstate` option without needing to disabling indexes**: With `BaseIndex::Init()` not calling `FindForkInGlobalIndex()` anymore, we can allow `reindex-chainstate` with active indexes. `reindex-chainstate` deletes the chain and rebuilds it later in `ThreadImport`, so there is no chain available during `BaseIndex::Init()`, which would lead to problems (see #24789). But now we'll only need the chain a bit later in `BaseIndex::ThreadSync`, which will wait for the reindex-chainstate in `ThreadImport` to finish and will continue syncing after that. ACKs for top commit: ryanofsky: Code review ACK 97844d9268b87b5d09b1091bfd0326ed18ce5b91. Just simple rebase since last review Tree-SHA512: e24973fc22e0b87a49026f4820aecb0a4e415f4d381bade9969dd31cf97afecfea0449dce7fcc797343b792199cc8287276d1f5ffa4433dcb54fb24a808db6fb Co-authored-by: Ryan Ofsky (cherry picked from commit 85885880a22bc5994d0615a3ba78103d9f358b7f) --- src/index/base.cpp | 19 +++++++++-- src/init.cpp | 19 +++-------- src/node/blockstorage.cpp | 2 ++ src/node/blockstorage.h | 1 + src/test/util/setup_common.cpp | 1 + test/functional/feature_coinstatsindex.py | 39 ++++++++++++++++++----- test/functional/p2p_blockfilters.py | 7 ---- test/functional/wallet_backup.py | 1 + test/functional/wallet_hd.py | 2 ++ 9 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 61b7e7ea768f..84ce98143288 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -22,6 +22,7 @@ using node::PruneLockInfo; using node::ReadBlockFromDisk; +using node::g_indexes_ready_to_sync; constexpr uint8_t DB_BEST_BLOCK{'B'}; @@ -81,14 +82,22 @@ bool BaseIndex::Init() if (locator.IsNull()) { SetBestBlockIndex(nullptr); } else { - SetBestBlockIndex(m_chainstate->FindForkInGlobalIndex(locator)); + // Setting the best block to the locator's top block. If it is not part of the + // best chain, we will rewind to the fork point during index sync + const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.at(0))}; + if (!locator_index) { + return InitError(strprintf(Untranslated("%s: best block of the index not found. Please rebuild the index."), GetName())); + } + SetBestBlockIndex(locator_index); } // Note: this will latch to true immediately if the user starts up with an empty // datadir and an index enabled. If this is the case, indexation will happen solely // via `BlockConnected` signals until, possibly, the next restart. m_synced = m_best_block_index.load() == active_chain.Tip(); - if (!m_synced) { + + // Skip pruning check if indexes are not ready to sync (because reindex-chainstate has wiped the chain). + if (!m_synced && g_indexes_ready_to_sync) { bool prune_violation = false; if (!m_best_block_index) { // index is not built yet @@ -143,6 +152,12 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain& void BaseIndex::ThreadSync() { + // Wait for a possible reindex-chainstate to finish until continuing + // with the index sync + while (!g_indexes_ready_to_sync) { + if (!m_interrupt.sleep_for(std::chrono::milliseconds(500))) return; + } + const CBlockIndex* pindex = m_best_block_index.load(); if (!m_synced) { auto& consensus_params = Params().GetConsensus(); diff --git a/src/init.cpp b/src/init.cpp index 27f73133230f..cb6620cbcf7c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -169,6 +169,7 @@ using node::CalculateCacheSizes; using node::DEFAULT_PERSIST_MEMPOOL; using node::DEFAULT_PRINTPRIORITY; using node::DEFAULT_STOPAFTERBLOCKIMPORT; +using node::g_indexes_ready_to_sync; using node::LoadChainstate; using node::MempoolPath; using node::ShouldPersistMempool; @@ -639,7 +640,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-addressindex", strprintf("Maintain a full address index, used to query for the balance, txids and unspent outputs for addresses (default: %u)", DEFAULT_ADDRESSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING); argsman.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk. This will also rebuild active optional indexes.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING); - argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead. Deactivate all optional indexes before running this.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING); + argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING); argsman.AddArg("-spentindex", strprintf("Maintain a full spent index, used to query the spending txid and input index for an outpoint (default: %u)", DEFAULT_SPENTINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING); argsman.AddArg("-timestampindex", strprintf("Maintain a timestamp index for block hashes, used to query blocks hashes by a range of timestamps (default: %u)", DEFAULT_TIMESTAMPINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING); argsman.AddArg("-txindex", strprintf("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)", DEFAULT_TXINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING); @@ -1372,19 +1373,6 @@ bool AppInitParameterInteraction(const ArgsManager& args) if (args.GetBoolArg("-peerbloomfilters", DEFAULT_PEERBLOOMFILTERS)) nLocalServices = ServiceFlags(nLocalServices | NODE_BLOOM); - if (args.GetBoolArg("-reindex-chainstate", false)) { - // indexes that must be deactivated to prevent index corruption, see #24630 - if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) { - return InitError(_("-reindex-chainstate option is not compatible with -coinstatsindex. Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.")); - } - if (g_enabled_filter_types.count(BlockFilterType::BASIC_FILTER)) { - return InitError(_("-reindex-chainstate option is not compatible with -blockfilterindex. Please temporarily disable blockfilterindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.")); - } - if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { - return InitError(_("-reindex-chainstate option is not compatible with -txindex. Please temporarily disable txindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.")); - } - } - try { const bool fQuorumVvecRequestsEnabled{llmq::GetEnabledQuorumVvecSyncEntries(args).size() > 0}; if (!args.GetBoolArg("-llmq-data-recovery", llmq::DEFAULT_ENABLE_QUORUM_DATA_RECOVERY) && fQuorumVvecRequestsEnabled) { @@ -2283,6 +2271,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) node.peerman->AddExtraHandler(std::make_unique(node.peerman.get(), *node.govman, *node.mn_sync, *node.connman, *node.netfulfilledman)); // ********************************************************* Step 8: start indexers + + // If reindex-chainstate was specified, delay syncing indexes until ThreadImport has reindexed the chain + if (!fReindexChainState) g_indexes_ready_to_sync = true; if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { g_txindex = std::make_unique(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex); if (!g_txindex->Start()) { diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ec84092af90c..339a3133e227 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -29,6 +29,7 @@ std::atomic_bool fImporting(false); std::atomic_bool fReindex(false); bool fPruneMode = false; uint64_t nPruneTarget = 0; +std::atomic_bool g_indexes_ready_to_sync{false}; bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const { @@ -961,5 +962,6 @@ void ThreadImport(ChainstateManager& chainman, std::vector vImportFile } } // End scope of CImportingNow chainman.ActiveChainstate().LoadMempool(mempool_path); + g_indexes_ready_to_sync = true; } } // namespace node diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ec7ba9c76e81..4318ae57478f 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -50,6 +50,7 @@ extern std::atomic_bool fImporting; extern std::atomic_bool fReindex; extern bool fPruneMode; extern uint64_t nPruneTarget; +extern std::atomic_bool g_indexes_ready_to_sync; // Because validation code takes pointers to the map's CBlockIndex objects, if // we ever switch to another associative container, we need to either use a diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index fcf071dcd598..50170ce9872a 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -223,6 +223,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve } bls::bls_legacy_scheme.store(true); + node::g_indexes_ready_to_sync = true; } BasicTestingSetup::~BasicTestingSetup() diff --git a/test/functional/feature_coinstatsindex.py b/test/functional/feature_coinstatsindex.py index eac67a631e3b..ef4d910bc907 100755 --- a/test/functional/feature_coinstatsindex.py +++ b/test/functional/feature_coinstatsindex.py @@ -54,6 +54,7 @@ def run_test(self): self._test_use_index_option() self._test_reorg_index() self._test_index_rejects_hash_serialized() + self._test_init_index_after_reorg() def block_subsidy(self, prev_bits): # Subsidy calculations valid till block 4500 @@ -75,6 +76,9 @@ def block_sanity_check(self, block_info, prev_height): block_info['new_outputs_ex_coinbase'] + block_info['coinbase'] + block_info['unspendable'] ) + def sync_index_node(self): + self.wait_until(lambda: self.nodes[1].getindexinfo()['coinstatsindex']['synced'] is True) + def _test_coin_stats_index(self): node = self.nodes[0] index_node = self.nodes[1] @@ -237,18 +241,16 @@ def _test_coin_stats_index(self): self.log.info("Test that the index works with -reindex") self.restart_node(1, extra_args=["-coinstatsindex", "-reindex"]) + self.sync_index_node() res11 = index_node.gettxoutsetinfo('muhash') assert_equal(res11, res10) - self.log.info("Test that -reindex-chainstate is disallowed with coinstatsindex") + self.log.info("Test that the index works with -reindex-chainstate") - self.stop_node(1) - self.nodes[1].assert_start_raises_init_error( - expected_msg='Error: -reindex-chainstate option is not compatible with -coinstatsindex. ' - 'Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.', - extra_args=['-coinstatsindex', '-reindex-chainstate'], - ) - self.restart_node(1, extra_args=["-coinstatsindex"]) + self.restart_node(1, extra_args=["-coinstatsindex", "-reindex-chainstate"]) + self.sync_index_node() + res12 = index_node.gettxoutsetinfo('muhash') + assert_equal(res12, res10) self.log.info("Test obtaining info for a non-existent block hash") assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height="ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", use_index=True) @@ -270,6 +272,7 @@ def _test_reorg_index(self): index_node = self.nodes[1] reorg_blocks = self.generatetoaddress(index_node, 2, getnewdestination()[2]) reorg_block = reorg_blocks[1] + self.sync_index_node() res_invalid = index_node.gettxoutsetinfo('muhash') index_node.invalidateblock(reorg_blocks[0]) assert_equal(index_node.gettxoutsetinfo('muhash')['height'], 110) @@ -309,6 +312,26 @@ def _test_index_rejects_hash_serialized(self): for use_index in {True, False, None}: assert_raises_rpc_error(-8, msg, self.nodes[1].gettxoutsetinfo, hash_type='hash_serialized_2', hash_or_height=111, use_index=use_index) + def _test_init_index_after_reorg(self): + self.log.info("Test a reorg while the index is deactivated") + index_node = self.nodes[1] + block = self.nodes[0].getbestblockhash() + self.generate(index_node, 2, sync_fun=self.no_op) + self.sync_index_node() + + # Restart without index + self.restart_node(1, extra_args=[]) + self.connect_nodes(0, 1) + index_node.invalidateblock(block) + self.generatetoaddress(index_node, 5, getnewdestination()[2]) + res = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=None, use_index=False) + + # Restart with index that still has its best block on the old chain + self.restart_node(1, extra_args=self.extra_args[1]) + self.sync_index_node() + res1 = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=None, use_index=True) + assert_equal(res["muhash"], res1["muhash"]) + if __name__ == '__main__': CoinStatsIndexTest().main() diff --git a/test/functional/p2p_blockfilters.py b/test/functional/p2p_blockfilters.py index 6d3902f62a74..b28bd45e635d 100755 --- a/test/functional/p2p_blockfilters.py +++ b/test/functional/p2p_blockfilters.py @@ -293,13 +293,6 @@ def run_test(self): msg = "Error: Unknown -blockfilterindex value abc." self.nodes[0].assert_start_raises_init_error(expected_msg=msg) - self.log.info("Test -blockfilterindex with -reindex-chainstate raises an error") - self.nodes[0].assert_start_raises_init_error( - expected_msg='Error: -reindex-chainstate option is not compatible with -blockfilterindex. ' - 'Please temporarily disable blockfilterindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.', - extra_args=['-blockfilterindex', '-reindex-chainstate'], - ) - self.test_special_transactions_in_filters() def create_simple_assetlock(self, base_tx, base_tx_value, amount_locked_1, amount_locked_2=0): diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index ba7007bee964..f68424dfec68 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -254,6 +254,7 @@ def run_test(self): shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'chainstate')) shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'evodb')) shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'llmq')) + shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'indexes')) self.start_three(["-nowallet"]) # Create new wallets for the three nodes. diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py index 87da58189f93..c7103ef65f3f 100755 --- a/test/functional/wallet_hd.py +++ b/test/functional/wallet_hd.py @@ -90,6 +90,7 @@ def run_test(self): shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "chainstate")) shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "evodb")) shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "llmq")) + shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "indexes")) shutil.copyfile( os.path.join(self.nodes[1].datadir, "hd.bak"), os.path.join(self.nodes[1].datadir, self.chain, "wallets", self.default_wallet_name, self.wallet_data_filename), @@ -117,6 +118,7 @@ def run_test(self): shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "chainstate")) shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "evodb")) shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "llmq")) + shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "indexes")) shutil.copyfile( os.path.join(self.nodes[1].datadir, "hd.bak"), os.path.join(self.nodes[1].datadir, self.chain, "wallets", self.default_wallet_name, self.wallet_data_filename), From a1ecdfbb63b5bd69e353e44b87ea291ad77a1e4a Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 3 Nov 2026 04:42:03 +0000 Subject: [PATCH 2/8] Merge bitcoin/bitcoin#33212: index: Don't commit state in BaseIndex::Rewind a602f6fb7bf5f9e57299f4d6e246c82379fad8d2 test: index with an unclean restart after a reorg (Martin Zumsande) 01b95ac6f496e24e525b2fc9d69ee8b543da65ff index: don't commit state in BaseIndex::Rewind (Martin Zumsande) Pull request description: The committed state of an index should never be ahead of the flushed chainstate. Otherwise, in the case of an unclean shutdown, the blocks necessary to revert from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular. Instead, the index state will be committed with the next ChainStateFlushed notification. Fixes #33208 ACKs for top commit: achow101: ACK a602f6fb7bf5f9e57299f4d6e246c82379fad8d2 stickies-v: re-ACK a602f6fb7bf5f9e57299f4d6e246c82379fad8d2 Tree-SHA512: 2559ea3fe066caf746a54ad7daac5031332f3976848e937c3dc8b35fa2ce925674115d8742458bf3703b3916f04f851c26523b6b94aeb1da651ba5a1b167a419 (cherry picked from commit 254fe94680795c28d13dd0b81574af28c222de5e) BACKPORT NOTE: The unclean-restart scenario calls syncwithvalidationinterfacequeue before the kill: the index stays "synced" across the reorg, so without it the kill can land before Rewind() and the test passes without the fix. Dash's TestNode has no kill_process(), so the functional test kills the process directly and expects the same exit code (-9, or 1 on Windows) that upstream's helper checks. --- src/index/base.cpp | 9 ++------- test/functional/feature_coinstatsindex.py | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 84ce98143288..9cb07f6ea184 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -259,18 +259,13 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti return false; } - // In the case of a reorg, ensure persisted block locator is not stale. + // Don't commit here - the committed index state must never be ahead of the + // flushed chainstate, otherwise unclean restarts would lead to index corruption. // Pruning has a minimum of 288 blocks-to-keep and getting the index // out of sync may be possible but a users fault. // In case we reorg beyond the pruned depth, ReadBlockFromDisk would // throw and lead to a graceful shutdown SetBestBlockIndex(new_tip); - if (!Commit()) { - // If commit fails, revert the best block index to avoid corruption. - SetBestBlockIndex(current_tip); - return false; - } - return true; } diff --git a/test/functional/feature_coinstatsindex.py b/test/functional/feature_coinstatsindex.py index ef4d910bc907..e9d62a22fce6 100755 --- a/test/functional/feature_coinstatsindex.py +++ b/test/functional/feature_coinstatsindex.py @@ -12,6 +12,7 @@ import struct from decimal import Decimal +import platform from test_framework.blocktools import ( create_block, @@ -332,6 +333,25 @@ def _test_init_index_after_reorg(self): res1 = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=None, use_index=True) assert_equal(res["muhash"], res1["muhash"]) + self.log.info("Test index with an unclean restart after a reorg") + self.restart_node(1, extra_args=self.extra_args[1]) + committed_height = index_node.getblockcount() + self.generate(index_node, 2, sync_fun=self.no_op) + self.sync_index_node() + block2 = index_node.getbestblockhash() + index_node.invalidateblock(block2) + self.generatetoaddress(index_node, 1, getnewdestination()[2], sync_fun=self.no_op) + self.sync_index_node() + # The index stays "synced" across the reorg, so make sure it has processed the rewind before the kill. + index_node.syncwithvalidationinterfacequeue() + index_node.process.kill() + index_node.process.wait() + assert index_node.is_node_stopped(expected_ret_code=1 if platform.system() == "Windows" else -9) + self.start_node(1, extra_args=self.extra_args[1]) + self.sync_index_node() + # Because of the unclean shutdown above, indexes reset to the point we last committed them to disk. + assert_equal(index_node.getindexinfo()['coinstatsindex']['best_block_height'], committed_height) + if __name__ == '__main__': CoinStatsIndexTest().main() From 7100eedb5c496927c5fd10a02ed741fb481b769a Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 3 Nov 2026 04:46:52 +0000 Subject: [PATCH 3/8] Merge bitcoin/bitcoin#34897: indexes: Don't commit ahead of the flushed chainstate e9ed898a0da064715c8ef66a71ee72c3a35e008b validation: Don't use m_chain.Tip() in FlushStateToDisk (Martin Zumsande) 3679f1ecf5ec25d9a40a23e48797a8c22fa3cb99 index: Don't commit ahead of the flushed chainstate (Martin Zumsande) 65735728a5a9c1803f1f5517717936581a4aa125 index: Remove return value from Commit() (Martin Zumsande) 09c06960c6a2b4d037dedbf727dcd5eca53b7c7a validation: track last flushed block (Martin Zumsande) 13c02b5466d9bebb2b3f39deb0ab1ca06b52f4cf test: add test for index commits ahead of the last flushed block (Martin Zumsande) Pull request description: If indexes commit their data ahead of the flushed chainstate, and there is an unclean shutdown, the index will be corrupted. This is especially the case for the coinstatsindex, which has state (the muhash) which can't easily be rolled back without access to the blocks. This was only partly fixed in #33212 (for reorg scenarios) but could still happen during initial sync. Fix this more thoroughly by having the node keep track of the last flushed block, and skipping index commits if the current block of the index is not an ancestor of the node's last flushed block (similar to the suggestion by stickies-v in https://github.com/bitcoin/bitcoin/pull/33212#pullrequestreview-31408570890. Fixes #33208 Fixes #34261 ACKs for top commit: achow101: ACK e9ed898a0da064715c8ef66a71ee72c3a35e008b sedited: Re-ACK e9ed898a0da064715c8ef66a71ee72c3a35e008b fjahr: re-ACK e9ed898a0da064715c8ef66a71ee72c3a35e008b Tree-SHA512: 7f4dc6fb942d6726587eb75dece24c79c679d8630320502aca9fa2d2b03b1d25999cd11255ec20344ebbb8985552747e2554e2557b9d2ad0c75db71652d615ab BACKPORT NOTE: - Commit() keeps Dash's existing LogError call; LogDebug becomes LogPrint(BCLog::COINDB, ...). - In FlushStateToDisk the last flushed block is recorded right after the coins flush, as upstream does; Dash then commits EvoDB in the same flush before ChainStateFlushed is sent. - src/test/chainstate_write_tests.cpp does not exist in Dash, so its two assertions are not ported. - baseindex_tests uses Dash's index API: Init() and Sync() are private here, so the test starts the index and holds back the sync thread with g_indexes_ready_to_sync to read the committed best block. (cherry picked from commit 582821e4a4e96e546f8e13d306e3533977e68a19) --- src/Makefile.test.include | 1 + src/index/base.cpp | 15 ++++++-- src/index/base.h | 12 ++---- src/test/baseindex_tests.cpp | 73 ++++++++++++++++++++++++++++++++++++ src/validation.cpp | 5 ++- src/validation.h | 4 ++ 6 files changed, 97 insertions(+), 13 deletions(-) create mode 100644 src/test/baseindex_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 90a0ed75ed64..74b70c9e453b 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -82,6 +82,7 @@ BITCOIN_TESTS =\ test/base32_tests.cpp \ test/base58_tests.cpp \ test/base64_tests.cpp \ + test/baseindex_tests.cpp \ test/bech32_tests.cpp \ test/bip32_tests.cpp \ test/bip324_tests.cpp \ diff --git a/src/index/base.cpp b/src/index/base.cpp index 9cb07f6ea184..52af7f15d62f 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -230,12 +230,23 @@ void BaseIndex::ThreadSync() } } -bool BaseIndex::Commit() +void BaseIndex::Commit() { // Don't commit anything if we haven't indexed any block yet // (this could happen if init is interrupted). bool ok = m_best_block_index != nullptr; if (ok) { + // Don't commit if the index best block is not an ancestor of the chainstate's last flushed + // block. Otherwise, after an unclean shutdown, the index could be + // persisted ahead of a chainstate it can no longer roll back to, which + // would corrupt indexes with state (e.g. coinstatsindex). + const CBlockIndex* index_tip = m_best_block_index.load(); + const CBlockIndex* last_flushed = WITH_LOCK(::cs_main, return m_chainstate->GetLastFlushedBlock()); + if (!last_flushed || last_flushed->GetAncestor(index_tip->nHeight) != index_tip) { + LogPrint(BCLog::COINDB, "Skipping commit, index is ahead of flushed chainstate (index height %d, last flush at height %d)\n", + index_tip->nHeight, last_flushed ? last_flushed->nHeight : -1); + return; + } CDBBatch batch(GetDB()); ok = CustomCommit(batch); if (ok) { @@ -245,9 +256,7 @@ bool BaseIndex::Commit() } if (!ok) { LogError("%s: Failed to commit latest %s state\n", __func__, GetName()); - return false; } - return true; } bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip) diff --git a/src/index/base.h b/src/index/base.h index db67924d9e75..1a6e5344ce35 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -82,14 +82,10 @@ class BaseIndex : public CValidationInterface void ThreadSync(); /// Write the current index state (eg. chain block locator and subclass-specific items) to disk. - /// - /// Recommendations for error handling: - /// If called on a successor of the previous committed best block in the index, the index can - /// continue processing without risk of corruption, though the index state will need to catch up - /// from further behind on reboot. If the new state is not a successor of the previous state (due - /// to a chain reorganization), the index must halt until Commit succeeds or else it could end up - /// getting corrupted. - bool Commit(); + /// Will skip the commit if no block has been indexed yet or if the index's best block is + /// ahead of the chainstate's last flushed block. This avoids persisting state an unclean shutdown + /// could not roll back from. A later call commits when the chainstate has flushed far enough. + void Commit(); /// Loop over disconnected blocks and call CustomRewind. bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip); diff --git a/src/test/baseindex_tests.cpp b/src/test/baseindex_tests.cpp new file mode 100644 index 000000000000..6f60fb935cd1 --- /dev/null +++ b/src/test/baseindex_tests.cpp @@ -0,0 +1,73 @@ +// Copyright (c) 2020-present 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