From 254fe94680795c28d13dd0b81574af28c222de5e Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 3 Nov 2026 04:42:03 +0000 Subject: [PATCH 1/6] 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 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 42f6f12df59c..b655fa4e552f 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -274,18 +274,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 ba5b44635d25..164ab1f81c39 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, @@ -333,6 +334,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 582821e4a4e96e546f8e13d306e3533977e68a19 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 3 Nov 2026 04:46:52 +0000 Subject: [PATCH 2/6] 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. --- 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 a145cbdec341..bbb90e659864 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 b655fa4e552f..e6643ec02a1e 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -245,12 +245,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) { @@ -260,9 +271,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