Write witness caches when writing the best block

For steady-state operation, this reduces the average time between wallet disk
writes from once per block to once per hour.

On -rescan, witness caches are only written out at the end along with the best
block, increasing speed while ensuring that on-disk state is kept consistent.

Witness caches are now never recreated during a -reindex, on the assumption that
the blocks themselves are not changing (the chain is just being reconstructed),
and so the witnesses will remain valid.

Part of #1749.

src/init.cpp

@@ -537,11 +537,6 @@ void ThreadImport(std::vector<boost::filesystem::path> vImportFiles)
     RenameThread("zcash-loadblk");
     // -reindex
     if (fReindex) {
-#ifdef ENABLE_WALLET
-        if (pwalletMain) {
-            pwalletMain->ClearNoteWitnessCache();
-        }
-#endif
         CImportingNow imp;
         int nFile = 0;
         while (true) {
@@ -1379,7 +1374,10 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
 
         CBlockIndex *pindexRescan = chainActive.Tip();
         if (GetBoolArg("-rescan", false))
+        {
+            pwalletMain->ClearNoteWitnessCache();
             pindexRescan = chainActive.Genesis();
+        }
         else
         {
             CWalletDB walletdb(strWalletFile);

src/primitives/block.h

@@ -200,6 +200,10 @@ struct CBlockLocator
     {
         return vHave.empty();
     }
+
+    friend bool operator==(const CBlockLocator& a, const CBlockLocator& b) {
+        return (a.vHave == b.vHave);
+    }
 };
 
 #endif // BITCOIN_PRIMITIVES_BLOCK_H

src/wallet/gtest/test_wallet.cpp

@@ -5,6 +5,7 @@
 #include "base58.h"
 #include "chainparams.h"
 #include "main.h"
+#include "primitives/block.h"
 #include "random.h"
 #include "wallet/wallet.h"
 #include "zcash/JoinSplit.hpp"
@@ -29,9 +30,11 @@ public:
 
     MOCK_METHOD2(WriteTx, bool(uint256 hash, const CWalletTx& wtx));
     MOCK_METHOD1(WriteWitnessCacheSize, bool(int64_t nWitnessCacheSize));
+    MOCK_METHOD1(WriteBestBlock, bool(const CBlockLocator& loc));
 };
 
-template void CWallet::WriteWitnessCache<MockWalletDB>(MockWalletDB& walletdb);
+template void CWallet::SetBestChainINTERNAL<MockWalletDB>(
+        MockWalletDB& walletdb, const CBlockLocator& loc);
 
 class TestWallet : public CWallet {
 public:
@@ -53,8 +56,8 @@ public:
     void DecrementNoteWitnesses(const CBlockIndex* pindex) {
         CWallet::DecrementNoteWitnesses(pindex);
     }
-    void WriteWitnessCache(MockWalletDB& walletdb) {
-        CWallet::WriteWitnessCache(walletdb);
+    void SetBestChain(MockWalletDB& walletdb, const CBlockLocator& loc) {
+        CWallet::SetBestChainINTERNAL(walletdb, loc);
     }
     bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx) {
         return CWallet::UpdatedNoteData(wtxIn, wtx);
@@ -918,6 +921,7 @@ TEST(wallet_tests, ClearNoteWitnessCache) {
 TEST(wallet_tests, WriteWitnessCache) {
     TestWallet wallet;
     MockWalletDB walletdb;
+    CBlockLocator loc;
 
     auto sk = libzcash::SpendingKey::random();
     wallet.AddSpendingKey(sk);
@@ -928,7 +932,7 @@ TEST(wallet_tests, WriteWitnessCache) {
     // TxnBegin fails
     EXPECT_CALL(walletdb, TxnBegin())
         .WillOnce(Return(false));
-    wallet.WriteWitnessCache(walletdb);
+    wallet.SetBestChain(walletdb, loc);
     EXPECT_CALL(walletdb, TxnBegin())
         .WillRepeatedly(Return(true));
 
@@ -937,14 +941,14 @@ TEST(wallet_tests, WriteWitnessCache) {
         .WillOnce(Return(false));
     EXPECT_CALL(walletdb, TxnAbort())
         .Times(1);
-    wallet.WriteWitnessCache(walletdb);
+    wallet.SetBestChain(walletdb, loc);
 
     // WriteTx throws
     EXPECT_CALL(walletdb, WriteTx(wtx.GetHash(), wtx))
         .WillOnce(ThrowLogicError());
     EXPECT_CALL(walletdb, TxnAbort())
         .Times(1);
-    wallet.WriteWitnessCache(walletdb);
+    wallet.SetBestChain(walletdb, loc);
     EXPECT_CALL(walletdb, WriteTx(wtx.GetHash(), wtx))
         .WillRepeatedly(Return(true));
 
@@ -953,26 +957,42 @@ TEST(wallet_tests, WriteWitnessCache) {
         .WillOnce(Return(false));
     EXPECT_CALL(walletdb, TxnAbort())
         .Times(1);
-    wallet.WriteWitnessCache(walletdb);
+    wallet.SetBestChain(walletdb, loc);
 
     // WriteWitnessCacheSize throws
     EXPECT_CALL(walletdb, WriteWitnessCacheSize(0))
         .WillOnce(ThrowLogicError());
     EXPECT_CALL(walletdb, TxnAbort())
         .Times(1);
-    wallet.WriteWitnessCache(walletdb);
+    wallet.SetBestChain(walletdb, loc);
     EXPECT_CALL(walletdb, WriteWitnessCacheSize(0))
         .WillRepeatedly(Return(true));
 
+    // WriteBestBlock fails
+    EXPECT_CALL(walletdb, WriteBestBlock(loc))
+        .WillOnce(Return(false));
+    EXPECT_CALL(walletdb, TxnAbort())
+        .Times(1);
+    wallet.SetBestChain(walletdb, loc);
+
+    // WriteBestBlock throws
+    EXPECT_CALL(walletdb, WriteBestBlock(loc))
+        .WillOnce(ThrowLogicError());
+    EXPECT_CALL(walletdb, TxnAbort())
+        .Times(1);
+    wallet.SetBestChain(walletdb, loc);
+    EXPECT_CALL(walletdb, WriteBestBlock(loc))
+        .WillRepeatedly(Return(true));
+
     // TxCommit fails
     EXPECT_CALL(walletdb, TxnCommit())
         .WillOnce(Return(false));
-    wallet.WriteWitnessCache(walletdb);
+    wallet.SetBestChain(walletdb, loc);
     EXPECT_CALL(walletdb, TxnCommit())
         .WillRepeatedly(Return(true));
 
     // Everything succeeds
-    wallet.WriteWitnessCache(walletdb);
+    wallet.SetBestChain(walletdb, loc);
 }
 
 TEST(wallet_tests, UpdateNullifierNoteMap) {

src/wallet/wallet.cpp

@@ -380,7 +380,7 @@ void CWallet::ChainTip(const CBlockIndex *pindex, const CBlock *pblock,
 void CWallet::SetBestChain(const CBlockLocator& loc)
 {
     CWalletDB walletdb(strWalletFile);
-    walletdb.WriteBestBlock(loc);
+    SetBestChainINTERNAL(walletdb, loc);
 }
 
 bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn, bool fExplicit)
@@ -741,10 +741,9 @@ void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex,
             }
         }
 
-        if (fFileBacked) {
-            CWalletDB walletdb(strWalletFile);
-            WriteWitnessCache(walletdb);
-        }
+        // For performance reasons, we write out the witness cache in
+        // CWallet::SetBestChain() (which also ensures that overall consistency
+        // of the wallet.dat is maintained).
     }
 }
 
@@ -779,10 +778,10 @@ void CWallet::DecrementNoteWitnesses(const CBlockIndex* pindex)
         }
         // TODO: If nWitnessCache is zero, we need to regenerate the caches (#1302)
         assert(nWitnessCacheSize > 0);
-        if (fFileBacked) {
-            CWalletDB walletdb(strWalletFile);
-            WriteWitnessCache(walletdb);
-        }
+
+        // For performance reasons, we write out the witness cache in
+        // CWallet::SetBestChain() (which also ensures that overall consistency
+        // of the wallet.dat is maintained).
     }
 }
 

src/wallet/wallet.h

@@ -636,35 +636,40 @@ protected:
     void DecrementNoteWitnesses(const CBlockIndex* pindex);
 
     template <typename WalletDB>
-    void WriteWitnessCache(WalletDB& walletdb) {
+    void SetBestChainINTERNAL(WalletDB& walletdb, const CBlockLocator& loc) {
         if (!walletdb.TxnBegin()) {
             // This needs to be done atomically, so don't do it at all
-            LogPrintf("WriteWitnessCache(): Couldn't start atomic write\n");
+            LogPrintf("SetBestChain(): Couldn't start atomic write\n");
             return;
         }
         try {
             for (std::pair<const uint256, CWalletTx>& wtxItem : mapWallet) {
                 if (!walletdb.WriteTx(wtxItem.first, wtxItem.second)) {
-                    LogPrintf("WriteWitnessCache(): Failed to write CWalletTx, aborting atomic write\n");
+                    LogPrintf("SetBestChain(): Failed to write CWalletTx, aborting atomic write\n");
                     walletdb.TxnAbort();
                     return;
                 }
             }
             if (!walletdb.WriteWitnessCacheSize(nWitnessCacheSize)) {
-                LogPrintf("WriteWitnessCache(): Failed to write nWitnessCacheSize, aborting atomic write\n");
+                LogPrintf("SetBestChain(): Failed to write nWitnessCacheSize, aborting atomic write\n");
+                walletdb.TxnAbort();
+                return;
+            }
+            if (!walletdb.WriteBestBlock(loc)) {
+                LogPrintf("SetBestChain(): Failed to write best block, aborting atomic write\n");
                 walletdb.TxnAbort();
                 return;
             }
         } catch (const std::exception &exc) {
             // Unexpected failure
-            LogPrintf("WriteWitnessCache(): Unexpected error during atomic write:\n");
+            LogPrintf("SetBestChain(): Unexpected error during atomic write:\n");
             LogPrintf("%s\n", exc.what());
             walletdb.TxnAbort();
             return;
         }
         if (!walletdb.TxnCommit()) {
             // Couldn't commit all to db, but in-memory state is fine
-            LogPrintf("WriteWitnessCache(): Couldn't commit atomic write\n");
+            LogPrintf("SetBestChain(): Couldn't commit atomic write\n");
             return;
         }
     }