Pass ZCIncrementalMerkleTree to wallet to prevent race conditions

8 years ago · de42390f90
6 changed files with 29 additions and 42 deletions
--- a/src/main.cpp
+++ b/src/main.cpp
@ -2398,13 +2398,16 @@ bool static DisconnectTip(CValidationState &state) {
    mempool.check(pcoinsTip);
    // Update chainActive and related variables.
    UpdateTip(pindexDelete->pprev);
+    // Get the current commitment tree
+    ZCIncrementalMerkleTree newTree;
+    assert(pcoinsTip->GetAnchorAt(pcoinsTip->GetBestAnchor(), newTree));
    // Let wallets know transactions went from 1-confirmed to
    // 0-confirmed or conflicted:
    BOOST_FOREACH(const CTransaction &tx, block.vtx) {
        SyncWithWallets(tx, NULL);
    }
    // Update cached incremental witnesses
-    GetMainSignals().ChainTip(pindexDelete, &block, false);
+    GetMainSignals().ChainTip(pindexDelete, &block, newTree, false);
    return true;
 }

@ -2429,6 +2432,9 @@ bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, CBlock *
            return AbortNode(state, "Failed to read block");
        pblock = &block;
    }
+    // Get the current commitment tree
+    ZCIncrementalMerkleTree oldTree;
+    assert(pcoinsTip->GetAnchorAt(pcoinsTip->GetBestAnchor(), oldTree));
    // Apply the block atomically to the chain state.
    int64_t nTime2 = GetTimeMicros(); nTimeReadFromDisk += nTime2 - nTime1;
    int64_t nTime3;
@ -2471,7 +2477,7 @@ bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, CBlock *
        SyncWithWallets(tx, pblock);
    }
    // Update cached incremental witnesses
-    GetMainSignals().ChainTip(pindexNew, pblock, true);
+    GetMainSignals().ChainTip(pindexNew, pblock, oldTree, true);

    int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1;
    LogPrint("bench", "  - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001);
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@ -16,7 +16,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) {
    g_signals.SyncTransaction.connect(boost::bind(&CValidationInterface::SyncTransaction, pwalletIn, _1, _2));
    g_signals.EraseTransaction.connect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1));
    g_signals.UpdatedTransaction.connect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1));
-    g_signals.ChainTip.connect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3));
+    g_signals.ChainTip.connect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3, _4));
    g_signals.SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1));
    g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1));
    g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1));
@ -27,7 +27,7 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) {
    g_signals.BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
    g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1));
    g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1));
-    g_signals.ChainTip.disconnect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3));
+    g_signals.ChainTip.disconnect(boost::bind(&CValidationInterface::ChainTip, pwalletIn, _1, _2, _3, _4));
    g_signals.SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1));
    g_signals.UpdatedTransaction.disconnect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1));
    g_signals.EraseTransaction.disconnect(boost::bind(&CValidationInterface::EraseFromWallet, pwalletIn, _1));
--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@ -8,6 +8,8 @@

 #include <boost/signals2/signal.hpp>

+#include "zcash/IncrementalMerkleTree.hpp"
+
 class CBlock;
 class CBlockIndex;
 struct CBlockLocator;
@ -31,7 +33,7 @@ class CValidationInterface {
 protected:
    virtual void SyncTransaction(const CTransaction &tx, const CBlock *pblock) {}
    virtual void EraseFromWallet(const uint256 &hash) {}
-    virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, bool added) {}
+    virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, ZCIncrementalMerkleTree tree, bool added) {}
    virtual void SetBestChain(const CBlockLocator &locator) {}
    virtual void UpdatedTransaction(const uint256 &hash) {}
    virtual void Inventory(const uint256 &hash) {}
@ -50,7 +52,7 @@ struct CMainSignals {
    /** Notifies listeners of an updated transaction without new data (for now: a coinbase potentially becoming visible). */
    boost::signals2::signal<void (const uint256 &)> UpdatedTransaction;
    /** Notifies listeners of a change to the tip of the active block chain. */
-    boost::signals2::signal<void (const CBlockIndex *, const CBlock *, bool)> ChainTip;
+    boost::signals2::signal<void (const CBlockIndex *, const CBlock *, ZCIncrementalMerkleTree, bool)> ChainTip;
    /** Notifies listeners of a new active block chain. */
    boost::signals2::signal<void (const CBlockLocator &)> SetBestChain;
    /** Notifies listeners about an inventory item being seen on the network. */
--- a/src/wallet/gtest/test_wallet.cpp
+++ b/src/wallet/gtest/test_wallet.cpp
@ -29,8 +29,8 @@ public:

    void IncrementNoteWitnesses(const CBlockIndex* pindex,
                                const CBlock* pblock,
-                                const CCoinsViewCache* pcoins) {
-        CWallet::IncrementNoteWitnesses(pindex, pblock, pcoins);
+                                ZCIncrementalMerkleTree tree) {
+        CWallet::IncrementNoteWitnesses(pindex, pblock, tree);
    }
    void DecrementNoteWitnesses() {
        CWallet::DecrementNoteWitnesses();
@ -328,10 +328,8 @@ TEST(wallet_tests, cached_witnesses_empty_chain) {

    CBlock block;
    block.vtx.push_back(wtx);
-    MockCCoinsViewCache coins;
-    // Empty chain, so we shouldn't try to fetch an anchor
-    EXPECT_CALL(coins, GetAnchorAt(_, _)).Times(0);
-    wallet.IncrementNoteWitnesses(NULL, &block, &coins);
+    ZCIncrementalMerkleTree tree;
+    wallet.IncrementNoteWitnesses(NULL, &block, tree);
    witnesses.clear();
    wallet.GetNoteWitnesses(notes, witnesses, anchor);
    EXPECT_TRUE((bool) witnesses[0]);
@ -344,9 +342,9 @@ TEST(wallet_tests, cached_witnesses_empty_chain) {

 TEST(wallet_tests, cached_witnesses_chain_tip) {
    TestWallet wallet;
-    MockCCoinsViewCache coins;
    uint256 anchor1;
    CBlock block1;
+    ZCIncrementalMerkleTree tree;

    auto sk = libzcash::SpendingKey::random();
    wallet.AddSpendingKey(sk);
@ -369,9 +367,7 @@ TEST(wallet_tests, cached_witnesses_chain_tip) {

        // First block (case tested in _empty_chain)
        block1.vtx.push_back(wtx);
-        EXPECT_CALL(coins, GetAnchorAt(_, _))
-            .Times(0);
-        wallet.IncrementNoteWitnesses(NULL, &block1, &coins);
+        wallet.IncrementNoteWitnesses(NULL, &block1, tree);
        // Called to fetch anchor
        wallet.GetNoteWitnesses(notes, witnesses, anchor1);
    }
@ -400,10 +396,8 @@ TEST(wallet_tests, cached_witnesses_chain_tip) {
        CBlock block2;
        block2.hashPrevBlock = block1.GetHash();
        block2.vtx.push_back(wtx);
-        EXPECT_CALL(coins, GetAnchorAt(anchor1, _))
-            .Times(2)
-            .WillRepeatedly(Return(true));
-        wallet.IncrementNoteWitnesses(NULL, &block2, &coins);
+        ZCIncrementalMerkleTree tree2 {tree};
+        wallet.IncrementNoteWitnesses(NULL, &block2, tree2);
        witnesses.clear();
        wallet.GetNoteWitnesses(notes, witnesses, anchor2);
        EXPECT_TRUE((bool) witnesses[0]);
@ -419,7 +413,7 @@ TEST(wallet_tests, cached_witnesses_chain_tip) {

        // Re-incrementing with the same block should give the same result
        uint256 anchor4;
-        wallet.IncrementNoteWitnesses(NULL, &block2, &coins);
+        wallet.IncrementNoteWitnesses(NULL, &block2, tree);
        witnesses.clear();
        wallet.GetNoteWitnesses(notes, witnesses, anchor4);
        EXPECT_TRUE((bool) witnesses[0]);
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@ -336,10 +336,11 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
    return false;
 }

-void CWallet::ChainTip(const CBlockIndex *pindex, const CBlock *pblock, bool added)
+void CWallet::ChainTip(const CBlockIndex *pindex, const CBlock *pblock,
+                       ZCIncrementalMerkleTree tree, bool added)
 {
    if (added) {
-        IncrementNoteWitnesses(pindex, pblock, pcoinsTip);
+        IncrementNoteWitnesses(pindex, pblock, tree);
    } else {
        DecrementNoteWitnesses();
    }
@ -594,7 +595,7 @@ void CWallet::AddToSpends(const uint256& wtxid)

 void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex,
                                     const CBlock* pblockIn,
-                                     const CCoinsViewCache* pcoins)
+                                     ZCIncrementalMerkleTree tree)
 {
    {
        LOCK(cs_wallet);
@ -618,23 +619,7 @@ void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex,
            pblock = &block;
        }

-        ZCIncrementalMerkleTree tree;
-        bool treeInitialised = false;
        for (const CTransaction& tx : pblock->vtx) {
-            if (!treeInitialised && tx.vjoinsplit.size() > 0) {
-                LOCK(cs_main);
-                // vAnchorCache will only be empty at the beginning
-                if (vAnchorCache.size() && !pcoins->GetAnchorAt(vAnchorCache.front(), tree)) {
-                    // This should not happen, because IncrementNoteWitnesses()
-                    // is only called when the chain tip updates, and the
-                    // anchors for the JoinSplits in that block should still be
-                    // cached.
-                    // TODO: Calculate the anchor from scratch?
-                    throw std::runtime_error("CWallet::IncrementNoteWitnesses(): anchor not cached");
-                }
-                treeInitialised = true;
-            }
-
            auto hash = tx.GetTxid();
            bool txIsOurs = mapWallet.count(hash);
            for (size_t i = 0; i < tx.vjoinsplit.size(); i++) {
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@ -588,7 +588,7 @@ public:
 protected:
    void IncrementNoteWitnesses(const CBlockIndex* pindex,
                                const CBlock* pblock,
-                                const CCoinsViewCache* pcoins);
+                                ZCIncrementalMerkleTree tree);
    void DecrementNoteWitnesses();

 private:
@ -810,7 +810,7 @@ public:
    CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
    CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const;
    CAmount GetChange(const CTransaction& tx) const;
-    void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, bool added);
+    void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, ZCIncrementalMerkleTree tree, bool added);
    void SetBestChain(const CBlockLocator& loc);

    DBErrors LoadWallet(bool& fFirstRunRet);