Auto merge of #1939 - str4d:1933-fix-bug, r=str4d

Only increment new notes on reindex

Addresses another issue in #1904.

When an existing one of our notes was found again, its cache was reset and it was re-witnessed. This would cause encountered notes to get out-of-sync with the otherwise-ignored newer notes, which could be a problem if the wallet data happens to be written out during a reindex.

doc/payment-api.md

@@ -151,7 +151,7 @@ RPC_WALLET_ERROR (-4) | _Unspecified problem with wallet_
 ----------------------| -------------------------------------
 "Could not find previous JoinSplit anchor" | Try restarting node with `-reindex`.
 "Error decrypting output note of previous JoinSplit: __"  |
-"Could not find witness for note commitment" | Try restarting node with `-reindex`.
+"Could not find witness for note commitment" | Try restarting node with `-rescan`.
 "Witness for note commitment is null" | Missing witness for note commitement.
 "Witness for spendable note does not have same anchor as change input" | Invalid anchor for spendable note witness.
 "Not enough funds to pay miners fee" | Retry with sufficient funds.

src/wallet/gtest/test_wallet.cpp

@@ -51,7 +51,7 @@ public:
 
     void IncrementNoteWitnesses(const CBlockIndex* pindex,
                                 const CBlock* pblock,
-                                ZCIncrementalMerkleTree tree) {
+                                ZCIncrementalMerkleTree& tree) {
         CWallet::IncrementNoteWitnesses(pindex, pblock, tree);
     }
     void DecrementNoteWitnesses(const CBlockIndex* pindex) {
@@ -82,6 +82,28 @@ CWalletTx GetValidSpend(const libzcash::SpendingKey& sk,
     return GetValidSpend(*params, sk, note, value);
 }
 
+JSOutPoint CreateValidBlock(TestWallet& wallet,
+                            const libzcash::SpendingKey& sk,
+                            const CBlockIndex& index,
+                            CBlock& block,
+                            ZCIncrementalMerkleTree& tree) {
+    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);
+
+    block.vtx.push_back(wtx);
+    wallet.IncrementNoteWitnesses(&index, &block, tree);
+
+    return jsoutpt;
+}
+
 TEST(wallet_tests, setup_datadir_location_run_as_first_test) {
     // Get temporary and unique path for file.
     boost::filesystem::path pathTemp = boost::filesystem::temp_directory_path() / boost::filesystem::unique_path();
@@ -572,27 +594,14 @@ TEST(wallet_tests, cached_witnesses_chain_tip) {
     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);
-
-        std::vector<JSOutPoint> notes {jsoutpt};
-        std::vector<boost::optional<ZCIncrementalWitness>> witnesses;
-
         // First block (case tested in _empty_chain)
-        block1.vtx.push_back(wtx);
         CBlockIndex index1(block1);
         index1.nHeight = 1;
-        wallet.IncrementNoteWitnesses(&index1, &block1, tree);
+        auto jsoutpt = CreateValidBlock(wallet, sk, index1, block1, tree);
+
         // Called to fetch anchor
+        std::vector<JSOutPoint> notes {jsoutpt};
+        std::vector<boost::optional<ZCIncrementalWitness>> witnesses;
         wallet.GetNoteWitnesses(notes, witnesses, anchor1);
     }
 
@@ -667,47 +676,21 @@ TEST(wallet_tests, CachedWitnessesDecrementFirst) {
     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)
         CBlock block1;
-        block1.vtx.push_back(wtx);
         CBlockIndex index1(block1);
         index1.nHeight = 1;
-        wallet.IncrementNoteWitnesses(&index1, &block1, tree);
+        CreateValidBlock(wallet, sk, index1, block1, tree);
     }
 
     {
-        // Second transaction (case tested in _chain_tip)
+        // Second block (case tested in _chain_tip)
-        auto wtx = GetValidReceive(sk, 50, true);
+        index2.nHeight = 2;
-        auto note = GetNote(sk, wtx, 0, 1);
+        auto jsoutpt = CreateValidBlock(wallet, sk, index2, block2, tree);
-        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);
 
+        // Called to fetch anchor
         std::vector<JSOutPoint> notes {jsoutpt};
         std::vector<boost::optional<ZCIncrementalWitness>> witnesses;
-
-        // Second block (case tested in _chain_tip)
-        block2.vtx.push_back(wtx);
-        index2.nHeight = 2;
-        wallet.IncrementNoteWitnesses(&index2, &block2, tree);
-        // Called to fetch anchor
         wallet.GetNoteWitnesses(notes, witnesses, anchor2);
     }
 
@@ -753,116 +736,77 @@ TEST(wallet_tests, CachedWitnessesDecrementFirst) {
 
 TEST(wallet_tests, CachedWitnessesCleanIndex) {
     TestWallet wallet;
-    CBlock block1;
+    std::vector<CBlock> blocks;
-    CBlock block2;
+    std::vector<CBlockIndex> indices;
-    CBlock block3;
+    std::vector<JSOutPoint> notes;
-    CBlockIndex index1(block1);
+    std::vector<uint256> anchors;
-    CBlockIndex index2(block2);
-    CBlockIndex index3(block3);
     ZCIncrementalMerkleTree tree;
+    ZCIncrementalMerkleTree riTree = tree;
+    std::vector<boost::optional<ZCIncrementalWitness>> witnesses;
 
     auto sk = libzcash::SpendingKey::random();
     wallet.AddSpendingKey(sk);
 
-    {
+    // Generate a chain
-        // First transaction (case tested in _empty_chain)
+    size_t numBlocks = WITNESS_CACHE_SIZE + 10;
-        auto wtx = GetValidReceive(sk, 10, true);
+    blocks.resize(numBlocks);
-        auto note = GetNote(sk, wtx, 0, 1);
+    indices.resize(numBlocks);
-        auto nullifier = note.nullifier(sk);
+    for (size_t i = 0; i < numBlocks; i++) {
+        indices[i].nHeight = i;
+        auto old = tree.root();
+        auto jsoutpt = CreateValidBlock(wallet, sk, indices[i], blocks[i], tree);
+        EXPECT_NE(old, tree.root());
+        notes.push_back(jsoutpt);
 
-        mapNoteData_t noteData;
+        witnesses.clear();
-        JSOutPoint jsoutpt {wtx.GetHash(), 0, 1};
+        uint256 anchor;
-        CNoteData nd {sk.address(), nullifier};
+        wallet.GetNoteWitnesses(notes, witnesses, anchor);
-        noteData[jsoutpt] = nd;
+        for (size_t j = 0; j <= i; j++) {
-        wtx.SetNoteData(noteData);
+            EXPECT_TRUE((bool) witnesses[j]);
-        wallet.AddToWallet(wtx, true, NULL);
+        }
-
+        anchors.push_back(anchor);
-        // First block (case tested in _empty_chain)
-        block1.vtx.push_back(wtx);
-        index1.nHeight = 1;
-        wallet.IncrementNoteWitnesses(&index1, &block1, tree);
     }
 
-    {
+    // Now pretend we are reindexing: the chain is cleared, and each block is
-        // Second transaction (case tested in _chain_tip)
+    // used to increment witnesses again.
-        auto wtx = GetValidReceive(sk, 50, true);
+    for (size_t i = 0; i < numBlocks; i++) {
-        auto note = GetNote(sk, wtx, 0, 1);
+        ZCIncrementalMerkleTree riPrevTree {riTree};
-        auto nullifier = note.nullifier(sk);
+        wallet.IncrementNoteWitnesses(&(indices[i]), &(blocks[i]), riTree);
-
-        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);
+        uint256 anchor;
-        EXPECT_TRUE((bool) witnesses[0]);
+        wallet.GetNoteWitnesses(notes, witnesses, anchor);
-        // Should equal third anchor because witness cache unaffected
+        for (size_t j = 0; j < numBlocks; j++) {
-        EXPECT_EQ(anchor3, anchor3a);
+            EXPECT_TRUE((bool) witnesses[j]);
+        }
+        // Should equal final anchor because witness cache unaffected
+        EXPECT_EQ(anchors.back(), anchor);
 
-        wallet.IncrementNoteWitnesses(&index2, &block2, tree);
+        if ((i == 5) || (i == 50)) {
-        uint256 anchor3b;
+            // Pretend a reorg happened that was recorded in the block files
-        witnesses.clear();
+            {
-        wallet.GetNoteWitnesses(notes, witnesses, anchor3b);
+                wallet.DecrementNoteWitnesses(&(indices[i]));
-        EXPECT_TRUE((bool) witnesses[0]);
+                witnesses.clear();
-        EXPECT_EQ(anchor3, anchor3b);
+                uint256 anchor;
+                wallet.GetNoteWitnesses(notes, witnesses, anchor);
+                for (size_t j = 0; j < numBlocks; j++) {
+                    EXPECT_TRUE((bool) witnesses[j]);
+                }
+                // Should equal final anchor because witness cache unaffected
+                EXPECT_EQ(anchors.back(), anchor);
+            }
 
-        // Pretend a reorg happened that was recorded in the block files
+            {
-        wallet.DecrementNoteWitnesses(&index2);
+                wallet.IncrementNoteWitnesses(&(indices[i]), &(blocks[i]), riPrevTree);
-        uint256 anchor3c;
+                witnesses.clear();
-        witnesses.clear();
+                uint256 anchor;
-        wallet.GetNoteWitnesses(notes, witnesses, anchor3c);
+                wallet.GetNoteWitnesses(notes, witnesses, anchor);
-        EXPECT_TRUE((bool) witnesses[0]);
+                for (size_t j = 0; j < numBlocks; j++) {
-        EXPECT_EQ(anchor3, anchor3c);
+                    EXPECT_TRUE((bool) witnesses[j]);
-
+                }
-        wallet.IncrementNoteWitnesses(&index2, &block2, tree);
+                // Should equal final anchor because witness cache unaffected
-        uint256 anchor3d;
+                EXPECT_EQ(anchors.back(), anchor);
-        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);
     }
 }
 

src/wallet/wallet.cpp

@@ -704,7 +704,8 @@ void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex,
                     // If this is our note, witness it
                     if (txIsOurs) {
                         JSOutPoint jsoutpt {hash, i, j};
-                        if (mapWallet[hash].mapNoteData.count(jsoutpt)) {
+                        if (mapWallet[hash].mapNoteData.count(jsoutpt) &&
+                                mapWallet[hash].mapNoteData[jsoutpt].witnessHeight < pindex->nHeight) {
                             CNoteData* nd = &(mapWallet[hash].mapNoteData[jsoutpt]);
                             if (nd->witnesses.size() > 0) {
                                 // We think this can happen because we write out the