wallet: don't witness viewing-key-only notes (fixes an abort on the next block)

A note found through an incoming viewing key only (z_importviewingkey)
has no full viewing key behind it. UpdateSaplingNullifierNoteMapWithTx
skips such notes, so they never get a nullifier. VerifyAndSetInitialWitness
treated "no nullifier" as "witness needs reseeding" on every call: it
cleared the note's witness, re-read the block from disk, reseeded it,
and then dereferenced the nullifier that never came. dragonxd aborted
in the txnotify thread (boost::optional "is_initialized" assert) on the
first block confirming a note to such an address, and again after every
restart, before the block index was flushed.

Such a note can't be spent, so it needs no witness. Skip it there; if
its spending key is imported later, it has a full viewing key and the
existing no-nullifier rule seeds it then. SaplingWitnessMinimumHeight
now takes the optional nullifier. A note without one is never extended
by BuildWitnessCache (its pending-note gate requires one), so it must
not lower the rebuild's start height. Every call site that worked before
passes a set nullifier and behaves as before.

The deref came in with the 2020 witness rework (6c48397cf). The new
gtest ViewingKeyOnlyNoteIsNotWitnessed fails on the old code (the note
reaches the reseed path).

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

src/gtest/test_wallet_notedata.cpp

@@ -4,9 +4,18 @@
 
 #include <gtest/gtest.h>
 
+#include <deque>
+
+#include "chain.h"
+#include "coins.h"
+#include "main.h"
+#include "primitives/block.h"
 #include "primitives/transaction.h"
+#include "random.h"
 #include "uint256.h"
 #include "wallet/wallet.h"
+#include "zcash/Note.hpp"
+#include "zcash/zip32.h"
 
 namespace {
 
@@ -15,6 +24,9 @@ public:
     bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx, bool fSameBlock = true) {
         return CWallet::UpdatedNoteData(wtxIn, wtx, fSameBlock);
     }
+    int VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessOnly) {
+        return CWallet::VerifyAndSetInitialWitness(pindex, witnessOnly);
+    }
 };
 
 libzcash::SaplingIncomingViewingKey TestIvk()
@@ -153,3 +165,108 @@ TEST(WalletNoteData, EmptyIncomingIsNoUpdate) {
     EXPECT_EQ(1u, wtx.mapSaplingNoteData.size());
     EXPECT_EQ(400, wtx.mapSaplingNoteData.at(op0).witnessHeight);
 }
+
+namespace {
+
+// A real encrypted Sapling output to pa (the wallet never checks its proof or value commitment).
+OutputDescription EncryptedOutput(const libzcash::SaplingPaymentAddress& pa, uint64_t value)
+{
+    libzcash::SaplingNote note(pa, value);
+    std::array<unsigned char, HUSH_MEMO_SIZE> memo;
+    memo.fill(0);
+    auto enc = libzcash::SaplingNotePlaintext(note, memo).encrypt(note.pk_d);
+    assert(static_cast<bool>(enc));
+    OutputDescription od;
+    od.cv = GetRandHash();
+    od.cm = note.cm().get();
+    od.ephemeralKey = enc->second.get_epk();
+    od.encCiphertext = enc->first;
+    return od;
+}
+
+// A three-block active chain (heights 0-2) whose block 1 holds the given txs, undone on destruction:
+// chainActive, mapBlockIndex and pcoinsTip are process-wide and every gtest suite shares the process.
+struct FakeActiveChain {
+    std::deque<CBlock> blocks;
+    std::deque<CBlockIndex> index;
+    CBlockIndex* savedTip;
+    CCoinsView coinsBase;
+    CCoinsViewCache coins{&coinsBase};
+    CCoinsViewCache* savedCoinsTip;
+
+    explicit FakeActiveChain(const std::vector<CTransaction>& inBlock1) {
+        LOCK(cs_main);
+        savedTip = chainActive.Tip();
+        savedCoinsTip = pcoinsTip;
+        pcoinsTip = &coins;
+        for (int h = 0; h <= 2; h++) {
+            blocks.emplace_back();
+            CBlock& block = blocks.back();
+            block.nTime = 1700000000 + h;
+            block.nNonce = uint256S(strprintf("%x", 0x700 + h));
+            if (h > 0)
+                block.hashPrevBlock = blocks[h - 1].GetHash();
+            if (h == 1)
+                block.vtx = inBlock1;
+            block.hashMerkleRoot = block.BuildMerkleTree();
+            index.emplace_back(block);
+            CBlockIndex& bi = index.back();
+            bi.SetHeight(h);
+            bi.pprev = h > 0 ? &index[h - 1] : nullptr;
+            bi.phashBlock = &mapBlockIndex.insert(std::make_pair(block.GetHash(), &bi)).first->first;
+        }
+        chainActive.SetTip(&index.back());
+    }
+    ~FakeActiveChain() {
+        LOCK(cs_main);
+        chainActive.SetTip(savedTip);
+        for (const CBlock& block : blocks)
+            mapBlockIndex.erase(block.GetHash());
+        pcoinsTip = savedCoinsTip;
+    }
+};
+
+}
+
+// A note found with an incoming viewing key only (z_importviewingkey) can't be spent and never gets
+// a nullifier, because deriving one takes the full viewing key. VerifyAndSetInitialWitness must leave
+// it alone. It used to clear it and reseed its witness from the block on disk on every call, then
+// abort on the missing nullifier the first time the note was confirmed. Here there are no block
+// files, so the old reseed path throws.
+TEST(WalletNoteData, ViewingKeyOnlyNoteIsNotWitnessed) {
+    NoteDataWallet wallet;
+    std::vector<unsigned char, secure_allocator<unsigned char>> raw(32, 0x3c);
+    HDSeed seed(raw);
+    auto sk = libzcash::SaplingExtendedSpendingKey::Master(seed);
+    auto pa = sk.DefaultAddress();
+
+    LOCK2(cs_main, wallet.cs_wallet);
+    ASSERT_TRUE(wallet.AddSaplingIncomingViewingKey(sk.expsk.full_viewing_key().in_viewing_key(), pa));
+    ASSERT_FALSE(wallet.HaveSaplingFullViewingKey(sk.expsk.full_viewing_key().in_viewing_key()));
+
+    CMutableTransaction mtx;
+    mtx.fOverwintered = true;
+    mtx.nVersion = SAPLING_TX_VERSION;
+    mtx.nVersionGroupId = SAPLING_VERSION_GROUP_ID;
+    mtx.vShieldedOutput.push_back(EncryptedOutput(pa, 5000));
+    CTransaction tx(mtx);
+
+    CWalletTx wtx(&wallet, tx);
+    mapSaplingNoteData_t found = wallet.FindMySaplingNotes(tx).first;
+    ASSERT_EQ(1u, found.size()) << "the viewing key finds the note";
+    wtx.SetSaplingNoteData(found);
+    ASSERT_TRUE(wallet.AddToWallet(wtx, true, nullptr));
+
+    FakeActiveChain chain({tx});
+    CWalletTx& stored = wallet.mapWallet.at(tx.GetHash());
+    stored.SetMerkleBranch(chain.blocks[1]);
+    ASSERT_EQ(2, stored.GetDepthInMainChain());
+
+    int minHeight = -1;
+    ASSERT_NO_THROW(minHeight = wallet.VerifyAndSetInitialWitness(chainActive.Tip(), false));
+    EXPECT_EQ(chainActive.Height(), minHeight) << "it must not pull the witness rebuild down to its height";
+    const SaplingNoteData& nd = stored.mapSaplingNoteData.at(SaplingOutPoint(tx.GetHash(), 0));
+    EXPECT_TRUE(nd.witnesses.empty());
+    EXPECT_FALSE(static_cast<bool>(nd.nullifier));
+    EXPECT_EQ(-1, nd.witnessHeight);
+}

src/wallet/wallet.cpp

@@ -1262,9 +1262,11 @@ void ClearSingleNoteWitnessCache(NoteData* nd)
     nd->witnessRootValidated = false;
 }
 
-int CWallet::SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessHeight, int nMinimumHeight)
+// A note without a nullifier is never extended by BuildWitnessCache (its pending-note gate requires one),
+// so it doesn't pull the rebuild's start height down.
+int CWallet::SaplingWitnessMinimumHeight(const boost::optional<uint256>& nullifier, int nWitnessHeight, int nMinimumHeight)
 {
-    if (GetSaplingSpendDepth(nullifier) <= WITNESS_CACHE_SIZE) {
+    if (nullifier && GetSaplingSpendDepth(*nullifier) <= WITNESS_CACHE_SIZE) {
         nMinimumHeight = min(nWitnessHeight, nMinimumHeight);
     }
     return nMinimumHeight;
@@ -1296,6 +1298,14 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
         uint256 blockRoot;
         uint256 witnessRoot;
 
+        //A note found with an incoming viewing key only (z_importviewingkey) can't be spent, so it needs
+        //no witness, and it never gets a nullifier: deriving one takes the full viewing key, which
+        //UpdateSaplingNullifierNoteMapWithTx skips without. Seeding it here meant clearing and reseeding
+        //it from disk on every call (the no-nullifier rule below), then aborting on the missing nullifier
+        //the first time it was confirmed. If its spending key is imported later, the rule below seeds it.
+        if (!HaveSaplingFullViewingKey(nd->ivk))
+          continue;
+
         if (!nd->nullifier)
           ::ClearSingleNoteWitnessCache(nd);
 
@@ -1307,7 +1317,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
 
           //Skip Validation when witness root has been validated
           if (nd->witnessRootValidated) {
-            nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+            nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
             continue;
           }
 
@@ -1319,12 +1329,12 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
             CBlockIndex* whIndex = chainActive[nd->witnessHeight];
             if (whIndex == NULL) {
               //witnessHeight strictly above the active chain (transient catch-up): cannot validate yet
-              nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+              nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
               continue;
             }
             if (nd->witnesses.front().root() == whIndex->hashFinalSaplingRoot) {
               nd->witnessRootValidated = true;
-              nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+              nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
               continue;
             }
             //root mismatch on the active chain -> desynced; fall through to rebuild below
@@ -1336,7 +1346,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
           blockRoot = pblockindex->hashFinalSaplingRoot;
           if (witnessRoot == blockRoot) {
             nd->witnessRootValidated = true;
-            nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+            nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
             continue;
           }
         }
@@ -1388,7 +1398,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
         }
         nd->witnessHeight = pblockindex->GetHeight();
         UpdateSaplingNullifierNoteMapWithTx(wtxItem.second);
-        nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+        nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
       }
     }
   }

src/wallet/wallet.h

@@ -895,7 +895,7 @@ public:
 
 protected:
 
-    int SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessHeight, int nMinimumHeight);
+    int SaplingWitnessMinimumHeight(const boost::optional<uint256>& nullifier, int nWitnessHeight, int nMinimumHeight);
 
     /**
      * pindex is the new tip being connected.