Merge bitcoin#15342: Suggested wallet code cleanups from bitcoin#14711

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky)
2c1fbaa Drop redundant get_value_or (Russell Yanofsky)
84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky)
2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky)
db2d093 Add suggested rescanblockchain comments (Russell Yanofsky)
a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky)
95a812b Rename ScanResult stop_block field (Russell Yanofsky)

Pull request description:

  This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging.

  There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames.

Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8

# Conflicts:
#	src/interfaces/chain.cpp
#	src/qt/test/wallettests.cpp
#	src/wallet/test/wallet_tests.cpp
#	src/wallet/wallet.cpp

Author: MarcoFalke

src/interfaces/chain.cpp

@@ -123,7 +123,7 @@ class LockImpl : public Chain::Lock
         CBlockIndex* block = LookupBlockIndex(hash);
         return block && block->GetAncestor(::ChainActive().Height()) == ::ChainActive().Tip();
     }
-    CBlockLocator getLocator() override { return ::ChainActive().GetLocator(); }
+    CBlockLocator getTipLocator() override { return ::ChainActive().GetLocator(); }
     Optional<int> findLocatorFork(const CBlockLocator& locator) override
     {
         LockAnnotation lock(::cs_main);

src/interfaces/chain.h

@@ -96,7 +96,7 @@ class Chain
         virtual bool isPotentialTip(const uint256& hash) = 0;
 
         //! Get locator for the current chain tip.
-        virtual CBlockLocator getLocator() = 0;
+        virtual CBlockLocator getTipLocator() = 0;
 
         //! Return height of the latest block common to locator and chain, which
         //! is guaranteed to be an ancestor of the block used to create the

src/qt/test/wallettests.cpp

@@ -124,8 +124,8 @@ void TestGUI()
         reserver.reserve();
         CWallet::ScanResult result = wallet->ScanForWalletTransactions(locked_chain->getBlockHash(0), {} /* stop_block */, reserver, true /* fUpdate */);
         QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);
-        QCOMPARE(result.stop_block, ::ChainActive().Tip()->GetBlockHash());
-        QVERIFY(result.failed_block.IsNull());
+        QCOMPARE(result.last_scanned_block, ::ChainActive().Tip()->GetBlockHash());
+        QVERIFY(result.last_failed_block.IsNull());
     }
     wallet->SetBroadcastTransactions(true);
 

src/wallet/rpcwallet.cpp

@@ -3749,8 +3749,8 @@ static UniValue rescanblockchain(const JSONRPCRequest& request)
             "2. \"stop_height\"     (numeric, optional) the last block height that should be scanned. If none is provided it will rescan up to the tip at return time of this call.\n"
             "\nResult:\n"
             "{\n"
-            "  \"start_height\"     (numeric) The block height where the rescan has started.\n"
-            "  \"stop_height\"      (numeric) The height of the last rescanned block.\n"
+            "  \"start_height\"     (numeric) The block height where the rescan started (the requested height or 0)\n"
+            "  \"stop_height\"      (numeric) The height of the last rescanned block. May be null in rare cases if there was a reorg and the call didn't scan any blocks because they were already scanned in the background.\n"
             "}\n"
             "\nExamples:\n"
             + HelpExampleCli("rescanblockchain", "100000 120000")
@@ -3794,6 +3794,11 @@ static UniValue rescanblockchain(const JSONRPCRequest& request)
 
         if (tip_height) {
             start_block = locked_chain->getBlockHash(start_height);
+            // If called with a stop_height, set the stop_height here to
+            // trigger a rescan to that height.
+            // If called without a stop height, leave stop_height as null here
+            // so rescan continues to the tip (even if the tip advances during
+            // rescan).
             if (stop_height) {
                 stop_block = locked_chain->getBlockHash(*stop_height);
             }
@@ -3813,7 +3818,7 @@ static UniValue rescanblockchain(const JSONRPCRequest& request)
     }
     UniValue response(UniValue::VOBJ);
     response.pushKV("start_height", start_height);
-    response.pushKV("stop_height", result.stop_height ? *result.stop_height : UniValue());
+    response.pushKV("stop_height", result.last_scanned_height ? *result.last_scanned_height : UniValue());
     return response;
 }
 

src/wallet/test/wallet_tests.cpp

@@ -57,9 +57,9 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
         reserver.reserve();
         CWallet::ScanResult result = wallet.ScanForWalletTransactions({} /* start_block */, {} /* stop_block */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
-        BOOST_CHECK(result.failed_block.IsNull());
-        BOOST_CHECK(result.stop_block.IsNull());
-        BOOST_CHECK(!result.stop_height);
+        BOOST_CHECK(result.last_failed_block.IsNull());
+        BOOST_CHECK(result.last_scanned_block.IsNull());
+        BOOST_CHECK(!result.last_scanned_height);
         BOOST_CHECK_EQUAL(wallet.GetBalance().m_mine_immature, 0);
     }
 
@@ -72,9 +72,9 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
         reserver.reserve();
         CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
-        BOOST_CHECK(result.failed_block.IsNull());
-        BOOST_CHECK_EQUAL(result.stop_block, newTip->GetBlockHash());
-        BOOST_CHECK_EQUAL(*result.stop_height, newTip->nHeight);
+        BOOST_CHECK(result.last_failed_block.IsNull());
+        BOOST_CHECK_EQUAL(result.last_scanned_block, newTip->GetBlockHash());
+        BOOST_CHECK_EQUAL(*result.last_scanned_height, newTip->nHeight);
         BOOST_CHECK_EQUAL(wallet.GetBalance().m_mine_immature, 1000 * COIN);
     }
 
@@ -91,9 +91,9 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
         reserver.reserve();
         CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE);
-        BOOST_CHECK_EQUAL(result.failed_block, oldTip->GetBlockHash());
-        BOOST_CHECK_EQUAL(result.stop_block, newTip->GetBlockHash());
-        BOOST_CHECK_EQUAL(*result.stop_height, newTip->nHeight);
+        BOOST_CHECK_EQUAL(result.last_failed_block, oldTip->GetBlockHash());
+        BOOST_CHECK_EQUAL(result.last_scanned_block, newTip->GetBlockHash());
+        BOOST_CHECK_EQUAL(*result.last_scanned_height, newTip->nHeight);
         BOOST_CHECK_EQUAL(wallet.GetBalance().m_mine_immature, 500 * COIN);
     }
 
@@ -109,9 +109,9 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
         reserver.reserve();
         CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE);
-        BOOST_CHECK_EQUAL(result.failed_block, newTip->GetBlockHash());
-        BOOST_CHECK(result.stop_block.IsNull());
-        BOOST_CHECK(!result.stop_height);
+        BOOST_CHECK_EQUAL(result.last_failed_block, newTip->GetBlockHash());
+        BOOST_CHECK(result.last_scanned_block.IsNull());
+        BOOST_CHECK(!result.last_scanned_height);
         BOOST_CHECK_EQUAL(wallet.GetBalance().m_mine_immature, 0);
     }
 }
@@ -349,9 +349,9 @@ class ListCoinsTestingSetup : public TestChain100Setup
         reserver.reserve();
         CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
-        BOOST_CHECK_EQUAL(result.stop_block, ::ChainActive().Tip()->GetBlockHash());
-        BOOST_CHECK_EQUAL(*result.stop_height, ::ChainActive().Height());
-        BOOST_CHECK(result.failed_block.IsNull());
+        BOOST_CHECK_EQUAL(result.last_scanned_block, ::ChainActive().Tip()->GetBlockHash());
+        BOOST_CHECK_EQUAL(*result.last_scanned_height, ::ChainActive().Height());
+        BOOST_CHECK(result.last_failed_block.IsNull());
     }
 
     ~ListCoinsTestingSetup()

src/wallet/wallet.cpp

@@ -2116,7 +2116,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
         ScanResult result = ScanForWalletTransactions(start_block, {} /* stop_block */, reserver, update);
         if (result.status == ScanResult::FAILURE) {
             int64_t time_max;
-            if (!chain().findBlock(result.failed_block, nullptr /* block */, nullptr /* time */, &time_max)) {
+            if (!chain().findBlock(result.last_failed_block, nullptr /* block */, nullptr /* time */, &time_max)) {
                 throw std::logic_error("ScanForWalletTransactions returned invalid block hash");
             }
             return time_max + TIMESTAMP_WINDOW + 1;
@@ -2130,15 +2130,17 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
  * from or to us. If fUpdate is true, found transactions that already
  * exist in the wallet will be updated.
  *
- * @param[in] start_block if not null, the scan will start at this block instead
- *            of the genesis block
- * @param[in] stop_block if not null, the scan will stop at this block instead
- *            of the chain tip
+ * @param[in] start_block Scan starting block. If block is not on the active
+ *                        chain, the scan will return SUCCESS immediately.
+ * @param[in] stop_block  Scan ending block. If block is not on the active
+ *                        chain, the scan will continue until it reaches the
+ *                        chain tip.
  *
- * @return ScanResult indicating success or failure of the scan. SUCCESS if
- * scan was successful. FAILURE if a complete rescan was not possible (due to
- * pruning or corruption). USER_ABORT if the rescan was aborted before it
- * could complete.
+ * @return ScanResult returning scan information and indicating success or
+ *         failure. Return status will be set to SUCCESS if scan was
+ *         successful. FAILURE if a complete rescan was not possible (due to
+ *         pruning or corruption). USER_ABORT if the rescan was aborted before
+ *         it could complete.
  *
  * @pre Caller needs to make sure start_block (and the optional stop_block) are on
  * the main chain after to the addition of any new keys you want to detect
@@ -2191,19 +2193,19 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
                     // marking transactions as coming from the wrong block.
                     // TODO: This should return success instead of failure, see
                     // https://github.com/bitcoin/bitcoin/pull/14711#issuecomment-458342518
-                    result.failed_block = block_hash;
+                    result.last_failed_block = block_hash;
                     result.status = ScanResult::FAILURE;
                     break;
                 }
                 for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
                     SyncTransaction(block.vtx[posInBlock], block_hash, posInBlock, fUpdate);
                 }
                 // scan succeeded, record block as most recent successfully scanned
-                result.stop_block = block_hash;
-                result.stop_height = *block_height;
+                result.last_scanned_block = block_hash;
+                result.last_scanned_height = *block_height;
             } else {
                 // could not scan block, keep scanning but record this block as the most recent failure
-                result.failed_block = block_hash;
+                result.last_failed_block = block_hash;
                 result.status = ScanResult::FAILURE;
             }
             if (block_hash == stop_block) {
@@ -2233,10 +2235,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
         }
         ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()), 100); // hide progress dialog in GUI
         if (block_height && fAbortRescan) {
-            WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height.value_or(0), progress_current);
+            WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", *block_height, progress_current);
             result.status = ScanResult::USER_ABORT;
         } else if (block_height && ShutdownRequested()) {
-            WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", block_height.value_or(0), progress_current);
+            WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", *block_height, progress_current);
             result.status = ScanResult::USER_ABORT;
         }
     }
@@ -4987,7 +4989,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         }
 
         auto locked_chain = chain.assumeLocked();  // Temporary. Removed in upcoming lock cleanup
-        walletInstance->ChainStateFlushed(locked_chain->getLocator());
+        walletInstance->ChainStateFlushed(locked_chain->getTipLocator());
 
         // Try to create wallet backup right after new wallet was created
         std::string strBackupWarning;
@@ -5000,7 +5002,6 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
                 return error(strBackupError);
             }
         }
-
     } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) {
         // Make it impossible to disable private keys after creation
         InitError(strprintf(_("Error loading %s: Private keys can only be disabled during creation"), walletFile));
@@ -5154,7 +5155,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
             }
         }
         walletInstance->WalletLogPrintf("Rescan completed in %15dms\n", GetTimeMillis() - nStart);
-        walletInstance->ChainStateFlushed(locked_chain->getLocator());
+        walletInstance->ChainStateFlushed(locked_chain->getTipLocator());
         walletInstance->database->IncrementUpdateCounter();
 
         // Restore wallet transaction metadata after -zapwallettxes=1

src/wallet/wallet.h

@@ -1006,14 +1006,14 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
         //! Hash and height of most recent block that was successfully scanned.
         //! Unset if no blocks were scanned due to read errors or the chain
         //! being empty.
-        uint256 stop_block;
-        Optional<int> stop_height;
+        uint256 last_scanned_block;
+        Optional<int> last_scanned_height;
 
         //! Height of the most recent block that could not be scanned due to
         //! read errors or pruning. Will be set if status is FAILURE, unset if
         //! status is SUCCESS, and may or may not be set if status is
         //! USER_ABORT.
-        uint256 failed_block;
+        uint256 last_failed_block;
     };
     ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate);
     void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override;