From a6b6f80db0bc1bf799ac2ead56d5b8b9decd8652 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 31 Aug 2026 20:52:40 -0500 Subject: [PATCH] wallet: stop an interrupted rescan from hiding funds, and un-latch fAbortRescan Two independent fund-visibility bugs on the rescan path, both pre-existing. 1. AN INTERRUPTED RESCAN RECORDED ITSELF AS COMPLETE. ScanForWalletTransactions returns a bare `int ret` (a found-tx count) on both its abort and shutdown bail-outs, so the caller could not tell "finished" from "stopped at block H". init.cpp then ran, unconditionally: pwalletMain->ScanForWalletTransactions(pindexRescan, true); pwalletMain->SetBestChain(chainActive.GetLocator()); // TIP locator An interrupted scan therefore stamped the wallet as scanned all the way to the chain tip. On the next start, the `chainActive.Tip() != pindexRescan` guard just above sees no work to do and skips the rescan entirely, so every transaction in the never-scanned range stays out of mapWallet -- permanently invisible to getbalance and unspendable. That is exactly the case `-rescan` exists for: a key import, right after ClearNoteWitnessCache has run. Adds CWallet::fLastRescanCompleted, set false at scan entry and true only on the normal exit, and gates the init.cpp locator write on it. Deliberately NOT overloading the int return, which callers already use as a tx count. 2. fAbortRescan WAS NEVER RESET. wallet.h declares it, AbortRescan() sets it true, and nothing in src/ ever sets it false. The abortrescan RPC is live. After one call, for the remaining lifetime of the process: - every ScanForWalletTransactions returns immediately, so re-running an import silently no-ops while the RPC still reports success; - BuildWitnessCache bails on the same flag on EVERY call, including the routine per-block extension from ChainTip. Witness heights then diverge across notes while the chain advances, and GetSaplingNoteWitnesses elects the majority root as the anchor and returns boost::none for every note that disagrees. Those notes still show in the balance but cannot be spent. Cleared at scan entry and consumed in the abort branch. Also in BuildWitnessCache's abort branch: stop clearing fRescanning. A witness rebuild is not a rescan and must not touch a flag ScanForWalletTransactions owns. 3. Both bail-out log lines had two format specifiers and one argument: LogPrintf("%s: Rescan aborted at block %d\n", pwalletMain->rescanHeight); tinyformat's "too many conversion specifiers" guard is disabled in this tree (tinyformat.h: `if(*fmt != '\0' && 0 ) // disabled due to complaints`), so this does not throw -- verified by compiling the exact call against this tree's tinyformat.h. It emits "3841207: Rescan aborted at block " with the height in the __func__ slot, the real height dropped, and the trailing newline swallowed so the next log line concatenates onto it. On the one path an operator has to diagnose an interrupted rescan, the only record was corrupt. Fixed at both sites, and the state updates moved above the log call. 4. Makes SetBestChainINTERNAL report whether the atomic write committed, so a checkpointing caller can tell (six failure paths returned void). Adds SetBestChainNoFlush, which opens the wallet DB with fFlushOnClose=false: the default ctor makes ~CDB run a full BDB txn_checkpoint over the whole cache, which is fine hourly but not from inside a scan -- the same flush wallet.cpp already documents avoiding elsewhere "for performance reasons". Nothing calls it yet. 5. SetBestChainINTERNAL took each CWalletTx by value, deep-copying every note's witness deque (WITNESS_CACHE_SIZE entries) per transaction per call, purely to serialize it. Now by const reference. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo --- src/init.cpp | 15 +++++++++++++-- src/wallet/wallet.cpp | 33 +++++++++++++++++++++++++++------ src/wallet/wallet.h | 32 +++++++++++++++++++++++--------- 3 files changed, 63 insertions(+), 17 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 4efe76104..ce8322dc4 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2654,8 +2654,19 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) nStart = GetTimeMillis(); pwalletMain->ScanForWalletTransactions(pindexRescan, true); LogPrintf(" rescan %15dms\n", GetTimeMillis() - nStart); - pwalletMain->SetBestChain(chainActive.GetLocator()); - nWalletDBUpdated++; + // ONLY record "scanned to the tip" if the scan actually reached it. An aborted or + // shutdown-interrupted scan that stamps a tip locator tells the next startup there is + // nothing left to scan (the chainActive.Tip() != pindexRescan guard above then skips + // the rescan entirely), so every transaction in the unscanned range stays out of + // mapWallet permanently: invisible to getbalance and unspendable. The scan writes its + // own locator at the interrupt point instead. + if (pwalletMain->fLastRescanCompleted) { + pwalletMain->SetBestChain(chainActive.GetLocator()); + nWalletDBUpdated++; + } else { + LogPrintf("Rescan did not complete; leaving the best-block locator at the scan's own " + "checkpoint so the remaining range is rescanned on the next start\n"); + } // Restore wallet transaction metadata after -zapwallettxes=1 if (GetBoolArg("-zapwallettxes", false) && GetArg("-zapwallettxes", "1") != "2") diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ca29538ad..20df4ea69 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -801,10 +801,18 @@ bool CWallet::CommitAutomatedTx(const CTransaction& tx) { void CWallet::SetBestChain(const CBlockLocator& loc) { + // Default ctor => fFlushOnClose=true => ~CDB runs a full BDB txn_checkpoint over the entire + // cache. Fine for the hourly/shutdown callers; ruinous inside a scan (see SetBestChainNoFlush). CWalletDB walletdb(strWalletFile); SetBestChainINTERNAL(walletdb, loc); } +bool CWallet::SetBestChainNoFlush(const CBlockLocator& loc) +{ + CWalletDB walletdb(strWalletFile, "r+", false); + return SetBestChainINTERNAL(walletdb, loc); +} + std::set> CWallet::GetNullifiersForAddresses( const std::set & addresses) { @@ -1436,8 +1444,9 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly) return; } if (pwalletMain->fAbortRescan) { + // Do NOT clear fRescanning here: a witness rebuild is not a rescan, and clearing it from + // this path desynchronises the flag from ScanForWalletTransactions, which owns it. LogPrintf("%s: rescan aborted during witness rebuild\n", __func__); - pwalletMain->fRescanning = false; return; } int h = pbi->GetHeight(); @@ -3434,6 +3443,12 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) if(fZdebug) LogPrintf("%s: fUpdate=%d now=%li\n",__func__,fUpdate,nNow); + // fAbortRescan is sticky: nothing else in the tree ever clears it, so without this a single + // `abortrescan` RPC would disable every later scan AND every later BuildWitnessCache for the + // lifetime of the process (BuildWitnessCache bails on the same flag), freezing witness heights + // while the chain advances and progressively rendering notes unspendable. + pwalletMain->fAbortRescan = false; + pwalletMain->fLastRescanCompleted = false; pwalletMain->fRescanning = true; CBlockIndex* pindex = pindexStart; pwalletMain->rescanStartHeight = pindex->GetHeight(); @@ -3456,15 +3471,18 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) { pwalletMain->rescanHeight = pindex->GetHeight(); if(pwalletMain->fAbortRescan) { - //TODO: should we update witness caches? - LogPrintf("%s: Rescan aborted at block %d\n", pwalletMain->rescanHeight); - pwalletMain->fRescanning = false; + // The witness caches do NOT need updating here: on resume each note's witnesses are + // re-derived from its own witnessHeight, independently of the locator, and + // witnessRootValidated is in-memory-only so a full validation pass runs next boot. + // What DOES need saving is the locator -- see the checkpoint below. + pwalletMain->fRescanning = false; + pwalletMain->fAbortRescan = false; // consume it; see the note at scan entry + LogPrintf("%s: Rescan aborted at block %d\n", __func__, pwalletMain->rescanHeight); return ret; } if (ShutdownRequested()) { - //TODO: should we update witness caches? - LogPrintf("%s: Rescan interrupted by shutdown request at block %d\n", pwalletMain->rescanHeight); pwalletMain->fRescanning = false; + LogPrintf("%s: Rescan interrupted by shutdown request at block %d\n", __func__, pwalletMain->rescanHeight); return ret; } @@ -3525,6 +3543,9 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) // we are no longer rescanning pwalletMain->fRescanning = false; + // Reached only by running the loop to the end of the active chain. Callers use this to decide + // whether it is honest to record the wallet as scanned up to the tip. + pwalletMain->fLastRescanCompleted = true; return ret; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 95527738b..107814971 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -833,6 +833,10 @@ public: bool fAutoShieldRunning = false; std::atomic fAbortRescan{false}; + //! True only when the last ScanForWalletTransactions ran to completion. An aborted or + //! shutdown-interrupted scan leaves this false, so callers must not record the wallet as + //! scanned up to the chain tip -- doing so makes the unscanned range permanently invisible. + bool fLastRescanCompleted = false; // abort current rescan void AbortRescan() { fAbortRescan = true; } // Are we currently aborting a rescan? @@ -904,17 +908,22 @@ protected: */ void DecrementNoteWitnesses(const CBlockIndex* pindex); + //! Returns true only if the atomic write actually committed. Callers that checkpoint during a + //! long scan need to know: a silently-failing checkpoint would otherwise be retried forever at + //! full cost while never making progress. template - void SetBestChainINTERNAL(WalletDB& walletdb, const CBlockLocator& loc) { + bool SetBestChainINTERNAL(WalletDB& walletdb, const CBlockLocator& loc) { if (!walletdb.TxnBegin()) { // This needs to be done atomically, so don't do it at all LogPrintf("SetBestChain(): Couldn't start atomic write\n"); - return; + return false; } try { LOCK(cs_wallet); for (std::pair& wtxItem : mapWallet) { - auto wtx = wtxItem.second; + // By reference: a copy here deep-copies every note's witness deque + // (WITNESS_CACHE_SIZE entries) for every transaction, on every call. + const CWalletTx& wtx = wtxItem.second; // We skip transactions for which mapSaplingNoteData // is empty. This covers transactions that have no Sapling data // (i.e. are purely transparent), as well as shielding and unshielding @@ -923,32 +932,33 @@ protected: if (!walletdb.WriteTx(wtxItem.first, wtx)) { LogPrintf("SetBestChain(): Failed to write CWalletTx, aborting atomic write\n"); walletdb.TxnAbort(); - return; + return false; } } } if (!walletdb.WriteWitnessCacheSize(nWitnessCacheSize)) { LogPrintf("SetBestChain(): Failed to write nWitnessCacheSize, aborting atomic write\n"); walletdb.TxnAbort(); - return; + return false; } if (!walletdb.WriteBestBlock(loc)) { LogPrintf("SetBestChain(): Failed to write best block, aborting atomic write\n"); walletdb.TxnAbort(); - return; + return false; } } catch (const std::exception &exc) { // Unexpected failure LogPrintf("SetBestChain(): Unexpected error during atomic write:\n"); LogPrintf("%s\n", exc.what()); walletdb.TxnAbort(); - return; + return false; } if (!walletdb.TxnCommit()) { // Couldn't commit all to db, but in-memory state is fine LogPrintf("SetBestChain(): Couldn't commit atomic write\n"); - return; + return false; } + return true; } private: @@ -1282,8 +1292,12 @@ public: void RunSaplingConsolidation(int blockHeight); void RunAutoShieldCoinbase(int blockHeight); bool CommitAutomatedTx(const CTransaction& tx); - /** Saves witness caches and best block locator to disk. */ + /** Saves witness caches and best block locator to disk. Overrides CValidationInterface. */ void SetBestChain(const CBlockLocator& loc); + /** As SetBestChain, but for use INSIDE a long scan: opens the wallet DB with fFlushOnClose=false + * so the call does not trigger a full BDB txn_checkpoint over the whole cache (see the comment + * at CWallet::SetBestChain), and reports whether the write actually committed. */ + bool SetBestChainNoFlush(const CBlockLocator& loc); std::set> GetNullifiersForAddresses(const std::set & addresses); bool IsNoteSaplingChange(const std::set> & nullifierSet, const libzcash::PaymentAddress & address, const SaplingOutPoint & entry);