Merge #2225: [wallet] Rescan chain for wallet transactions process re…

…-written

030467f wallet: ScanForWalletTransactions rewrite, locking cs_main and cs_wallet only where is needed. (furszy)
de3e3a7 [wallet] Make sure pindex is non-null before possibly referencing in LogPrintf call (Karl-Johan Alm)
146c843 wallet: speeding up ScanForWalletTransactions, removing a bad "always rescan the entire chain after zerocoin activation".. (furszy)

Pull request description:

  Have re-organized the `CWallet::ScanForWalletTransactions` code to lock `cs_main` and `cs_wallet` only where is needed, preparing the ground to be able to almost completely remove `cs_main` lock dependency after merging #2209 (it will only be needed to advance the index to the next block, the rest of the process will run `cs_main` lock free).

  Plus, corrected an unneeded forced scan of the entire chain after zerocoin activation, was happening even when the first key in the wallet could had occurred way after it. Moving through the chain without any purpose.

ACKs for top commit:
  random-zebra:
    ACK 030467f
  Fuzzbawls:
    ACK 030467f

Tree-SHA512: 7ec051908bdaf7e04326d4348db93112408b251d558427ee95bfc6261125d75289482f4acd3c8db1c629968cf5d949ae05b46631148cfd6431a5986d47daef41

src/wallet/wallet.cpp

@@ -1786,23 +1786,32 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate, b
 
     CBlockIndex* pindex = pindexStart;
     {
-        LOCK2(cs_main, cs_wallet);
-
+        LOCK(cs_main);
         // no need to read and scan block, if block was created before
         // our wallet birthday (as adjusted for block time variability)
-        while (pindex && nTimeFirstKey && (pindex->GetBlockTime() < (nTimeFirstKey - 7200)) &&
-                (pindex->nHeight < 1 || !consensus.NetworkUpgradeActive(pindex->nHeight - 1, Consensus::UPGRADE_ZC)))
+        while (pindex && nTimeFirstKey && (pindex->GetBlockTime() < (nTimeFirstKey - 7200)))
             pindex = chainActive.Next(pindex);
+    }
 
+    {
         ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup
-        double dProgressStart = Checkpoints::GuessVerificationProgress(pindex, false);
-        double dProgressTip = Checkpoints::GuessVerificationProgress(chainActive.Tip(), false);
-        std::set<uint256> setAddedToWallet;
+        const double dProgressStart = Checkpoints::GuessVerificationProgress(pindex, false);
+        const CBlockIndex* tip = nullptr;
+        double dProgressTip = 0.0;
         std::vector<uint256> myTxHashes;
-        while (pindex) {
-            if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0)
-                ShowProgress(_("Rescanning..."), std::max(1, std::min(99, (int)((Checkpoints::GuessVerificationProgress(pindex, false) - dProgressStart) / (dProgressTip - dProgressStart) * 100))));
 
+        double gvp = dProgressStart;
+        while (pindex) {
+            gvp = Checkpoints::GuessVerificationProgress(pindex, false);
+            if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0) {
+                ShowProgress(_("Rescanning..."), std::max(1, std::min(99, (int) ((gvp - dProgressStart) /
+                                                                                 (dProgressTip - dProgressStart) *
+                                                                                 100))));
+            }
+            if (GetTime() >= nNow + 60) {
+                nNow = GetTime();
+                LogPrintf("Still rescanning. At block %d. Progress=%f\n", pindex->nHeight, gvp);
+            }
             if (fromStartup && ShutdownRequested()) {
                 return -1;
             }
@@ -1812,31 +1821,41 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate, b
                 LogPrintf("Unable to read block %d (%s) from disk.", pindex->nHeight, pindex->GetBlockHash().ToString());
                 return -1;
             }
-            int posInBlock;
-            for (posInBlock = 0; posInBlock < (int)block.vtx.size(); posInBlock++) {
-                const auto& tx = block.vtx[posInBlock];
-                if (AddToWalletIfInvolvingMe(tx, pindex->GetBlockHash(), posInBlock, fUpdate)) {
-                    myTxHashes.push_back(tx->GetHash());
-                    ret++;
+
+            {
+                LOCK2(cs_main, cs_wallet);
+                if (tip != chainActive.Tip()) {
+                    tip = chainActive.Tip();
+                    // in case the tip has changed, update progress max
+                    dProgressTip = Checkpoints::GuessVerificationProgress(tip, false);
                 }
-            }
 
-            // Sapling
-            // This should never fail: we should always be able to get the tree
-            // state on the path to the tip of our chain
-            if (pindex->pprev) {
-                if (Params().GetConsensus().NetworkUpgradeActive(pindex->pprev->nHeight,  Consensus::UPGRADE_V5_0)) {
-                    SaplingMerkleTree saplingTree;
-                    assert(pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingRoot, saplingTree));
-                    // Increment note witness caches
-                    ChainTipAdded(pindex, &block, saplingTree);
+                if (!chainActive.Contains(pindex)) {
+                    // Abort scan if current block is no longer active, to prevent
+                    // marking transactions as coming from the wrong block.
+                    break;
+                }
+                for (int posInBlock = 0; posInBlock < (int) block.vtx.size(); posInBlock++) {
+                    const auto& tx = block.vtx[posInBlock];
+                    if (AddToWalletIfInvolvingMe(tx, pindex->GetBlockHash(), posInBlock, fUpdate)) {
+                        myTxHashes.push_back(tx->GetHash());
+                        ret++;
+                    }
                 }
-            }
 
-            pindex = chainActive.Next(pindex);
-            if (GetTime() >= nNow + 60) {
-                nNow = GetTime();
-                LogPrintf("Still rescanning. At block %d. Progress=%f\n", pindex->nHeight, Checkpoints::GuessVerificationProgress(pindex));
+                // Sapling
+                // This should never fail: we should always be able to get the tree
+                // state on the path to the tip of our chain
+                if (pindex->pprev) {
+                    if (Params().GetConsensus().NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_V5_0)) {
+                        SaplingMerkleTree saplingTree;
+                        assert(pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingRoot, saplingTree));
+                        // Increment note witness caches
+                        ChainTipAdded(pindex, &block, saplingTree);
+                    }
+                }
+
+                pindex = chainActive.Next(pindex);
             }
         }