Delay caching of nullifiers when wallet is locked

Closes #1502

qa/rpc-tests/wallet_nullifiers.py

@@ -115,10 +115,16 @@ class WalletNullifiersTest (BitcoinTestFramework):
         zaddrremaining = zsendmanynotevalue - zsendmany2notevalue - zsendmanyfee
         assert_equal(self.nodes[3].z_getbalance(myzaddr3), zsendmany2notevalue)
         assert_equal(self.nodes[2].z_getbalance(myzaddr), zaddrremaining)
-        assert_equal(self.nodes[1].z_getbalance(myzaddr), zaddrremaining)
+
+        # Parallel encrypted wallet can't cache nullifiers for received notes,
+        # and therefore can't detect spends. So it sees a balance corresponding
+        # to the sum of both notes it received (one as change).
+        # TODO: Devise a way to avoid this issue (#)
+        assert_equal(self.nodes[1].z_getbalance(myzaddr), zsendmanynotevalue + zaddrremaining)
 
         # send node 2 zaddr on node 1 to taddr
-        # This requires that node 1 be unlocked
+        # This requires that node 1 be unlocked, which triggers caching of
+        # uncached nullifiers.
         self.nodes[1].walletpassphrase("test", 600)
         mytaddr1 = self.nodes[1].getnewaddress();
         recipients = []
@@ -144,6 +150,9 @@ class WalletNullifiersTest (BitcoinTestFramework):
         self.sync_all()
 
         # check zaddr balance
+        # Now that the encrypted wallet has been unlocked, the note nullifiers
+        # have been cached and spent notes can be detected. Thus the two wallets
+        # are in agreement once more.
         zsendmany3notevalue = Decimal('1.0')
         zaddrremaining2 = zaddrremaining - zsendmany3notevalue - zsendmanyfee
         assert_equal(self.nodes[1].z_getbalance(myzaddr), zaddrremaining2)

src/wallet/gtest/test_wallet.cpp

@@ -390,6 +390,37 @@ TEST(wallet_tests, set_invalid_note_addrs_in_cwallettx) {
     EXPECT_THROW(wtx.SetNoteData(noteData), std::logic_error);
 }
 
+TEST(wallet_tests, GetNoteNullifier) {
+    CWallet wallet;
+
+    auto sk = libzcash::SpendingKey::random();
+    auto address = sk.address();
+    auto dec = ZCNoteDecryption(sk.viewing_key());
+
+    auto wtx = GetValidReceive(sk, 10, true);
+    auto note = GetNote(sk, wtx, 0, 1);
+    auto nullifier = note.nullifier(sk);
+
+    auto hSig = wtx.vjoinsplit[0].h_sig(
+        *params, wtx.joinSplitPubKey);
+
+    auto ret = wallet.GetNoteNullifier(
+        wtx.vjoinsplit[0],
+        address,
+        dec,
+        hSig, 1);
+    EXPECT_NE(nullifier, ret);
+
+    wallet.AddSpendingKey(sk);
+
+    ret = wallet.GetNoteNullifier(
+        wtx.vjoinsplit[0],
+        address,
+        dec,
+        hSig, 1);
+    EXPECT_EQ(nullifier, ret);
+}
+
 TEST(wallet_tests, FindMyNotes) {
     CWallet wallet;
 
@@ -795,6 +826,41 @@ TEST(wallet_tests, WriteWitnessCache) {
     wallet.WriteWitnessCache(walletdb);
 }
 
+TEST(wallet_tests, UpdateNullifierNoteMap) {
+    TestWallet wallet;
+    uint256 r {GetRandHash()};
+    CKeyingMaterial vMasterKey (r.begin(), r.end());
+
+    auto sk = libzcash::SpendingKey::random();
+    wallet.AddSpendingKey(sk);
+
+    ASSERT_TRUE(wallet.EncryptKeys(vMasterKey));
+
+    auto wtx = GetValidReceive(sk, 10, true);
+    auto note = GetNote(sk, wtx, 0, 1);
+    auto nullifier = note.nullifier(sk);
+
+    // Pretend that we called FindMyNotes while the wallet was locked
+    mapNoteData_t noteData;
+    JSOutPoint jsoutpt {wtx.GetHash(), 0, 1};
+    CNoteData nd {sk.address()};
+    noteData[jsoutpt] = nd;
+    wtx.SetNoteData(noteData);
+
+    wallet.AddToWallet(wtx, true, NULL);
+    EXPECT_EQ(0, wallet.mapNullifiersToNotes.count(nullifier));
+
+    EXPECT_FALSE(wallet.UpdateNullifierNoteMap());
+
+    ASSERT_TRUE(wallet.Unlock(vMasterKey));
+
+    EXPECT_TRUE(wallet.UpdateNullifierNoteMap());
+    EXPECT_EQ(1, wallet.mapNullifiersToNotes.count(nullifier));
+    EXPECT_EQ(wtx.GetHash(), wallet.mapNullifiersToNotes[nullifier].hash);
+    EXPECT_EQ(0, wallet.mapNullifiersToNotes[nullifier].js);
+    EXPECT_EQ(1, wallet.mapNullifiersToNotes[nullifier].n);
+}
+
 TEST(wallet_tests, UpdatedNoteData) {
     TestWallet wallet;
 

src/wallet/rpcwallet.cpp

@@ -1877,6 +1877,7 @@ Value walletpassphrase(const Array& params, bool fHelp)
             "walletpassphrase <passphrase> <timeout>\n"
             "Stores the wallet decryption key in memory for <timeout> seconds.");
 
+    pwalletMain->UpdateNullifierNoteMap();
     pwalletMain->TopUpKeyPool();
 
     int64_t nSleepTime = params[1].get_int64();

src/wallet/wallet.cpp

@@ -863,12 +863,47 @@ void CWallet::MarkDirty()
     }
 }
 
+/**
+ * Ensure that every note in the wallet has a cached nullifier.
+ */
+bool CWallet::UpdateNullifierNoteMap()
+{
+    {
+        LOCK(cs_wallet);
+
+        if (IsLocked())
+            return false;
+
+        ZCNoteDecryption dec;
+        for (std::pair<const uint256, CWalletTx>& wtxItem : mapWallet) {
+            for (mapNoteData_t::value_type& item : wtxItem.second.mapNoteData) {
+                if (!item.second.nullifier) {
+                    auto i = item.first.js;
+                    GetNoteDecryptor(item.second.address, dec);
+                    auto hSig = wtxItem.second.vjoinsplit[i].h_sig(
+                        *pzcashParams, wtxItem.second.joinSplitPubKey);
+                    item.second.nullifier = GetNoteNullifier(
+                        wtxItem.second.vjoinsplit[i],
+                        item.second.address,
+                        dec,
+                        hSig,
+                        item.first.n);
+                }
+            }
+            UpdateNullifierNoteMap(wtxItem.second);
+        }
+    }
+    return true;
+}
+
 void CWallet::UpdateNullifierNoteMap(const CWalletTx& wtx)
 {
     {
         LOCK(cs_wallet);
         for (const mapNoteData_t::value_type& item : wtx.mapNoteData) {
-            mapNullifiersToNotes[item.second.nullifier] = item.first;
+            if (item.second.nullifier) {
+                mapNullifiersToNotes[*item.second.nullifier] = item.first;
+            }
         }
     }
 }
@@ -1092,6 +1127,32 @@ void CWallet::EraseFromWallet(const uint256 &hash)
 }
 
 
+/**
+ * Returns a nullifier if the SpendingKey is available
+ * Throws std::runtime_error if the decryptor doesn't match this note
+ */
+boost::optional<uint256> CWallet::GetNoteNullifier(const JSDescription& jsdesc,
+                                                   const libzcash::PaymentAddress& address,
+                                                   const ZCNoteDecryption& dec,
+                                                   const uint256& hSig,
+                                                   uint8_t n) const
+{
+    boost::optional<uint256> ret;
+    auto note_pt = libzcash::NotePlaintext::decrypt(
+        dec,
+        jsdesc.ciphertexts[n],
+        jsdesc.ephemeralKey,
+        hSig,
+        (unsigned char) n);
+    auto note = note_pt.note(address);
+    // SpendingKeys are only available if the wallet is unlocked
+    libzcash::SpendingKey key;
+    if (GetSpendingKey(address, key)) {
+        ret = note.nullifier(key);
+    }
+    return ret;
+}
+
 /**
  * Finds all output notes in the given transaction that have been sent to
  * PaymentAddresses in this wallet.
@@ -1106,28 +1167,28 @@ mapNoteData_t CWallet::FindMyNotes(const CTransaction& tx) const
     uint256 hash = tx.GetHash();
 
     mapNoteData_t noteData;
-    libzcash::SpendingKey key;
     for (size_t i = 0; i < tx.vjoinsplit.size(); i++) {
         auto hSig = tx.vjoinsplit[i].h_sig(*pzcashParams, tx.joinSplitPubKey);
         for (uint8_t j = 0; j < tx.vjoinsplit[i].ciphertexts.size(); j++) {
             for (const NoteDecryptorMap::value_type& item : mapNoteDecryptors) {
                 try {
-                    auto note_pt = libzcash::NotePlaintext::decrypt(
-                        item.second,
-                        tx.vjoinsplit[i].ciphertexts[j],
-                        tx.vjoinsplit[i].ephemeralKey,
-                        hSig,
-                        (unsigned char) j);
                     auto address = item.first;
-                    // Decryptors are only cached when SpendingKeys are added
-                    assert(GetSpendingKey(address, key));
-                    auto note = note_pt.note(address);
                     JSOutPoint jsoutpt {hash, i, j};
-                    CNoteData nd {address, note.nullifier(key)};
-                    noteData.insert(std::make_pair(jsoutpt, nd));
+                    auto nullifier = GetNoteNullifier(
+                        tx.vjoinsplit[i],
+                        address,
+                        item.second,
+                        hSig, j);
+                    if (nullifier) {
+                        CNoteData nd {address, *nullifier};
+                        noteData.insert(std::make_pair(jsoutpt, nd));
+                    } else {
+                        CNoteData nd {address};
+                        noteData.insert(std::make_pair(jsoutpt, nd));
+                    }
                     break;
                 } catch (const std::runtime_error &) {
-                    // Couldn't decrypt with this spending key
+                    // Couldn't decrypt with this decryptor
                 } catch (const std::exception &exc) {
                     // Unexpected failure
                     LogPrintf("FindMyNotes(): Unexpected error while testing decrypt:\n");
@@ -3335,7 +3396,7 @@ void CWallet::GetFilteredNotes(std::vector<CNotePlaintextEntry> & outEntries, st
             }
 
             // skip note which has been spent
-            if (ignoreSpent && IsSpent(nd.nullifier)) {
+            if (ignoreSpent && nd.nullifier && IsSpent(*nd.nullifier)) {
                 continue;
             }
 

src/wallet/wallet.h

@@ -201,12 +201,18 @@ class CNoteData
 public:
     libzcash::PaymentAddress address;
 
-    // It's okay to cache the nullifier in the wallet, because we are storing
-    // the spending key there too, which could be used to derive this.
-    // If PR #1210 is merged, we need to revisit the threat model and decide
-    // whether it is okay to store this unencrypted while the spending key is
-    // encrypted.
-    uint256 nullifier;
+    /**
+     * Cached note nullifier. May not be set if the wallet was not unlocked when
+     * this was CNoteData was created. If not set, we always assume that the
+     * note has not been spent.
+     *
+     * It's okay to cache the nullifier in the wallet, because we are storing
+     * the spending key there too, which could be used to derive this.
+     * If PR #1210 is merged, we need to revisit the threat model and decide
+     * whether it is okay to store this unencrypted while the spending key is
+     * encrypted.
+     */
+    boost::optional<uint256> nullifier;
 
     /**
      * Cached incremental witnesses for spendable Notes.
@@ -215,6 +221,7 @@ public:
     std::list<ZCIncrementalWitness> witnesses;
 
     CNoteData() : address(), nullifier() { }
+    CNoteData(libzcash::PaymentAddress a) : address {a}, nullifier() { }
     CNoteData(libzcash::PaymentAddress a, uint256 n) : address {a}, nullifier {n} { }
 
     ADD_SERIALIZE_METHODS;
@@ -704,7 +711,59 @@ public:
         nWitnessCacheSize = 0;
     }
 
+    /**
+     * The reverse mapping of nullifiers to notes.
+     *
+     * The mapping cannot be updated while an encrypted wallet is locked,
+     * because we need the SpendingKey to create the nullifier (#1502). This has
+     * several implications for transactions added to the wallet while locked:
+     *
+     * - Parent transactions can't be marked dirty when a child transaction that
+     *   spends their output notes is updated.
+     *
+     *   - We currently don't cache any note values, so this is not a problem,
+     *     yet.
+     *
+     * - GetFilteredNotes can't filter out spent notes.
+     *
+     *   - Per the comment in CNoteData, we assume that if we don't have a
+     *     cached nullifier, the note is not spent.
+     *
+     * Another more problematic implication is that the wallet can fail to
+     * detect transactions on the blockchain that spend our notes. There are two
+     * possible cases in which this could happen:
+     *
+     * - We receive a note when the wallet is locked, and then spend it using a
+     *   different wallet client.
+     *
+     * - We spend from a PaymentAddress we control, then we export the
+     *   SpendingKey and import it into a new wallet, and reindex/rescan to find
+     *   the old transactions.
+     *
+     * The wallet will only miss "pure" spends - transactions that are only
+     * linked to us by the fact that they contain notes we spent. If it also
+     * sends notes to us, or interacts with our transparent addresses, we will
+     * detect the transaction and add it to the wallet (again without caching
+     * nullifiers for new notes). As by default JoinSplits send change back to
+     * the origin PaymentAddress, the wallet should rarely miss transactions.
+     *
+     * To work around these issues, whenever the wallet is unlocked, we scan all
+     * cached notes, and cache any missing nullifiers. Since the wallet must be
+     * unlocked in order to spend notes, this means that GetFilteredNotes will
+     * always behave correctly within that context (and any other uses will give
+     * correct responses afterwards), for the transactions that the wallet was
+     * able to detect. Any missing transactions can be rediscovered by:
+     *
+     * - Unlocking the wallet (to fill all nullifier caches).
+     *
+     * - Restarting the node with -reindex (which operates on a locked wallet
+     *   but with the now-cached nullifiers).
+     *
+     * Several rounds of this may be required to incrementally fill the
+     * nullifier caches of discovered notes.
+     */
     std::map<uint256, JSOutPoint> mapNullifiersToNotes;
+
     std::map<uint256, CWalletTx> mapWallet;
 
     int64_t nOrderPosNext;
@@ -810,6 +869,7 @@ public:
     TxItems OrderedTxItems(std::list<CAccountingEntry>& acentries, std::string strAccount = "");
 
     void MarkDirty();
+    bool UpdateNullifierNoteMap();
     void UpdateNullifierNoteMap(const CWalletTx& wtx);
     bool AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletDB* pwalletdb);
     void SyncTransaction(const CTransaction& tx, const CBlock* pblock);
@@ -850,6 +910,12 @@ public:
 
     std::set<CTxDestination> GetAccountAddresses(std::string strAccount) const;
 
+    boost::optional<uint256> GetNoteNullifier(
+        const JSDescription& jsdesc,
+        const libzcash::PaymentAddress& address,
+        const ZCNoteDecryption& dec,
+        const uint256& hSig,
+        uint8_t n) const;
     mapNoteData_t FindMyNotes(const CTransaction& tx) const;
     bool IsFromMe(const uint256& nullifier) const;
     void GetNoteWitnesses(