From 30abce7a998181e28e2f93256c159f259513e1a7 Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Wed, 22 Feb 2017 13:07:23 -0500
Subject: [PATCH] Improve ScanForWalletTransactions return value

Change ScanForWalletTransactions return value so it is possible to distinguish
scans that skip reading every block (due to the nTimeFirstKey optimization)
from scans that fail while reading the chainActive.Tip() block. Return value is
now non-null in the non-failing case.

This change doesn't affect any user-visible behavior, it is only an internal
API improvement. The only code currently using the ScanForWalletTransactions
return value is in importmulti, and importmulti always calls
ScanForWalletTransactions with a pindex pointing to the first block in
chainActive whose block time is >= (nLowestTimestamp - 7200), while
ScanForWalletTransactions would only return null without reading blocks when
pindex and every block after it had a block time < (nTimeFirstKey - 7200).
These conditions could never happen at the same time because nTimeFirstKey <=
nLowestTimestamp.

I'm planning to make a more substantial API improvement in the future (making
ScanForWalletTransactions private and exposing a higher level rescan method to
RPC code), but Matt Corallo <git@bluematt.me> pointed out this odd behavior
introduced by e2e2f4c "Return errors from importmulti if complete rescans are
not successful" yesterday, so I'm following up now to get rid of badness
introduced by that merge.
---
 src/wallet/test/wallet_tests.cpp | 11 +++++++++++
 src/wallet/wallet.cpp            |  7 ++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 7ac2112dd29..c521be8b35c 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -426,6 +426,17 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
         BOOST_CHECK_EQUAL(response.write(), strprintf("[{\"success\":false,\"error\":{\"code\":-1,\"message\":\"Failed to rescan before time %d, transactions may be missing.\"}},{\"success\":true}]", newTip->GetBlockTimeMax()));
         ::pwalletMain = backup;
     }
+
+    // Verify ScanForWalletTransactions does not return null when the scan is
+    // elided due to the nTimeFirstKey optimization.
+    {
+        CWallet wallet;
+        {
+            LOCK(wallet.cs_wallet);
+            wallet.UpdateTimeFirstKey(newTip->GetBlockTime() + 7200 + 1);
+        }
+        BOOST_CHECK_EQUAL(newTip, wallet.ScanForWalletTransactions(newTip));
+    }
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 63501b04be4..13de9ca8e17 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1547,16 +1547,17 @@ void CWalletTx::GetAccountAmounts(const string& strAccount, CAmount& nReceived,
  * exist in the wallet will be updated.
  *
  * Returns pointer to the first block in the last contiguous range that was
- * successfully scanned.
- *
+ * successfully scanned or elided (elided if pIndexStart points at a block
+ * before CWallet::nTimeFirstKey). Returns null if there is no such range, or
+ * the range doesn't include chainActive.Tip().
  */
 CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate)
 {
-    CBlockIndex* ret = nullptr;
     int64_t nNow = GetTime();
     const CChainParams& chainParams = Params();
 
     CBlockIndex* pindex = pindexStart;
+    CBlockIndex* ret = pindexStart;
     {
         LOCK2(cs_main, cs_wallet);