From dad162a89a524a2bcc7dc72abc07ccb0970ea16b Mon Sep 17 00:00:00 2001 From: DanS Date: Sun, 23 Aug 2026 02:52:34 +0200 Subject: [PATCH] wallet: pick the autoshield destination by seed re-derivation, in-gap only resolveDestination() took the first spendable address in std::set order, which orders on the raw Sapling diversifier (zcash/Address.hpp:95-98) -- uncorrelated with anything the operator can see, and unstable across restarts as addresses are added. Worse, GetSaplingPaymentAddresses() also returns z_importkey/z_importwallet addresses, and CKeyMetadata is NOT evidence of provenance: both hdKeypath and seedFp are copied verbatim out of the import source (rpcdump.cpp:511-516 -> wallet.cpp:5440-5441) with no verification. A crafted import can therefore claim this wallet's seedFp and keypath m/32'/coin'/0' and capture every shielded mining reward into a key no seed restore can reproduce. Filtering on metadata would not have caught that. Derive instead. A bare -mnemonic/-hdseed restore pre-derives exactly -mnemonicsaplinggap sapling accounts from index 0 with saplingAccountCounter reset (init.cpp:2349-2355), so the only self-recoverable destinations are the default addresses of m/32'/'/i' for i below the gap. Walk that window from the seed and take the lowest index the wallet holds a spending key for. Deriving is the only authoritative test and cannot be spoofed. When nothing in the window is held yet, derive the next account -- but only if it will land inside the window. GenerateNewSaplingZKey does not derive at saplingAccountCounter: its do/while skips indices already held (wallet.cpp:150-157), so a bare counter-below-gap test is unsound. Predict the lowest free index at or above the counter and post-verify the returned address. If no free account remains below the gap, refuse the round and leave the coinbase transparent -- transparent funds are still recoverable through the 1000-key transparent gap, an unfindable note is not. Refusing is safe: main_impl turns a false return into a clean skip, and main() always advances nextAutoShield and clears fAutoShieldRunning, so a refused round cannot latch the feature off. Co-Authored-By: Claude Opus 5 (1M context) --- .../asyncrpcoperation_autoshieldcoinbase.cpp | 130 +++++++++++++++--- 1 file changed, 111 insertions(+), 19 deletions(-) diff --git a/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp b/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp index 60c9eda17..aa79af438 100644 --- a/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp +++ b/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp @@ -93,13 +93,30 @@ void AsyncRPCOperation_autoshieldcoinbase::main() { getId(), getStateAsString(), numTxCreated_, FormatMoney(amountShielded_)); } -// Enumerate wallet-owned Sapling addresses and pick a spendable one; if none -// exists, generate a fresh one (needs an unlocked wallet, which the caller has -// already ensured). Caller must hold cs_wallet. +// Resolve the Sapling destination for auto-shielded coinbase. +// +// Recoverability is the hard requirement: coinbase we shield must land in an +// address that a bare -mnemonic/-hdseed restore of THIS wallet's seed re-derives +// on its own. A restore pre-derives exactly -mnemonicsaplinggap sapling accounts +// starting at index 0, with saplingAccountCounter reset to 0 (init.cpp:2349-2355), +// so the only self-recoverable destinations are the default addresses of +// m/32'/'/i' for i < gap. +// +// We therefore DERIVE those accounts from the seed and pick the lowest index the +// wallet already holds. Deriving is the only authoritative test. In particular +// CKeyMetadata is NOT evidence of provenance: z_importkey / z_importwallet copy +// both hdKeypath and seedFp verbatim out of the import source +// (wallet.cpp:5522-5529 <- rpcdump.cpp:511-516), so a foreign key can claim any +// keypath and any seed fingerprint. Filtering on metadata would let an imported +// key win as "account 0" and silently receive every shielded reward. +// +// Caller must hold cs_wallet and must already have checked the wallet is unlocked. bool AsyncRPCOperation_autoshieldcoinbase::resolveDestination( libzcash::SaplingPaymentAddress& destOut, std::string& destStrOut) { - // 1. Explicit -autoshieldaddress override (validated + spend-key-checked at init) + // 1. Explicit -autoshieldaddress override (validated as a spendable Sapling + // zaddr at init.cpp:2494-2506). This also serves as the per-process cache + // for whatever step 2/3 resolved. if (!pwalletMain->autoShieldAddress.empty()) { auto decoded = DecodePaymentAddress(pwalletMain->autoShieldAddress); if (boost::get(&decoded) != nullptr) { @@ -111,30 +128,105 @@ bool AsyncRPCOperation_autoshieldcoinbase::resolveDestination( return false; } - // 2. Reuse the first spendable wallet-owned Sapling address (std::set order - // is deterministic, so this is stable across rounds/restarts). - std::set addrs; - pwalletMain->GetSaplingPaymentAddresses(addrs); - for (const auto& a : addrs) { - libzcash::SaplingExtendedSpendingKey extsk; - if (pwalletMain->GetSaplingExtendedSpendingKey(a, extsk)) { - destOut = a; - destStrOut = EncodePaymentAddress(a); - // Cache it so we keep reusing the same address. + // 2. Walk the restore window m/32'/coin'/[0, gap)' derived from the seed. + HDSeed seed; + if (!pwalletMain->GetHDSeedForDerivation(seed)) { + LogPrintf("%s: no HD seed available; refusing to pick an autoshield destination\n", getId()); + return false; + } + + // Mirror init.cpp:2349-2350's own clamp, and cap into the hardened index + // space so (i | ZIP32_HARDENED_KEY_LIMIT) below stays well formed. + int64_t gapArg = GetArg("-mnemonicsaplinggap", 100); + if (gapArg < 0) { + gapArg = 0; + } + if (gapArg > (int64_t)ZIP32_HARDENED_KEY_LIMIT) { + gapArg = (int64_t)ZIP32_HARDENED_KEY_LIMIT; + } + const uint32_t saplingGap = (uint32_t)gapArg; + + // Same derivation path as CWallet::GenerateNewSaplingZKey (wallet.cpp:139-152). + const uint32_t bip44CoinType = Params().BIP44CoinType(); + auto m = libzcash::SaplingExtendedSpendingKey::Master(seed); + auto m_32h = m.Derive(32 | ZIP32_HARDENED_KEY_LIMIT); + auto m_32h_cth = m_32h.Derive(bip44CoinType | ZIP32_HARDENED_KEY_LIMIT); + + for (uint32_t i = 0; i < saplingGap; i++) { + auto xsk = m_32h_cth.Derive(i | ZIP32_HARDENED_KEY_LIMIT); + auto addr = xsk.DefaultAddress(); + + // Spendable AND registered: GetSaplingExtendedSpendingKey resolves + // addr -> ivk -> fvk -> spending key (keystore.cpp:215-223), so a hit + // means the wallet both recognises notes sent to `addr` and can spend + // them. Exactly the pair of properties the shield needs. Lowest index + // wins: stable for the life of the wallet and reproducible from the seed + // alone, unlike std::set order over the random diversifier + // (zcash/Address.hpp:95-98). + libzcash::SaplingExtendedSpendingKey held; + if (pwalletMain->GetSaplingExtendedSpendingKey(addr, held)) { + destOut = addr; + destStrOut = EncodePaymentAddress(addr); + // Cache for the life of the process; step 1 short-circuits later + // rounds. Safe: we only cache post-validation. pwalletMain->autoShieldAddress = destStrOut; + LogPrintf("%s: autoshield destination %s (seed-derived sapling account %u, gap %u)\n", + getId(), destStrOut, (unsigned)i, (unsigned)saplingGap); return true; } } - // 3. No spendable z-addr yet: create one (requires unlocked wallet / HD seed). - if (pwalletMain->IsLocked()) { + // 3. Nothing usable in the window yet: derive the next account, but only if + // GenerateNewSaplingZKey will land INSIDE the window. It does NOT derive + // at saplingAccountCounter: its do/while skips every index whose spending + // key we already hold (wallet.cpp:150-157), so a bare "counter < gap" + // test is unsound - counter 98 with gap 100 can still land on 100. + // Predict min{ i >= counter : we do not hold i } instead. + const uint32_t counter = pwalletMain->GetHDChain().saplingAccountCounter; + uint32_t predicted = saplingGap; // sentinel: "would land outside the window" + for (uint32_t i = counter; i < saplingGap; i++) { + auto xsk = m_32h_cth.Derive(i | ZIP32_HARDENED_KEY_LIMIT); + if (!pwalletMain->HaveSaplingSpendingKey(xsk.expsk.full_viewing_key())) { + predicted = i; + break; + } + } + + if (predicted == saplingGap) { + LogPrintf("%s: no free sapling account below -mnemonicsaplinggap=%u (account counter is %u). " + "A newly derived z-address would NOT be re-derived by a seed restore, so the " + "shielded coinbase could not be recovered from the seed alone. Refusing to " + "autoshield this round. Fix: point -autoshieldaddress at an existing in-gap " + "wallet z-address, or raise -mnemonicsaplinggap here AND use the same value on " + "any future restore.\n", + getId(), (unsigned)saplingGap, (unsigned)counter); return false; } + + if (pwalletMain->IsLocked()) { + LogPrintf("%s: wallet is locked; cannot derive an autoshield destination z-address\n", getId()); + return false; + } + try { - destOut = pwalletMain->GenerateNewSaplingZKey(); - destStrOut = EncodePaymentAddress(destOut); + auto expectedAddr = m_32h_cth.Derive(predicted | ZIP32_HARDENED_KEY_LIMIT).DefaultAddress(); + libzcash::SaplingPaymentAddress newAddr = pwalletMain->GenerateNewSaplingZKey(); + + // Post-verify rather than trust the prediction: cheap, and it closes the + // whole class of "the counter moved further than expected" bugs. + if (!(newAddr == expectedAddr)) { + LogPrintf("%s: newly derived z-address is not sapling account %u as predicted " + "(counter %u -> %u); not using it as the autoshield destination\n", + getId(), (unsigned)predicted, (unsigned)counter, + (unsigned)pwalletMain->GetHDChain().saplingAccountCounter); + return false; + } + + destOut = newAddr; + destStrOut = EncodePaymentAddress(newAddr); pwalletMain->autoShieldAddress = destStrOut; - LogPrintf("%s: generated new autoshield destination z-address %s\n", getId(), destStrOut); + LogPrintf("%s: generated new autoshield destination z-address %s (seed-derived sapling account %u)\n", + getId(), destStrOut, (unsigned)predicted); return true; } catch (const std::exception& e) { LogPrintf("%s: could not generate a destination z-address: %s\n", getId(), e.what());