From 560063156a9ead2e21f9de4ee301d564a24488bd Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 14 Mar 2024 12:39:15 -0400 Subject: [PATCH] wallet: Avoid potentially writing incorrect best block locator I noticed while debugging #24230 that the ChainStateFlushed notification can be sent with a locator pointing to a different block than the block in the last BlockConnected notification. This is bad because it could cause the wallet to record that it is synced to a later block than it ever processed, and make it potentially fail to scan blocks for relevant transactions if it is unloaded and reloaded. This problem should probably not happen in practice, because normally the locator in the ChainStateFlushed notification does point to the block sent in the last BlockConnected notification. But because of the way ActivateBestChain accumulates block connected notifications, and the fact that FlushStateToDisk is called from many different code paths, this isn't guaranteed. (The new assumeutxo logic also introduces a situation where this is never the case: when the background chain reaches the snapshot height and validates the snapshot, the background chain is flushed before block connected notifications are sent.) In any case, it is better if the wallet writes a locator actually pointing to the last block that it processed instead of writing the locator validation code passes to ChainStateFlushed, so this commit switches to the right locator. A good followup to this change would probably be to drop the ChainStateFlushed locator argument entirely so it is not misused in the future. Indexing code already stopped using this locator argument a long time ago for identifying the best block (in 4368384f1d267b011e03a805f934f5c47e2ca1b2 from #14121), but it is still using the locator argument to log diagnostic warnings. --- src/interfaces/chain.h | 9 +-------- src/node/interfaces.cpp | 13 +------------ src/wallet/wallet.cpp | 18 +++++++++++------- src/wallet/wallet.h | 2 +- 4 files changed, 14 insertions(+), 28 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 9da5cb96373..d9b6be0acce 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -137,13 +137,6 @@ public: //! pruned), and contains transactions. virtual bool haveBlockOnDisk(int height) = 0; - //! Get locator for the current chain tip. - virtual CBlockLocator getTipLocator() = 0; - - //! Return a locator that refers to a block in the active chain. - //! If specified block is not in the active chain, return locator for the latest ancestor that is in the chain. - virtual CBlockLocator getActiveChainLocator(const uint256& block_hash) = 0; - //! Return height of the highest block on chain in common with the locator, //! which will either be the original block used to create the locator, //! or one of its ancestors. @@ -315,7 +308,7 @@ public: virtual void blockConnected(ChainstateRole role, const BlockInfo& block) {} virtual void blockDisconnected(const BlockInfo& block) {} virtual void updatedBlockTip() {} - virtual void chainStateFlushed(ChainstateRole role, const CBlockLocator& locator) {} + virtual void chainStateFlushed(ChainstateRole role) {} }; //! Register handler for notifications. diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f9a372e3de9..60c38983e1b 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -452,7 +452,7 @@ public: m_notifications->updatedBlockTip(); } void ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) override { - m_notifications->chainStateFlushed(role, locator); + m_notifications->chainStateFlushed(role); } std::shared_ptr m_notifications; }; @@ -536,17 +536,6 @@ public: const CBlockIndex* block{chainman().ActiveChain()[height]}; return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0; } - CBlockLocator getTipLocator() override - { - LOCK(::cs_main); - return chainman().ActiveChain().GetLocator(); - } - CBlockLocator getActiveChainLocator(const uint256& block_hash) override - { - LOCK(::cs_main); - const CBlockIndex* index = chainman().m_blockman.LookupBlockIndex(block_hash); - return GetLocator(index); - } std::optional findLocatorFork(const CBlockLocator& locator) override { LOCK(::cs_main); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9c15c2a8275..eeccf398bc3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -627,15 +627,19 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, return false; } -void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) +void CWallet::chainStateFlushed(ChainstateRole role) { // 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 || role == ChainstateRole::BACKGROUND) { return; } - WalletBatch batch(GetDatabase()); - batch.WriteBestBlock(loc); + CBlockLocator loc; + WITH_LOCK(cs_wallet, chain().findBlock(m_last_block_processed, FoundBlock().locator(loc))); + if (!loc.IsNull()) { + WalletBatch batch(GetDatabase()); + batch.WriteBestBlock(loc); + } } void CWallet::SetMinVersion(enum WalletFeature nVersion, WalletBatch* batch_in) @@ -1913,8 +1917,8 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc result.last_scanned_height = block_height; if (save_progress && next_interval) { - CBlockLocator loc = m_chain->getActiveChainLocator(block_hash); - + CBlockLocator loc; + chain().findBlock(block_hash, FoundBlock().locator(loc)); if (!loc.IsNull()) { WalletLogPrintf("Saving scan progress %d.\n", block_height); WalletBatch batch(GetDatabase()); @@ -3003,7 +3007,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } if (chain) { - walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain->getTipLocator()); + walletInstance->chainStateFlushed(ChainstateRole::NORMAL); } } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) { // Make it impossible to disable private keys after creation @@ -3290,7 +3294,7 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf } } walletInstance->m_attaching_chain = false; - walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain.getTipLocator()); + walletInstance->chainStateFlushed(ChainstateRole::NORMAL); walletInstance->GetDatabase().IncrementUpdateCounter(); } walletInstance->m_attaching_chain = false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8b0ee222768..cf1e3345884 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -790,7 +790,7 @@ public: /** should probably be renamed to IsRelevantToMe */ bool IsFromMe(const CTransaction& tx) const; CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const; - void chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) override; + void chainStateFlushed(ChainstateRole role) override; DBErrors LoadWallet();