From ee9bb263be1fabb8ae5c000bf95232abbd652e7a Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:44:27 +0700 Subject: [PATCH 01/15] test: Test bdb_ro dump of wallet without reset LSNs BACKPORT NOTE: only the test_node.py hunk of this commit is taken; it lets wait_until_stopped() accept an explicit expected_ret_code, which kill_process() (bitcoin#32069) needs. The tool_wallet.py test of this commit depends on the rest of bitcoin#26606 (independent BDB parser) and is not backported. --- test/functional/test_framework/test_node.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 90cb83ba782c..88170d53e93f 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -456,8 +456,9 @@ def is_node_stopped(self, *, expected_stderr="", expected_ret_code=0): return True def wait_until_stopped(self, *, timeout=BITCOIND_PROC_WAIT_TIMEOUT, expect_error=False, **kwargs): - expected_ret_code = 1 if expect_error else 0 # Whether node shutdown return EXIT_FAILURE or EXIT_SUCCESS - wait_until_helper(lambda: self.is_node_stopped(expected_ret_code=expected_ret_code, **kwargs), timeout=timeout, timeout_factor=self.timeout_factor) + if "expected_ret_code" not in kwargs: + kwargs["expected_ret_code"] = 1 if expect_error else 0 # Whether node shutdown return EXIT_FAILURE or EXIT_SUCCESS + wait_until_helper(lambda: self.is_node_stopped(**kwargs), timeout=timeout, timeout_factor=self.timeout_factor) def replace_in_config(self, replacements): """ From 059ce4a6dc4c0dd49f15b13b550eaa31125768f6 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:44:35 +0700 Subject: [PATCH 02/15] partial Merge bitcoin/bitcoin#32069: test: fix intermittent failure in wallet_reorgsrestore.py 36b0713edc4655f6e0c291975d6d280fbc89cf2e test: fix intermittent failure in wallet_reorgsrestore.py (furszy) Pull request description: In response to #32066 intermittent failure. Wait until the node's process has fully stopped before starting a new instance of it. Same behavior as in the [tool_wallet.py](https://github.com/bitcoin/bitcoin/blob/698f86964c68041d938aaf54fdd39466266c371c/test/functional/tool_wallet.py#L540) test. ACKs for top commit: maflcko: lgtm ACK 36b0713edc4655f6e0c291975d6d280fbc89cf2e Chand-ra: tACK [36b0713](https://github.com/bitcoin/bitcoin/commit/36b0713edc4655f6e0c291975d6d280fbc89cf2e) Tree-SHA512: 8e01493ef1fb58589479f3e12d7429d02ca75a2183d5f79d3b6a2fbf13334878926274a20857f1b4729afc1d30b65789daed229ce06ba236b91d949b73f45d5a BACKPORT NOTE: only the TestNode.kill_process() helper is taken. The tool_wallet.py caller depends on bitcoin#26606 and the wallet_reorgsrestore.py caller on bitcoin#31757, neither of which is in Dash. --- test/functional/test_framework/test_node.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 88170d53e93f..da038c2740ca 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -460,6 +460,11 @@ def wait_until_stopped(self, *, timeout=BITCOIND_PROC_WAIT_TIMEOUT, expect_error kwargs["expected_ret_code"] = 1 if expect_error else 0 # Whether node shutdown return EXIT_FAILURE or EXIT_SUCCESS wait_until_helper(lambda: self.is_node_stopped(**kwargs), timeout=timeout, timeout_factor=self.timeout_factor) + def kill_process(self): + self.process.kill() + self.wait_until_stopped(expected_ret_code=1 if platform.system() == "Windows" else -9) + assert self.is_node_stopped() + def replace_in_config(self, replacements): """ Perform replacements in the configuration file. From 7e86e4693176cf1b128b31593a7973f122ef6d2d Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:48:06 +0700 Subject: [PATCH 03/15] partial Merge bitcoin/bitcoin#30221: wallet: Ensure best block matches wallet scan state BACKPORT NOTE: - Dash's SyncTransaction() keeps its WalletBatch parameter; only the return type changes. - The fFirstRun block of CWallet::Create() takes cs_wallet once at its top as upstream does; the three inner LOCK(walletInstance->cs_wallet) of Dash's HD-chain and descriptor setup are dropped since the lock is already held there. - src/bench/wallet_migration.cpp does not exist in Dash, so its hunk is not applied. - The wallet_descriptor.py hunk depends on bitcoin#29112 (sqlite concurrent writes test) and the wallet_reorgsrestore.py hunk on bitcoin#31757 (unclean shutdown reorg test); neither is in Dash, so those test changes are not applied. ---- 30a94b1ab9ae850d55cb9eb606a06890437bc75e test, wallet: Remove concurrent writes test (Ava Chow) b44b7c03fef01e0b5db704e50762b3d16b3da69e wallet: Write best block record on unload (Ava Chow) 876a2585a8b69e12ac171a0d9ff5aab864067c42 wallet: Remove unnecessary database Close step on shutdown (Ava Chow) 98a1a5275c8c395fe47ff7f10109d75b06bc391d wallet: Remove chainStateFlushed (Ava Chow) 7fd3e1cf0c88553e0722048ce488f265883558e7 wallet, bench: Write a bestblock record in WalletMigration (Ava Chow) 6d3a8b195a826448c021dd189255ca41ba70cc5a wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed (Ava Chow) 7bacabb204b6c34f9545f0b37e2c66296ad2c0de wallet: Update best block record after block dis/connect (Ava Chow) Pull request description: Implements the idea discussed in https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2010579484 Currently, `m_last_block_processed` and `m_last_block_processed_height` are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive. This PR changes those last block fields to be updated whenever the wallet makes a change to the db for new transaction state found in new blocks. Whenever a block is received that contains a transaction relevant to the wallet, the last block locator will now be written to disk. Furthermore, every block disconnection will now write an updated locator. To ensure that the locator is relatively recent and loading rescans are fairly quick in the event of unplanned shutdown, it is also now written every 144 blocks (~1 day). Additionally it is now written when the wallet is unloaded so that it is accurate when the wallet is loaded again. Lastly, the `chainstateFlushed` notification in the wallet is changed to be a no-op. The best block locator record is no longer written when `chainstateFlushed` is received from the node since it should already be mostly up to date. ACKs for top commit: rkrux: ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e mzumsande: Code Review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e ryanofsky: Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments. Tree-SHA512: 46117541f8aaf13dde57430e813b4bbbd5e146e2632769675803c8e65a82f149a7cc6026489a127d32684b90124bd2b7c28216dbcfa6a47447300e8f3814e029 --- src/wallet/interfaces.cpp | 4 +- src/wallet/load.cpp | 7 --- src/wallet/load.h | 4 -- src/wallet/test/wallet_tests.cpp | 6 +- src/wallet/wallet.cpp | 103 ++++++++++++++++++++----------- src/wallet/wallet.h | 18 +++--- 6 files changed, 81 insertions(+), 61 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index f03fd57b3d22..1d8b92a35662 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -966,7 +966,7 @@ class WalletLoaderImpl : public WalletLoader m_context.args = &args; m_context.node_context = &node_context; } - ~WalletLoaderImpl() override { UnloadWallets(m_context); } + ~WalletLoaderImpl() override { stop(); } //! ChainClient methods void registerRpcs() override @@ -977,7 +977,7 @@ class WalletLoaderImpl : public WalletLoader bool load() override { return LoadWallets(m_context); } void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); } void flush() override { return FlushWallets(m_context); } - void stop() override { return StopWallets(m_context); } + void stop() override { return UnloadWallets(m_context); } void setMockTime(int64_t time) override { return SetMockTime(time); } //! WalletLoader methods diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index b3cd26909bbf..317fa2de6988 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -169,13 +169,6 @@ void FlushWallets(WalletContext& context) } } -void StopWallets(WalletContext& context) -{ - for (const std::shared_ptr& pwallet : GetWallets(context)) { - pwallet->Close(); - } -} - void UnloadWallets(WalletContext& context) { auto wallets = GetWallets(context); diff --git a/src/wallet/load.h b/src/wallet/load.h index 88973e428c63..c4d70a394a9b 100644 --- a/src/wallet/load.h +++ b/src/wallet/load.h @@ -35,10 +35,6 @@ void StartWallets(WalletContext& context, CScheduler& scheduler); //! Flush all wallets in preparation for shutdown. void FlushWallets(WalletContext& context); -//! Stop all wallets. Wallets will be flushed first. -void StopWallets(WalletContext& context); - -//! Close all wallets. void UnloadWallets(WalletContext& context); } // namespace wallet diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 20db3c28a0c5..dfec1db4d420 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -169,8 +169,8 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) { CBlockLocator locator; - BOOST_CHECK(!WalletBatch{wallet.GetDatabase()}.ReadBestBlock(locator)); - BOOST_CHECK(locator.IsNull()); + BOOST_CHECK(WalletBatch{wallet.GetDatabase()}.ReadBestBlock(locator)); + BOOST_CHECK(!locator.IsNull() && locator.vHave.front() == newTip->GetBlockHash()); } CWallet::ScanResult result = wallet.ScanForWalletTransactions(/*start_block=*/oldTip->GetBlockHash(), /*start_height=*/oldTip->nHeight, /*max_height=*/{}, reserver, /*fUpdate=*/false, /*save_progress=*/true); @@ -183,7 +183,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) { CBlockLocator locator; BOOST_CHECK(WalletBatch{wallet.GetDatabase()}.ReadBestBlock(locator)); - BOOST_CHECK(!locator.IsNull()); + BOOST_CHECK(!locator.IsNull() && locator.vHave.front() == newTip->GetBlockHash()); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e1e3c7736dfb..b1cc78bfe798 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -148,6 +148,7 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet interfaces::Chain& chain = wallet->chain(); std::string name = wallet->GetName(); + WITH_LOCK(wallet->cs_wallet, wallet->WriteBestBlock()); // Unregister with the validation interface which also drops shared pointers. wallet->m_chain_notifications_handler.reset(); @@ -671,15 +672,20 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, return false; } -void CWallet::chainStateFlushed(const CBlockLocator& loc) +void CWallet::SetLastBlockProcessedInMem(int block_height, uint256 block_hash) { - // Don't update the best block until the chain is attached so that in case of a shutdown, - // the rescan will be restarted at next startup. - if (m_attaching_chain) { - return; - } - WalletBatch batch(GetDatabase()); - batch.WriteBestBlock(loc); + AssertLockHeld(cs_wallet); + + m_last_block_processed = block_hash; + m_last_block_processed_height = block_height; +} + +void CWallet::SetLastBlockProcessed(int block_height, uint256 block_hash) +{ + AssertLockHeld(cs_wallet); + + SetLastBlockProcessedInMem(block_height, block_hash); + WriteBestBlock(); } void CWallet::SetMinVersion(enum WalletFeature nVersion, WalletBatch* batch_in) @@ -1535,10 +1541,10 @@ void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingSt fAnonymizableTallyCachedNonDenom = false; } -void CWallet::SyncTransaction(const CTransactionRef& ptx, const SyncTxState& state, WalletBatch& batch, bool update_tx, bool rescanning_old_block) +bool CWallet::SyncTransaction(const CTransactionRef& ptx, const SyncTxState& state, WalletBatch& batch, bool update_tx, bool rescanning_old_block) { if (!AddToWalletIfInvolvingMe(ptx, state, batch, update_tx, rescanning_old_block)) - return; // Not one of ours + return false; // Not one of ours // If a transaction changes 'conflicted' state, that changes the balance // available of the outputs it spends. So force those to be @@ -1547,6 +1553,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const SyncTxState& sta fAnonymizableTallyCached = false; fAnonymizableTallyCachedNonDenom = false; + return true; } void CWallet::transactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime) { @@ -1611,8 +1618,9 @@ void CWallet::blockConnected(const interfaces::BlockInfo& block) assert(block.data); LOCK(cs_wallet); - m_last_block_processed_height = block.height; - m_last_block_processed = block.hash; + // Update the best block in memory first. This will set the best block's height, which is + // needed by MarkConflicted. + SetLastBlockProcessedInMem(block.height, block.hash); // No need to scan block if it was created before the wallet birthday. // Uses chain max time and twice the grace period to adjust time for block time variability. @@ -1620,11 +1628,17 @@ void CWallet::blockConnected(const interfaces::BlockInfo& block) // Scan block WalletBatch batch(GetDatabase()); + bool wallet_updated = false; for (size_t index = 0; index < block.data->vtx.size(); index++) { - SyncTransaction(block.data->vtx[index], TxStateConfirmed{block.hash, block.height, static_cast(index)}, batch); + wallet_updated |= SyncTransaction(block.data->vtx[index], TxStateConfirmed{block.hash, block.height, static_cast(index)}, batch); transactionRemovedFromMempool(block.data->vtx[index], MemPoolRemovalReason::BLOCK); } + // Update on disk if this block resulted in us updating a tx, or periodically every 144 blocks (~1 day) + if (wallet_updated || block.height % 144 == 0) { + WriteBestBlock(); + } + // reset cache to make sure no longer immature coins are included fAnonymizableTallyCached = false; fAnonymizableTallyCachedNonDenom = false; @@ -1639,9 +1653,6 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) // be unconfirmed, whether or not the transaction is added back to the mempool. // User may have to call abandontransaction again. It may be addressed in the // future with a stickier abandoned state or even removing abandontransaction call. - m_last_block_processed_height = block.height - 1; - m_last_block_processed = *Assert(block.prev_hash); - int disconnect_height = block.height; WalletBatch batch(GetDatabase()); @@ -1677,6 +1688,9 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) // reset cache to make sure no longer mature coins are excluded fAnonymizableTallyCached = false; fAnonymizableTallyCachedNonDenom = false; + + // Update the best block + SetLastBlockProcessed(block.height - 1, *Assert(block.prev_hash)); } void CWallet::updatedBlockTip() @@ -2636,7 +2650,7 @@ DBErrors CWallet::LoadWallet() if (HaveChain()) { const std::optional tip_height = chain().getHeight(); if (tip_height) { - SetLastBlockProcessed(*tip_height, chain().getBlockHash(*tip_height)); + SetLastBlockProcessedInMem(*tip_height, chain().getBlockHash(*tip_height)); for (auto& pair : mapWallet) { for(unsigned int i = 0; i < pair.second.tx->vout.size(); ++i) { COutPoint outpoint(pair.first, i); @@ -3374,6 +3388,8 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri !walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); if (fFirstRun) { + LOCK(walletInstance->cs_wallet); + walletInstance->SetMinVersion(FEATURE_LATEST); walletInstance->InitWalletFlags(wallet_creation_flags); @@ -3401,7 +3417,6 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } // add default account newHdChain.AddAccount(); - LOCK(walletInstance->cs_wallet); if (auto spk_man = walletInstance->GetLegacyScriptPubKeyMan()) { if (!spk_man->AddHDChainSingle(newHdChain)) { error = strprintf(_("%s failed"), "AddHDChainSingle"); @@ -3420,7 +3435,6 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri mnemonic = args.GetArg("-mnemonic", ""); mnemonic_passphrase = args.GetArg("-mnemonicpassphrase", ""); - LOCK(walletInstance->cs_wallet); if (auto spk_man = walletInstance->GetLegacyScriptPubKeyMan()) { spk_man->GenerateNewHDChain(mnemonic, mnemonic_passphrase); } @@ -3432,7 +3446,6 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri args.ForceRemoveArg("mnemonicpassphrase"); } // Otherwise, do not create a new HD chain - LOCK(walletInstance->cs_wallet); if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { SecureString mnemonic, mnemonic_passphrase; mnemonic.reserve(256); @@ -3455,7 +3468,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } if (chain) { - walletInstance->chainStateFlushed(chain->getTipLocator()); + std::optional tip_height = chain->getHeight(); + if (tip_height) { + walletInstance->SetLastBlockProcessed(*tip_height, chain->getBlockHash(*tip_height)); + } } // Try to create wallet backup right after new wallet was created @@ -3668,16 +3684,11 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf } // Register wallet with validationinterface. It's done before rescan to avoid - // missing block connections between end of rescan and validation subscribing. - // Because of wallet lock being hold, block connection notifications are going to - // be pending on the validation-side until lock release. It's likely to have - // block processing duplicata (if rescan block range overlaps with notification one) - // but we guarantee at least than wallet state is correct after notifications delivery. - // However, chainStateFlushed notifications are ignored until the rescan is finished - // so that in case of a shutdown event, the rescan will be repeated at the next start. - // This is temporary until rescan and notifications delivery are unified under same - // interface. - walletInstance->m_attaching_chain = true; //ignores chainStateFlushed notifications + // missing block connections during the rescan. + // Because of the wallet lock being held, block connection notifications are going to + // be pending on the validation-side until lock release. Blocks that are connected while the + // rescan is ongoing will not be processed in the rescan but with the block connected notifications, + // so the wallet will only be completeley synced after the notifications delivery. walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance); // If rescan_required = true, rescan_height remains equal to 0 @@ -3754,16 +3765,22 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf walletInstance->WalletLogPrintf("Rescanning last %i blocks (from block %i)...\n", *tip_height - rescan_height, rescan_height); { WalletRescanReserver reserver(*walletInstance); - if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(chain.getBlockHash(rescan_height), rescan_height, /*max_height=*/{}, reserver, /*fUpdate=*/true, /*save_progress=*/true).status)) { + if (!reserver.reserve()) { + error = _("Failed to acquire rescan reserver during wallet initialization"); + return false; + } + ScanResult scan_res = walletInstance->ScanForWalletTransactions(chain.getBlockHash(rescan_height), rescan_height, /*max_height=*/{}, reserver, /*fUpdate=*/true, /*save_progress=*/true); + if (ScanResult::SUCCESS != scan_res.status) { error = _("Failed to rescan the wallet during initialization"); return false; } + // Set and update the best block record + // Set last block scanned as the last block processed as it may be different in case the case of a reorg. + // Also save the best block locator because rescanning only updates it intermittently. + walletInstance->SetLastBlockProcessed(*scan_res.last_scanned_height, scan_res.last_scanned_block); } - walletInstance->m_attaching_chain = false; - walletInstance->chainStateFlushed(chain.getTipLocator()); walletInstance->GetDatabase().IncrementUpdateCounter(); } - walletInstance->m_attaching_chain = false; return true; } @@ -3822,6 +3839,8 @@ void CWallet::InitAutoBackup() bool CWallet::BackupWallet(const std::string& strDest) const { + // TODO: bitcoin#30678 added a best block write here that bitcoin#30221 then replaced with this call; drop its hunk when backporting it + WITH_LOCK(cs_wallet, WriteBestBlock()); return GetDatabase().Backup(strDest); } @@ -5243,6 +5262,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it bool was_loaded = false; if (auto wallet = GetWallet(context, wallet_name)) { + // TODO: bitcoin#30678 flushed the best block here; RemoveWallet() writes it since bitcoin#30221, drop that hunk when backporting it if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { return util::Error{_("Unable to unload the wallet before migrating")}; } @@ -5463,4 +5483,17 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } return res; } + +void CWallet::WriteBestBlock() const +{ + AssertLockHeld(cs_wallet); + + if (!m_last_block_processed.IsNull()) { + CBlockLocator loc; + chain().findBlock(m_last_block_processed, FoundBlock().locator(loc)); + + WalletBatch batch(GetDatabase()); + batch.WriteBestBlock(loc); + } +} } // namespace wallet diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 45699678003b..98a8711441bf 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -329,7 +329,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::atomic fAbortRescan{false}; // reset by WalletRescanReserver::reserve() std::atomic fScanningWallet{false}; // controlled by WalletRescanReserver - std::atomic m_attaching_chain{false}; std::atomic m_scanning_with_passphrase{false}; std::atomic m_scanning_start{SteadyClock::time_point{}}; std::atomic m_scanning_progress{0}; @@ -420,7 +419,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void SyncMetaData(std::pair) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void SyncTransaction(const CTransactionRef& tx, const SyncTxState& state, WalletBatch& batch, bool update_tx = true, bool rescanning_old_block = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool SyncTransaction(const CTransactionRef& tx, const SyncTxState& state, WalletBatch& batch, bool update_tx = true, bool rescanning_old_block = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** WalletFlags set on this wallet. */ std::atomic m_wallet_flags{0}; @@ -498,6 +497,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati static NodeClock::time_point GetDefaultNextResend(); + // Update last block processed in memory only + void SetLastBlockProcessedInMem(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + PlatformKeyStatus GetPlatformKeySource(const ScriptPubKeyMan*& source) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); PlatformKeyStatus DerivePlatformKey(const PlatformKeyRequest& request, platformkeys::ExtKey256& out) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -968,7 +970,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** should probably be renamed to IsRelevantToMe */ bool IsFromMe(const CTransaction& tx) const; CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const; - void chainStateFlushed(const CBlockLocator& loc) override; DBErrors LoadWallet(); void AutoLockMasternodeCollaterals(); @@ -1206,13 +1207,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati assert(m_last_block_processed_height >= 0); return m_last_block_processed; } - /** Set last block processed height, currently only use in unit test */ - void SetLastBlockProcessed(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) - { - AssertLockHeld(cs_wallet); - m_last_block_processed_height = block_height; - m_last_block_processed = block_hash; - }; + /** Set last block processed height, and write to database */ + void SetLastBlockProcessed(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Write the current best block to database */ + void WriteBestBlock() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Connect the signals from ScriptPubKeyMans to the signals in CWallet void ConnectScriptPubKeyManNotifiers(); From ee7021f42443b285eded20595deb98f77de02f17 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:48:06 +0700 Subject: [PATCH 04/15] partial Merge bitcoin/bitcoin#32580: wallet, test: best block locator matches scan state follow-ups BACKPORT NOTE: the wallet_reorgsrestore.py hunk extends the bitcoin#31757 scenario, which is not in Dash, so it is not applied. --- 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b wallet, test: best block locator matches scan state follow-ups (rkrux) Pull request description: Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in `AttachChain`, add not null locator check in `WriteBestBlock`. Add log and few assertions in `wallet_reorgstore` test. ACKs for top commit: achow101: ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b pablomartin4btc: cr-ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b Tree-SHA512: 34edde55beef5714cea2e1131c29b57da2dc32ea091cd81878014de503c128f02c3ab88aee1e456541d7937e033dca5a81b03e9e2888cf781d71b62ad9b5ca5c --- src/wallet/wallet.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b1cc78bfe798..6e1e581e1f98 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3706,11 +3706,9 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf const std::optional tip_height = chain.getHeight(); if (tip_height) { - walletInstance->m_last_block_processed = chain.getBlockHash(*tip_height); - walletInstance->m_last_block_processed_height = *tip_height; + walletInstance->SetLastBlockProcessedInMem(*tip_height, chain.getBlockHash(*tip_height)); } else { - walletInstance->m_last_block_processed.SetNull(); - walletInstance->m_last_block_processed_height = -1; + walletInstance->SetLastBlockProcessedInMem(-1, uint256()); } if (tip_height && *tip_height != rescan_height) @@ -3775,7 +3773,7 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf return false; } // Set and update the best block record - // Set last block scanned as the last block processed as it may be different in case the case of a reorg. + // Set last block scanned as the last block processed as it may be different in case of a reorg. // Also save the best block locator because rescanning only updates it intermittently. walletInstance->SetLastBlockProcessed(*scan_res.last_scanned_height, scan_res.last_scanned_block); } @@ -5492,8 +5490,10 @@ void CWallet::WriteBestBlock() const CBlockLocator loc; chain().findBlock(m_last_block_processed, FoundBlock().locator(loc)); - WalletBatch batch(GetDatabase()); - batch.WriteBestBlock(loc); + if (!loc.IsNull()) { + WalletBatch batch(GetDatabase()); + batch.WriteBestBlock(loc); + } } } } // namespace wallet From a4cc3c082734568ee016c3b846a6ae28deb429f5 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:49:02 +0700 Subject: [PATCH 05/15] Merge bitcoin/bitcoin#32878: index: fix wrong assert of current_tip == m_best_block_index 3aef38f44b76dfda77f47dc1a0e1fdc6ff3c7766 test: exercise index reorg assertion failure (furszy) acf50233cdfbb336c87d95d97db90a149e131052 index: fix wrong assert of current_tip == m_best_block_index (Hao Xu) Pull request description: In BaseIndex::Sync(), pindex in `Rewind(pindex, pindex_next->pprev)` isn't always equal to m_best_block_index since m_best_block_index is updated every SYNC_LOCATOR_WRITE_INTERVAL seconds, during which multiple pindex update could happen. Thus the assert here is wrong. ACKs for top commit: achow101: ACK 3aef38f44b76dfda77f47dc1a0e1fdc6ff3c7766 furszy: ACK 3aef38f mzumsande: Code Review ACK 3aef38f44b76dfda77f47dc1a0e1fdc6ff3c7766 Tree-SHA512: 3ef9cc6dfdec10a9f95d7414c6a11aa216e4cf5974440d80ab19fc919abd2a3bd4c875718c9dc94523c33826f8582ec5a016374deb8fb2d35cd2fb7799b5c82e BACKPORT NOTE: Init() and StartBackgroundSync() are not split in Dash (bitcoin#27607), so the test calls Start(). The sync log line keeps Dash's LogPrintf form. --- src/index/base.cpp | 11 ++-- src/test/blockfilter_index_tests.cpp | 77 ++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 6 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 42f6f12df59c..afb819ff174d 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -177,8 +177,8 @@ void BaseIndex::ThreadSync() if (!m_synced) { auto& consensus_params = Params().GetConsensus(); - std::chrono::steady_clock::time_point last_log_time{0s}; - std::chrono::steady_clock::time_point last_locator_write_time{0s}; + auto last_log_time{NodeClock::now()}; + auto last_locator_write_time{last_log_time}; while (true) { if (m_interrupt) { SetBestBlockIndex(pindex); @@ -222,14 +222,14 @@ void BaseIndex::ThreadSync() return; } - auto current_time{std::chrono::steady_clock::now()}; - if (last_log_time + SYNC_LOG_INTERVAL < current_time) { + auto current_time{NodeClock::now()}; + if (current_time - last_log_time >= SYNC_LOG_INTERVAL) { LogPrintf("Syncing %s with block chain from height %d\n", GetName(), pindex->nHeight); last_log_time = current_time; } - if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) { + if (current_time - last_locator_write_time >= SYNC_LOCATOR_WRITE_INTERVAL) { SetBestBlockIndex(pindex); last_locator_write_time = current_time; // No need to handle errors in Commit. See rationale above. @@ -267,7 +267,6 @@ bool BaseIndex::Commit() bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip) { - 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})) { diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index 1a8612eb1c64..ba5db2a09f0c 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -17,6 +17,7 @@ #include #include +#include using node::BlockAssembler; using node::CBlockTemplate; @@ -304,4 +305,80 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_init_destroy, BasicTestingSetup) BOOST_CHECK(filter_index == nullptr); } +class IndexReorgCrash : public BaseIndex +{ +private: + std::unique_ptr m_db; + std::shared_future m_blocker; + int m_blocking_height; + +public: + explicit IndexReorgCrash(std::unique_ptr chain, std::shared_future blocker, + int blocking_height) : BaseIndex(std::move(chain), "test index"), m_blocker(blocker), + m_blocking_height(blocking_height) + { + const fs::path path = gArgs.GetDataDirNet() / "index"; + fs::create_directories(path); + m_db = std::make_unique(path / "db", /*n_cache_size=*/0, /*f_memory=*/true, /*f_wipe=*/false); + } + + bool AllowPrune() const override { return false; } + BaseIndex::DB& GetDB() const override { return *m_db; } + + bool CustomAppend(const interfaces::BlockInfo& block) override + { + // Simulate a delay so new blocks can get connected during the initial sync + if (block.height == m_blocking_height) m_blocker.wait(); + + // Move mock time forward so the best index gets updated only when we are not at the blocking height + if (block.height == m_blocking_height - 1 || block.height > m_blocking_height) { + SetMockTime(GetTime() + 31s); + } + + return true; + } +}; + +BOOST_FIXTURE_TEST_CASE(index_reorg_crash, BuildChainTestingSetup) +{ + // Enable mock time + SetMockTime(GetTime()); + + std::promise promise; + std::shared_future blocker(promise.get_future()); + int blocking_height = WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip()->nHeight); + + IndexReorgCrash index(interfaces::MakeChain(m_node), blocker, blocking_height); + BOOST_REQUIRE(index.Start()); + + auto func_wait_until = [&](int height, std::chrono::milliseconds timeout) { + auto deadline = std::chrono::steady_clock::now() + timeout; + while (index.GetSummary().best_block_height < height) { + if (std::chrono::steady_clock::now() > deadline) { + BOOST_FAIL(strprintf("Timeout waiting for index height %d (current: %d)", height, index.GetSummary().best_block_height)); + return; + } + std::this_thread::sleep_for(100ms); + } + }; + + // Wait until the index is one block before the fork point + func_wait_until(blocking_height - 1, /*timeout=*/5s); + + // Create a fork to trigger the reorg + std::vector> fork; + const CBlockIndex* prev_tip = WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip()->pprev); + BOOST_REQUIRE(BuildChain(prev_tip, GetScriptForDestination(PKHash(GenerateRandomKey().GetPubKey())), 3, fork)); + + for (const auto& block : fork) { + BOOST_REQUIRE(m_node.chainman->ProcessNewBlock(block, /*force_processing=*/true, nullptr)); + } + + // Unblock the index thread so it can process the reorg + promise.set_value(); + // Wait for the index to reach the new tip + func_wait_until(blocking_height + 2, 5s); + index.Stop(); +} + BOOST_AUTO_TEST_SUITE_END() From 4bdfb4ed9cf0a2bfedce0d936d7bb19d2133ec19 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:49:02 +0700 Subject: [PATCH 06/15] 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 --- src/index/base.cpp | 9 ++------- test/functional/feature_coinstatsindex.py | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index afb819ff174d..06cb4ebcf115 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -273,18 +273,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..c70eb0c0bf69 100755 --- a/test/functional/feature_coinstatsindex.py +++ b/test/functional/feature_coinstatsindex.py @@ -333,6 +333,21 @@ 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() + index_node.kill_process() + 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 ce05350c1cbf231964e13ce84a20555e3092f5cf Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:49:21 +0700 Subject: [PATCH 07/15] Merge bitcoin/bitcoin#32882: index: remove unnecessary locator cleaning in BaseIndex::Init() facd01e6ffbbd019312f370a3807de0b95bbd745 refactor: remove redundant locator cleanup in BaseIndex::Init() (Hao Xu) Pull request description: Leverage locator.IsNull() to simplify ReadBestBlock() and remove unnecessary SetNull(). ACKs for top commit: l0rinc: ACK facd01e6ffbbd019312f370a3807de0b95bbd745 furszy: utACK facd01e6ffbbd019312f370a3807de0b95bbd745 sedited: ACK facd01e6ffbbd019312f370a3807de0b95bbd745 Tree-SHA512: ca418a3d4c72b9bfdb194e03d5acfacf7b567850ab385ad3a1ec6d070eedef4aea2bb8335f8bfe0316e84c22583111fc202f7603044c5d4d05754db23f75b47f BACKPORT NOTE: GovCollateralIndex::CanSync() is adapted to the new ReadBestBlock() signature. --- src/index/base.cpp | 12 ++++++------ src/index/base.h | 3 ++- src/index/govcollateralindex.cpp | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 06cb4ebcf115..be16ab4856ed 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -55,13 +55,16 @@ BaseIndex::DB::DB(const fs::path& path, size_t n_cache_size, bool f_memory, bool .options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()}} {} -bool BaseIndex::DB::ReadBestBlock(CBlockLocator& locator) const +CBlockLocator BaseIndex::DB::ReadBestBlock() const { + CBlockLocator locator; + bool success = Read(DB_BEST_BLOCK, locator); if (!success) { locator.SetNull(); } - return success; + + return locator; } void BaseIndex::DB::WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator) @@ -80,10 +83,7 @@ BaseIndex::~BaseIndex() bool BaseIndex::Init() { - CBlockLocator locator; - if (!GetDB().ReadBestBlock(locator)) { - locator.SetNull(); - } + const auto locator{GetDB().ReadBestBlock()}; LOCK(cs_main); CChain& active_chain = m_chainstate->m_chain; diff --git a/src/index/base.h b/src/index/base.h index db67924d9e75..0efa72bff7b9 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -49,7 +49,8 @@ class BaseIndex : public CValidationInterface bool f_memory = false, bool f_wipe = false, bool f_obfuscate = false); /// Read block locator of the chain that the index is in sync with. - bool ReadBestBlock(CBlockLocator& locator) const; + /// Note, the returned locator will be empty if no record exists. + CBlockLocator ReadBestBlock() const; /// Write block locator of the chain that the index is in sync with. void WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator); diff --git a/src/index/govcollateralindex.cpp b/src/index/govcollateralindex.cpp index cab00c694e1c..9f26c7a7ae21 100644 --- a/src/index/govcollateralindex.cpp +++ b/src/index/govcollateralindex.cpp @@ -126,9 +126,9 @@ BaseIndex::DB& GovCollateralIndex::GetDB() const { return *m_db; } bool GovCollateralIndex::CanSync(Chainstate& chainstate) const { AssertLockHeld(::cs_main); - CBlockLocator locator; + const auto locator{m_db->ReadBestBlock()}; const CBlockIndex* best_block{nullptr}; - if (m_db->ReadBestBlock(locator) && !locator.IsNull()) { + if (!locator.IsNull()) { best_block = chainstate.FindForkInGlobalIndex(locator); } for (const CBlockIndex* block{chainstate.m_chain.Tip()}; block != best_block; block = block->pprev) { From 7ad807013acd2fac9eaa159a0b5bf9bd5e91f6a8 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:49:38 +0700 Subject: [PATCH 08/15] Merge bitcoin/bitcoin#32875: index: handle case where pindex_prev equals chain tip in NextSyncBlock() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit db3c25cfae1cc70171cc9bd921a2bf9be2c9da7e index: add explicit early exit in NextSyncBlock() when the input is the chain tip (Hao Xu) Pull request description: When `pindex_prev` is the chain tip, `NextSyncBlock()` should return `nullptr` (there's no next block to sync). Currently this works, but relies on the fork/reorg path to produce the result: `chain.Next(tip)` returns `nullptr`, falling through to `FindFork(tip)` which returns `tip` itself, then `chain.Next(tip)` returns `nullptr` again. Add an explicit early return for this case. This makes `NextSyncBlock()`'s three cases self-documenting: 1. next block exists on the active chain — return it 2. at the chain tip — return `nullptr` 3. on a stale branch — find the fork point and return the block after it It also makes the existing comment ("Since block is not in the chain") accurate — the tip is in the chain, so it shouldn't reach that path. ACKs for top commit: optout21: crACK db3c25cfae1cc70171cc9bd921a2bf9be2c9da7e furszy: ACK db3c25cfae1cc70171cc9bd921a2bf9be2c9da7e stickies-v: ACK db3c25cfae1cc70171cc9bd921a2bf9be2c9da7e Tree-SHA512: c1633bb3d3ffed2643c8e174c2b2283deaeffd0a06eb3fb2cf03eed097bfdc5cf6fbd7ebdcdb44fd56f712be8004ad46c730a33990fb1fc6cfa629c9b14f8cd9 --- src/index/base.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index be16ab4856ed..004a1f5d9fee 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -157,11 +157,17 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain& return chain.Genesis(); } - const CBlockIndex* pindex = chain.Next(pindex_prev); - if (pindex) { + if (const auto* pindex{chain.Next(pindex_prev)}) { return pindex; } + // If there is no next block, we might be synced + if (pindex_prev == chain.Tip()) { + return nullptr; + } + + // 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)); } From df7ac41204287005851d562d1d466617588d7d62 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 4 Nov 2026 01:51:17 +0700 Subject: [PATCH 09/15] partial Merge bitcoin/bitcoin#34897: indexes: Don't commit ahead of the flushed chainstate BACKPORT NOTE: - The last flushed block is recorded right after the coins flush; Dash then commits EvoDB in the same flush before ChainStateFlushed is sent. - The skip message gets Dash's explicit trailing newline; Commit() keeps Dash's LogError form. - src/test/chainstate_write_tests.cpp does not exist in Dash, so its two assertions are not applied. - baseindex_tests: Init() and Sync() are not callable on their own in Dash (bitcoin#27607, bitcoin#28955 are not backported), so the test starts the index and holds back the sync thread with g_indexes_ready_to_sync to read the committed best block. ---- 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 --- 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 f2c44568ef60..efc9a88fcfc3 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 004a1f5d9fee..c85329eddfac 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -251,12 +251,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) { + LogDebug(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) { @@ -266,9 +277,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 0efa72bff7b9..66026397ff87 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -83,14 +83,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..6c579635d3fa --- /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