DragonX/dragonx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 3 Wiki Activity

wallet: pick the autoshield destination by seed re-derivation, in-gap only

Browse Source 
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'/<coin>'/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) <noreply@anthropic.com>
This commit is contained in:
DanS
2026-08-23 02:52:34 +02:00
parent bb292a89cc
commit dad162a89a
1 changed files with 111 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

130
src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp
View File

@@ -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'/<coin>'/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<libzcash::SaplingPaymentAddress>(&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<libzcash::SaplingPaymentAddress> 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());