From 9a8f17b2c8108d7d8e0a3755118729a2636cbb24 Mon Sep 17 00:00:00 2001 From: DanS Date: Tue, 25 Aug 2026 07:59:13 +0200 Subject: [PATCH] wallet: bound autoshield rounds and lock their inputs Two defects in a single autoshield round, both of the quiet kind. The size estimate reserved a flat 2000 bytes for "header + sietch outputs", but every autoshield tx carries three Sapling OutputDescriptions -- the change note plus the two Sietch dummies -- and the real fixed cost is ~2937 bytes. Measured across seven live mainnet coinbase shields: 113.5 bytes per input and 2936.6 +/- 0.8 bytes fixed, of which 3 * 948 = 2844 is the output descriptions. The estimate was therefore short by ~937 bytes before a single input was counted. Inputs are charged AUTOSHIELD_CTXIN_DUST_SIZE = 148, which is conservative for the default P2PK coinbase but exact for P2PKH, so with a P2PKH coinbase a backlog of 1332..1337 utxos passed the estimate and built a tx over MAX_TX_SIZE_AFTER_SAPLING. CommitTransaction calls AddToWallet before AcceptToMemoryPool, so a rejected oversize tx leaves its inputs reading as spent. z_shieldcoinbase caps a manual shield at SHIELD_COINBASE_DEFAULT_LIMIT = 50 utxos; autoshield dropped that cap and relied on the byte estimate alone. Restore one -- AUTOSHIELD_MAX_INPUTS = 400 -- so the byte arithmetic is no longer the only thing between a large backlog and an oversize transaction. The remainder is shielded on the next round. Second, the proof build deliberately runs without cs_wallet so wallet RPCs are not stalled, which leaves a multi-second window in which a concurrent z_shieldcoinbase or z_sendmany can re-select the same coinbase outputs. AvailableCoins already honours IsLockedCoin and z_shieldcoinbase already brackets its selection with LockCoin/UnlockCoin; autoshield made zero LockCoin calls. Take the locks under cs_wallet at selection time and release them via RAII, since several early returns sit between selection and commit and a leaked lock would exclude those coins from every future round. Verified on an isolated regtest chain with a 540-utxo backlog: round 1 logged "reached per-round input cap (400)" and committed exactly 400 inputs in a 48351-byte tx (estimate 62300, limit 200000) round 2 took the remaining 151; backlog drained 540 -> 0 listlockunspent showed 400 coins locked mid-round and 0 afterwards 48351 bytes for 400 inputs implies 2937 bytes of fixed overhead, agreeing with the mainnet measurement to 14 bytes Co-Authored-By: Claude Opus 5 (1M context) --- .../asyncrpcoperation_autoshieldcoinbase.cpp | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp b/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp index aa79af438..74af78a9b 100644 --- a/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp +++ b/src/wallet/asyncrpcoperation_autoshieldcoinbase.cpp @@ -22,6 +22,18 @@ extern std::string randomSietchZaddr(); // Serialized-size estimates for one spent input (kept in sync with rpcwallet.cpp) static const size_t AUTOSHIELD_CTXIN_DUST_SIZE = 148; +// Every autoshield tx carries THREE Sapling OutputDescriptions -- the change +// note to destZaddr plus the two Sietch dummies -- at ~948 bytes each. Reserving +// 2000 for "header + sietch outputs" was ~900 bytes short before a single input +// was counted, so a large enough round could build a tx over MAX_TX_SIZE. +static const size_t AUTOSHIELD_SAPLING_OUTPUT_SIZE = 948; +static const size_t AUTOSHIELD_TX_OVERHEAD = (3 * AUTOSHIELD_SAPLING_OUTPUT_SIZE) + 256; +// Hard cap on inputs per round, mirroring z_shieldcoinbase's +// SHIELD_COINBASE_DEFAULT_LIMIT. The byte estimate alone is not a safe bound: +// with a P2PKH coinbase (-mineraddress) the 148-byte figure is exact rather than +// conservative, so an under-estimate translates directly into an oversize tx. +// The remainder is simply shielded on the next round. +static const size_t AUTOSHIELD_MAX_INPUTS = 400; static const size_t AUTOSHIELD_CTXIN_P2SH_SIZE = 400; // Expire unmined autoshield txs after this many blocks, so a tx cannot straddle // a network-upgrade activation. @@ -258,6 +270,27 @@ bool AsyncRPCOperation_autoshieldcoinbase::main_impl() { libzcash::SaplingPaymentAddress destZaddr; std::string destStr; std::vector inputs; + + // Proof building below runs WITHOUT cs_wallet (deliberately, so wallet RPCs + // are not stalled), which leaves a multi-second window in which a manual + // z_shieldcoinbase or z_sendmany over the same miner address would re-select + // these same coinbase outputs. AvailableCoins honours IsLockedCoin, so lock + // them for the duration exactly as z_shieldcoinbase does. RAII because there + // are several early returns between here and commit, and a leaked lock would + // silently exclude those coins from every future round. + struct ScopedCoinLocks { + std::vector locked; + ~ScopedCoinLocks() { + // A destructor is noexcept by default; letting the lock acquisition + // escape would turn a contended mutex into std::terminate. + try { + if (locked.empty()) return; + LOCK2(cs_main, pwalletMain->cs_wallet); + // UnlockCoin takes a non-const reference (upstream signature). + for (COutPoint& op : locked) pwalletMain->UnlockCoin(op); + } catch (...) {} + } + } coinLocks; CAmount shieldedValue = 0; unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING; @@ -280,7 +313,7 @@ bool AsyncRPCOperation_autoshieldcoinbase::main_impl() { // AvailableCoins with fOnlySpendable already excludes immature coinbase // (< COINBASE_MATURITY) and outputs we don't own, so external // -mineraddress / pool coinbase naturally yields zero inputs. - size_t estimatedTxSize = 2000; // header + sietch outputs headroom + size_t estimatedTxSize = AUTOSHIELD_TX_OVERHEAD; std::vector vecOutputs; pwalletMain->AvailableCoins(vecOutputs, true, NULL, false, true); for (const COutput& out : vecOutputs) { @@ -293,6 +326,11 @@ bool AsyncRPCOperation_autoshieldcoinbase::main_impl() { } size_t increase = (boost::get(&address) != nullptr) ? AUTOSHIELD_CTXIN_P2SH_SIZE : AUTOSHIELD_CTXIN_DUST_SIZE; + if (inputs.size() >= AUTOSHIELD_MAX_INPUTS) { + LogPrintf("%s: reached per-round input cap (%d); deferring remaining coinbase to next round\n", + opid, (int)AUTOSHIELD_MAX_INPUTS); + break; + } if (estimatedTxSize + increase >= max_tx_size) { // Size-safe batch; the remainder is shielded next round. LogPrintf("%s: reached per-tx size cap; deferring remaining coinbase to next round\n", opid); @@ -305,6 +343,12 @@ bool AsyncRPCOperation_autoshieldcoinbase::main_impl() { inputs.push_back(utxo); shieldedValue += out.tx->vout[out.i].nValue; } + + for (const ShieldCoinbaseUTXO& t : inputs) { + COutPoint outpt(t.txid, t.vout); + pwalletMain->LockCoin(outpt); + coinLocks.locked.push_back(outpt); + } } CAmount fee = pwalletMain->autoShieldFee;