From 358011bd5479b5e3723dd641a5d611259bca0000 Mon Sep 17 00:00:00 2001 From: DanS Date: Tue, 25 Aug 2026 17:41:32 +0200 Subject: [PATCH] wallet: resolve the autoshield destination at startup pwalletMain->autoShieldAddress was only ever written by a round running in the current process, so on any node without an explicit -autoshieldaddress, z_autoshieldstatus reported an empty destination from startup until the first round fired. disabled_reason was empty on that path too (rpcwallet.cpp), so the RPC showed autoshield true, running false, no address and no explanation -- the exact silent state z_autoshieldstatus was added to eliminate. On a restored wallet, which pre-derives the whole -mnemonicsaplinggap window and therefore always has an in-gap account to pick, the answer was known at startup and simply not computed. Split the read-only half of resolveDestination into a free function shared with init: the configured override if set, else the lowest in-gap account m/32'/coin'/i' the wallet already holds. init calls it once when autoshield is enabled and no explicit address was given. It deliberately does not generate a key. Deriving a fresh sapling account as a side effect of populating a status field would mutate the wallet to make an RPC prettier, so step 3 of resolveDestination -- the generation path, which must stay inside the operation where an unlocked wallet is already established -- is left where it was. A brand-new wallet holds nothing in-gap, so the field stays empty there and disabled_reason now says why instead of being blank. Behaviour is otherwise unchanged: same derivation, same lowest-index-wins rule, same refusal to trust CKeyMetadata, same caching for the life of the process. Verified on an isolated regtest chain, 12/12: fresh wallet -> address "", reason "no destination resolved yet; one will be derived from the HD seed on the first round" after one round -> address set, reason empty after RESTART -> address visible with NO block mined since (height 12 both sides), and z_listaddresses still holds exactly 1 address, so init derived nothing -autoshieldaddress-> still overrides the derived destination, and the round shields into it Co-Authored-By: Claude Opus 5 (1M context) --- src/init.cpp | 20 +++ .../asyncrpcoperation_autoshieldcoinbase.cpp | 116 ++++++++++++------ .../asyncrpcoperation_autoshieldcoinbase.h | 20 +++ src/wallet/rpcwallet.cpp | 2 +- 4 files changed, 122 insertions(+), 36 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 2aa86dc46..7e1519769 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -60,6 +60,7 @@ #include "wallet/wallet.h" #include "wallet/walletdb.h" #include "wallet/asyncrpcoperation_saplingconsolidation.h" +#include "wallet/asyncrpcoperation_autoshieldcoinbase.h" #include "wallet/asyncrpcoperation_sweep.h" #endif #include @@ -2556,6 +2557,25 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) return InitError("Wallet must hold the spending key of -autoshieldaddress (else shielded coinbase would be unrecoverable)"); } pwalletMain->autoShieldAddress = autoShieldAddress; + } else { + // No explicit destination. Resolve the seed-derived one now, read-only, + // so z_autoshieldstatus can say where coinbase will go BEFORE the first + // round rather than reporting an empty string until one fires. This + // never generates a key: a fresh account must not be a side effect of + // populating a status field. A brand-new wallet holds nothing in-gap + // yet, so the field stays empty and the RPC explains why. + LOCK(pwalletMain->cs_wallet); + if (!pwalletMain->IsLocked()) { + libzcash::SaplingPaymentAddress destAddr; + std::string destStr; + uint32_t destAccount = AUTOSHIELD_ACCOUNT_NONE; + if (ResolveAutoShieldDestinationReadOnly(destAddr, destStr, destAccount) + == AutoShieldDestStatus::Resolved) { + pwalletMain->autoShieldAddress = destStr; + LogPrintf("%s: autoshield destination %s (seed-derived sapling account %u)\n", + __func__, destStr, (unsigned)destAccount); + } + } } } diff --git a/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp b/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp index 74af78a9b..d50273364 100644 --- a/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp +++ b/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp @@ -105,50 +105,34 @@ void AsyncRPCOperation_autoshieldcoinbase::main() { getId(), getStateAsString(), numTxCreated_, FormatMoney(amountShielded_)); } -// 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) { +// Read-only half of destination resolution, shared with init.cpp so the answer to +// "where will auto-shielding send?" is available before the first round runs rather +// than only after one has fired. Mutates nothing: no key generation, no caching. +AutoShieldDestStatus ResolveAutoShieldDestinationReadOnly( + libzcash::SaplingPaymentAddress& destOut, std::string& destStrOut, uint32_t& accountOut) { + accountOut = AUTOSHIELD_ACCOUNT_NONE; // 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. + // zaddr in init.cpp). This doubles as the per-process cache for whatever + // the derivation below resolved on an earlier round. if (!pwalletMain->autoShieldAddress.empty()) { auto decoded = DecodePaymentAddress(pwalletMain->autoShieldAddress); if (boost::get(&decoded) != nullptr) { destOut = boost::get(decoded); destStrOut = pwalletMain->autoShieldAddress; - return true; + return AutoShieldDestStatus::Resolved; } - LogPrintf("%s: configured -autoshieldaddress is not a valid Sapling address\n", getId()); - return false; + return AutoShieldDestStatus::InvalidOverride; } // 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; + return AutoShieldDestStatus::NoSeed; } - // 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. + // Mirror init.cpp'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; @@ -179,15 +163,77 @@ bool AsyncRPCOperation_autoshieldcoinbase::resolveDestination( 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; + accountOut = i; + return AutoShieldDestStatus::Resolved; } } + return AutoShieldDestStatus::NotFound; +} + +// 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) { + + uint32_t account = AUTOSHIELD_ACCOUNT_NONE; + switch (ResolveAutoShieldDestinationReadOnly(destOut, destStrOut, account)) { + case AutoShieldDestStatus::Resolved: + // Cache for the life of the process; the override branch of the resolver + // short-circuits later rounds. Safe: we only cache post-validation. + pwalletMain->autoShieldAddress = destStrOut; + if (account == AUTOSHIELD_ACCOUNT_NONE) { + LogPrintf("%s: autoshield destination %s (configured)\n", getId(), destStrOut); + } else { + LogPrintf("%s: autoshield destination %s (seed-derived sapling account %u)\n", + getId(), destStrOut, (unsigned)account); + } + return true; + case AutoShieldDestStatus::InvalidOverride: + LogPrintf("%s: configured -autoshieldaddress is not a valid Sapling address\n", getId()); + return false; + case AutoShieldDestStatus::NoSeed: + LogPrintf("%s: no HD seed available; refusing to pick an autoshield destination\n", getId()); + return false; + case AutoShieldDestStatus::NotFound: + break; // nothing in the window yet: fall through and derive one + } + + // Re-establish the derivation context the resolver used, for step 3 below. + HDSeed seed; + if (!pwalletMain->GetHDSeedForDerivation(seed)) { + LogPrintf("%s: no HD seed available; refusing to pick an autoshield destination\n", getId()); + return false; + } + 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; + 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); + // 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 diff --git a/src/wallet/asyncrpcoperation_autoshieldcoinbase.h b/src/wallet/asyncrpcoperation_autoshieldcoinbase.h index 98dd5c734..46ad01946 100644 --- a/src/wallet/asyncrpcoperation_autoshieldcoinbase.h +++ b/src/wallet/asyncrpcoperation_autoshieldcoinbase.h @@ -14,6 +14,26 @@ // Default fee for automatic coinbase-shielding transactions static const CAmount DEFAULT_AUTOSHIELD_FEE = 10000; +// Sentinel for "not a derived account" (i.e. the configured -autoshieldaddress). +static const uint32_t AUTOSHIELD_ACCOUNT_NONE = UINT32_MAX; + +enum class AutoShieldDestStatus { + Resolved, // destOut/destStrOut are set + NotFound, // no in-gap account held yet; the operation will derive one + InvalidOverride, // -autoshieldaddress is set but is not a Sapling address + NoSeed, // no HD seed available (e.g. locked wallet) +}; + +// Resolve the auto-shield destination WITHOUT mutating the wallet: the configured +// -autoshieldaddress if set, else the lowest in-gap seed-derived account the wallet +// already holds. It deliberately does NOT generate a key, so init can call it purely +// to answer "where will this send?" -- deriving a fresh account as a side effect of +// populating a status field would be wrong. The operation's own resolveDestination +// falls through to generation when this returns NotFound. +// Caller must hold cs_wallet. +AutoShieldDestStatus ResolveAutoShieldDestinationReadOnly( + libzcash::SaplingPaymentAddress& destOut, std::string& destStrOut, uint32_t& accountOut); + // A periodic, wallet-local operation that drains matured *transparent* coinbase // UTXOs into a wallet-owned Sapling z-address in size-bounded batches. It is the // automatic sibling of the manual z_shieldcoinbase RPC and mirrors the dispatch diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 08f3117a7..98fb82c29 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3415,7 +3415,7 @@ UniValue z_autoshieldstatus(const UniValue& params, bool fHelp, const CPubKey& m why = strprintf("deferred while %s is running; rounds resume when it finishes", pwalletMain->fSweepRunning ? "z_sweep" : "sapling consolidation"); } else if (pwalletMain->autoShieldAddress.empty()) { - why = ""; + why = "no destination resolved yet; one will be derived from the HD seed on the first round"; } ret.push_back(Pair("disabled_reason", why));