Auto merge of zcash#1904 - str4d:1749-write-witness-cache-with-best-b…

…lock, r=ebfull

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 jl777#1749.

src/init.cpp

@@ -542,11 +542,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) {
@@ -1387,7 +1382,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 "utiltest.h"
 #include "wallet/wallet.h"
@@ -30,9 +31,11 @@ class MockWalletDB {
 
     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:
@@ -54,8 +57,8 @@ class TestWallet : public CWallet {
     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);
@@ -748,6 +751,121 @@ TEST(wallet_tests, CachedWitnessesDecrementFirst) {
     }
 }
 
+TEST(wallet_tests, CachedWitnessesCleanIndex) {
+    TestWallet wallet;
+    CBlock block1;
+    CBlock block2;
+    CBlock block3;
+    CBlockIndex index1(block1);
+    CBlockIndex index2(block2);
+    CBlockIndex index3(block3);
+    ZCIncrementalMerkleTree tree;
+
+    auto sk = libzcash::SpendingKey::random();
+    wallet.AddSpendingKey(sk);
+
+    {
+        // First transaction (case tested in _empty_chain)
+        auto wtx = GetValidReceive(sk, 10, true);
+        auto note = GetNote(sk, wtx, 0, 1);
+        auto nullifier = note.nullifier(sk);
+
+        mapNoteData_t noteData;
+        JSOutPoint jsoutpt {wtx.GetHash(), 0, 1};
+        CNoteData nd {sk.address(), nullifier};
+        noteData[jsoutpt] = nd;
+        wtx.SetNoteData(noteData);
+        wallet.AddToWallet(wtx, true, NULL);
+
+        // First block (case tested in _empty_chain)
+        block1.vtx.push_back(wtx);
+        index1.nHeight = 1;
+        wallet.IncrementNoteWitnesses(&index1, &block1, tree);
+    }
+
+    {
+        // Second transaction (case tested in _chain_tip)
+        auto wtx = GetValidReceive(sk, 50, true);
+        auto note = GetNote(sk, wtx, 0, 1);
+        auto nullifier = note.nullifier(sk);
+
+        mapNoteData_t noteData;
+        JSOutPoint jsoutpt {wtx.GetHash(), 0, 1};
+        CNoteData nd {sk.address(), nullifier};
+        noteData[jsoutpt] = nd;
+        wtx.SetNoteData(noteData);
+        wallet.AddToWallet(wtx, true, NULL);
+
+        // Second block (case tested in _chain_tip)
+        block2.vtx.push_back(wtx);
+        index2.nHeight = 2;
+        wallet.IncrementNoteWitnesses(&index2, &block2, tree);
+    }
+
+    {
+        // Third transaction
+        auto wtx = GetValidReceive(sk, 20, true);
+        auto note = GetNote(sk, wtx, 0, 1);
+        auto nullifier = note.nullifier(sk);
+
+        mapNoteData_t noteData;
+        JSOutPoint jsoutpt {wtx.GetHash(), 0, 1};
+        CNoteData nd {sk.address(), nullifier};
+        noteData[jsoutpt] = nd;
+        wtx.SetNoteData(noteData);
+        wallet.AddToWallet(wtx, true, NULL);
+
+        std::vector<JSOutPoint> notes {jsoutpt};
+        std::vector<boost::optional<ZCIncrementalWitness>> witnesses;
+        uint256 anchor3;
+
+        // Third block
+        block3.vtx.push_back(wtx);
+        index3.nHeight = 3;
+        wallet.IncrementNoteWitnesses(&index3, &block3, tree);
+        wallet.GetNoteWitnesses(notes, witnesses, anchor3);
+
+        // Now pretend we are reindexing: the chain is cleared, and each block is
+        // used to increment witnesses again.
+        wallet.IncrementNoteWitnesses(&index1, &block1, tree);
+        uint256 anchor3a;
+        witnesses.clear();
+        wallet.GetNoteWitnesses(notes, witnesses, anchor3a);
+        EXPECT_TRUE((bool) witnesses[0]);
+        // Should equal third anchor because witness cache unaffected
+        EXPECT_EQ(anchor3, anchor3a);
+
+        wallet.IncrementNoteWitnesses(&index2, &block2, tree);
+        uint256 anchor3b;
+        witnesses.clear();
+        wallet.GetNoteWitnesses(notes, witnesses, anchor3b);
+        EXPECT_TRUE((bool) witnesses[0]);
+        EXPECT_EQ(anchor3, anchor3b);
+
+        // Pretend a reorg happened that was recorded in the block files
+        wallet.DecrementNoteWitnesses(&index2);
+        uint256 anchor3c;
+        witnesses.clear();
+        wallet.GetNoteWitnesses(notes, witnesses, anchor3c);
+        EXPECT_TRUE((bool) witnesses[0]);
+        EXPECT_EQ(anchor3, anchor3c);
+
+        wallet.IncrementNoteWitnesses(&index2, &block2, tree);
+        uint256 anchor3d;
+        witnesses.clear();
+        wallet.GetNoteWitnesses(notes, witnesses, anchor3d);
+        EXPECT_TRUE((bool) witnesses[0]);
+        EXPECT_EQ(anchor3, anchor3d);
+
+        wallet.IncrementNoteWitnesses(&index3, &block3, tree);
+        uint256 anchor3e;
+        witnesses.clear();
+        wallet.GetNoteWitnesses(notes, witnesses, anchor3e);
+        EXPECT_TRUE((bool) witnesses[0]);
+        EXPECT_EQ(anchor3, anchor3e);
+    }
+}
+
 TEST(wallet_tests, ClearNoteWitnessCache) {
     TestWallet wallet;
 
@@ -798,6 +916,7 @@ TEST(wallet_tests, ClearNoteWitnessCache) {
 TEST(wallet_tests, WriteWitnessCache) {
     TestWallet wallet;
     MockWalletDB walletdb;
+    CBlockLocator loc;
 
     auto sk = libzcash::SpendingKey::random();
     wallet.AddSpendingKey(sk);
@@ -808,7 +927,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));
 
@@ -817,14 +936,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));
 
@@ -833,26 +952,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).
     }
 }
 
@@ -757,16 +756,19 @@ void CWallet::DecrementNoteWitnesses(const CBlockIndex* pindex)
                 CNoteData* nd = &(item.second);
                 // Check the validity of the cache
                 assert(nWitnessCacheSize >= nd->witnesses.size());
-                // Witnesses being decremented should always be either -1
-                // (never incremented or decremented) or equal to pindex
-                assert((nd->witnessHeight == -1) ||
-                       (nd->witnessHeight == pindex->nHeight));
-                if (nd->witnesses.size() > 0) {
-                    nd->witnesses.pop_front();
+                // Only increment witnesses that are not above the current height
+                if (nd->witnessHeight <= pindex->nHeight) {
+                    // Witnesses being decremented should always be either -1
+                    // (never incremented or decremented) or equal to pindex
+                    assert((nd->witnessHeight == -1) ||
+                           (nd->witnessHeight == pindex->nHeight));
+                    if (nd->witnesses.size() > 0) {
+                        nd->witnesses.pop_front();
+                    }
+                    // pindex is the block being removed, so the new witness cache
+                    // height is one below it.
+                    nd->witnessHeight = pindex->nHeight - 1;
                 }
-                // pindex is the block being removed, so the new witness cache
-                // height is one below it.
-                nd->witnessHeight = pindex->nHeight - 1;
             }
         }
         nWitnessCacheSize -= 1;
@@ -779,10 +781,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 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     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;
         }
     }
@@ -954,6 +959,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const;
     CAmount GetChange(const CTransaction& tx) const;
     void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, ZCIncrementalMerkleTree tree, bool added);
+    /** Saves witness caches and best block locator to disk. */
     void SetBestChain(const CBlockLocator& loc);
 
     DBErrors LoadWallet(bool& fFirstRunRet);