gtest: notecache review fixes

- Take LOCK2(cs_main, cs_wallet) before adding keys in SetUp: both
  AddSaplingZKey and AddSaplingIncomingViewingKey assert cs_wallet is
  held, which aborts a DEBUG_LOCKORDER build.
- TIP_HEIGHT becomes a namespace-scope constexpr. As a static class
  member it was ODR-used by EXPECT_EQ without a definition, which fails
  to link as C++11 at -O0.
- TearDown restores the previous chainActive tip and erases only the
  mapBlockIndex entries the fixture inserted.
- Add a tx mined only in a stale side block. The legacy tx_height model
  returns that block's height, as a txindex can, and the tx must still
  be excluded at every minDepth.
- The encrypted variant asserts HaveSaplingSpendingKey directly, and a
  new test unlocks the keystore and checks GetSaplingSpendingKey (the
  other loop the crypter change touched).
- The legacy-loop comment now says its two LogPrintf lines are dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/gtest/test_wallet_notecache.cpp

@@ -28,9 +28,10 @@ namespace {
 
 typedef std::function<int(const uint256&)> TxHeightFn;
 
-// CWallet::GetFilteredNotes as of 69bb035e7, verbatim except that tx_height() is passed in. This is
+// CWallet::GetFilteredNotes as of 69bb035e7, verbatim except that tx_height() is passed in and its two
-// the reference the optimized walk must reproduce entry for entry and in the same order (z_sendmany
+// LogPrintf lines are dropped. This is the reference the optimized walk must reproduce entry for
-// sorts the result with an unstable sort, so the order is observable).
+// entry and in the same order (z_sendmany sorts the result with an unstable sort, so the order is
+// observable).
 void LegacyFilteredNotes(
     CWallet& wallet,
     std::vector<SaplingNoteEntry>& saplingEntries,
@@ -119,8 +120,13 @@ void LegacyFilteredNotes(
 class NoteCacheWallet : public CWallet {
 public:
     bool EncryptKeys(CKeyingMaterial& vMasterKeyIn) { return CCryptoKeyStore::EncryptKeys(vMasterKeyIn); }
+    bool Unlock(const CKeyingMaterial& vMasterKeyIn) { return CCryptoKeyStore::Unlock(vMasterKeyIn); }
 };
 
+// The fake active chain is 0..TIP_HEIGHT; STALE_HEIGHT is a side block forked off it.
+constexpr int TIP_HEIGHT = 30;
+constexpr int STALE_HEIGHT = 25;
+
 std::array<unsigned char, HUSH_MEMO_SIZE> MemoFor(uint64_t value)
 {
     std::array<unsigned char, HUSH_MEMO_SIZE> memo;
@@ -192,13 +198,11 @@ void ExpectSameEntries(const std::vector<SaplingNoteEntry>& want, const std::vec
 }
 
 // A wallet with notes at several depths, spent and unspent, locked and unlocked, spendable and
-// viewing-key-only, plus txs that carry no wallet notes; mined into a fake active chain. Everything it
+// viewing-key-only, one mined only in a stale block, plus txs that carry no wallet notes; mined into a
-// touches in the process-wide chain state (chainActive, mapBlockIndex, pcoinsTip) is undone in
+// fake active chain. TearDown restores the process-wide chain state it changed (the chainActive tip,
-// TearDown, because every gtest suite runs in the same process.
+// its own mapBlockIndex entries, pcoinsTip), because every gtest suite runs in the same process.
 class WalletNoteCache : public ::testing::Test {
 protected:
-    static constexpr int TIP_HEIGHT = 30;
-
     NoteCacheWallet wallet;
     SaplingPaymentAddress paA, paB, paC;    // A and C spendable, B viewing key only
     std::map<uint256, int> depthOf;         // txid -> depth to mine it at (absent: never mined)
@@ -210,13 +214,21 @@ protected:
     CCoinsViewCache* savedCoinsTip = nullptr;
     std::deque<CBlock> blocks;
     std::deque<CBlockIndex> index;
-    std::map<uint256, int> minedHeight;     // txid -> height of the block holding it
+    std::vector<uint256> insertedBlocks;    // mapBlockIndex keys this fixture added
+    CBlockIndex* savedTip = nullptr;
+    std::map<uint256, int> minedHeight;     // txid -> height of the block holding it (stale ones too)
+    uint256 staleTx;                        // only ever mined in the side block at STALE_HEIGHT
 
     void SetUp() override {
         // tx_height()/GetTransaction dereference pcoinsTip; an empty view answers "not found".
         savedCoinsTip = pcoinsTip;
         pcoinsTip = &coins;
+        {
+            LOCK(cs_main);
+            savedTip = chainActive.Tip();
+        }
 
+        LOCK2(cs_main, wallet.cs_wallet);
         auto skA = TestKey(0), skB = TestKey(1), skC = TestKey(2);
         paA = skA.DefaultAddress();
         paB = skB.DefaultAddress();
@@ -226,13 +238,13 @@ protected:
         ASSERT_TRUE(wallet.AddSaplingZKey(skC, paC));
         SaplingPaymentAddress foreign = TestKey(9).DefaultAddress();   // not in the wallet
 
-        LOCK2(cs_main, wallet.cs_wallet);
         AddTx(MakeTx({MakeOutput(paA, 1000), MakeOutput(paB, 2000)}), 1);
         uint256 tx2 = AddTx(MakeTx({MakeOutput(paA, 3000)}), 5);
         AddTx(MakeTx({MakeOutput(paA, 4000), MakeOutput(paC, 5000), MakeOutput(paB, 6000)}), 11);
         AddTx(MakeTx({MakeOutput(paC, 7000)}), 21);
         AddTx(MakeTx({MakeOutput(paA, 8000)}), -1);                      // never mined: depth -1
         AddTx(MakeTx({MakeOutput(foreign, 50)}), 3);                     // no wallet notes at all
+        staleTx = AddTx(MakeTx({MakeOutput(paA, 11000)}), -1);         // mined in a stale block only
 
         // A note spent by a mined tx, and one whose spending tx never confirmed.
         const uint256 nfSpent = uint256S("5a1d"), nfPending = uint256S("9e9d");
@@ -255,11 +267,9 @@ protected:
 
     void TearDown() override {
         LOCK(cs_main);
-        chainActive.SetTip(nullptr);
+        chainActive.SetTip(savedTip);
-        for (const CBlockIndex& bi : index) {
+        for (const uint256& hash : insertedBlocks)
-            const uint256 hash = bi.GetBlockHash();
             mapBlockIndex.erase(hash);
-        }
         pcoinsTip = savedCoinsTip;
     }
 
@@ -275,7 +285,21 @@ protected:
         return tx.GetHash();
     }
 
-    // Builds an active chain 0..TIP_HEIGHT and puts each wallet tx into the block at its depth.
+    CBlockIndex& AddBlock(CBlock& block, int height, CBlockIndex* pprev) {
+        block.hashMerkleRoot = block.BuildMerkleTree();
+        index.emplace_back(block);
+        CBlockIndex& bi = index.back();
+        bi.SetHeight(height);
+        bi.pprev = pprev;
+        auto inserted = mapBlockIndex.insert(std::make_pair(block.GetHash(), &bi));
+        EXPECT_TRUE(inserted.second) << "block hash collision at height " << height;
+        insertedBlocks.push_back(block.GetHash());
+        bi.phashBlock = &inserted.first->first;
+        return bi;
+    }
+
+    // Builds an active chain 0..TIP_HEIGHT and puts each wallet tx into the block at its depth, plus a
+    // side block at STALE_HEIGHT (not on the active chain) holding staleTx.
     void Mine() {
         for (int h = 0; h <= TIP_HEIGHT; h++) {
             blocks.emplace_back();
@@ -288,24 +312,27 @@ protected:
                 if (TIP_HEIGHT - d.second + 1 == h)
                     block.vtx.push_back(wallet.mapWallet.at(d.first));
             }
-            block.hashMerkleRoot = block.BuildMerkleTree();
+            AddBlock(block, h, h > 0 ? &index[h - 1] : nullptr);
-
-            index.emplace_back(block);
-            CBlockIndex& bi = index.back();
-            bi.SetHeight(h);
-            bi.pprev = h > 0 ? &index[h - 1] : nullptr;
-            BlockMap::iterator it = mapBlockIndex.insert(std::make_pair(block.GetHash(), &bi)).first;
-            bi.phashBlock = &it->first;
         }
-        chainActive.SetTip(&index.back());
+        chainActive.SetTip(&index[TIP_HEIGHT]);
         for (const auto& d : depthOf) {
             int h = TIP_HEIGHT - d.second + 1;
             wallet.mapWallet.at(d.first).SetMerkleBranch(blocks[h]);
             minedHeight[d.first] = h;
         }
+
+        blocks.emplace_back();
+        CBlock& side = blocks.back();
+        side.nTime = 1800000000;
+        side.hashPrevBlock = blocks[STALE_HEIGHT - 1].GetHash();
+        side.vtx.push_back(wallet.mapWallet.at(staleTx));
+        AddBlock(side, STALE_HEIGHT, &index[STALE_HEIGHT - 1]);
+        wallet.mapWallet.at(staleTx).SetMerkleBranch(side);
+        minedHeight[staleTx] = STALE_HEIGHT;   // a txindex can still point at a stale block
     }
 
-    // What tx_height() returns on a -txindex node: the height of the block holding the tx, 0 if none.
+    // What tx_height() returns on a -txindex node: the height of the block the txindex points at
+    // (possibly stale), 0 if none.
     TxHeightFn TxIndexHeight() const {
         return [this](const uint256& txid) {
             auto it = minedHeight.find(txid);
@@ -393,7 +420,8 @@ TEST_F(WalletNoteCache, MinDepthAboveOneUsesWalletHeight) {
 }
 
 TEST_F(WalletNoteCache, HeightFromDepth) {
-    LOCK(cs_main);
+    LOCK2(cs_main, wallet.cs_wallet);
+    EXPECT_EQ(-1, wallet.mapWallet.at(staleTx).GetDepthInMainChain()) << "a stale block is not the active chain";
     EXPECT_EQ(TIP_HEIGHT, HeightFromDepth(1));
     EXPECT_EQ(TIP_HEIGHT - 10, HeightFromDepth(11));
     EXPECT_EQ(0, HeightFromDepth(0)) << "mempool";
@@ -449,6 +477,20 @@ protected:
 };
 
 TEST_F(WalletNoteCacheEncrypted, LockedWalletMatchesLegacy) {
+    EXPECT_TRUE(wallet.HaveSaplingSpendingKey(TestKey(0).expsk.full_viewing_key()));
+    EXPECT_FALSE(wallet.HaveSaplingSpendingKey(TestKey(1).expsk.full_viewing_key())) << "B is viewing key only";
+    EXPECT_FALSE(wallet.HaveSaplingSpendingKey(TestKey(9).expsk.full_viewing_key()));
     EXPECT_EQ(6u, Filtered({}, 1, INT_MAX, true, true, false).size()) << "A's and C's keys are still known";
     ExpectEquivalent({1, 11});
 }
+
+TEST_F(WalletNoteCacheEncrypted, UnlockedWalletFindsSpendingKeys) {
+    libzcash::SaplingExtendedSpendingKey sk;
+    EXPECT_FALSE(wallet.GetSaplingSpendingKey(TestKey(0).expsk.full_viewing_key(), sk)) << "locked";
+    ASSERT_TRUE(wallet.Unlock(CKeyingMaterial(32, 0x42)));
+    ASSERT_TRUE(wallet.GetSaplingSpendingKey(TestKey(0).expsk.full_viewing_key(), sk));
+    EXPECT_TRUE(sk.DefaultAddress() == paA);
+    ASSERT_TRUE(wallet.GetSaplingSpendingKey(TestKey(2).expsk.full_viewing_key(), sk));
+    EXPECT_TRUE(sk.DefaultAddress() == paC);
+    EXPECT_FALSE(wallet.GetSaplingSpendingKey(TestKey(9).expsk.full_viewing_key(), sk));
+}