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 <noreply@anthropic.com>

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);
+}

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();
         });
 

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 ||
+            if ( nDepth < minDepth || nDepth > maxDepth) {
-                wtx.GetDepthInMainChain() > maxDepth) {
                 continue;
             }
         }
         nTxs++;
 
-        for (auto & pair : wtx.mapSaplingNoteData) {
+        for (const auto & pair : wtx.mapSaplingNoteData) {
-            SaplingOutPoint op = pair.first;
+            const SaplingOutPoint& op = pair.first;
-            SaplingNoteData nd = pair.second;
+            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 });
         }
     }