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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo

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")

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<std::pair<libzcash::PaymentAddress, uint256>> CWallet::GetNullifiersForAddresses(
         const std::set<libzcash::PaymentAddress> & 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;
 }
 

src/wallet/wallet.h

@@ -833,6 +833,10 @@ public:
     bool fAutoShieldRunning           = false;
 
     std::atomic<bool> 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 <typename WalletDB>
-    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<const uint256, CWalletTx>& 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<std::pair<libzcash::PaymentAddress, uint256>> GetNullifiersForAddresses(const std::set<libzcash::PaymentAddress> & addresses);
     bool IsNoteSaplingChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const libzcash::PaymentAddress & address, const SaplingOutPoint & entry);