Add stop_block out arg to ScanForWalletTransactions

Accurately reports the last block successfully scanned, replacing a return of
the chain tip, which represented possibly inaccurated data in a race condition.

5 files changed, 32 insertions, 24 deletions

diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
index 5646070a3f..610d83acb6 100644
--- a/src/qt/test/wallettests.cpp
+++ b/src/qt/test/wallettests.cpp
@@ -147,11 +147,12 @@ void TestGUI()
         WalletRescanReserver reserver(wallet.get());
         reserver.reserve();
         const CBlockIndex* const null_block = nullptr;
-        const CBlockIndex* stop_block;
+        const CBlockIndex *stop_block, *failed_block;
         QCOMPARE(
-            wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, stop_block, true /* fUpdate */),
+            wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, failed_block, stop_block, true /* fUpdate */),
             CWallet::ScanResult::SUCCESS);
-        QCOMPARE(stop_block, null_block);
+        QCOMPARE(stop_block, chainActive.Tip());
+        QCOMPARE(failed_block, null_block);
     }
     wallet->SetBroadcastTransactions(true);
 
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index c3f12f373d..3c92eb3120 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3332,13 +3332,12 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
         }
     }
 
-    const CBlockIndex* stopBlock;
+    const CBlockIndex *failed_block, *stopBlock;
     CWallet::ScanResult result =
-        pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, stopBlock, true);
+        pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, failed_block, stopBlock, true);
     switch (result) {
     case CWallet::ScanResult::SUCCESS:
-        stopBlock = pindexStop ? pindexStop : pChainTip;
-        break;
+        break; // stopBlock set by ScanForWalletTransactions
     case CWallet::ScanResult::FAILURE:
         throw JSONRPCError(RPC_MISC_ERROR, "Rescan failed. Potentially corrupted data files.");
     case CWallet::ScanResult::USER_ABORT:
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index dccda1acf1..46d9b5b238 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -53,9 +53,10 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
         AddKey(wallet, coinbaseKey);
         WalletRescanReserver reserver(&wallet);
         reserver.reserve();
-        const CBlockIndex* stop_block;
-        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, stop_block), CWallet::ScanResult::SUCCESS);
-        BOOST_CHECK_EQUAL(stop_block, null_block);
+        const CBlockIndex *stop_block, *failed_block;
+        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
+        BOOST_CHECK_EQUAL(failed_block, null_block);
+        BOOST_CHECK_EQUAL(stop_block, newTip);
         BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 100 * COIN);
     }
 
@@ -70,9 +71,10 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
         AddKey(wallet, coinbaseKey);
         WalletRescanReserver reserver(&wallet);
         reserver.reserve();
-        const CBlockIndex* stop_block;
-        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, stop_block), CWallet::ScanResult::FAILURE);
-        BOOST_CHECK_EQUAL(oldTip, stop_block);
+        const CBlockIndex *stop_block, *failed_block;
+        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::FAILURE);
+        BOOST_CHECK_EQUAL(failed_block, oldTip);
+        BOOST_CHECK_EQUAL(stop_block, newTip);
         BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN);
     }
 
@@ -291,9 +293,10 @@ public:
         WalletRescanReserver reserver(wallet.get());
         reserver.reserve();
         const CBlockIndex* const null_block = nullptr;
-        const CBlockIndex* stop_block;
-        BOOST_CHECK_EQUAL(wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, stop_block), CWallet::ScanResult::SUCCESS);
-        BOOST_CHECK_EQUAL(stop_block, null_block);
+        const CBlockIndex *stop_block, *failed_block;
+        BOOST_CHECK_EQUAL(wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
+        BOOST_CHECK_EQUAL(stop_block, chainActive.Tip());
+        BOOST_CHECK_EQUAL(failed_block, null_block);
     }
 
     ~ListCoinsTestingSetup()
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 28ee32b128..3c25a67fd1 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1613,9 +1613,9 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
     }
 
     if (startBlock) {
-        const CBlockIndex* failedBlock;
+        const CBlockIndex *failedBlock, *stop_block;
         // TODO: this should take into account failure by ScanResult::USER_ABORT
-        if (ScanResult::FAILURE == ScanForWalletTransactions(startBlock, nullptr, reserver, failedBlock, update)) {
+        if (ScanResult::FAILURE == ScanForWalletTransactions(startBlock, nullptr, reserver, failedBlock, stop_block, update)) {
             return failedBlock->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1;
         }
     }
@@ -1628,8 +1628,10 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
  * exist in the wallet will be updated.
  *
  * @param[in] pindexStop if not a nullptr, the scan will stop at this block-index
- * @param[out] failed_block if FAILURE is returned, will be set to the most
- *    recent block that could not be scanned, otherwise nullptr
+ * @param[out] failed_block if FAILURE is returned, the most recent block
+ *     that could not be scanned, otherwise nullptr
+ * @param[out] stop_block the most recent block that could be scanned,
+ *     otherwise nullptr if no block could be scanned
  *
  * @return ScanResult indicating success or failure of the scan. SUCCESS if
  * scan was successful. FAILURE if a complete rescan was not possible (due to
@@ -1640,7 +1642,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
  * the main chain after to the addition of any new keys you want to detect
  * transactions for.
  */
-CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, bool fUpdate)
+CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, const CBlockIndex*& stop_block, bool fUpdate)
 {
     int64_t nNow = GetTime();
     const CChainParams& chainParams = Params();
@@ -1694,7 +1696,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const
                 for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
                     SyncTransaction(block.vtx[posInBlock], pindex, posInBlock, fUpdate);
                 }
+                // scan succeeded, record block as most recent successfully scanned
+                stop_block = pindex;
             } else {
+                // could not scan block, keep scanning but record this block as the most recent failure
                 failed_block = pindex;
             }
             if (pindex == pindexStop) {
@@ -4174,8 +4179,8 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         nStart = GetTimeMillis();
         {
             WalletRescanReserver reserver(walletInstance.get());
-            const CBlockIndex* stop_block;
-            if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(pindexRescan, nullptr, reserver, stop_block, true))) {
+            const CBlockIndex *stop_block, *failed_block;
+            if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(pindexRescan, nullptr, reserver, failed_block, stop_block, true))) {
                 InitError(_("Failed to rescan the wallet during initialization"));
                 return nullptr;
             }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 08184bdc22..5ba5514fc2 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -902,7 +902,7 @@ public:
         FAILURE,
         USER_ABORT
     };
-    ScanResult ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, bool fUpdate = false);
+    ScanResult ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, const CBlockIndex*& stop_block, bool fUpdate = false);
     void TransactionRemovedFromMempool(const CTransactionRef &ptx) override;
     void ReacceptWalletTransactions();
     void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);