2 Commits

Author SHA1 Message Date
7a62fc4877 wallet: break the sweep/consolidation/autoshield deadlock
A successful-but-incomplete sweep round deliberately returns with fSweepRunning
still set and nextSweep unadvanced, as a "keep draining next block" baton. Every
early return in RunSaplingSweep, though, leaves that baton set without
re-dispatching -- and RunSaplingConsolidation, which is gated on fSweepRunning,
then returns without advancing nextConsolidation. So the "consolidation is
within 5 blocks" blackout at the top of RunSaplingSweep never lifts: sweep waits
on consolidation, consolidation waits on sweep, and neither runs again.

That much is pre-existing. What is new is that autoshield now shares the gate --
RunAutoShieldCoinbase returns early on fSweepRunning || fConsolidationRunning --
so a wedged sweep silently disables coinbase shielding too, with
z_autoshieldstatus reporting autoshield true, running false, and no reason.

Only honour the baton while a sweep operation is genuinely in flight: if the
operation for saplingSweepOperationId is absent or has reached a terminal state,
drop the stale flag and let the checks below decide afresh. The drain model is
unchanged -- nextSweep is still unadvanced, so the next block re-dispatches.

Also report the deferral in z_autoshieldstatus, so mutual exclusion with sweep
or consolidation reads as a deferral rather than an unexplained idle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 07:59:13 +02:00
9a8f17b2c8 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) <noreply@anthropic.com>
2026-08-25 07:59:13 +02:00
3 changed files with 76 additions and 1 deletions

View File

@@ -22,6 +22,18 @@ extern std::string randomSietchZaddr();
// Serialized-size estimates for one spent input (kept in sync with rpcwallet.cpp) // Serialized-size estimates for one spent input (kept in sync with rpcwallet.cpp)
static const size_t AUTOSHIELD_CTXIN_DUST_SIZE = 148; 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; static const size_t AUTOSHIELD_CTXIN_P2SH_SIZE = 400;
// Expire unmined autoshield txs after this many blocks, so a tx cannot straddle // Expire unmined autoshield txs after this many blocks, so a tx cannot straddle
// a network-upgrade activation. // a network-upgrade activation.
@@ -258,6 +270,27 @@ bool AsyncRPCOperation_autoshieldcoinbase::main_impl() {
libzcash::SaplingPaymentAddress destZaddr; libzcash::SaplingPaymentAddress destZaddr;
std::string destStr; std::string destStr;
std::vector<ShieldCoinbaseUTXO> inputs; std::vector<ShieldCoinbaseUTXO> 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<COutPoint> 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; CAmount shieldedValue = 0;
unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING; 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 // AvailableCoins with fOnlySpendable already excludes immature coinbase
// (< COINBASE_MATURITY) and outputs we don't own, so external // (< COINBASE_MATURITY) and outputs we don't own, so external
// -mineraddress / pool coinbase naturally yields zero inputs. // -mineraddress / pool coinbase naturally yields zero inputs.
size_t estimatedTxSize = 2000; // header + sietch outputs headroom size_t estimatedTxSize = AUTOSHIELD_TX_OVERHEAD;
std::vector<COutput> vecOutputs; std::vector<COutput> vecOutputs;
pwalletMain->AvailableCoins(vecOutputs, true, NULL, false, true); pwalletMain->AvailableCoins(vecOutputs, true, NULL, false, true);
for (const COutput& out : vecOutputs) { for (const COutput& out : vecOutputs) {
@@ -293,6 +326,11 @@ bool AsyncRPCOperation_autoshieldcoinbase::main_impl() {
} }
size_t increase = (boost::get<CScriptID>(&address) != nullptr) size_t increase = (boost::get<CScriptID>(&address) != nullptr)
? AUTOSHIELD_CTXIN_P2SH_SIZE : AUTOSHIELD_CTXIN_DUST_SIZE; ? 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) { if (estimatedTxSize + increase >= max_tx_size) {
// Size-safe batch; the remainder is shielded next round. // Size-safe batch; the remainder is shielded next round.
LogPrintf("%s: reached per-tx size cap; deferring remaining coinbase to next round\n", opid); 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); inputs.push_back(utxo);
shieldedValue += out.tx->vout[out.i].nValue; 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; CAmount fee = pwalletMain->autoShieldFee;

View File

@@ -3408,6 +3408,12 @@ UniValue z_autoshieldstatus(const UniValue& params, bool fHelp, const CPubKey& m
why = "disabled by -autoshield=0"; why = "disabled by -autoshield=0";
} else if (pwalletMain->IsLocked()) { } else if (pwalletMain->IsLocked()) {
why = "wallet is locked; rounds are skipped until it is unlocked"; why = "wallet is locked; rounds are skipped until it is unlocked";
} else if (pwalletMain->fSweepRunning || pwalletMain->fConsolidationRunning) {
// Autoshield is mutually exclusive with sweep and consolidation. Without
// this the RPC reports autoshield=true, running=false and an empty
// reason while no round can actually start.
why = strprintf("deferred while %s is running; rounds resume when it finishes",
pwalletMain->fSweepRunning ? "z_sweep" : "sapling consolidation");
} else if (pwalletMain->autoShieldAddress.empty()) { } else if (pwalletMain->autoShieldAddress.empty()) {
why = ""; why = "";
} }

View File

@@ -592,6 +592,31 @@ void CWallet::RunSaplingSweep(int blockHeight) {
// masked an unsynchronized mutation.) cs_wallet is recursive, so this is // masked an unsynchronized mutation.) cs_wallet is recursive, so this is
// safe even on any path that already holds it. // safe even on any path that already holds it.
LOCK(cs_wallet); LOCK(cs_wallet);
// Stale-baton guard. A successful-but-incomplete sweep round deliberately
// returns with fSweepRunning still set and nextSweep unadvanced (see
// AsyncRPCOperation_sweep::main), as a "continue draining next block" baton.
// But every early return below leaves that baton set WITHOUT re-dispatching,
// and RunSaplingConsolidation -- which is gated on fSweepRunning -- then
// returns without advancing nextConsolidation, so the "consolidation is
// within 5 blocks" blackout at the top of this function never lifts. That
// is a self-sustaining three-way deadlock: sweep waits on consolidation,
// consolidation waits on sweep, and autoshield shares the same gate, so a
// wedged sweep silently disables coinbase shielding forever.
// Only honour the baton while a sweep operation genuinely is in flight.
if (fSweepRunning) {
std::shared_ptr<AsyncRPCQueue> sweepQueue = getAsyncRPCQueue();
std::shared_ptr<AsyncRPCOperation> inFlightSweep =
(sweepQueue != nullptr) ? sweepQueue->getOperationForId(saplingSweepOperationId) : nullptr;
bool inFlight = (inFlightSweep != nullptr) &&
(inFlightSweep->isReady() || inFlightSweep->isExecuting());
if (!inFlight) {
LogPrintf("%s: clearing stale fSweepRunning at blockHeight=%d (no sweep operation in flight)\n",
__func__, blockHeight);
fSweepRunning = false;
}
}
if (!fSweepEnabled) { if (!fSweepEnabled) {
return; return;
} }