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..7efd70f6a --- /dev/null +++ b/qa/rpc-tests/wallet_nullifiers.py @@ -0,0 +1,162 @@ +#!/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) + + # 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}) + 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) + + # 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 (#1528) + 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, which triggers caching of + # uncached nullifiers. + self.nodes[1].walletpassphrase("test", 600) + 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 + # 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) + assert_equal(self.nodes[2].z_getbalance(myzaddr), zaddrremaining2) + +if __name__ == '__main__': + WalletNullifiersTest().main () diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 70f4d5c6d..6939ae2b7 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,12 +390,72 @@ 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, 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; + + auto sk = libzcash::SpendingKey::random(); + 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}; + CNoteData nd {sk.address(), nullifier}; + EXPECT_EQ(1, noteMap.count(jsoutpt)); + 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); @@ -398,6 +466,13 @@ TEST(wallet_tests, find_note_in_tx) { 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]); } @@ -757,6 +832,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..4d6608570 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1877,6 +1877,8 @@ 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(); int64_t nSleepTime = params[1].get_int64(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3777f62a4..5e98e8dc2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -863,12 +863,50 @@ void CWallet::MarkDirty() } } -void CWallet::UpdateNullifierNoteMap(const CWalletTx& wtx) +/** + * 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); + } + } + UpdateNullifierNoteMapWithTx(wtxItem.second); + } + } + return true; +} + +/** + * Update mapNullifiersToNotes with the cached nullifiers in this tx. + */ +void CWallet::UpdateNullifierNoteMapWithTx(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; + } } } } @@ -881,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 @@ -891,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) { @@ -1092,6 +1130,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 +1170,33 @@ 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 + } 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 LogPrintf("FindMyNotes(): Unexpected error while testing decrypt:\n"); @@ -3335,7 +3404,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..42d985045 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -201,12 +201,19 @@ 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 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; /** * Cached incremental witnesses for spendable Notes. @@ -215,6 +222,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 +712,56 @@ 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). + */ std::map mapNullifiersToNotes; + std::map mapWallet; int64_t nOrderPosNext; @@ -810,7 +867,8 @@ public: TxItems OrderedTxItems(std::list& acentries, std::string strAccount = ""); void MarkDirty(); - void UpdateNullifierNoteMap(const CWalletTx& wtx); + bool UpdateNullifierNoteMap(); + 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); @@ -850,6 +908,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(