wallet/main: break the replay -> witness-rebuild -> kill loop on large wallets

A v1.3.0 node with a ~13.6k-note wallet (-dbcache=4084) looked hung for hours:
every start replayed ~62k blocks from a chainstate 26 days stale, then ran a
47-60 min all-core witness rebuild holding LOCK2(cs_main, cs_wallet) (RPC answered
HTTP 500 "Work queue depth exceeded", peers timed out), the GUI killed it before
anything was flushed, and the next start did it all again. Even after escaping,
each new block cost ~30s under LOCK2, so the node fell behind a 36s chain.

D1  BuildWitnessCache: the post-rebuild latch computed witnesses.front().root()
    (~2.4ms) for every pending note on every block, only to feed a log line that
    is printed for bulk rebuilds alone. The anchor check now runs on the Phase-2
    workers and only for a bulk rebuild; the per-block path just sets the latch.
    Phase 2 also runs in parallel whenever the job is big (notes x commitments
    >= 2000), not only for a bulk range, so busy blocks no longer extend every
    note serially. If the OS refuses a worker thread, the remaining shards run
    inline instead of unwinding past joinable threads (std::terminate).

D2  FlushStateToDisk: the coins cache was only flushed on shutdown, gettxoutsetinfo
    or after 24h; on this chain it stays a few MiB, so the size triggers never
    fire and a node that never reached 24h uptime or a clean stop kept its
    chainstate (and block index) weeks behind. Flush at least hourly and every
    DATABASE_FLUSH_BLOCKS (5000) connected blocks -- the latter also inside one
    long ActivateBestChain -- and right after the startup ActivateBestChain when
    it moved the tip.

D3  UpdatedNoteData: a replayed or rescanned tx arrives with FindMySaplingNotes
    data (no nullifier). Witnesses and witnessHeight were carried over but the
    nullifier was dropped, so VerifyAndSetInitialWitness cleared and reseeded the
    witnesses on the next block -- the replay above rewrote 3,750 txs and forced
    the full rebuild. Carry the nullifier with the witnesses, unless the tx was
    re-mined in a different block (its notes moved; reseed as before). When the
    merged data is identical, don't rewrite the tx; just clear the in-memory
    witnessRootValidated so the next block re-checks the kept roots once (a
    rescan still finds and reseeds a witness that went bad, without rebuilding
    the healthy ones). The carry loop no longer deep-copies every note.

    SetBestChain: with hourly coin flushes the wallet locator must not run ahead
    of the async notifier, or a crash permanently skips the blocks it had not yet
    delivered. Stamp the block the wallet's state reflects (pindexWalletTip,
    seeded in init before ThreadNotifyWallets starts -- at the startup rescan's
    checkpoint if that rescan was interrupted -- and advanced contiguously by
    ChainTip), like zcashd's ChainTipAdded. ThreadNotifyWallets no longer hands
    the wallet an empty block after a fatal block-read failure.

Tests: new gtest test_wallet_notedata.cpp (5 tests); 23/23 gtests pass, and the
two core D3 tests fail with D3 reverted. Regtest A/B against master with a
1,008-note wallet: notify-thread CPU per block 1,780ms -> 3ms; worst LOCK2 RPC
stall 1.6-2.9s -> 10-15ms on ordinary blocks and 4.6-7.1s -> ~1.6s on a block
carrying a 40-output self-send; a z_importkey rescan went from 35 reseeds plus a
bulk rebuild (10.6s) to 0 reseeds, same balance; after kill -9 at 150 blocks a
build with a 100-block flush threshold restarted at 100, master at 0. Two
adversarial review rounds; every confirmed finding is addressed here.

Not covered: the hourly SetBestChain/flush paths were reviewed, not exercised
(GetTimeMicros ignores mocktime); no run yet against a copy of a real large wallet.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/Makefile.gtest.include

@@ -14,7 +14,8 @@ hush_gtest_SOURCES = \
 	gtest/test_randomx_preverify.cpp \
 	gtest/test_hdtransparent.cpp \
 	gtest/test_mnemonic_compat.cpp \
-	gtest/test_stratum_jobid.cpp
+	gtest/test_stratum_jobid.cpp \
+	gtest/test_wallet_notedata.cpp
 
 hush_gtest_CPPFLAGS = $(AM_CPPFLAGS) -DMULTICORE -fopenmp -DBINARY_OUTPUT -DCURVE_ALT_BN128 -DSTATIC $(BITCOIN_INCLUDES)
 hush_gtest_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)

src/gtest/test_wallet_notedata.cpp

@@ -0,0 +1,155 @@
+// Copyright (c) 2016-2024 The Hush developers
+// Distributed under the GPLv3 software license, see the accompanying
+// file COPYING or https://www.gnu.org/licenses/gpl-3.0.en.html
+
+#include <gtest/gtest.h>
+
+#include "primitives/transaction.h"
+#include "uint256.h"
+#include "wallet/wallet.h"
+
+namespace {
+
+class NoteDataWallet : public CWallet {
+public:
+    bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx, bool fSameBlock = true) {
+        return CWallet::UpdatedNoteData(wtxIn, wtx, fSameBlock);
+    }
+};
+
+libzcash::SaplingIncomingViewingKey TestIvk()
+{
+    return libzcash::SaplingIncomingViewingKey(uint256S("0101010101010101010101010101010101010101010101010101010101010101"));
+}
+
+// A note as the wallet holds it once witnessed: nullifier set, witnesses cached at a height.
+SaplingNoteData WitnessedNote(const SaplingMerkleTree& tree, int height, const uint256& nullifier)
+{
+    SaplingNoteData nd(TestIvk(), nullifier);
+    nd.witnesses.push_front(tree.witness());
+    nd.witnessHeight = height;
+    return nd;
+}
+
+// A note as FindMySaplingNotes returns it during a replay or rescan: no nullifier, no witnesses.
+SaplingNoteData FreshNote()
+{
+    return SaplingNoteData(TestIvk());
+}
+
+}
+
+// A replayed or rescanned tx must keep the witnesses AND the nullifier it already had, and must not
+// report an update: dropping the nullifier made the next block clear and reseed every carried witness.
+TEST(WalletNoteData, ReplayKeepsNullifierAndWitnesses) {
+    NoteDataWallet wallet;
+    CWalletTx wtx(&wallet, CTransaction());
+    uint256 txid = uint256S("aa");
+    SaplingOutPoint op0(txid, 0);
+
+    SaplingMerkleTree tree;
+    tree.append(uint256S("01"));
+    const uint256 nf = uint256S("f00d");
+    wtx.mapSaplingNoteData[op0] = WitnessedNote(tree, 100, nf);
+    wtx.mapSaplingNoteData[op0].witnessRootValidated = true;
+    auto before = wtx.mapSaplingNoteData;
+
+    CWalletTx replay(&wallet, CTransaction());
+    replay.mapSaplingNoteData[op0] = FreshNote();
+
+    EXPECT_FALSE(wallet.UpdatedNoteData(replay, wtx));
+    ASSERT_EQ(1u, wtx.mapSaplingNoteData.size());
+    const SaplingNoteData& nd = wtx.mapSaplingNoteData.at(op0);
+    ASSERT_TRUE(static_cast<bool>(nd.nullifier));
+    EXPECT_EQ(nf, *nd.nullifier);
+    EXPECT_EQ(100, nd.witnessHeight);
+    EXPECT_TRUE(nd.witnesses == before.at(op0).witnesses);
+    // ...but the kept witness is re-checked against the active chain once (in-memory flag only).
+    EXPECT_FALSE(nd.witnessRootValidated);
+}
+
+// A tx re-mined in a different block (reorg) has its notes at new tree positions: the old nullifier
+// must not be carried, so VerifyAndSetInitialWitness reseeds the note at its new position.
+TEST(WalletNoteData, MovedTxDropsNullifier) {
+    NoteDataWallet wallet;
+    CWalletTx wtx(&wallet, CTransaction());
+    SaplingOutPoint op0(uint256S("ee"), 0);
+
+    SaplingMerkleTree tree;
+    tree.append(uint256S("04"));
+    wtx.mapSaplingNoteData[op0] = WitnessedNote(tree, 500, uint256S("cafe"));
+
+    CWalletTx moved(&wallet, CTransaction());
+    moved.mapSaplingNoteData[op0] = FreshNote();
+
+    EXPECT_TRUE(wallet.UpdatedNoteData(moved, wtx, false));
+    const SaplingNoteData& nd = wtx.mapSaplingNoteData.at(op0);
+    EXPECT_FALSE(static_cast<bool>(nd.nullifier));
+}
+
+// When the incoming data really adds something (a note for a newly added key), the tx is updated,
+// and the notes it already had keep their witnesses, witness height and nullifier together.
+TEST(WalletNoteData, NewNoteUpdatesAndKeepsExistingNoteState) {
+    NoteDataWallet wallet;
+    CWalletTx wtx(&wallet, CTransaction());
+    uint256 txid = uint256S("bb");
+    SaplingOutPoint op0(txid, 0);
+    SaplingOutPoint op1(txid, 1);
+
+    SaplingMerkleTree tree;
+    tree.append(uint256S("02"));
+    const uint256 nf = uint256S("beef");
+    wtx.mapSaplingNoteData[op0] = WitnessedNote(tree, 200, nf);
+
+    CWalletTx incoming(&wallet, CTransaction());
+    incoming.mapSaplingNoteData[op0] = FreshNote();
+    incoming.mapSaplingNoteData[op1] = FreshNote();
+
+    EXPECT_TRUE(wallet.UpdatedNoteData(incoming, wtx));
+    ASSERT_EQ(2u, wtx.mapSaplingNoteData.size());
+    const SaplingNoteData& nd0 = wtx.mapSaplingNoteData.at(op0);
+    ASSERT_TRUE(static_cast<bool>(nd0.nullifier));
+    EXPECT_EQ(nf, *nd0.nullifier);
+    EXPECT_EQ(200, nd0.witnessHeight);
+    EXPECT_EQ(1u, nd0.witnesses.size());
+    const SaplingNoteData& nd1 = wtx.mapSaplingNoteData.at(op1);
+    EXPECT_FALSE(static_cast<bool>(nd1.nullifier));
+    EXPECT_TRUE(nd1.witnesses.empty());
+    EXPECT_EQ(-1, nd1.witnessHeight);
+}
+
+// Nothing is carried for a note that has no witnesses (PART B2): its height and nullifier describe
+// no witness, so they must not survive onto the incoming data.
+TEST(WalletNoteData, UnwitnessedNoteCarriesNothing) {
+    NoteDataWallet wallet;
+    CWalletTx wtx(&wallet, CTransaction());
+    SaplingOutPoint op0(uint256S("cc"), 0);
+
+    SaplingNoteData stale(TestIvk(), uint256S("dead"));
+    stale.witnessHeight = 300;
+    wtx.mapSaplingNoteData[op0] = stale;
+
+    CWalletTx incoming(&wallet, CTransaction());
+    incoming.mapSaplingNoteData[op0] = FreshNote();
+
+    EXPECT_TRUE(wallet.UpdatedNoteData(incoming, wtx));
+    const SaplingNoteData& nd = wtx.mapSaplingNoteData.at(op0);
+    EXPECT_FALSE(static_cast<bool>(nd.nullifier));
+    EXPECT_EQ(-1, nd.witnessHeight);
+    EXPECT_TRUE(nd.witnesses.empty());
+}
+
+// Incoming data with no Sapling notes never touches the wallet's note data.
+TEST(WalletNoteData, EmptyIncomingIsNoUpdate) {
+    NoteDataWallet wallet;
+    CWalletTx wtx(&wallet, CTransaction());
+    SaplingOutPoint op0(uint256S("dd"), 0);
+    SaplingMerkleTree tree;
+    tree.append(uint256S("03"));
+    wtx.mapSaplingNoteData[op0] = WitnessedNote(tree, 400, uint256S("abcd"));
+
+    CWalletTx incoming(&wallet, CTransaction());
+    EXPECT_FALSE(wallet.UpdatedNoteData(incoming, wtx));
+    EXPECT_EQ(1u, wtx.mapSaplingNoteData.size());
+    EXPECT_EQ(400, wtx.mapSaplingNoteData.at(op0).witnessHeight);
+}

src/init.cpp

@@ -1016,7 +1016,8 @@ extern int32_t HUSH_REWIND;
 
 // --- Adaptive coins-cache sizing -------------------------------------------------------------
 // The in-memory UTXO/coins cache (nCoinCacheUsage) is the biggest lever on IBD speed: a bigger
-// cache means far fewer chainstate flushes to disk. We size it to use most of RAM, but a scheduled
+// cache means fewer size-triggered chainstate flushes to disk (the chainstate is also flushed at
+// least every DATABASE_FLUSH_INTERVAL / DATABASE_FLUSH_BLOCKS, see main.h). We size it to use most of RAM, but a scheduled
 // background task (AdjustCoinCacheForMemoryPressure, registered in AppInit2) shrinks the target when
 // free system memory runs low — e.g. the user opens other apps — and grows it back when memory frees
 // up, always leaving a reserve free for the rest of the system. The existing per-block flush
@@ -2229,6 +2230,9 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
 
     // ********************************************************* Step 8: load wallet
 #ifdef ENABLE_WALLET
+    // Set when the startup rescan below is interrupted: the wallet's state then ends at the scan's
+    // own checkpoint, not at the tip the notifier starts from.
+    bool fWalletScanIncomplete = false;
     if (fDisableWallet) {
         pwalletMain = NULL;
         LogPrintf("Wallet disabled!\n");
@@ -2666,6 +2670,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
             } 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");
+                fWalletScanIncomplete = true;
             }
 
             // Restore wallet transaction metadata after -zapwallettxes=1
@@ -2736,6 +2741,21 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
         {
             LOCK(cs_main);
             pindexLastTip = chainActive.Tip();
+#ifdef ENABLE_WALLET
+            // The wallet's state reflects the block the notifier starts from, unless the startup
+            // rescan was interrupted: then it ends at the scan's checkpoint, and the hourly
+            // SetBestChain must keep stamping that so the next start rescans the rest.
+            if (pwalletMain) {
+                const CBlockIndex* pindexWallet = pindexLastTip;
+                if (fWalletScanIncomplete) {
+                    CWalletDB walletdb(pwalletMain->strWalletFile);
+                    CBlockLocator locator;
+                    if (walletdb.ReadBestBlock(locator))
+                        pindexWallet = FindForkInGlobalIndex(chainActive, locator);
+                }
+                pwalletMain->SetWalletTip(pindexWallet);
+            }
+#endif
         }
         boost::function<void()> threadnotifywallets = boost::bind(&ThreadNotifyWallets, pindexLastTip);
         threadGroup.create_thread(
@@ -2777,8 +2797,24 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
         uiInterface.InitMessage(_("Activating best chain..."));
         // scan for better chains in the block chain database, that are not yet connected in the active best chain
         CValidationState state;
+        const CBlockIndex* pindexBeforeActivate = NULL;
+        {
+            LOCK(cs_main);
+            pindexBeforeActivate = chainActive.Tip();
+        }
         if ( !ActivateBestChain(true,state))
             strErrors << "Failed to connect best block";
+        else {
+            // Persist a startup replay now: the wallet's witness rebuild runs next and can hold
+            // cs_main for a long time, and a node killed before any later flush would replay it all again.
+            bool fTipMoved;
+            {
+                LOCK(cs_main);
+                fTipMoved = chainActive.Tip() != pindexBeforeActivate;
+            }
+            if (fTipMoved)
+                FlushStateToDisk();
+        }
     } else {
         LogPrintf("HUSH_REWIND < 0\n");
     }

src/main.cpp

@@ -3738,6 +3738,7 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) {
     static int64_t nLastWrite = 0;
     static int64_t nLastFlush = 0;
     static int64_t nLastSetChain = 0;
+    static int nLastFlushHeight = -1;
     std::set<int> setFilesToPrune;
     bool fFlushForPrune = false;
     try {
@@ -3763,6 +3764,9 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) {
         if (nLastSetChain == 0) {
             nLastSetChain = nNow;
         }
+        if (nLastFlushHeight < 0) {
+            nLastFlushHeight = chainActive.Height();
+        }
         size_t cacheSize = pcoinsTip->DynamicMemoryUsage();
         // The cache is large and close to the limit, but we have time now (not in the middle of a block processing).
         bool fCacheLarge = mode == FLUSH_STATE_PERIODIC && cacheSize * (10.0/9) > nCoinCacheUsage;
@@ -3772,8 +3776,10 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) {
         bool fPeriodicWrite = mode == FLUSH_STATE_PERIODIC && nNow > nLastWrite + (int64_t)DATABASE_WRITE_INTERVAL * 1000000;
         // It's been very long since we flushed the cache. Do this infrequently, to optimize cache usage.
         bool fPeriodicFlush = mode == FLUSH_STATE_PERIODIC && nNow > nLastFlush + (int64_t)DATABASE_FLUSH_INTERVAL * 1000000;
+        // Many blocks connected since the last flush, e.g. a long replay inside one ActivateBestChain call.
+        bool fBlockCountFlush = (mode == FLUSH_STATE_IF_NEEDED || mode == FLUSH_STATE_PERIODIC) && chainActive.Height() - nLastFlushHeight >= DATABASE_FLUSH_BLOCKS;
         // Combine all conditions that result in a full cache flush.
-        bool fDoFullFlush = (mode == FLUSH_STATE_ALWAYS) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune;
+        bool fDoFullFlush = (mode == FLUSH_STATE_ALWAYS) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune || fBlockCountFlush;
         // Write blocks and block index to disk.
         if (fDoFullFlush || fPeriodicWrite) {
             // Depend on nMinDiskSpace to ensure we can write block index
@@ -3827,6 +3833,7 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) {
             if (!pcoinsTip->Flush())
                 return AbortNode(state, "Failed to write to coin database");
             nLastFlush = nNow;
+            nLastFlushHeight = chainActive.Height();
         }
         if ((mode == FLUSH_STATE_ALWAYS || mode == FLUSH_STATE_PERIODIC) && nNow > nLastSetChain + (int64_t)DATABASE_WRITE_INTERVAL * 1000000) {
             // Update best block in wallet (so we can detect restored wallets).

src/main.h

@@ -143,8 +143,15 @@ static const unsigned int MAX_LOCATOR_SZ = 101;
 static unsigned int BLOCK_DOWNLOAD_WINDOW = 1024;
 /** Time to wait (in seconds) between writing blocks/block index to disk. */
 static const unsigned int DATABASE_WRITE_INTERVAL = 60 * 60;
-/** Time to wait (in seconds) between flushing chainstate to disk. */
-static const unsigned int DATABASE_FLUSH_INTERVAL = 24 * 60 * 60;
+/** Time to wait (in seconds) between flushing chainstate to disk. Upstream uses 24h to keep a large
+ *  coins cache warm, but DragonX's cache stays a few MiB (private chain, few transparent outputs), so
+ *  the size triggers never fire and this timer is all that persists the chainstate while a node runs:
+ *  a node that never reached 24h uptime or a clean shutdown replayed weeks of blocks on every start. */
+static const unsigned int DATABASE_FLUSH_INTERVAL = 60 * 60;
+/** Also flush chainstate once this many blocks were connected since the last flush. A long
+ *  ActivateBestChain (startup replay, catching up after downtime) only flushes IF_NEEDED until it
+ *  returns, so without this a node killed part-way through loses the whole replay. */
+static const int DATABASE_FLUSH_BLOCKS = 5000;
 /** Maximum length of reject messages. */
 static const unsigned int MAX_REJECT_MESSAGE_LENGTH = 111;
 static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60;

src/validationinterface.cpp

@@ -203,6 +203,9 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip)
                     _("Error: A fatal internal error occurred, see debug.log for details"),
                     "", CClientUIInterface::MSG_ERROR);
                 StartShutdown();
+                // Don't hand the wallet an empty block as if it had been processed.
+                fDisconnectIncomplete = true;
+                break;
             }
 
             // Let wallets know transactions went from 1-confirmed to
@@ -239,6 +242,8 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip)
                     _("Error: A fatal internal error occurred, see debug.log for details"),
                     "", CClientUIInterface::MSG_ERROR);
                 StartShutdown();
+                // Don't hand the wallet an empty block as if it had been processed.
+                break;
             }
 
             // Tell wallet about transactions that went from mempool

src/wallet/wallet.cpp

@@ -43,6 +43,7 @@
 #include "wallet/asyncrpcoperation_autoshieldcoinbase.h"
 #include <random>
 #include <limits>
+#include <system_error>
 #include <thread>
 #include <atomic>
 #include <mutex>
@@ -575,6 +576,23 @@ void CWallet::ChainTip(const CBlockIndex *pindex,
         DecrementNoteWitnesses(pindex);
         UpdateNullifierNoteMapForBlock(pblock);
     }
+
+    // Track the block the wallet's state reflects, advancing only along blocks delivered in order:
+    // a gap (the startup rescan was interrupted, so the wallet sits at its checkpoint behind the
+    // notifier) pins the pointer, and SetBestChain keeps stamping the checkpoint.
+    LOCK(cs_wallet);
+    if (added) {
+        if (pindexWalletTip == nullptr || pindex->pprev == pindexWalletTip)
+            pindexWalletTip = pindex;
+    } else if (pindex == pindexWalletTip) {
+        pindexWalletTip = pindex->pprev;
+    }
+}
+
+void CWallet::SetWalletTip(const CBlockIndex* pindex)
+{
+    LOCK(cs_wallet);
+    pindexWalletTip = pindex;
 }
 
 void CWallet::RunSaplingSweep(int blockHeight) {
@@ -801,10 +819,22 @@ bool CWallet::CommitAutomatedTx(const CTransaction& tx) {
 
 void CWallet::SetBestChain(const CBlockLocator& loc)
 {
+    // FlushStateToDisk passes the chain tip, but ThreadNotifyWallets delivers blocks to the wallet
+    // asynchronously and can lag it. Stamping a locator ahead of what the wallet has processed makes
+    // the next start skip those blocks for good (no rescan, no notification): notes received in them
+    // go missing and spends in them go unseen. So stamp the block the wallet's state reflects (seeded
+    // by init before the notifier starts, then advanced by ChainTip). Init's own calls, made before
+    // it is seeded, pass the right locator themselves.
+    CBlockLocator locator = loc;
+    {
+        LOCK2(cs_main, cs_wallet);
+        if (pindexWalletTip != nullptr)
+            locator = chainActive.GetLocator(pindexWalletTip);
+    }
     // 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);
+    SetBestChainINTERNAL(walletdb, locator);
 }
 
 bool CWallet::SetBestChainNoFlush(const CBlockLocator& loc)
@@ -1477,17 +1507,25 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly)
   work.reserve(pending.size());
   for (const PendingNote& p : pending) work.push_back(p.nd);
 
-  // Only a substantial bulk rebuild (e.g. a one-time post-upgrade repair) is worth parallelizing
-  // and logging; routine 1-block tip extension runs serially to avoid per-block thread-spawn
-  // overhead and log spam.
+  // Only a substantial bulk rebuild (e.g. a one-time post-upgrade repair) is logged and has its
+  // anchors checked; routine 1-block tip extension stays quiet to avoid per-block log spam.
   const bool bulkRebuild = (rangeLen > 100);
 
+  // Parallelize whenever the extension itself is expensive, not only for a bulk range: a routine
+  // 1-block extension appends each of the block's commitments to every lagging note, so a large
+  // wallet and a busy block (13k notes x 20 commitments) is seconds of hashing under
+  // cs_main/cs_wallet. Spawning the threads costs ~1ms, so only small jobs stay serial.
+  uint64_t nCms = 0;
+  for (const std::vector<uint256>& cms : blockCms)
+    nCms += cms.size();
+  const bool fParallel = bulkRebuild || (uint64_t)work.size() * nCms >= 2000;
+
   int nPar = (int)GetArg("-witnessbuildthreads", 0);
   if (nPar <= 0) nPar = (int)std::thread::hardware_concurrency();
   if (nPar <= 0) nPar = 1;
   if (nPar > (int)work.size()) nPar = (int)work.size();
   if (nPar < 1) nPar = 1;
-  if (!bulkRebuild) nPar = 1;
+  if (!fParallel) nPar = 1;
 
   std::atomic<bool> failed(false);
   std::mutex failMtx;
@@ -1499,14 +1537,27 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly)
   // clone-every-block path for A/B verification.
   bool fastRebuild = GetBoolArg("-witnessfastrebuild", true);
 
+  // A bulk rebuild reports how many notes failed to reach the canonical anchor. root() walks the
+  // full depth-32 path (~2.4ms), so the check runs here on the workers and never on the routine
+  // per-block path, where it used to hold cs_main/cs_wallet for ~30s per block on a ~13k-note
+  // wallet purely to feed a log line that path never prints.
+  const uint256 canonicalRoot = pindex->hashFinalSaplingRoot;
+  std::atomic<int> nUnrecoverable(0);
+  auto checkRoot = [&](const SaplingNoteData* nd) {
+    if (bulkRebuild && (nd->witnesses.empty() || nd->witnesses.front().root() != canonicalRoot))
+      nUnrecoverable++;
+  };
+
   auto worker = [&](int tid) {
     try {
       for (size_t k = (size_t)tid; k < work.size(); k += (size_t)nPar) {
         SaplingNoteData* nd = work[k];
         int startH = nd->witnessHeight;
         int nBlocks = tipHeight - startH;
-        if (nBlocks <= 0)
+        if (nBlocks <= 0) {
+          checkRoot(nd);
           continue;
+        }
 
         if (!fastRebuild || nBlocks <= CACHE) {
           // Reference / shallow path: clone every block (preserves pre-existing older snapshots).
@@ -1547,6 +1598,7 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly)
             nd->witnesses.pop_back();
           nd->witnessHeight = tipHeight;
         }
+        checkRoot(nd);
       }
     } catch (const std::exception& e) {
       std::lock_guard<std::mutex> lk(failMtx);
@@ -1561,9 +1613,18 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly)
   if (nPar <= 1) {
     worker(0);
   } else {
+    // If the OS refuses a thread (task limit, memory), run the remaining shards here instead of
+    // letting the exception unwind past joinable threads (std::terminate). Each shard still runs
+    // exactly once, so the result is unchanged.
     std::vector<std::thread> threads;
     threads.reserve(nPar);
-    for (int t = 0; t < nPar; t++) threads.emplace_back(worker, t);
+    int t = 0;
+    try {
+      for (; t < nPar; t++) threads.emplace_back(worker, t);
+    } catch (const std::system_error& e) {
+      LogPrintf("%s: could only start %d of %d witness threads (%s); running the rest inline\n", __func__, t, nPar, e.what());
+    }
+    for (int u = t; u < nPar; u++) worker(u);
     for (std::thread& th : threads) th.join();
   }
   int64_t tExtend = GetTimeMicros() - e0;
@@ -1578,20 +1639,16 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly)
   // is unrecoverable here: it is left flagged (the GetSaplingNoteWitnesses majority-anchor guard
   // skips it for spends) and reported, rather than spun on indefinitely. witnessRootValidated is
   // in-memory only, so a fresh validation pass still runs on each restart and after any reorg
-  // (DecrementNoteWitnesses clears it), keeping the heal self-correcting.
+  // (DecrementNoteWitnesses clears it), keeping the heal self-correcting. The unrecoverable count
+  // itself comes from checkRoot() on the workers, and only for a bulk rebuild.
   if (!work.empty()) {
-    const uint256& canonicalRoot = pindex->hashFinalSaplingRoot;
-    int nUnrecoverable = 0;
-    for (SaplingNoteData* nd : work) {
-      if (nd->witnesses.empty() || nd->witnesses.front().root() != canonicalRoot)
-        nUnrecoverable++;
+    for (SaplingNoteData* nd : work)
       nd->witnessRootValidated = true;
-    }
     if (bulkRebuild) {
       LogPrintf("%s: rebuilt %u note witness cache(s) to height %d in %ldms using %d thread(s)%s\n",
                 __func__, (unsigned)work.size(), tipHeight, (long)((tRead + tExtend) / 1000), nPar,
-                nUnrecoverable
-                  ? strprintf(" [WARNING: %d note(s) could not be rebuilt to the canonical anchor and were skipped]", nUnrecoverable).c_str()
+                nUnrecoverable.load()
+                  ? strprintf(" [WARNING: %d note(s) could not be rebuilt to the canonical anchor and were skipped]", nUnrecoverable.load()).c_str()
                   : "");
     }
   }
@@ -1913,6 +1970,8 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD
         bool fUpdated = false;
         if (!fInsertedNew)
         {
+            // Re-mined in a different block (a reorg): its notes sit at new tree positions.
+            bool fSameBlock = wtx.hashBlock.IsNull() || wtxIn.hashBlock.IsNull() || wtxIn.hashBlock == wtx.hashBlock;
             // Merge
             if (!wtxIn.hashBlock.IsNull() && wtxIn.hashBlock != wtx.hashBlock)
             {
@@ -1925,7 +1984,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD
                 wtx.nIndex = wtxIn.nIndex;
                 fUpdated = true;
             }
-            if (UpdatedNoteData(wtxIn, wtx)) {
+            if (UpdatedNoteData(wtxIn, wtx, fSameBlock)) {
                 fUpdated = true;
             }
             if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe)
@@ -1962,29 +2021,66 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD
     return true;
 }
 
-bool CWallet::UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx)
+// True if both maps hold the same notes with identical persisted state (every field SaplingNoteData
+// serializes). SaplingNoteData::operator== ignores the witnesses, so it cannot tell a real change
+// from a redundant update.
+static bool SaplingNoteDataIdentical(const mapSaplingNoteData_t& a, const mapSaplingNoteData_t& b)
+{
+    if (a.size() != b.size())
+        return false;
+    for (const mapSaplingNoteData_t::value_type& item : a) {
+        mapSaplingNoteData_t::const_iterator it = b.find(item.first);
+        if (it == b.end())
+            return false;
+        const SaplingNoteData& x = item.second;
+        const SaplingNoteData& y = it->second;
+        if (!(x.ivk == y.ivk) || x.nullifier != y.nullifier || x.witnessHeight != y.witnessHeight || x.witnesses != y.witnesses)
+            return false;
+    }
+    return true;
+}
+
+bool CWallet::UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx, bool fSameBlock)
 {
     bool unchangedSaplingFlag = (wtxIn.mapSaplingNoteData.empty() || wtxIn.mapSaplingNoteData == wtx.mapSaplingNoteData);
-    if (!unchangedSaplingFlag) {
-        auto tmp = wtxIn.mapSaplingNoteData;
-        // Ensure we keep any cached witnesses we may already have
+    if (unchangedSaplingFlag)
+        return false;
 
-        for (const std::pair <SaplingOutPoint, SaplingNoteData> nd : wtx.mapSaplingNoteData) {
-            if (tmp.count(nd.first) && nd.second.witnesses.size() > 0) {
-                tmp.at(nd.first).witnesses.assign(
-                        nd.second.witnesses.cbegin(), nd.second.witnesses.cend());
-                //PART B2: only carry over witnessHeight TOGETHER with the witnesses it describes.
-                //Copying it unconditionally (when witnesses are NOT copied) advances the height past
-                //the actual witness state for a whole tx's notes at once = the batch desync originator.
-                tmp.at(nd.first).witnessHeight = nd.second.witnessHeight;
-            }
+    auto tmp = wtxIn.mapSaplingNoteData;
+    // Ensure we keep any cached witnesses we may already have
+
+    for (const mapSaplingNoteData_t::value_type& nd : wtx.mapSaplingNoteData) {
+        if (tmp.count(nd.first) && nd.second.witnesses.size() > 0) {
+            tmp.at(nd.first).witnesses.assign(
+                    nd.second.witnesses.cbegin(), nd.second.witnesses.cend());
+            //PART B2: only carry over witnessHeight TOGETHER with the witnesses it describes.
+            //Copying it unconditionally (when witnesses are NOT copied) advances the height past
+            //the actual witness state for a whole tx's notes at once = the batch desync originator.
+            tmp.at(nd.first).witnessHeight = nd.second.witnessHeight;
+            //The nullifier is derived from the note and its witness position, so it travels with the
+            //witnesses too. wtxIn comes from FindMySaplingNotes, which never sets it; dropping it made
+            //VerifyAndSetInitialWitness clear and reseed the witnesses just carried over, so replaying
+            //or rescanning already-witnessed txs forced a rebuild of every one of their notes. A tx
+            //re-mined in another block is the exception: its notes moved, so the old nullifier is
+            //wrong and dropping it still reseeds them at the new position.
+            if (nd.second.nullifier && fSameBlock)
+                tmp.at(nd.first).nullifier = nd.second.nullifier;
         }
-
-        // Now copy over the updated note data
-        wtx.mapSaplingNoteData = tmp;
     }
 
-    return !unchangedSaplingFlag;
+    // A replayed or rescanned tx usually carries nothing new; don't rewrite it to disk. Do have
+    // VerifyAndSetInitialWitness re-check the kept witnesses against the active chain once (the
+    // flag is in memory only): the chain may have moved under them, and it keeps a rescan able to
+    // find and reseed a witness that went bad, without rebuilding the healthy ones.
+    if (SaplingNoteDataIdentical(tmp, wtx.mapSaplingNoteData)) {
+        for (mapSaplingNoteData_t::value_type& item : wtx.mapSaplingNoteData)
+            item.second.witnessRootValidated = false;
+        return false;
+    }
+
+    // Now copy over the updated note data
+    wtx.mapSaplingNoteData = tmp;
+    return true;
 }
 
 /**

src/wallet/wallet.h

@@ -965,8 +965,12 @@ private:
     template <class T>
     void SyncMetaData(std::pair<typename TxSpendMap<T>::iterator, typename TxSpendMap<T>::iterator>);
 
+    // Last block ThreadNotifyWallets delivered to ChainTip, i.e. the block the wallet's state
+    // reflects (guarded by cs_wallet). SetBestChain stamps this instead of the chain tip.
+    const CBlockIndex* pindexWalletTip = nullptr;
+
 protected:
-    bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx);
+    bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx, bool fSameBlock);
     void MarkAffectedTransactionsDirty(const CTransaction& tx);
 
     /* the hd chain data model (chain counters) */
@@ -1294,6 +1298,8 @@ public:
     bool CommitAutomatedTx(const CTransaction& tx);
     /** Saves witness caches and best block locator to disk. Overrides CValidationInterface. */
     void SetBestChain(const CBlockLocator& loc);
+    // Seed the block the wallet's state reflects, before ThreadNotifyWallets starts delivering.
+    void SetWalletTip(const CBlockIndex* pindex);
     /** 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. */