From 98a1a5275c8c395fe47ff7f10109d75b06bc391d Mon Sep 17 00:00:00 2001
From: Ava Chow <github@achow101.com>
Date: Mon, 3 Jun 2024 16:25:33 -0400
Subject: [PATCH] wallet: Remove chainStateFlushed

chainStateFlushed is no longer needed since the best block is updated
after a block is scanned. Since the chainstate being flushed does not
necessarily coincide with the wallet having processed said block, it
does not entirely make sense for the wallet to be recording that block
as its best block, and this can cause race conditions where some blocks
are not processed. Thus, remove this notification.
---
 src/bench/wallet_migration.cpp |  1 -
 src/wallet/wallet.cpp          | 23 -----------------------
 src/wallet/wallet.h            |  2 --
 3 files changed, 26 deletions(-)

diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp
index a172ec4483..13146fe289 100644
--- a/src/bench/wallet_migration.cpp
+++ b/src/bench/wallet_migration.cpp
@@ -28,7 +28,6 @@ static void WalletMigration(benchmark::Bench& bench)
 
     // Setup legacy wallet
     std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase());
-    wallet->chainStateFlushed(ChainstateRole::NORMAL, CBlockLocator{});
     LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM();
     WalletBatch batch{wallet->GetDatabase()};
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index a90c4de54c..5ff3d1433d 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -652,17 +652,6 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
     return false;
 }
 
-void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc)
-{
-    // 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);
-}
-
 void CWallet::SetLastBlockProcessedInMem(int block_height, uint256 block_hash)
 {
     AssertLockHeld(cs_wallet);
@@ -3143,11 +3132,6 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
     // 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.
-    // 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
     walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance);
 
     // If rescan_required = true, rescan_height remains equal to 0
@@ -3233,14 +3217,12 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
                 error = _("Failed to rescan the wallet during initialization");
                 return false;
             }
-            walletInstance->m_attaching_chain = 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;
 
     return true;
 }
@@ -4229,11 +4211,6 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
             return util::Error{_("Error: This wallet is already a descriptor wallet")};
         }
 
-        // Flush chain state before unloading wallet
-        CBlockLocator locator;
-        WITH_LOCK(wallet->cs_wallet, context.chain->findBlock(wallet->GetLastBlockHash(), FoundBlock().locator(locator)));
-        if (!locator.IsNull()) wallet->chainStateFlushed(ChainstateRole::NORMAL, locator);
-
         if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) {
             return util::Error{_("Unable to unload the wallet before migrating")};
         }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 9334aa0485..04363e607d 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -305,7 +305,6 @@ private:
 
     std::atomic<bool> fAbortRescan{false};
     std::atomic<bool> fScanningWallet{false}; // controlled by WalletRescanReserver
-    std::atomic<bool> m_attaching_chain{false};
     std::atomic<bool> m_scanning_with_passphrase{false};
     std::atomic<SteadyClock::time_point> m_scanning_start{SteadyClock::time_point{}};
     std::atomic<double> m_scanning_progress{0};
@@ -787,7 +786,6 @@ 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;
 
     DBErrors LoadWallet();