audit: fix ADDR send amplification and the IVK-only witness deref (UNTESTED)

Two more findings from the general audit. Compile-verified only, like the rest of
this branch.

main.cpp SendMessages / ADDR: the per-address loop called
    pto->PushAddrMessage(msgMaker.Make(flags, msg_type, pto->vAddrToSend))
i.e. it pushed the ENTIRE vector once per accepted address. One 24-byte getaddr
therefore produced N messages of N addresses each instead of one message of N --
on a live addrman (GetAddr returning ~494) that is ~494 x ~14.8 KB = ~7.3 MB of
upstream for a 24-byte request, and up to ~30 MB with a full addrman. All of it
serialized, with a double-SHA256 checksum per message, while cs_main is held, so
each burst also stalls block validation and RPC.

The locally built vAddr was accumulated and then discarded, and the
`vAddr.resize(MAX_ADDR_TO_SEND)` sat exactly where upstream has `vAddr.clear()` --
it can never fire, because vAddr.size() already equals MAX_ADDR_TO_SEND there.
This is a local regression, not inherited: 512da314a rewrote the correct upstream
form into this one.

Restores the upstream idiom (accumulate, flush in MAX_ADDR_TO_SEND batches, send
the remainder after the loop) and hoists the msg_type/make_flags selection out of
the loop, since neither varies per address.

wallet.cpp VerifyAndSetInitialWitness: five sites dereferenced
`*item.second.nullifier` on a boost::optional that can legitimately be unset. A
Sapling note discovered through an imported INCOMING viewing key has no nullifier
-- computing one requires the full viewing key, and z_importviewingkey calls only
AddSaplingIncomingViewingKey. Dereferencing it aborts the daemon with a boost
assertion rather than an RPC error, at the end of the import's own rescan; and
because the IVK-only note data is already persisted in the wallet transaction and
BuildWitnessCache is re-driven from ChainTip on every connected block, the node
then fails the same way on every restart.

The codebase already had the right pattern in the two neighbouring functions --
DecrementNoteWitnesses guards with `if (nd->nullifier && ...)` and BuildWitnessCache
with `if (!nd->nullifier) continue;` -- so only VerifyAndSetInitialWitness was
missing it. Those guards also establish the intended semantics: a note whose spend
depth cannot be computed is treated as UNSPENT, keeping its witness rather than
pruning a note we cannot prove spent. Adds a boost::optional overload of
SaplingWitnessMinimumHeight doing exactly that, and routes all five sites through it.

NOT fixed here, deliberately, because none can be done responsibly without tests:
  - the inline TLS handshake on ThreadSocketHandler (architectural: one slow
    unauthenticated connection can freeze all P2P I/O for up to 60s);
  - the getblocktemplate function-static leaking a CBlockTemplate per concurrent
    caller, assigned with cs_main released;
  - z_sendmany's pre-flight size estimate omitting Sapling spends, so a wallet with
    ~499+ notes builds an oversize, unbroadcastable transaction after minutes of
    proving;
  - z_shieldcoinbase's opt-in donation paying a hardcoded upstream-Hush z-address
    that no DragonX party can spend (a policy decision, not a code fix).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo

src/bitcoin-cli.cpp

@@ -257,14 +257,18 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
     event_base_dispatch(base.get());
 
     if (response.status == 0) {
+        // Report the port we ACTUALLY dialled. ASSETCHAINS_RPCPORT is a separate global that is
+        // initialised to the mainnet default at the top of this file and never assigned here, so
+        // using it made every failure claim port 21769 no matter what -rpcport was given -- which
+        // reads as "your -rpcport was ignored" and sends you chasing a config bug that isn't there.
         throw CConnectionFailed(strprintf("couldn't connect to server at port %d : %s (code %d)\n(make sure server is running and you are connecting to the correct RPC port)", 
-            ASSETCHAINS_RPCPORT, http_errorstring(response.error), response.error));
+            port, http_errorstring(response.error), response.error));
     } else if (response.status == HTTP_UNAUTHORIZED) {
         throw std::runtime_error("incorrect rpcuser or rpcpassword (authorization failed)");
     } else if (response.status >= 400 && response.status != HTTP_BAD_REQUEST && response.status != HTTP_NOT_FOUND && response.status != HTTP_INTERNAL_SERVER_ERROR) {
         throw std::runtime_error(strprintf("server returned HTTP error %d", response.status));
     } else if (response.body.empty()) {
-        throw std::runtime_error(strprintf("no response from server at port %d", ASSETCHAINS_RPCPORT ));
+        throw std::runtime_error(strprintf("no response from server at port %d", port));
     }
 
     // Parse reply

src/httpserver.cpp

@@ -420,6 +420,10 @@ static void libevent_log_cb(int severity, const char *msg)
         LogPrint("libevent", "libevent: %s\n", msg);
 }
 
+/** Cap on the combined size of an HTTP request line + headers. libevent's default is EV_SIZE_MAX,
+ *  i.e. unbounded, and it buffers before any ACL or auth check runs. */
+static const size_t MAX_HEADERS_SIZE = 8192;
+
 bool InitHTTPServer()
 {
     struct evhttp* http = 0;
@@ -467,6 +471,10 @@ bool InitHTTPServer()
 
     evhttp_set_timeout(http, GetArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT));
     evhttp_set_max_body_size(http, MAX_SIZE);
+    // libevent defaults max_headers_size to EV_SIZE_MAX, so without this a single connection can
+    // stream an unbounded request line / header block and grow RSS ~1:1 with bytes sent, BEFORE the
+    // -rpcallowip ACL or auth check runs (both happen after libevent has parsed the request).
+    evhttp_set_max_headers_size(http, MAX_HEADERS_SIZE);
     evhttp_set_gencb(http, http_request_cb, NULL);
 
     if (!HTTPBindAddresses(http)) {

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/main.cpp

@@ -8336,6 +8336,24 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
         // Message: addr
         if (fSendTrickle)
         {
+            // Accumulate into vAddr and send it ONCE (or in MAX_ADDR_TO_SEND-sized batches).
+            // This loop previously pushed the ENTIRE pto->vAddrToSend on every accepted address,
+            // so a single 24-byte getaddr produced N messages of N addresses each instead of one
+            // message of N -- ~500x the intended bandwidth on a typical addrman, all serialized
+            // (with per-message double-SHA256 checksums) while cs_main is held. The locally built
+            // vAddr was accumulated and then discarded, and the vAddr.resize(MAX_ADDR_TO_SEND) was
+            // a no-op standing where upstream has vAddr.clear().
+            const char* msg_type;
+            int make_flags;
+            if (pto->m_wants_addrv2) {
+                msg_type = NetMsgType::ADDRV2;
+                make_flags = ADDRV2_FORMAT;
+            } else {
+                msg_type = NetMsgType::ADDR;
+                make_flags = 0;
+            }
+            const CNetMsgMaker msgMaker(std::min(pto->nVersion, PROTOCOL_VERSION));
+
             vector<CAddress> vAddr;
             vAddr.reserve(pto->vAddrToSend.size());
             BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend)
@@ -8343,29 +8361,17 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
                 if (pto->AddAddressIfNotAlreadyKnown(addr))
                 {
                     vAddr.push_back(addr);
-
                     if (vAddr.size() >= MAX_ADDR_TO_SEND)
                     {
-                      // Should be impossible since we always check size before adding to
-                      // vAddrToSend. Recover by trimming the vector.
-                      vAddr.resize(MAX_ADDR_TO_SEND);
+                        pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
+                        vAddr.clear();
                     }
-                    const char* msg_type;
-                    int make_flags;
-                    if (pto->m_wants_addrv2) {
-                        msg_type = NetMsgType::ADDRV2;
-                        make_flags = ADDRV2_FORMAT;
-                    } else {
-                        msg_type = NetMsgType::ADDR;
-                        make_flags = 0;
-                    }
-                    pto->PushAddrMessage(CNetMsgMaker(std::min(pto->nVersion, PROTOCOL_VERSION)).Make(make_flags, msg_type, pto->vAddrToSend));
-
                 }
             }
 
             pto->vAddrToSend.clear();
-            vAddr.clear();
+            if (!vAddr.empty())
+                pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
         }
 
         CNodeState &state = *State(pto->GetId());

src/net.h

@@ -607,7 +607,11 @@ public:
         // Known checking here is only to save space from duplicates.
         // SendMessages will filter it again for knowns that were added
         // after addresses were pushed.
-        if (_addr.IsValid() && !IsAddressKnown(addr) && addr_format_supported) {
+        // NOTE: _addr (the address being queued), NOT addr (this peer's own address, net.h ~416).
+        // Testing the member made the filter constant for the connection's lifetime: once the peer's
+        // own address entered its addrKnown -- routine, via the remote's AdvertizeLocal -- every
+        // relay path to it silently no-opped until the daily addrKnown.reset().
+        if (_addr.IsValid() && !IsAddressKnown(_addr) && addr_format_supported) {
 
             if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) {
                 vAddrToSend[insecure_rand() % vAddrToSend.size()] = _addr;

src/rpc/blockchain.cpp

@@ -534,6 +534,10 @@ UniValue getblockdeltas(const UniValue& params, bool fHelp, const CPubKey& mypk)
     if (fHelp || params.size() != 1)
         throw runtime_error("");
 
+    // Reads mapBlockIndex / chainActive (and, below, pcoinsTip's mutable anchor cache),
+    // all of which are cs_main-guarded. Every sibling RPC in this file locks; this one did not.
+    LOCK(cs_main);
+
     std::string strHash = params[0].get_str();
     uint256 hash(uint256S(strHash));
 
@@ -602,11 +606,19 @@ UniValue getblockhashes(const UniValue& params, bool fHelp, const CPubKey& mypk)
 
     std::vector<std::pair<uint256, unsigned int> > blockHashes;
 
-    if (fActiveOnly)
+    {
+        // The lock must SPAN GetTimestampIndex: with fActiveOnly it calls blockOnchainActive() for
+        // every row, which reads mapBlockIndex and chainActive. The previous form was
+        //     if (fActiveOnly)
+        //         LOCK(cs_main);
+        // and LOCK() declares a scoped object, so as an unbraced substatement it was constructed
+        // and destroyed on that line -- the walk then ran completely unsynchronised. Taken
+        // unconditionally here: this RPC is explorer-only and not hot, and a conditional lock is
+        // exactly the shape that produced the bug.
         LOCK(cs_main);
-
-    if (!GetTimestampIndex(high, low, fActiveOnly, blockHashes)) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for block hashes");
+        if (!GetTimestampIndex(high, low, fActiveOnly, blockHashes)) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for block hashes");
+        }
     }
 
     UniValue result(UniValue::VARR);
@@ -877,6 +889,10 @@ UniValue getblockmerkletree(const UniValue& params, bool fHelp, const CPubKey& m
             + HelpExampleRpc("getblockmerkletree", "290000")
         );
 
+    // Reads mapBlockIndex / chainActive (and, below, pcoinsTip's mutable anchor cache),
+    // all of which are cs_main-guarded. Every sibling RPC in this file locks; this one did not.
+    LOCK(cs_main);
+
     CBlockIndex* phushblockindex;
     uint256 blockRoot;
     SaplingMerkleTree tree;

src/rpc/mining.cpp

@@ -759,9 +759,25 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp, const CPubKey& myp
 #ifdef ENABLE_WALLET
         CReserveKey reservekey(pwalletMain);
         LEAVE_CRITICAL_SECTION(cs_main);
-        pblocktemplate = CreateNewBlockWithKey(reservekey,pindexPrevNew->GetHeight()+1,HUSH_MAXGPUCOUNT,false);
+        // MUST re-enter cs_main before letting an exception escape. The enclosing LOCK(cs_main) is
+        // a scoped CMutexLock whose owns_lock is still true, so if CreateNewBlockWithKey throws
+        // (any wallet/BDB fault: disk full, EMFILE, a corrupt wallet.dat) its destructor unlocks an
+        // already-unlocked mutex during unwinding -> BOOST_VERIFY -> SIGABRT. Asserts cannot be
+        // compiled out here (main.cpp #errors on NDEBUG), so this aborts the daemon instead of
+        // returning the actionable error, and the abort happens inside unwinding so nothing is logged.
+        try {
+            pblocktemplate = CreateNewBlockWithKey(reservekey,pindexPrevNew->GetHeight()+1,HUSH_MAXGPUCOUNT,false);
+        } catch (...) {
+            ENTER_CRITICAL_SECTION(cs_main);
+            throw;
+        }
 #else
-        pblocktemplate = CreateNewBlockWithKey();
+        try {
+            pblocktemplate = CreateNewBlockWithKey();
+        } catch (...) {
+            ENTER_CRITICAL_SECTION(cs_main);
+            throw;
+        }
 #endif
         ENTER_CRITICAL_SECTION(cs_main);
         if (!pblocktemplate)

src/wallet/rpcdump.cpp

@@ -299,7 +299,10 @@ UniValue importprivkey(const UniValue& params, bool fHelp, const CPubKey& mypk)
     bool fRescan = true;
     if (params.size() > 2)
         fRescan = params[2].get_bool();
-    if ( fRescan && params.size() == 4 )
+    // '> 3', not '== 4': with the optional 5th (secret_key) argument present the equality test
+    // failed and height silently stayed 0, rescanning from genesis. Every sibling RPC in this file
+    // already uses the '>' form.
+    if ( fRescan && params.size() > 3 )
         height = params[3].get_int();
 
 

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)
 {
@@ -1232,6 +1240,16 @@ int CWallet::SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessH
     return nMinimumHeight;
 }
 
+int CWallet::SaplingWitnessMinimumHeight(const boost::optional<uint256>& nullifier, int nWitnessHeight, int nMinimumHeight)
+{
+    // No nullifier => an incoming-viewing-key-only note (z_importviewingkey without the full
+    // viewing key). Spend depth is unknowable, so treat it as unspent and keep its witness.
+    if (!nullifier) {
+        return min(nWitnessHeight, nMinimumHeight);
+    }
+    return SaplingWitnessMinimumHeight(*nullifier, nWitnessHeight, nMinimumHeight);
+}
+
 int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessOnly)
 {
   LOCK2(cs_main, cs_wallet);
@@ -1269,7 +1287,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
 
           //Skip Validation when witness root has been validated
           if (nd->witnessRootValidated) {
-            nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+            nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
             continue;
           }
 
@@ -1281,12 +1299,12 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
             CBlockIndex* whIndex = chainActive[nd->witnessHeight];
             if (whIndex == NULL) {
               //witnessHeight strictly above the active chain (transient catch-up): cannot validate yet
-              nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+              nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
               continue;
             }
             if (nd->witnesses.front().root() == whIndex->hashFinalSaplingRoot) {
               nd->witnessRootValidated = true;
-              nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+              nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
               continue;
             }
             //root mismatch on the active chain -> desynced; fall through to rebuild below
@@ -1298,7 +1316,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
           blockRoot = pblockindex->hashFinalSaplingRoot;
           if (witnessRoot == blockRoot) {
             nd->witnessRootValidated = true;
-            nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+            nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
             continue;
           }
         }
@@ -1350,7 +1368,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
         }
         nd->witnessHeight = pblockindex->GetHeight();
         UpdateSaplingNullifierNoteMapWithTx(wtxItem.second);
-        nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight);
+        nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight);
       }
     }
   }
@@ -1436,8 +1454,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 +3453,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();
@@ -3448,6 +3473,51 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate)
             pwalletMain->rescanHeight = pindex ? pindex->GetHeight() : 0;
         }
 
+        // --- interrupt checkpoint setup -------------------------------------------------------
+        // Where does the wallet currently believe it has scanned to? A checkpoint may only ever
+        // ADVANCE that point, and only if this scan is contiguous with it. The RPC entry points
+        // (rescan / importprivkey / z_importkey / z_importviewingkey) take a caller-supplied start
+        // height validated only against chainActive.Height(), so a scan can legitimately begin far
+        // ABOVE the persisted locator -- writing a checkpoint from such a scan would mark the
+        // skipped range as scanned and hide any funds in it.
+        CBlockIndex* pindexPersisted = NULL;
+        {
+            CWalletDB walletdb(strWalletFile, "r+", false);
+            CBlockLocator locPersisted;
+            if (walletdb.ReadBestBlock(locPersisted))
+                pindexPersisted = FindForkInGlobalIndex(chainActive, locPersisted);
+        }
+        const bool fMayCheckpoint = pindexPersisted != NULL &&
+            pindexStart->GetHeight() <= pindexPersisted->GetHeight() + 1;
+        if (!fMayCheckpoint) {
+            LogPrintf("%s: scan starts at %d but the wallet is persisted at %d; progress will NOT be "
+                      "checkpointed on interrupt (a non-contiguous scan cannot safely advance the locator)\n",
+                      __func__, pindexStart->GetHeight(),
+                      pindexPersisted ? pindexPersisted->GetHeight() : -1);
+        }
+
+        // Persist progress when the scan is cut short. `pindexStopped` is the block we were ABOUT to
+        // scan, so the last fully-processed block is its parent. Resume restarts AT the locator's own
+        // block (CChain::GetLocator pushes it first; FindForkInGlobalIndex returns it), giving one
+        // block of deliberate overlap -- idempotent, because AddToWallet only merges when the tx is
+        // already present. No witness work is needed: witnesses are re-derived from each note's own
+        // witnessHeight, and witnessRootValidated is in-memory-only so every note is revalidated
+        // against hashFinalSaplingRoot on the next start.
+        auto checkpointProgress = [&](const CBlockIndex* pindexStopped) {
+            if (!fMayCheckpoint || !pindexStopped || !pindexStopped->pprev)
+                return;
+            const CBlockIndex* pindexDone = pindexStopped->pprev;
+            if (pindexDone->GetHeight() <= pindexPersisted->GetHeight())
+                return;   // never move the locator backwards
+            if (SetBestChainNoFlush(chainActive.GetLocator(pindexDone))) {
+                LogPrintf("%s: checkpointed scan progress at height %d\n", __func__, pindexDone->GetHeight());
+            } else {
+                LogPrintf("%s: FAILED to checkpoint scan progress at height %d; the scan will replay "
+                          "from height %d on the next start\n", __func__, pindexDone->GetHeight(),
+                          pindexPersisted->GetHeight());
+            }
+        };
+
         ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup
         double dProgressStart = Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), pindex, false);
         double dProgressTip = Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), chainActive.LastTip(), false);
@@ -3456,15 +3526,20 @@ 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);
+                checkpointProgress(pindex);
                 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);
+                checkpointProgress(pindex);
                 return ret;
             }
 
@@ -3525,6 +3600,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?
@@ -892,6 +896,12 @@ public:
 protected:
 
     int SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessHeight, int nMinimumHeight);
+    //! Overload for a note whose nullifier may be unset. A note discovered through an imported
+    //! INCOMING viewing key has no nullifier (computing one needs the full viewing key), so
+    //! dereferencing the optional aborts the daemon. Treats such a note as unspent, which is the
+    //! conservative direction: it keeps the witness alive rather than pruning a note we cannot
+    //! prove spent.
+    int SaplingWitnessMinimumHeight(const boost::optional<uint256>& nullifier, int nWitnessHeight, int nMinimumHeight);
 
     /**
      * pindex is the new tip being connected.
@@ -904,17 +914,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 +938,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 +1298,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);