diff --git a/src/gtest/test_wallet_notecache.cpp b/src/gtest/test_wallet_notecache.cpp index c0006dfc8..7ae9e18a8 100644 --- a/src/gtest/test_wallet_notecache.cpp +++ b/src/gtest/test_wallet_notecache.cpp @@ -240,9 +240,13 @@ protected: lockedOp = SaplingOutPoint(tx2, 0); wallet.LockNote(lockedOp); + AddMoreTxs(); Mine(); } + // Lets a test add txs before the chain is built. + virtual void AddMoreTxs() {} + void TearDown() override { LOCK(cs_main); chainActive.SetTip(nullptr); @@ -362,3 +366,37 @@ TEST_F(WalletNoteCache, FixtureExercisesEveryFilter) { TEST_F(WalletNoteCache, FilteredNotesMatchLegacy) { ExpectEquivalent({0, 1}); } + +// Two notes recorded under A's viewing key that it can't actually decrypt (real outputs to someone +// else): output 0 is spent by a mined tx, output 1 is locked. If the walk decrypted either one, the +// decrypt assert would abort. +class WalletNoteCacheUndecryptable : public WalletNoteCache { +protected: + uint256 txBad; + + void AddMoreTxs() override { + SaplingPaymentAddress foreign = TestKey(9).DefaultAddress(); + CTransaction tx = MakeTx({MakeOutput(foreign, 1), MakeOutput(foreign, 2)}); + txBad = tx.GetHash(); + const SaplingIncomingViewingKey ivkA = TestKey(0).expsk.full_viewing_key().in_viewing_key(); + const uint256 nf = uint256S("bad5"); + + CWalletTx wtx(&wallet, tx); + wtx.mapSaplingNoteData[SaplingOutPoint(txBad, 0)] = SaplingNoteData(ivkA, nf); + wtx.mapSaplingNoteData[SaplingOutPoint(txBad, 1)] = SaplingNoteData(ivkA); + EXPECT_TRUE(wallet.AddToWallet(wtx, true, nullptr)); + depthOf[txBad] = 4; + txids.push_back(txBad); + + AddTx(MakeTx({}, {nf}), 4); + wallet.LockNote(SaplingOutPoint(txBad, 1)); + } +}; + +// Spent and locked notes are dropped before the decryption (~0.5-0.8ms per note), not after it. +TEST_F(WalletNoteCacheUndecryptable, SpentAndLockedNotesAreNotDecrypted) { + auto entries = Filtered({}, 1, INT_MAX, true, false, true); + EXPECT_EQ(7u, entries.size()) << "the fixture's 8 notes less tx2's locked one"; + for (const SaplingNoteEntry& e : entries) + EXPECT_FALSE(e.op.hash == txBad); +} diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 0ea4fa1df..22ff1ec6c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4662,7 +4662,7 @@ UniValue z_listreceivedbyaddress(const UniValue& params, bool fHelp, const CPubK } obj.push_back(Pair("outindex", (int)entry.op.n)); obj.push_back(Pair("rawconfirmations", entry.confirmations)); - auto wtx = pwalletMain->mapWallet.at(entry.op.hash); //.ToString()); + const CWalletTx& wtx = pwalletMain->mapWallet.at(entry.op.hash); obj.push_back(Pair("time", wtx.GetTxTime())); obj.push_back(Pair("confirmations", dpowconfs)); @@ -5376,7 +5376,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp, const CPubKey& mypk) // sort notes from largest to smallest, which means // we will spend the largest first std::sort(saplingEntries.begin(), saplingEntries.end(), - [](SaplingNoteEntry i, SaplingNoteEntry j) -> bool { + [](const SaplingNoteEntry& i, const SaplingNoteEntry& j) -> bool { return i.note.value() > j.note.value(); }); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 20565003d..482118474 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5758,35 +5758,54 @@ void CWallet::GetFilteredNotes( unsigned int nTxs = 0, nNotes = 0, nSpent = 0; for (auto & p : mapWallet) { - CWalletTx wtx = p.second; + // By reference: a copy deep-copies every note's witness list (up to WITNESS_CACHE_SIZE + // witnesses each), and it was made before any filter ran. + const CWalletTx& wtx = p.second; + + // Most wallet txs (coinbase, sends from other wallets) carry no notes of ours. + if (wtx.mapSaplingNoteData.empty()) + continue; // Filter the transactions before checking for notes if (!CheckFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0) continue; + // cs_main is held, so the depth can't change during the walk. + const int nDepth = wtx.GetDepthInMainChain(); if (minDepth > 1) { int nHeight = tx_height(wtx.GetHash()); if ( nHeight == 0 ) { continue; } - int nDepth = wtx.GetDepthInMainChain(); int dpowconfs = hush_dpowconfs(nHeight,nDepth); if ( dpowconfs < minDepth || dpowconfs > maxDepth) { continue; } } else { - if ( wtx.GetDepthInMainChain() < minDepth || - wtx.GetDepthInMainChain() > maxDepth) { + if ( nDepth < minDepth || nDepth > maxDepth) { continue; } } nTxs++; - for (auto & pair : wtx.mapSaplingNoteData) { - SaplingOutPoint op = pair.first; - SaplingNoteData nd = pair.second; + for (const auto & pair : wtx.mapSaplingNoteData) { + const SaplingOutPoint& op = pair.first; + const SaplingNoteData& nd = pair.second; nNotes++; + // The cheap filters run before the decryption, which costs ~0.5-0.8ms of Jubjub + // arithmetic per note. Spent notes stay in the wallet (-deletetx is off by default). + if (ignoreSpent && nd.nullifier && IsSaplingSpent(*nd.nullifier)) { + nSpent++; + continue; + } + + // skip locked notes + if (ignoreLocked && IsLockedNote(op)) { + LogPrint("zrpcunsafe", "%s: skipping locked note %s\n", __func__, op.hash.ToString().substr(0,10).c_str()); + continue; + } + auto maybe_pt = SaplingNotePlaintext::decrypt( wtx.vShieldedOutput[op.n].encCiphertext, nd.ivk, @@ -5804,11 +5823,6 @@ void CWallet::GetFilteredNotes( continue; } - if (ignoreSpent && nd.nullifier && IsSaplingSpent(*nd.nullifier)) { - nSpent++; - continue; - } - // skip notes which cannot be spent if (requireSpendingKey) { libzcash::SaplingIncomingViewingKey ivk; @@ -5820,15 +5834,9 @@ void CWallet::GetFilteredNotes( } } - // skip locked notes - if (ignoreLocked && IsLockedNote(op)) { - LogPrint("zrpcunsafe", "%s: skipping locked note %s\n", __func__, op.hash.ToString().substr(0,10).c_str()); - continue; - } - auto note = notePt.note(nd.ivk).get(); saplingEntries.push_back(SaplingNoteEntry { - op, pa, note, notePt.memo(), wtx.GetDepthInMainChain() }); + op, pa, note, notePt.memo(), nDepth }); } }