From fb2653ed0eee430e10f95ae1f1424120de17de4a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Oct 2016 15:13:28 -0500 Subject: [PATCH 1/7] Add unit test that fails when calling FindMyNotes on a locked wallet --- src/wallet/gtest/test_wallet.cpp | 40 +++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 70f4d5c6d..fc7255b41 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -35,6 +35,14 @@ class TestWallet : public CWallet { public: TestWallet() : CWallet() { } + bool EncryptKeys(CKeyingMaterial& vMasterKeyIn) { + return CCryptoKeyStore::EncryptKeys(vMasterKeyIn); + } + + bool Unlock(const CKeyingMaterial& vMasterKeyIn) { + return CCryptoKeyStore::Unlock(vMasterKeyIn); + } + void IncrementNoteWitnesses(const CBlockIndex* pindex, const CBlock* pblock, ZCIncrementalMerkleTree tree) { @@ -382,7 +390,7 @@ TEST(wallet_tests, set_invalid_note_addrs_in_cwallettx) { EXPECT_THROW(wtx.SetNoteData(noteData), std::logic_error); } -TEST(wallet_tests, find_note_in_tx) { +TEST(wallet_tests, FindMyNotes) { CWallet wallet; auto sk = libzcash::SpendingKey::random(); @@ -401,6 +409,36 @@ TEST(wallet_tests, find_note_in_tx) { EXPECT_EQ(nd, noteMap[jsoutpt]); } +TEST(wallet_tests, FindMyNotesInEncryptedWallet) { + 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); + + auto noteMap = wallet.FindMyNotes(wtx); + EXPECT_EQ(2, noteMap.size()); + + JSOutPoint jsoutpt {wtx.GetHash(), 0, 1}; + CNoteData nd {sk.address(), nullifier}; + EXPECT_EQ(1, noteMap.count(jsoutpt)); + EXPECT_NE(nd, noteMap[jsoutpt]); + + ASSERT_TRUE(wallet.Unlock(vMasterKey)); + + noteMap = wallet.FindMyNotes(wtx); + EXPECT_EQ(2, noteMap.size()); + EXPECT_EQ(1, noteMap.count(jsoutpt)); + EXPECT_EQ(nd, noteMap[jsoutpt]); +} + TEST(wallet_tests, get_conflicted_notes) { CWallet wallet; From 52fdce985f25689ce0b86c76b7f740369de46e64 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Oct 2016 17:11:29 -0500 Subject: [PATCH 2/7] Add RPC test showing correct handling of JS txns from blockchain For unencrypted wallets, the wallet correctly tracks JoinSplits made for their zkey in a different wallet. --- qa/pull-tester/rpc-tests.sh | 1 + qa/rpc-tests/wallet_nullifiers.py | 141 ++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+) create mode 100755 qa/rpc-tests/wallet_nullifiers.py diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index e75999199..5292eb1e8 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -12,6 +12,7 @@ export BITCOIND=${REAL_BITCOIND} testScripts=( 'wallet.py' + 'wallet_nullifiers.py' 'listtransactions.py' 'mempool_resurrect_test.py' 'txn_doublespend.py' diff --git a/qa/rpc-tests/wallet_nullifiers.py b/qa/rpc-tests/wallet_nullifiers.py new file mode 100755 index 000000000..c90fab77f --- /dev/null +++ b/qa/rpc-tests/wallet_nullifiers.py @@ -0,0 +1,141 @@ +#!/usr/bin/env python2 +# Copyright (c) 2016 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import * +from time import * + +class WalletNullifiersTest (BitcoinTestFramework): + + def run_test (self): + # add zaddr to node 0 + myzaddr0 = self.nodes[0].z_getnewaddress() + + # send node 0 taddr to zaddr to get out of coinbase + mytaddr = self.nodes[0].getnewaddress(); + recipients = [] + recipients.append({"address":myzaddr0, "amount":10.0}) + myopid = self.nodes[0].z_sendmany(mytaddr, recipients) + + opids = [] + opids.append(myopid) + + timeout = 120 + status = None + for x in xrange(1, timeout): + results = self.nodes[0].z_getoperationresult(opids) + if len(results)==0: + sleep(1) + else: + status = results[0]["status"] + assert_equal("success", status) + mytxid = results[0]["result"]["txid"] + break + + self.nodes[0].generate(1) + self.sync_all() + + # add zaddr to node 2 + myzaddr = self.nodes[2].z_getnewaddress() + + # import node 2 zaddr into node 1 + myzkey = self.nodes[2].z_exportkey(myzaddr) + self.nodes[1].z_importkey(myzkey) + + # send node 0 zaddr to note 2 zaddr + recipients = [] + recipients.append({"address":myzaddr, "amount":7.0}) + myopid = self.nodes[0].z_sendmany(myzaddr0, recipients) + + opids = [] + opids.append(myopid) + + timeout = 120 + status = None + for x in xrange(1, timeout): + results = self.nodes[0].z_getoperationresult(opids) + if len(results)==0: + sleep(1) + else: + status = results[0]["status"] + assert_equal("success", status) + mytxid = results[0]["result"]["txid"] + break + + self.nodes[0].generate(1) + self.sync_all() + + # check zaddr balance + zsendmanynotevalue = Decimal('7.0') + assert_equal(self.nodes[2].z_getbalance(myzaddr), zsendmanynotevalue) + assert_equal(self.nodes[1].z_getbalance(myzaddr), zsendmanynotevalue) + + # add zaddr to node 3 + myzaddr3 = self.nodes[3].z_getnewaddress() + + # send node 2 zaddr to note 3 zaddr + recipients = [] + recipients.append({"address":myzaddr3, "amount":2.0}) + myopid = self.nodes[2].z_sendmany(myzaddr, recipients) + + opids = [] + opids.append(myopid) + + timeout = 120 + status = None + for x in xrange(1, timeout): + results = self.nodes[2].z_getoperationresult(opids) + if len(results)==0: + sleep(1) + else: + status = results[0]["status"] + assert_equal("success", status) + mytxid = results[0]["result"]["txid"] + break + + self.nodes[2].generate(1) + self.sync_all() + + # check zaddr balance + zsendmany2notevalue = Decimal('2.0') + zsendmanyfee = Decimal('0.0001') + 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) + + # send node 2 zaddr on node 1 to taddr + mytaddr1 = self.nodes[1].getnewaddress(); + recipients = [] + recipients.append({"address":mytaddr1, "amount":1.0}) + myopid = self.nodes[1].z_sendmany(myzaddr, recipients) + + opids = [] + opids.append(myopid) + + timeout = 120 + status = None + for x in xrange(1, timeout): + results = self.nodes[1].z_getoperationresult(opids) + if len(results)==0: + sleep(1) + else: + status = results[0]["status"] + assert_equal("success", status) + mytxid = results[0]["result"]["txid"] + break + + self.nodes[1].generate(1) + self.sync_all() + + # check zaddr balance + zsendmany3notevalue = Decimal('1.0') + zaddrremaining2 = zaddrremaining - zsendmany3notevalue - zsendmanyfee + assert_equal(self.nodes[1].z_getbalance(myzaddr), zaddrremaining2) + assert_equal(self.nodes[2].z_getbalance(myzaddr), zaddrremaining2) + +if __name__ == '__main__': + WalletNullifiersTest().main () From 8f445ee774606f60c812af548b5afc8002842303 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Oct 2016 17:57:24 -0500 Subject: [PATCH 3/7] Break the RPC test by encrypting the mirroring wallet --- qa/rpc-tests/wallet_nullifiers.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/qa/rpc-tests/wallet_nullifiers.py b/qa/rpc-tests/wallet_nullifiers.py index c90fab77f..64d9c3892 100755 --- a/qa/rpc-tests/wallet_nullifiers.py +++ b/qa/rpc-tests/wallet_nullifiers.py @@ -45,6 +45,16 @@ class WalletNullifiersTest (BitcoinTestFramework): myzkey = self.nodes[2].z_exportkey(myzaddr) self.nodes[1].z_importkey(myzkey) + # encrypt node 1 wallet and wait to terminate + self.nodes[1].encryptwallet("test") + bitcoind_processes[1].wait() + + # restart node 1 + self.nodes[1] = start_node(1, self.options.tmpdir) + connect_nodes_bi(self.nodes, 0, 1) + connect_nodes_bi(self.nodes, 1, 2) + self.sync_all() + # send node 0 zaddr to note 2 zaddr recipients = [] recipients.append({"address":myzaddr, "amount":7.0}) @@ -108,6 +118,8 @@ class WalletNullifiersTest (BitcoinTestFramework): assert_equal(self.nodes[1].z_getbalance(myzaddr), zaddrremaining) # send node 2 zaddr on node 1 to taddr + # This requires that node 1 be unlocked + self.nodes[1].walletpassphrase("test", 600) mytaddr1 = self.nodes[1].getnewaddress(); recipients = [] recipients.append({"address":mytaddr1, "amount":1.0}) From 1a62587e9af30e458a12a5aad5b0dd8873e479ca Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Oct 2016 21:49:14 -0500 Subject: [PATCH 4/7] Delay caching of nullifiers when wallet is locked Closes #1502 --- qa/rpc-tests/wallet_nullifiers.py | 13 ++++- src/wallet/gtest/test_wallet.cpp | 66 ++++++++++++++++++++++ src/wallet/rpcwallet.cpp | 1 + src/wallet/wallet.cpp | 91 ++++++++++++++++++++++++++----- src/wallet/wallet.h | 78 ++++++++++++++++++++++++-- 5 files changed, 226 insertions(+), 23 deletions(-) diff --git a/qa/rpc-tests/wallet_nullifiers.py b/qa/rpc-tests/wallet_nullifiers.py index 64d9c3892..dcb634347 100755 --- a/qa/rpc-tests/wallet_nullifiers.py +++ b/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) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index fc7255b41..5b485be15 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/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; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 747682136..7775fc771 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1877,6 +1877,7 @@ Value walletpassphrase(const Array& params, bool fHelp) "walletpassphrase \n" "Stores the wallet decryption key in memory for seconds."); + pwalletMain->UpdateNullifierNoteMap(); pwalletMain->TopUpKeyPool(); int64_t nSleepTime = params[1].get_int64(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3777f62a4..69c30b24d 100644 --- a/src/wallet/wallet.cpp +++ b/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& 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 CWallet::GetNoteNullifier(const JSDescription& jsdesc, + const libzcash::PaymentAddress& address, + const ZCNoteDecryption& dec, + const uint256& hSig, + uint8_t n) const +{ + boost::optional 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 & outEntries, st } // skip note which has been spent - if (ignoreSpent && IsSpent(nd.nullifier)) { + if (ignoreSpent && nd.nullifier && IsSpent(*nd.nullifier)) { continue; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index af54bf2cd..015da56e5 100644 --- a/src/wallet/wallet.h +++ b/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 nullifier; /** * Cached incremental witnesses for spendable Notes. @@ -215,6 +221,7 @@ public: std::list 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 mapNullifiersToNotes; + std::map mapWallet; int64_t nOrderPosNext; @@ -810,6 +869,7 @@ public: TxItems OrderedTxItems(std::list& 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 GetAccountAddresses(std::string strAccount) const; + boost::optional 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( From ddea44a27e9f5609751cc3dd0ccbe6b915d65c4e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 13 Oct 2016 16:56:27 -0500 Subject: [PATCH 5/7] Update comments --- qa/rpc-tests/wallet_nullifiers.py | 2 +- src/wallet/wallet.h | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/qa/rpc-tests/wallet_nullifiers.py b/qa/rpc-tests/wallet_nullifiers.py index dcb634347..7efd70f6a 100755 --- a/qa/rpc-tests/wallet_nullifiers.py +++ b/qa/rpc-tests/wallet_nullifiers.py @@ -119,7 +119,7 @@ class WalletNullifiersTest (BitcoinTestFramework): # 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 (#) + # TODO: Devise a way to avoid this issue (#1528) assert_equal(self.nodes[1].z_getbalance(myzaddr), zsendmanynotevalue + zaddrremaining) # send node 2 zaddr on node 1 to taddr diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 015da56e5..fafad6d12 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -208,9 +208,10 @@ public: * * 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. + * If the wallet is encrypted, this means that someone with access to the + * locked wallet cannot spend notes, but can connect received notes to the + * transactions they are spent in. This is the same security semantics as + * for transparent addresses. */ boost::optional nullifier; @@ -758,9 +759,6 @@ public: * * - 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 mapNullifiersToNotes; From a581fe2aaef4bdd7f3c3969b0ce719642f0201d9 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 14 Oct 2016 09:03:59 -0500 Subject: [PATCH 6/7] Only ignore runtime errors caused by failed note decryption --- src/wallet/gtest/test_wallet.cpp | 8 +++++++- src/wallet/wallet.cpp | 7 ++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 5b485be15..6939ae2b7 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -425,13 +425,19 @@ TEST(wallet_tests, FindMyNotes) { CWallet wallet; auto sk = libzcash::SpendingKey::random(); - wallet.AddSpendingKey(sk); + auto sk2 = libzcash::SpendingKey::random(); + wallet.AddSpendingKey(sk2); auto wtx = GetValidReceive(sk, 10, true); auto note = GetNote(sk, wtx, 0, 1); auto nullifier = note.nullifier(sk); auto noteMap = wallet.FindMyNotes(wtx); + EXPECT_EQ(0, noteMap.size()); + + wallet.AddSpendingKey(sk); + + noteMap = wallet.FindMyNotes(wtx); EXPECT_EQ(2, noteMap.size()); JSOutPoint jsoutpt {wtx.GetHash(), 0, 1}; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 69c30b24d..70c87dad5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1187,7 +1187,12 @@ mapNoteData_t CWallet::FindMyNotes(const CTransaction& tx) const noteData.insert(std::make_pair(jsoutpt, nd)); } break; - } catch (const std::runtime_error &) { + } catch (const std::runtime_error &err) { + if (memcmp("Could not decrypt message", err.what(), 25) != 0) { + // Unexpected failure + LogPrintf("FindMyNotes(): Unexpected runtime error while testing decrypt:\n"); + LogPrintf("%s\n", err.what()); + } // else // Couldn't decrypt with this decryptor } catch (const std::exception &exc) { // Unexpected failure From 6e263a5fd3a6933d0525f07c8991368dc43e0fde Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 16 Oct 2016 16:26:51 -0500 Subject: [PATCH 7/7] Address review comments --- src/wallet/rpcwallet.cpp | 1 + src/wallet/wallet.cpp | 11 +++++++---- src/wallet/wallet.h | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7775fc771..4d6608570 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1877,6 +1877,7 @@ Value walletpassphrase(const Array& params, bool fHelp) "walletpassphrase \n" "Stores the wallet decryption key in memory for seconds."); + // No need to check return values, because the wallet was unlocked above pwalletMain->UpdateNullifierNoteMap(); pwalletMain->TopUpKeyPool(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 70c87dad5..5e98e8dc2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -890,13 +890,16 @@ bool CWallet::UpdateNullifierNoteMap() item.first.n); } } - UpdateNullifierNoteMap(wtxItem.second); + UpdateNullifierNoteMapWithTx(wtxItem.second); } } return true; } -void CWallet::UpdateNullifierNoteMap(const CWalletTx& wtx) +/** + * Update mapNullifiersToNotes with the cached nullifiers in this tx. + */ +void CWallet::UpdateNullifierNoteMapWithTx(const CWalletTx& wtx) { { LOCK(cs_wallet); @@ -916,7 +919,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD { mapWallet[hash] = wtxIn; mapWallet[hash].BindWallet(this); - UpdateNullifierNoteMap(mapWallet[hash]); + UpdateNullifierNoteMapWithTx(mapWallet[hash]); AddToSpends(hash); } else @@ -926,7 +929,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD pair::iterator, bool> ret = mapWallet.insert(make_pair(hash, wtxIn)); CWalletTx& wtx = (*ret.first).second; wtx.BindWallet(this); - UpdateNullifierNoteMap(wtx); + UpdateNullifierNoteMapWithTx(wtx); bool fInsertedNew = ret.second; if (fInsertedNew) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index fafad6d12..42d985045 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -868,7 +868,7 @@ public: void MarkDirty(); bool UpdateNullifierNoteMap(); - void UpdateNullifierNoteMap(const CWalletTx& wtx); + void UpdateNullifierNoteMapWithTx(const CWalletTx& wtx); bool AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletDB* pwalletdb); void SyncTransaction(const CTransaction& tx, const CBlock* pblock); bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pblock, bool fUpdate);