From 03f83b9b0d95e07524649b65ed3b37e75a13ac76 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 30 Nov 2016 14:04:37 +1300 Subject: [PATCH 1/4] 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 | 8 +++---- src/primitives/block.h | 4 ++++ src/wallet/gtest/test_wallet.cpp | 40 ++++++++++++++++++++++++-------- src/wallet/wallet.cpp | 17 +++++++------- src/wallet/wallet.h | 17 +++++++++----- 5 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index dcb317b7d..ee0b27613 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -537,11 +537,6 @@ void ThreadImport(std::vector 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); diff --git a/src/primitives/block.h b/src/primitives/block.h index 6180fb2ae..6b3f13a86 100644 --- a/src/primitives/block.h +++ b/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 diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index d30944954..0530bdb53 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/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& walletdb); +template void CWallet::SetBestChainINTERNAL( + 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) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 51ed76f3a..c9dac701d 100644 --- a/src/wallet/wallet.cpp +++ b/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). } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 85f960eb1..cc0197f14 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -636,35 +636,40 @@ protected: void DecrementNoteWitnesses(const CBlockIndex* pindex); template - 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& 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; } } From d85758f5cc144382cf0e268450a72a07d4f26778 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 7 Dec 2016 19:30:30 +1300 Subject: [PATCH 2/4] Document behaviour of CWallet::SetBestChain --- src/wallet/wallet.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index cc0197f14..3d721fc40 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -959,6 +959,7 @@ public: 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); From eeee6d5d6c1edb0eb2559d9d5f914dc580d7ef9d Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 10 Dec 2016 00:51:32 +1300 Subject: [PATCH 3/4] Add a reindex test that fails because of a bug in decrementing witness caches Ref: https://github.com/zcash/zcash/pull/1904#issuecomment-265992988 --- src/wallet/gtest/test_wallet.cpp | 115 +++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 0530bdb53..eaee37f48 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -871,6 +871,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 notes {jsoutpt}; + std::vector> 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; From 9d2cc3a78445c5256bc0d6c20884edc5cffc742f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 10 Dec 2016 00:56:32 +1300 Subject: [PATCH 4/4] Make the test pass by fixing the bug! --- src/wallet/wallet.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c9dac701d..b424b8def 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -756,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;