From 1a62587e9af30e458a12a5aad5b0dd8873e479ca Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Oct 2016 21:49:14 -0500 Subject: [PATCH] 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(