From b911189ac602ad209ead758dc1814f1ececc3d67 Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 15 Oct 2026 21:03:46 -0500 Subject: [PATCH] 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 --- src/gtest/test_wallet_notedata.cpp | 117 +++++++++++++++++++++++++++++ src/wallet/wallet.cpp | 24 ++++-- src/wallet/wallet.h | 2 +- 3 files changed, 135 insertions(+), 8 deletions(-) diff --git a/src/gtest/test_wallet_notedata.cpp b/src/gtest/test_wallet_notedata.cpp index fddee8875..bdc2ad8bb 100644 --- a/src/gtest/test_wallet_notedata.cpp +++ b/src/gtest/test_wallet_notedata.cpp @@ -4,9 +4,18 @@ #include +#include + +#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 memo; + memo.fill(0); + auto enc = libzcash::SaplingNotePlaintext(note, memo).encrypt(note.pk_d); + assert(static_cast(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 blocks; + std::deque index; + CBlockIndex* savedTip; + CCoinsView coinsBase; + CCoinsViewCache coins{&coinsBase}; + CCoinsViewCache* savedCoinsTip; + + explicit FakeActiveChain(const std::vector& 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> 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(nd.nullifier)); + EXPECT_EQ(-1, nd.witnessHeight); +} diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 425bda60d..fe8a97ecf 100644 --- a/src/wallet/wallet.cpp +++ b/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& 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); } } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ebcb64bb1..bd3421b8f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -895,7 +895,7 @@ public: protected: - int SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessHeight, int nMinimumHeight); + int SaplingWitnessMinimumHeight(const boost::optional& nullifier, int nWitnessHeight, int nMinimumHeight); /** * pindex is the new tip being connected.