From bfc13345658665c281ef1474f6858fff7539bbf6 Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 15 Oct 2026 14:37:55 -0500 Subject: [PATCH] wallet: walk GetFilteredNotes by reference, cheap filters first GetFilteredNotes copied every CWalletTx by value before any filter ran, then copied each SaplingNoteData by value again. Both copies deep-copy the note's witness list (up to WITNESS_CACHE_SIZE witnesses, ~100KB per note on a mainnet wallet): ~40us per note per pass, measured. Spent and locked notes were also fully decrypted (~0.6ms of Jubjub arithmetic per note) before the spent and locked checks threw them away. -deletetx is off by default, so spent notes accumulate. - iterate mapWallet and mapSaplingNoteData by const reference; - skip txs with no notes of ours before CheckFinalTx; - read GetDepthInMainChain once per tx instead of 2+N times (cs_main is held, so it can't change during the walk); - run the spent and locked checks before the decryption; - z_listreceivedbyaddress no longer copies the whole CWalletTx per entry to read its time, and z_sendmany's sort comparator takes its entries by reference. The filters have no side effects, so the result set and its order are unchanged; the A0 equivalence test passes unchanged. One deliberate difference: a spent or locked note whose ciphertext no longer decrypts under its ivk is now skipped instead of tripping the decrypt assert. The new SpentAndLockedNotesAreNotDecrypted test pins this (it aborts on the old ordering). Co-Authored-By: Claude Opus 5.5 --- src/gtest/test_wallet_notecache.cpp | 38 ++++++++++++++++++++++++ src/wallet/rpcwallet.cpp | 4 +-- src/wallet/wallet.cpp | 46 +++++++++++++++++------------ 3 files changed, 67 insertions(+), 21 deletions(-) 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 }); } }