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 4368384f1d from #14121), but it is
still using the locator argument to log diagnostic warnings.

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.

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<Chain::Notifications> 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<int> findLocatorFork(const CBlockLocator& locator) override
     {
         LOCK(::cs_main);

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;
     }
+    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> 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<CWallet>& 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;

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();