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
audit: mechanical fixes from the general audit (UNTESTED - see below)
2026-08-31 22:11:42 -05:00 · 2026-08-31 22:08:29 -05:00 · 2026-08-31 20:54:37 -05:00 · 2026-08-31 20:52:40 -05:00 · 2026-08-31 17:35:52 -05:00 · 2026-08-31 13:29:43 -05:00
14 changed files with 487 additions and 79 deletions
--- a/qa/r4-salvaged-wallet-harness.sh
+++ b/qa/r4-salvaged-wallet-harness.sh
@@ -0,0 +1,87 @@
+#!/usr/bin/env bash
+# r4 now-tier acceptance harness. MUST run inside a network namespace: the v1.0.x binaries
+# predate the regtest seed-injection fix and would otherwise dial the live DragonX network.
+set -u
+OLD=/home/dev/dragonx/release/dragonx-1.0.1-linux-amd64
+NEW=/home/dev/dragonx-dev/src
+ROOT=/tmp/claude-1000/-home-dev/45a644d6-ea0b-4b7c-8ec7-6ccb1a64afa7/scratchpad/r4lab
+PASS=0; FAIL=0
+ok(){ echo "  PASS  $1"; PASS=$((PASS+1)); }
+no(){ echo "  FAIL  $1"; FAIL=$((FAIL+1)); }
+
+# --- hard guardrail: refuse to run unisolated ---
+if [ "$(ip route show 2>/dev/null | wc -l)" != "0" ]; then
+  echo "REFUSING: not in an isolated netns (routes present). Run under: unshare -rn"; exit 90
+fi
+ip link set lo up 2>/dev/null
+echo "isolation: $(ip route show | wc -l) routes, $(ip -o link | wc -l) interface(s)"
+
+conf(){ printf 'regtest=1\nrpcuser=t\nrpcpassword=t\nlisten=0\ndnsseed=0\n' > "$1/DRAGONX.conf"; }
+start(){ # $1=bindir $2=datadir $3=extra
+  "$1/dragonxd" -regtest -datadir="$2" -connect=0 -listen=0 -dnsseed=0 $3 -daemon >/dev/null 2>&1
+  for i in $(seq 40); do "$1/dragonx-cli" -regtest -datadir="$2" -rpcuser=t -rpcpassword=t getblockcount >/dev/null 2>&1 && return 0; sleep 2; done
+  return 1; }
+cli(){ "$1/dragonx-cli" -regtest -datadir="$2" -rpcuser=t -rpcpassword=t "${@:3}" 2>&1; }
+stopn(){ cli "$1" "$2" stop >/dev/null 2>&1; sleep 6; }
+
+rm -rf "$ROOT"; mkdir -p "$ROOT"
+
+echo; echo "### build victim wallets with the OLD binary (v1.0.1) ###"
+mkdir -p "$ROOT/base/regtest"; conf "$ROOT/base/regtest"
+start "$OLD" "$ROOT/base/regtest" "" || { echo "old node failed to start"; exit 91; }
+[ "$(cli "$OLD" "$ROOT/base/regtest" getconnectioncount)" = "0" ] && ok "victim-maker has 0 peers (isolated)" || no "victim-maker NOT isolated -- ABORT"
+cli "$OLD" "$ROOT/base/regtest" getnewaddress   >/dev/null
+cli "$OLD" "$ROOT/base/regtest" z_getnewaddress >/dev/null
+ZBEFORE=$(cli "$OLD" "$ROOT/base/regtest" z_listaddresses | tr -d ' \n')
+stopn "$OLD" "$ROOT/base/regtest"
+
+for v in C A B; do cp -a "$ROOT/base" "$ROOT/$v"; done
+
+# A = salvaged by v1.0.1 (drops hdchain).  B = A then USED on v1.0.1 (persists a bogus chain).
+start "$OLD" "$ROOT/A/regtest" "-salvagewallet" && stopn "$OLD" "$ROOT/A/regtest"
+start "$OLD" "$ROOT/B/regtest" "-salvagewallet" && stopn "$OLD" "$ROOT/B/regtest"
+start "$OLD" "$ROOT/B/regtest" "" && { cli "$OLD" "$ROOT/B/regtest" z_getnewaddress >/dev/null; stopn "$OLD" "$ROOT/B/regtest"; }
+
+echo "  A hdchain records: $(strings "$ROOT/A/regtest/regtest/wallet.dat" | grep -c hdchain)  (expect 0)"
+echo "  B hdchain records: $(strings "$ROOT/B/regtest/regtest/wallet.dat" | grep -c hdchain)  (expect >=1)"
+
+echo; echo "### open each on the NEW binary ###"
+for v in C A B; do
+  D="$ROOT/$v/regtest"; L="$D/regtest/debug.log"
+  WDAT="$D/regtest/wallet.dat"
+  [ -f "$WDAT" ] || { echo "  FAIL  $v: wallet.dat not found at $WDAT"; exit 92; }
+  # NOT a byte-identical check: normal startup (keypool top-up, bestblock) rewrites wallet.dat for
+  # ANY wallet, healthy ones included -- verified with a control. The precise claim is that the
+  # degraded path never SYNTHESISES an hdchain record, so count that instead.
+  HD1=$(strings "$WDAT" | grep -c hdchain)
+  : > "$L" 2>/dev/null
+  if start "$NEW" "$D" "-exportdir=$D/exp"; then
+    STARTED=yes; DEG=$(grep -c "DEGRADED" "$L" 2>/dev/null)
+    MISS=$(grep -c "hdchain record is missing" "$L" 2>/dev/null)
+    MISM=$(grep -c "does not belong to this wallet" "$L" 2>/dev/null)
+    if [ "$v" = "A" ]; then
+      mkdir -p "$D/exp"; EXP=$(cli "$NEW" "$D" z_exportwallet r4dump 2>&1 | head -1)
+      DUMP=$(find "$D" -name 'r4dump' 2>/dev/null | head -1)
+      ZNEW=$(cli "$NEW" "$D" z_getnewaddress); TNEW=$(cli "$NEW" "$D" getnewaddress)
+    fi
+    stopn "$NEW" "$D"
+  else STARTED=no; DEG=0; MISS=0; MISM=0; fi
+  HD2=$(strings "$WDAT" | grep -c hdchain)
+
+  case $v in
+    C) [ "$STARTED" = yes ] && ok "C healthy wallet opens" || no "C healthy wallet failed to open"
+       [ "$DEG" = "0" ] && ok "C no false positive (not flagged degraded)" || no "C FALSE POSITIVE: healthy wallet flagged" ;;
+    A) [ "$STARTED" = yes ] && ok "A salvaged wallet opens (was DB_CORRUPT before)" || no "A salvaged wallet still refuses to open"
+       [ "$MISS" -ge 1 ] && ok "A flagged: hdchain missing" || no "A not flagged as missing-hdchain"
+       [ "$HD1" = "0" ] && [ "$HD2" = "0" ] && ok "A no hdchain synthesised (degraded path persists nothing)" || no "A hdchain record appeared ($HD1 -> $HD2)"
+       echo "$ZNEW" | grep -qi 'error' && ok "A z_getnewaddress refused cleanly (derivation gated)" || no "A z_getnewaddress derived anyway: $ZNEW"
+       echo "$TNEW" | grep -qiE '^R[a-zA-Z0-9]+$' && ok "A getnewaddress still works (legacy random t-key)" || no "A getnewaddress broke: $TNEW"
+       if [ -n "${DUMP:-}" ] && [ -f "$DUMP" ]; then
+         grep -qE '^# HDSeed=[0-9a-f]' "$DUMP" && no "E z_exportwallet emitted an HDSeed line on a degraded wallet" || ok "E z_exportwallet emitted no bogus HDSeed line"
+       else echo "  SKIP  E (no dump produced: $EXP)"; fi ;;
+    B) [ "$STARTED" = yes ] && ok "B poisoned wallet opens" || no "B poisoned wallet failed to open"
+       [ "$MISM" -ge 1 ] && ok "B CASE-3 DETECTOR FIRED (seedFp mismatch)" || no "B case-3 detector did NOT fire" ;;
+  esac
+done
+echo; echo "### $PASS passed, $FAIL failed ###"
+exit $FAIL
--- a/src/bitcoin-cli.cpp
+++ b/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
--- a/src/httpserver.cpp
+++ b/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)) {
--- a/src/hush_nSPV_fullnode.h
+++ b/src/hush_nSPV_fullnode.h
@@ -660,7 +660,7 @@ void hush_nSPVreq(CNode *pfrom,std::vector<uint8_t> request) // received a reque
                struct NSPV_utxosresp U;
                if ( len >= 2 && len < 64+5 && request[1] < 64 && (request[1] == len-3 || request[1] == len-7 || request[1] == len-11) )
                {
-                    int32_t skipcount = 0; char coinaddr[64]; uint32_t filter; uint8_t isCC = 0;
+                    int32_t skipcount = 0; char coinaddr[64]; uint32_t filter = 0; uint8_t isCC = 0;  // only assigned on the len-11 form; the other two passed it on uninitialized
                    memcpy(coinaddr,&request[2],request[1]);   // request[1] < 64 bounds the copy + the terminator write
                    coinaddr[request[1]] = 0;
                    if ( request[1] == len-3 )
@@ -698,7 +698,7 @@ void hush_nSPVreq(CNode *pfrom,std::vector<uint8_t> request) // received a reque
                struct NSPV_txidsresp T;
                if ( len >= 2 && len < 64+5 && request[1] < 64 && (request[1] == len-3 || request[1] == len-7 || request[1] == len-11) )
                {
-                    int32_t skipcount = 0; char coinaddr[64]; uint32_t filter; uint8_t isCC = 0;
+                    int32_t skipcount = 0; char coinaddr[64]; uint32_t filter = 0; uint8_t isCC = 0;  // only assigned on the len-11 form; the other two passed it on uninitialized
                    memcpy(coinaddr,&request[2],request[1]);   // request[1] < 64 bounds the copy + the terminator write
                    coinaddr[request[1]] = 0;
                    if ( request[1] == len-3 )
--- a/src/hush_utils.h
+++ b/src/hush_utils.h
@@ -1783,11 +1783,21 @@ void hush_args(char *argv0)
    fprintf(stderr,".oO Starting %s Full Node (Extreme Privacy!) with genproc=%d notary=%d\n",name.c_str(),HUSH_MININGTHREADS, IS_HUSH_NOTARY);

    vector<string> DRAGONX_nodes = {};
-    // Only DRAGONX connects to these by default, other chains must opt-in via -connect/-addnode
+    // Only DRAGONX connects to these by default, other chains must opt-in via -connect/-addnode.
+    // Never on regtest or testnet: regtest reuses mainnet's network magic, so injecting the mainnet
+    // seeds here makes a supposedly isolated node handshake production peers and pull their headers
+    // into its own index -- which is exactly what it did, and why multi-node rpc-tests were talking
+    // to the live chain. NOTE: hush_args() runs between ParseParameters() and ReadConfigFile()
+    // (bitcoind.cpp:115/144/158), so this sees a command-line -regtest (how qa/rpc-tests starts
+    // nodes) but NOT a bare "regtest=1" in the config file.
    const bool isdragonx = strncmp(name.c_str(), "DRAGONX",7) == 0 ? true : false;

-    LogPrint("net", "%s: isdragonx=%d\n", __func__, isdragonx);
-    if (isdragonx) {
+    // Seed injection only -- isdragonx itself must stay true here, because it also selects
+    // ac_private, ac_algo, blocktime and the reward/halving schedule below.
+    const bool isnotmainnet = GetBoolArg("-regtest", false) || GetBoolArg("-testnet", false);
+
+    LogPrint("net", "%s: isdragonx=%d isnotmainnet=%d\n", __func__, isdragonx, isnotmainnet);
+    if (isdragonx && !isnotmainnet) {
        // node8-node10 are PLACEHOLDERS with no DNS records yet. A hostname that
        // does not resolve is harmless here: ThreadOpenAddedConnections just fails
        // to open the connection and retries on its 2-minute cycle. Reserving the
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -2260,10 +2260,22 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
        if (nLoadWalletRet != DB_LOAD_OK)
        {
            if (nLoadWalletRet == DB_CORRUPT)
-                strErrors << _("Error loading wallet.dat: Wallet corrupted. If this wallet was last opened "
-                               "by an older version, move wallet.dat aside and restore from your seed "
-                               "phrase with -mnemonic=\"<your seed phrase>\" -rescan (see debug.log for "
-                               "the specific record at fault).") << "\n";
+            {
+                // Abort HERE, as the DB_NEED_REWRITE branch below already does. Falling through
+                // runs several hundred more lines of initialisation against a wallet we have just
+                // declared corrupt -- including SetHDSeedOrigin(), which WRITES to it, and the
+                // rescan and SetBestChain that follow.
+                //
+                // The old text advised restoring with -mnemonic. That is wrong twice over:
+                // -usemnemonic defaulted to 0 in v1.0.3 so many such wallets never had a phrase,
+                // and SetHDSeedFromMnemonic refuses a non-empty wallet, so "move wallet.dat aside"
+                // would discard every non-HD key the salvage preserved.
+                strErrors << _("Error loading wallet.dat: the wallet database is corrupt. Your keys may "
+                               "still be intact -- do NOT delete or replace wallet.dat. Back it up now, "
+                               "and see debug.log for the specific record at fault.") << "\n";
+                LogPrintf("%s", strErrors.str());
+                return InitError(strErrors.str());
+            }
            else if (nLoadWalletRet == DB_NONCRITICAL_ERROR)
            {
                string msg(_("Warning: error reading wallet.dat! All keys read correctly, but transaction data"
@@ -2642,8 +2654,19 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
            nStart = GetTimeMillis();
            pwalletMain->ScanForWalletTransactions(pindexRescan, true);
            LogPrintf(" rescan      %15dms\n", GetTimeMillis() - nStart);
+            // 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")
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -8336,20 +8336,13 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
        // Message: addr
        if (fSendTrickle)
        {
-            vector<CAddress> vAddr;
-            vAddr.reserve(pto->vAddrToSend.size());
-            BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend)
-            {
-                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);
-                    }
+            // 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) {
@@ -8359,13 +8352,26 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
                msg_type = NetMsgType::ADDR;
                make_flags = 0;
            }
-                    pto->PushAddrMessage(CNetMsgMaker(std::min(pto->nVersion, PROTOCOL_VERSION)).Make(make_flags, msg_type, pto->vAddrToSend));
+            const CNetMsgMaker msgMaker(std::min(pto->nVersion, PROTOCOL_VERSION));

+            vector<CAddress> vAddr;
+            vAddr.reserve(pto->vAddrToSend.size());
+            BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend)
+            {
+                if (pto->AddAddressIfNotAlreadyKnown(addr))
+                {
+                    vAddr.push_back(addr);
+                    if (vAddr.size() >= MAX_ADDR_TO_SEND)
+                    {
+                        pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
+                        vAddr.clear();
+                    }
                }
            }

            pto->vAddrToSend.clear();
-            vAddr.clear();
+            if (!vAddr.empty())
+                pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
        }

        CNodeState &state = *State(pto->GetId());
--- a/src/net.h
+++ b/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;
--- a/src/rpc/blockchain.cpp
+++ b/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,12 +606,20 @@ 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");
        }
+    }

    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;
--- a/src/rpc/mining.cpp
+++ b/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);
+        // 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
+        try {
            pblocktemplate = CreateNewBlockWithKey();
+        } catch (...) {
+            ENTER_CRITICAL_SECTION(cs_main);
+            throw;
+        }
 #endif
        ENTER_CRITICAL_SECTION(cs_main);
        if (!pblocktemplate)
@@ -1112,6 +1128,27 @@ static bool StratumMinerRecvLines(int fd, std::string& buf, int timeout_ms, std:
 // authorize, receive work + the per-height RandomX key, then vary the block nNonce, hash with
 // RandomX (byte-identical to CheckRandomXSolution via GetRandomXInput), and submit a 32-byte
 // solution when the block hash meets target. Exists to validate the -stratum RandomX pool path.
+//! Upper bound on how long stratummine will hold an RPC worker thread (and a 256 MB RandomX
+//! cache). An unbounded caller-supplied deadline pins both indefinitely.
+static const int64_t MAX_STRATUMMINE_TIMEOUT = 3600;
+
+//! Parse a 64-char hex field from mining.notify into a uint256.
+//! These fields come from whatever host the operator pointed us at, and both primitives below are
+//! unforgiving: uint256's vector constructor asserts on a wrong-size input (uint256.cpp:30, and
+//! NDEBUG is never defined for this build so the assert is live in release), while ParseHex
+//! silently truncates at the first non-hex character. A short or garbled field would therefore
+//! abort the daemon rather than be rejected. Returns false instead.
+static bool StratumHex256(const std::string& hex, uint256& out)
+{
+    if (hex.size() != 64 || !IsHex(hex))
+        return false;
+    std::vector<unsigned char> v = ParseHex(hex);
+    if (v.size() != 32)
+        return false;
+    out = uint256(v);
+    return true;
+}
+
 UniValue stratummine(const UniValue& params, bool fHelp, const CPubKey& mypk)
 {
    if (fHelp || params.size() < 2 || params.size() > 4)
@@ -1129,10 +1166,24 @@ UniValue stratummine(const UniValue& params, bool fHelp, const CPubKey& mypk)
    if (ASSETCHAINS_ALGO != ASSETCHAINS_RANDOMX)
        throw JSONRPCError(RPC_MISC_ERROR, "stratummine only supports RandomX chains");

+    // This is a reference miner for exercising -stratum, not a production facility: it dials an
+    // operator-supplied host, blocks an RPC worker for the whole run, and parses whatever that host
+    // chooses to send back. Require an explicit opt-in, and exempt regtest so the test suite can
+    // still drive it. NOTE: do NOT gate this on fExperimentalMode -- that defaults to TRUE
+    // (init.cpp:1195), so it would leave the RPC exposed on every node and the gate would be a no-op.
+    if (!GetBoolArg("-stratummine", false) && Params().NetworkIDString() != "regtest")
+        throw JSONRPCError(RPC_MISC_ERROR,
+            "stratummine is a test-only reference miner and is disabled by default; "
+            "restart with -stratummine to enable it");
+
    const std::string host = params[0].get_str();
    const int port = params[1].get_int();
    const std::string addr = params.size() > 2 ? params[2].get_str() : "x";
-    const int64_t timeout = params.size() > 3 ? params[3].get_int64() : 120;
+    int64_t timeout = params.size() > 3 ? params[3].get_int64() : 120;
+    if (timeout < 1)
+        timeout = 1;
+    if (timeout > MAX_STRATUMMINE_TIMEOUT)
+        timeout = MAX_STRATUMMINE_TIMEOUT;
    const int64_t deadline = GetTime() + timeout;

    // connect (blocking TCP)
@@ -1166,6 +1217,11 @@ UniValue stratummine(const UniValue& params, bool fHelp, const CPubKey& mypk)
    uint256 hashPrevBlock, hashMerkleRoot, hashReserved;

    auto processLine = [&](const std::string& line) {
+      // Every get_str()/get_int() below throws on a type mismatch, and this lambda runs inside the
+      // window where the RandomX cache and VM are allocated and the socket is open -- all of which
+      // are released only on the normal path. A malformed server message must therefore never
+      // escape from here, or it leaks 256 MB and the fd on its way out.
+      try {
        UniValue v;
        if (!v.read(line)) return;
        const UniValue& id = find_value(v, "id");
@@ -1185,16 +1241,26 @@ UniValue stratummine(const UniValue& params, bool fHelp, const CPubKey& mypk)
            poolTarget = UintToArith256(uint256S(p[0].get_str()));
            haveTarget = true;
        } else if (m == "mining.notify" && p.size() >= 7) {
+            // Validate every fixed-width field before committing any of it, so a malformed job is
+            // ignored outright rather than half-applied over the previous one.
+            uint256 prev, merkle, reserved;
+            if (!StratumHex256(p[2].get_str(), prev) ||
+                !StratumHex256(p[3].get_str(), merkle) ||
+                !StratumHex256(p[4].get_str(), reserved))
+                return;
            jobId          = p[0].get_str();
            nVersion       = bswap_32((uint32_t)strtoul(p[1].get_str().c_str(), NULL, 16));
-            hashPrevBlock  = uint256(ParseHex(p[2].get_str()));
-            hashMerkleRoot = uint256(ParseHex(p[3].get_str()));
-            hashReserved   = uint256(ParseHex(p[4].get_str()));
+            hashPrevBlock  = prev;
+            hashMerkleRoot = merkle;
+            hashReserved   = reserved;
            timeHex        = p[5].get_str();
            nTime          = bswap_32((uint32_t)strtoul(timeHex.c_str(), NULL, 16));
            nBits          = bswap_32((uint32_t)strtoul(p[6].get_str().c_str(), NULL, 16));
            haveJob = true;
        }
+      } catch (const std::exception&) {
+        // Malformed message from the server: ignore the line and keep mining.
+      }
    };

    std::string buf;
@@ -1302,7 +1368,7 @@ static const CRPCCommand commands[] =
 { //  category              name                      actor (function)         okSafeMode
  //  --------------------- ------------------------  -----------------------  ----------
 #ifndef WIN32
-    { "mining",             "stratummine",            &stratummine,            true  },
+    { "mining",             "stratummine",            &stratummine,            false },
 #endif
    { "mining",             "getlocalsolps",          &getlocalsolps,          true  },
    { "mining",             "getnetworksolps",        &getnetworksolps,        true  },
--- a/src/wallet/rpcdump.cpp
+++ b/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();


@@ -746,10 +749,17 @@ UniValue dumpwallet_impl(const UniValue& params, bool fHelp, bool fDumpZKeys)
        HDSeed hdSeed;
        // Dump the 64-byte derivation seed (for mnemonic wallets this is the
        // expanded BIP39 seed), so re-importing the hex reproduces the same keys.
-        pwalletMain->GetHDSeedForDerivation(hdSeed);
+        // The return MUST be checked: on failure hdSeed is default-constructed, and emitting it
+        // anyway writes a blank seed next to a legitimate-looking BLAKE2b-of-empty fingerprint --
+        // a backup that looks valid and restores nothing. The per-key dump below is still a
+        // complete backup without this line.
+        if (pwalletMain->GetHDSeedForDerivation(hdSeed)) {
            auto rawSeed = hdSeed.RawSeed();
            file << strprintf("# HDSeed=%s fingerprint=%s", HexStr(rawSeed.begin(), rawSeed.end()), hdSeed.Fingerprint().GetHex());
            file << "\n";
+        } else {
+            file << "# HDSeed unavailable (wallet locked, no HD seed, or hdchain unproven)\n";
+        }
    }
    file << "\n";
    for (std::vector<std::pair<int64_t, CKeyID> >::const_iterator it = vKeyBirth.begin(); it != vKeyBirth.end(); it++) {
--- 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<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();
@@ -2813,6 +2832,15 @@ bool CWallet::SetHDSeedFromMnemonic(const std::string& phrase)

 bool CWallet::GetHDSeedForDerivation(HDSeed& seedOut) const
 {
+    // Single choke point for every HD derivation in the wallet. When the hdchain record could not
+    // be trusted at load time we do not know whether fMnemonicSeed should be true, and guessing
+    // wrong derives into an entirely different key tree -- so refuse rather than guess. Callers
+    // surface this as a clean error (z_getnewaddress, sendmany, shieldcoinbase) or skip
+    // (autoshield). Transparent address generation falls back to the legacy random-key path,
+    // because hdChain.seedFp stays null and so IsHDTransparentEnabled() is false.
+    if (fHDChainUnproven)
+        return false;
+
    HDSeed stored;
    if (!GetHDSeed(stored))
        return false;
@@ -3425,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();
@@ -3439,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);
@@ -3447,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);
+                // 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;
            }

@@ -3516,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;
 }

--- a/src/wallet/wallet.h
+++ b/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:
@@ -961,6 +977,11 @@ protected:

    /* the hd chain data model (chain counters) */
    CHDChain hdChain;
+    //! Set at load time when hdChain cannot be trusted to describe this wallet's HD seed -- the
+    //! record was missing, or its seedFp does not match the seed actually loaded. While true the
+    //! wallet is fully usable for existing keys but refuses to DERIVE new ones, because it cannot
+    //! tell which key tree it belongs to. Never serialized; recomputed on every load.
+    bool fHDChainUnproven = false;

 public:
    /*
@@ -1277,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);

@@ -1415,6 +1440,12 @@ public:
    void SetHDChain(const CHDChain& chain, bool memonly);
    const CHDChain& GetHDChain() const { return hdChain; }

+    //! Mark hdChain as untrustworthy for derivation (see the member's declaration). Set only by
+    //! CWalletDB::LoadWallet; there is deliberately no way to clear it short of reloading, so a
+    //! degraded wallet cannot be talked back into deriving without a real repair.
+    void SetHDChainUnproven() { fHDChainUnproven = true; }
+    bool IsHDChainUnproven() const { return fHDChainUnproven; }
+
    /* Record (in memory and in wallet.dat) how this wallet's HD seed came to
       exist. Best-effort: a failed write is logged, not fatal — the next start
       simply re-classifies, and re-classification always errs toward
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -421,6 +421,15 @@ public:
    // True when that record had to be repaired on read (see the "hdchain" case
    // in ReadKeyValue); LoadWallet rewrites it in full form afterwards.
    bool fHDChainRepaired;
+    // True once an "hdchain" record was ENCOUNTERED, whether or not it parsed. This is what
+    // separates the two failure shapes: a v1.0.3-or-earlier -salvagewallet drops the record
+    // entirely (its IsKeyType has no "hdchain" case), so absent == salvaged and recoverable,
+    // while present-but-unreadable means wider file damage.
+    bool fHDChainSeen;
+    // Fingerprint taken from the KEY of the hdseed/chdseed record. Available even for an
+    // encrypted wallet, where the seed itself cannot be read at load time.
+    bool fHDSeedSeen;
+    uint256 hdSeedFpSeen;

    CWalletScanState() {
        nKeys = nCKeys = nKeyMeta = nZKeys = nCZKeys = nZKeyMeta = nSapZAddrs = 0;
@@ -429,6 +438,8 @@ public:
        nFileVersion = 0;
        fHDChainRead = false;
        fHDChainRepaired = false;
+        fHDChainSeen = false;
+        fHDSeedSeen = false;
    }
 };

@@ -836,6 +847,8 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
                strErr = "Error reading wallet database: LoadHDSeed failed";
                return false;
            }
+            wss.fHDSeedSeen = true;
+            wss.hdSeedFpSeen = seedFp;
        }
        else if (strType == "chdseed")
        {
@@ -849,9 +862,15 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
                return false;
            }
            wss.fIsEncrypted = true;
+            // The fingerprint is the plaintext KEY of the chdseed record, so this works while
+            // the wallet is locked and the seed itself is unreadable.
+            wss.fHDSeedSeen = true;
+            wss.hdSeedFpSeen = seedFp;
        }
        else if (strType == "hdchain")
        {
+            // Record the ENCOUNTER before any parsing can fail.
+            wss.fHDChainSeen = true;
            CHDChain chain;
            // Keep an untouched copy: a failed >> has already consumed part of ssValue.
            CDataStream ssRetry(ssValue.begin(), ssValue.end(), ssValue.GetType(), ssValue.GetVersion());
@@ -1046,21 +1065,58 @@ DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
        }
    }

-    // A wallet that holds an HD seed but whose hdchain record is missing or
-    // unreadable is NOT safe to run. hdChain would fall back to its SetNull
-    // defaults (walletdb.h:105-113), which (a) clears fMnemonicSeed, switching
-    // HD derivation from the 64-byte BIP39 seed to the raw 32-byte entropy
-    // (CWallet::GetHDSeedForDerivation, wallet.cpp:2615-2633) -> an entirely
-    // different key tree, and (b) resets saplingAccountCounter to 0, so the
-    // next GenerateNewSaplingZKey walks back over accounts that already exist.
-    // Both are silent today (a bad hdchain read is only DB_NONCRITICAL_ERROR).
-    // Fail loud instead of quietly deriving into the wrong tree.
+    // A wallet that holds an HD seed but no usable hdchain record cannot safely DERIVE: hdChain
+    // falls back to its SetNull defaults, clearing fMnemonicSeed and so switching derivation from
+    // the 64-byte BIP39 seed to the raw 32-byte entropy -- an entirely different key tree.
+    //
+    // saplingAccountCounter was previously listed here as a second hazard. It is not one:
+    // GenerateNewSaplingZKey loops `do {...} while (HaveSaplingSpendingKey(...))` and
+    // DeriveNewChildKey loops `while (HaveKey(...))`, so a counter that starts low walks forward
+    // past accounts that already exist rather than colliding with them.
+    //
+    // Two shapes reach here and only one is real corruption:
+    //  (a) the record is ABSENT -- the signature of a -salvagewallet run by v1.0.3 or earlier,
+    //      whose IsKeyType has no "hdchain" case, so salvage dropped it. The keys are intact.
+    //      Refusing strands the wallet with no way back: the node aborts before the RPC server
+    //      exists, and -mnemonic refuses a non-empty wallet, so there is no user-executable
+    //      recovery path at all.
+    //  (b) the record is PRESENT but unreadable -- wider file damage. Keep refusing.
+    if (pwallet->HaveHDSeed() && !wss.fHDChainRead && wss.fHDChainSeen)
+    {
+        LogPrintf("Error loading wallet.dat: the hdchain record is present but unreadable. Your keys "
+                  "are intact -- do NOT delete or replace wallet.dat. Back it up and see debug.log.\n");
+        return DB_CORRUPT;
+    }
+
+    {
+        const char* pszDegraded = NULL;
        if (pwallet->HaveHDSeed() && !wss.fHDChainRead)
        {
-        LogPrintf("Error loading wallet.dat: HD seed present but the hdchain record is missing or corrupt. "
-                  "Recover by restoring from the seed phrase: move wallet.dat aside and start with "
-                  "-mnemonic=\"<your seed phrase>\" -rescan\n");
-        return DB_CORRUPT;
+            pszDegraded = "the hdchain record is missing (an older -salvagewallet drops it)";
+        }
+        // A chain WAS read, but it does not belong to the seed we loaded. No legitimate writer can
+        // produce that -- InstallHDSeed always stores seedFp = seed.Fingerprint(). It is the mark
+        // of a wallet that lost its hdchain to an old salvage and was then USED on that old build,
+        // which persists a SetNull-derived chain carrying a null seedFp. Such a wallet otherwise
+        // starts up perfectly clean and derives into the WRONG TREE forever, silently -- strictly
+        // worse than failing to open, which is why it is worth detecting here.
+        else if (pwallet->HaveHDSeed() && wss.fHDSeedSeen &&
+                 pwallet->GetHDChain().seedFp != wss.hdSeedFpSeen)
+        {
+            pszDegraded = "the hdchain record does not belong to this wallet's HD seed";
+        }
+
+        if (pszDegraded != NULL)
+        {
+            pwallet->SetHDChainUnproven();
+            LogPrintf("Wallet opened in DEGRADED mode: %s. Existing keys are intact, spendable and "
+                      "receivable, but no NEW HD-derived key can be generated and new transparent "
+                      "addresses will not be recoverable from a seed phrase. Back up wallet.dat now "
+                      "and do NOT delete or replace it.\n", pszDegraded);
+            // An old salvage also dropped defaultkey and bestblock, which would make this look like
+            // a first run and skip the rescan, leaving a permanently zero balance.
+            SoftSetBoolArg("-rescan", true);
+        }
    }

    // Any wallet corruption at all: skip any rewriting or