From 34b94e761ac254f49d9efe30681ec5eeac77c40b Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 15 Oct 2026 14:45:49 -0500 Subject: [PATCH] wallet: don't copy every encrypted Sapling key on each lookup CCryptoKeyStore::HaveSaplingSpendingKey and GetSaplingSpendingKey scan mapCryptedSaplingSpendingKeys with `for (auto entry : ...)`. That copies each entry (an extended full viewing key plus its encrypted-secret vector) for every key visited, on every call. GetFilteredNotes' requireSpendingKey check makes one such call per note, and the history RPCs make one per output per address: ~22ns per key per call, measured (11us per note on a 1024-key wallet). Iterate by const reference. The equivalence test gains a locked, encrypted-keystore variant, LockedWalletMatchesLegacy, so spendability coming from the encrypted map is covered too. Co-Authored-By: Claude Opus 5.5 --- src/gtest/test_wallet_notecache.cpp | 24 +++++++++++++++++++++++- src/wallet/crypter.cpp | 2 +- src/wallet/crypter.h | 2 +- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/gtest/test_wallet_notecache.cpp b/src/gtest/test_wallet_notecache.cpp index dd54dea47..8a9357e63 100644 --- a/src/gtest/test_wallet_notecache.cpp +++ b/src/gtest/test_wallet_notecache.cpp @@ -116,6 +116,11 @@ void LegacyFilteredNotes( } } +class NoteCacheWallet : public CWallet { +public: + bool EncryptKeys(CKeyingMaterial& vMasterKeyIn) { return CCryptoKeyStore::EncryptKeys(vMasterKeyIn); } +}; + std::array MemoFor(uint64_t value) { std::array memo; @@ -194,7 +199,7 @@ class WalletNoteCache : public ::testing::Test { protected: static constexpr int TIP_HEIGHT = 30; - CWallet wallet; + NoteCacheWallet wallet; SaplingPaymentAddress paA, paB, paC; // A and C spendable, B viewing key only std::map depthOf; // txid -> depth to mine it at (absent: never mined) std::vector txids; @@ -430,3 +435,20 @@ TEST_F(WalletNoteCacheUndecryptable, SpentAndLockedNotesAreNotDecrypted) { for (const SaplingNoteEntry& e : entries) EXPECT_FALSE(e.op.hash == txBad); } + +// The same wallet with its keystore encrypted and locked: spendability then comes from the +// encrypted-key map (CCryptoKeyStore::HaveSaplingSpendingKey), and nothing else may change. +class WalletNoteCacheEncrypted : public WalletNoteCache { +protected: + void AddMoreTxs() override { + CKeyingMaterial masterKey(32, 0x42); + ASSERT_TRUE(wallet.EncryptKeys(masterKey)); + ASSERT_TRUE(wallet.IsCrypted()); + ASSERT_TRUE(wallet.IsLocked()); + } +}; + +TEST_F(WalletNoteCacheEncrypted, LockedWalletMatchesLegacy) { + EXPECT_EQ(6u, Filtered({}, 1, INT_MAX, true, true, false).size()) << "A's and C's keys are still known"; + ExpectEquivalent({1, 11}); +} diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index fa29dba19..336b07f3a 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -591,7 +591,7 @@ bool CCryptoKeyStore::GetSaplingSpendingKey(const libzcash::SaplingFullViewingKe if (!IsCrypted()) return CBasicKeyStore::GetSaplingSpendingKey(fvk, skOut); - for (auto entry : mapCryptedSaplingSpendingKeys) { + for (const auto& entry : mapCryptedSaplingSpendingKeys) { if (entry.first.fvk == fvk) { const std::vector &vchCryptedSecret = entry.second; return DecryptSaplingSpendingKey(vMasterKey, vchCryptedSecret, entry.first, skOut); diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index d79a1b34b..282e39a85 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -265,7 +265,7 @@ public: LOCK(cs_SpendingKeyStore); if (!IsCrypted()) return CBasicKeyStore::HaveSaplingSpendingKey(fvk); - for (auto entry : mapCryptedSaplingSpendingKeys) { + for (const auto& entry : mapCryptedSaplingSpendingKeys) { if (entry.first.fvk == fvk) { return true; }