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

doc/man/dragonx-cli.1

@@ -3,7 +3,7 @@
 .SH NAME
 dragonx-cli \- manual page for dragonx-cli v1.3.0
 .SH DESCRIPTION
-DragonX RPC client version v1.3.0
+DragonX RPC client version v1.3.0\-af7d9e230
 .PP
 In order to ensure you are adequately protecting your privacy when using
 DragonX, please see <https://dragonx.is/security/>.

doc/man/dragonx-tx.1

@@ -3,7 +3,7 @@
 .SH NAME
 dragonx-tx \- manual page for dragonx-tx v1.3.0
 .SH DESCRIPTION
-hush\-tx utility version v1.3.0
+hush\-tx utility version v1.3.0\-af7d9e230
 .SS "Usage:"
 .TP
 hush\-tx [options] <hex\-tx> [commands]

doc/man/dragonxd.1

@@ -3,7 +3,7 @@
 .SH NAME
 dragonxd \- manual page for dragonxd v1.3.0
 .SH DESCRIPTION
-DragonX Daemon version v1.3.0
+DragonX Daemon version v1.3.0\-af7d9e230
 .PP
 In order to ensure you are adequately protecting your privacy when using
 DragonX, please see <https://dragonx.is/security/>.

src/Makefile.gtest.include

@@ -14,8 +14,7 @@ hush_gtest_SOURCES = \
 	gtest/test_randomx_preverify.cpp \
 	gtest/test_hdtransparent.cpp \
 	gtest/test_mnemonic_compat.cpp \
-	gtest/test_stratum_jobid.cpp \
-	gtest/test_wallet_notedata.cpp
+	gtest/test_stratum_jobid.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

@@ -1,155 +0,0 @@
-// 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/httpserver.cpp

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

src/init.cpp

@@ -1016,8 +1016,7 @@ 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 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
+// cache means far fewer chainstate flushes to disk. 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
@@ -2230,9 +2229,6 @@ 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");
@@ -2670,7 +2666,6 @@ 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
@@ -2741,21 +2736,6 @@ 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(
@@ -2797,24 +2777,8 @@ 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,7 +3738,6 @@ 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 {
@@ -3764,9 +3763,6 @@ 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;
@@ -3776,10 +3772,8 @@ 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 || fBlockCountFlush;
+        bool fDoFullFlush = (mode == FLUSH_STATE_ALWAYS) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune;
         // Write blocks and block index to disk.
         if (fDoFullFlush || fPeriodicWrite) {
             // Depend on nMinDiskSpace to ensure we can write block index
@@ -3833,7 +3827,6 @@ 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).
@@ -8343,6 +8336,24 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
         // Message: addr
         if (fSendTrickle)
         {
+            // Accumulate into vAddr and send it ONCE (or in MAX_ADDR_TO_SEND-sized batches).
+            // This loop previously pushed the ENTIRE pto->vAddrToSend on every accepted address,
+            // so a single 24-byte getaddr produced N messages of N addresses each instead of one
+            // message of N -- ~500x the intended bandwidth on a typical addrman, all serialized
+            // (with per-message double-SHA256 checksums) while cs_main is held. The locally built
+            // vAddr was accumulated and then discarded, and the vAddr.resize(MAX_ADDR_TO_SEND) was
+            // a no-op standing where upstream has vAddr.clear().
+            const char* msg_type;
+            int make_flags;
+            if (pto->m_wants_addrv2) {
+                msg_type = NetMsgType::ADDRV2;
+                make_flags = ADDRV2_FORMAT;
+            } else {
+                msg_type = NetMsgType::ADDR;
+                make_flags = 0;
+            }
+            const CNetMsgMaker msgMaker(std::min(pto->nVersion, PROTOCOL_VERSION));
+
             vector<CAddress> vAddr;
             vAddr.reserve(pto->vAddrToSend.size());
             BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend)
@@ -8350,29 +8361,17 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
                 if (pto->AddAddressIfNotAlreadyKnown(addr))
                 {
                     vAddr.push_back(addr);
-
                     if (vAddr.size() >= MAX_ADDR_TO_SEND)
                     {
-                      // Should be impossible since we always check size before adding to
-                      // vAddrToSend. Recover by trimming the vector.
-                      vAddr.resize(MAX_ADDR_TO_SEND);
+                        pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
+                        vAddr.clear();
                     }
-                    const char* msg_type;
-                    int make_flags;
-                    if (pto->m_wants_addrv2) {
-                        msg_type = NetMsgType::ADDRV2;
-                        make_flags = ADDRV2_FORMAT;
-                    } else {
-                        msg_type = NetMsgType::ADDR;
-                        make_flags = 0;
-                    }
-                    pto->PushAddrMessage(CNetMsgMaker(std::min(pto->nVersion, PROTOCOL_VERSION)).Make(make_flags, msg_type, pto->vAddrToSend));
-
                 }
             }
 
             pto->vAddrToSend.clear();
-            vAddr.clear();
+            if (!vAddr.empty())
+                pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr));
         }
 
         CNodeState &state = *State(pto->GetId());

src/main.h

@@ -143,15 +143,8 @@ 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. 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;
+/** Time to wait (in seconds) between flushing chainstate to disk. */
+static const unsigned int DATABASE_FLUSH_INTERVAL = 24 * 60 * 60;
 /** 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/net.h

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

src/rpc/blockchain.cpp

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

src/rpc/mining.cpp

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

src/validationinterface.cpp

@@ -203,9 +203,6 @@ 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
@@ -242,8 +239,6 @@ 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/version.h

@@ -22,13 +22,7 @@
 
 // network protocol versioning
 // DragonX 1.0.0 - bumped to separate from old HUSH/DragonX nodes with RandomX bug
-// DragonX 1.3.0 - bumped for the consensus changes in this release (BLOCK_VALID_CONTEXT
-//                 no longer overwrites the validity level, the dead CBOPRET coinbase
-//                 price check is gone, and pow.cpp lost 157 lines). MIN_PEER_PROTO_VERSION
-//                 is deliberately NOT raised: it currently equals this value, and raising
-//                 it would disconnect every node still on v1.0.3 -- which is all users,
-//                 since no release has ever been installable (unsigned archives).
-static const int PROTOCOL_VERSION = 2000001;
+static const int PROTOCOL_VERSION = 2000000;
 //! initial proto version, to be increased after version/verack negotiation
 static const int INIT_PROTO_VERSION = 209;
 //! In this version, 'getheaders' was introduced.

src/wallet/rpcdump.cpp

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

src/wallet/wallet.cpp

@@ -43,7 +43,6 @@
 #include "wallet/asyncrpcoperation_autoshieldcoinbase.h"
 #include <random>
 #include <limits>
-#include <system_error>
 #include <thread>
 #include <atomic>
 #include <mutex>
@@ -576,23 +575,6 @@ 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) {
@@ -819,22 +801,10 @@ 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, locator);
+    SetBestChainINTERNAL(walletdb, loc);
 }
 
 bool CWallet::SetBestChainNoFlush(const CBlockLocator& loc)
@@ -1270,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);
@@ -1307,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;
           }
 
@@ -1319,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
@@ -1336,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;
           }
         }
@@ -1388,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);
       }
     }
   }
@@ -1507,25 +1487,17 @@ 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 logged and has its
-  // anchors checked; routine 1-block tip extension stays quiet to avoid per-block log spam.
+  // 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.
   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 (!fParallel) nPar = 1;
+  if (!bulkRebuild) nPar = 1;
 
   std::atomic<bool> failed(false);
   std::mutex failMtx;
@@ -1537,27 +1509,14 @@ 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) {
-          checkRoot(nd);
+        if (nBlocks <= 0)
           continue;
-        }
 
         if (!fastRebuild || nBlocks <= CACHE) {
           // Reference / shallow path: clone every block (preserves pre-existing older snapshots).
@@ -1598,7 +1557,6 @@ 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);
@@ -1613,18 +1571,9 @@ 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);
-    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 (int t = 0; t < nPar; t++) threads.emplace_back(worker, t);
     for (std::thread& th : threads) th.join();
   }
   int64_t tExtend = GetTimeMicros() - e0;
@@ -1639,16 +1588,20 @@ 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. The unrecoverable count
-  // itself comes from checkRoot() on the workers, and only for a bulk rebuild.
+  // (DecrementNoteWitnesses clears it), keeping the heal self-correcting.
   if (!work.empty()) {
-    for (SaplingNoteData* nd : work)
+    const uint256& canonicalRoot = pindex->hashFinalSaplingRoot;
+    int nUnrecoverable = 0;
+    for (SaplingNoteData* nd : work) {
+      if (nd->witnesses.empty() || nd->witnesses.front().root() != canonicalRoot)
+        nUnrecoverable++;
       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.load()
-                  ? strprintf(" [WARNING: %d note(s) could not be rebuilt to the canonical anchor and were skipped]", nUnrecoverable.load()).c_str()
+                nUnrecoverable
+                  ? strprintf(" [WARNING: %d note(s) could not be rebuilt to the canonical anchor and were skipped]", nUnrecoverable).c_str()
                   : "");
     }
   }
@@ -1970,8 +1923,6 @@ 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)
             {
@@ -1984,7 +1935,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD
                 wtx.nIndex = wtxIn.nIndex;
                 fUpdated = true;
             }
-            if (UpdatedNoteData(wtxIn, wtx, fSameBlock)) {
+            if (UpdatedNoteData(wtxIn, wtx)) {
                 fUpdated = true;
             }
             if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe)
@@ -2021,66 +1972,29 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD
     return true;
 }
 
-// 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 CWallet::UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx)
 {
     bool unchangedSaplingFlag = (wtxIn.mapSaplingNoteData.empty() || wtxIn.mapSaplingNoteData == wtx.mapSaplingNoteData);
-    if (unchangedSaplingFlag)
-        return false;
+    if (!unchangedSaplingFlag) {
+        auto tmp = wtxIn.mapSaplingNoteData;
+        // Ensure we keep any cached witnesses we may already have
 
-    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;
+        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;
+            }
         }
+
+        // Now copy over the updated note data
+        wtx.mapSaplingNoteData = tmp;
     }
 
-    // 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;
+    return !unchangedSaplingFlag;
 }
 
 /**

src/wallet/wallet.h

@@ -896,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.
@@ -965,12 +971,8 @@ 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 fSameBlock);
+    bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx);
     void MarkAffectedTransactionsDirty(const CTransaction& tx);
 
     /* the hd chain data model (chain counters) */
@@ -1298,8 +1300,6 @@ 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. */