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

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

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

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

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

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

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

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

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

src/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/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());

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,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;

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)

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

@@ -1240,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);
@@ -1277,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;
           }
 
@@ -1289,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
@@ -1306,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;
           }
         }
@@ -1358,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);
       }
     }
   }

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.