security: apply 2026-06 source-review fixes (consensus, memory, crypto, FFI, P2P, wallet)

qa/pull-tester/rpc-tests.sh

@@ -10,25 +10,29 @@ export BITCOIND=${REAL_BITCOIND}
 
 #Run the tests
 
+# SUP-04: the shielded-pool regression tests below were re-enabled (they had been
+# commented out, leaving ZClassic's core privacy features unguarded in CI). If any
+# fails, triage and fix the underlying issue or file a tracking ticket with a code
+# comment — do NOT silently re-comment it.
 testScripts=(
-    # 'paymentdisclosure.py'
+    'paymentdisclosure.py'
     'prioritisetransaction.py'
-    # 'wallet_treestate.py'
-    # 'wallet_anchorfork.py'
+    'wallet_treestate.py'
+    'wallet_anchorfork.py'
     # 'wallet_changeindicator.py'
     'wallet_import_export.py'
-    # 'wallet_protectcoinbase.py'
-    # 'wallet_shieldcoinbase_sprout.py'
-    # 'wallet_shieldcoinbase_sapling.py'
-    # 'wallet_listreceived.py'
+    'wallet_protectcoinbase.py'
+    'wallet_shieldcoinbase_sprout.py'
+    'wallet_shieldcoinbase_sapling.py'
+    'wallet_listreceived.py'
     # 'wallet.py'
     # 'wallet_overwintertx.py'
     'wallet_persistence.py'
-    # 'wallet_nullifiers.py'
+    'wallet_nullifiers.py'
     # 'wallet_1941.py'
     'wallet_addresses.py'
     'wallet_sapling.py'
-    # 'wallet_listnotes.py'
+    'wallet_listnotes.py'
     # 'mergetoaddress_sprout.py'
     # 'mergetoaddress_sapling.py'
     'listtransactions.py'
@@ -40,8 +44,8 @@ testScripts=(
     'rest.py'
     'mempool_spendcoinbase.py'
     'mempool_reorg.py'
-    # 'mempool_tx_input_limit.py'
-    # 'mempool_nu_activation.py'
+    'mempool_tx_input_limit.py'
+    'mempool_nu_activation.py'
     'mempool_tx_expiry.py'
     'httpbasics.py'
     'zapwallettxes.py'
@@ -58,8 +62,8 @@ testScripts=(
     'blockchain.py'
     'disablewallet.py'
     'zcjoinsplit.py'
-    # 'zcjoinsplitdoublespend.py'
-    # 'zkey_import_export.py'
+    'zcjoinsplitdoublespend.py'
+    'zkey_import_export.py'
     'reorg_limit.py'
     'getblocktemplate.py'
     'bip65-cltv-p2p.py'

src/bootstrap.cpp

@@ -268,17 +268,25 @@ static size_t DiscoverBootstrapPeersFromSocket(SOCKET socket, const CService& pe
 
     std::vector<CAddress> vAddr;
     try {
-        addrPayload >> vAddr;
+        // MEM-03: read and bound the element COUNT before allocating/deserializing
+        // the vector, mirroring the normal addr handler's 1000-entry bound. The
+        // previous `addrPayload >> vAddr` deserialized the whole list (up to the
+        // 2 MiB message cap) before the size guard below could fire.
+        uint64_t nAddr = ReadCompactSize(addrPayload);
+        if (nAddr > 1000) {
+            LogPrint("net", "bootstrap discovery: oversized addr (%llu) from %s\n", (unsigned long long)nAddr, peerAddress.ToStringIPPort());
+            return 0;
+        }
+        vAddr.reserve(nAddr);
+        for (uint64_t i = 0; i < nAddr; ++i) {
+            CAddress a;
+            addrPayload >> a;
+            vAddr.push_back(a);
+        }
     } catch (const std::exception& e) {
         LogPrint("net", "bootstrap discovery: malformed addr from %s: %s\n", peerAddress.ToStringIPPort(), e.what());
         return 0;
     }
-    // Mirror the addr-message bound enforced by the normal net handler so a
-    // misbehaving peer cannot make us iterate an enormous list.
-    if (vAddr.size() > 1000) {
-        LogPrint("net", "bootstrap discovery: oversized addr (%u) from %s\n", (unsigned int)vAddr.size(), peerAddress.ToStringIPPort());
-        return 0;
-    }
 
     size_t appended = 0;
     for (size_t i = 0; i < vAddr.size() && out.size() < BOOTSTRAP_DISCOVERY_MAX_RESULTS; ++i) {

src/bootstrapvalidation.cpp

@@ -103,7 +103,10 @@ static void RefreshFinalizationHoldLocked()
     g_finalizationHold.store(provisional || tipHold, std::memory_order_relaxed);
 }
 
-static const int64_t BOOTSTRAPVAL_BATCH_MS = 80;          // cs_main per-batch budget
+// PERF-04: cs_main per-batch budget. Lowered 80 -> 20 ms so the background UTXO
+// validator holds cs_main for far shorter spans, reducing stalls to live message
+// processing / block relay (and peer timeouts on slower hardware) while it runs.
+static const int64_t BOOTSTRAPVAL_BATCH_MS = 20;          // cs_main per-batch budget
 static const size_t BOOTSTRAPVAL_FLUSH_CAP = 300 * (1 << 20); // in-mem coin cache cap
 
 static boost::filesystem::path ScratchDir()

src/consensus/consensus.h

@@ -20,6 +20,21 @@ static const int32_t SAPLING_MIN_TX_VERSION = 4;
 static const int32_t SAPLING_MAX_TX_VERSION = 4;
 /** The maximum allowed size for a serialized block, in bytes (network rule) */
 static const unsigned int MAX_BLOCK_SIZE = 200000;
+
+/** The maximum block size we tolerate when loading blocks from disk during -reindex,
+ *  -loadblock, or bootstrap.dat import. The canonical mainnet chain contains 1,272 blocks
+ *  whose serialized size is strictly between 200000 and 2000000 bytes (max observed
+ *  1,999,599 B as of height ~3.1M). These blocks are accepted on the normal P2P/ConnectBlock
+ *  path via the local GENEROUS_BLOCK_SIZE_LIMIT in CheckBlock ("checkpoint validates
+ *  correctness" + hash chain). LoadExternalBlockFile was not widened when the generous
+ *  tolerance was added, causing silent drops (nSize check + undersized CBufferedFile).
+ *  This constant makes the import path match CheckBlock so that -reindex can rebuild
+ *  real mainnet history. Safety remains: subsequent ProcessNewBlock still runs CheckBlock
+ *  (which applies the same generous limit) and the checkpoint hash proof for historical
+ *  blocks. See BLK-01 (Critical in 2026-06 full source review, rev 3).
+ */
+static const unsigned int GENEROUS_BLOCK_SIZE_LIMIT = 2000000;
+
 /** The maximum allowed number of signature check operations in a block (network rule) */
 static const unsigned int MAX_BLOCK_SIGOPS = 20000;
 /** The maximum size of a transaction (network rule) */

src/httpserver.cpp

@@ -336,6 +336,14 @@ static bool HTTPBindAddresses(struct evhttp* http)
             endpoints.push_back(std::make_pair(host, port));
         }
     } else { // No specific bind address specified, bind to any
+        // WAL-02: -rpcallowip was set without -rpcbind, so the RPC port binds on
+        // ALL interfaces and is reachable from the network, protected only by the
+        // IP ACL plus the RPC password. Warn loudly — a broad ACL (e.g.
+        // -rpcallowip=0.0.0.0/0) then exposes dumpprivkey/dumpwallet/z_exportkey to
+        // the internet behind the password alone.
+        LogPrintf("WARNING: -rpcallowip was specified without -rpcbind; binding RPC to all interfaces "
+                  "(0.0.0.0 and ::). The RPC port is now network-reachable, guarded only by the IP ACL "
+                  "and password. Set -rpcbind=127.0.0.1 (or a specific address) unless this is intended.\n");
         endpoints.push_back(std::make_pair("::", defaultPort));
         endpoints.push_back(std::make_pair("0.0.0.0", defaultPort));
     }

src/init.cpp

@@ -766,6 +766,14 @@ static bool check_file_hash(const std::string& path, const std::string& hash)
     SHA256 buff;
     while (!feof(file)){
         size = fread(buffer.data(), 1, kHashReadBufSize, file);
+        // RUST-02: on a real I/O error fread sets ferror() (not feof()) and returns
+        // 0, so the original `while(!feof)` loop would spin forever at 100% CPU.
+        // Bail out instead of hanging node startup.
+        if (size == 0 && ferror(file)) {
+            LogPrintf("%s: I/O error while reading for hash check\n", path);
+            fclose(file);
+            return false;
+        }
         buff.update(buffer.data(), size);
     }
     std::string buff_hash = buff.hash();

src/main.cpp

@@ -1227,6 +1227,14 @@ bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidatio
     // limit is meant to be a live consensus rule for new blocks, enforce it in
     // ContextualCheckTransaction (which has nHeight), gated on its activation
     // height — not in this non-contextual check.
+    //
+    // CON-04 (DECISION REQUIRED — intentionally NOT changed here): restoring the
+    // 102000-byte post-Sapling tx limit for new transactions is a TIGHTENING of the
+    // rules, i.e. a soft fork. It MUST be staged at a fixed future activation height
+    // (so the 1,272 historical >200 KB blocks and any historical >102 KB txs stay
+    // valid) and added in ContextualCheckTransaction. That height is a policy choice
+    // for the maintainers; this review does not pick one, so runtime behavior is
+    // left unchanged (current rule = generous 2 MB).
     const unsigned int GENEROUS_TX_SIZE_LIMIT = 2000000; // 2MB, matches GENEROUS_BLOCK_SIZE_LIMIT
     if (::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION) > GENEROUS_TX_SIZE_LIMIT)
         return state.DoS(100, error("CheckTransaction(): size limits failed"),
@@ -4143,6 +4151,21 @@ bool ReceivedBlockTransactions(const CBlock &block, CValidationState& state, CBl
         // value propagation; descendants will get nChain*Value = none until
         // a later checkpoint or reindex can re-establish a known-good total.
         pindexNew->nSproutValue = boost::none;
+        // CON-02/03: nSaplingValue is a plain CAmount (not boost::optional, see
+        // chain.h), so unlike Sprout it cannot carry an "unknown" sentinel; we
+        // store 0 here. nChainSaplingValue is forced to none just below and the
+        // accumulation loops propagate none to every descendant (they guard on
+        // pprev->nChainSaplingValue). The ZIP-209 turnstile in ConnectBlock only
+        // runs when nChainSaplingValue is present, so for an overflowed block (and
+        // its descendants) the turnstile is SKIPPED, not enforced — i.e. the
+        // substituted 0 can never cause a wrong turnstile PASS on corrupted data,
+        // but the turnstile is also not enforced across the unknown window. That
+        // trade-off (check-skipped, not fail-closed rejection) is acceptable for
+        // this corruption-recovery path; a checkpoint/reindex re-establishes a
+        // known-good total. Carrying a true per-block "unknown" would require
+        // making nSaplingValue optional, which changes the on-disk
+        // CDiskBlockIndex format; deferred. The actual UB (raw signed '+') is
+        // fixed at the CON-01 sites.
         pindexNew->nSaplingValue = 0;
     }
     pindexNew->nChainSproutValue = boost::none;
@@ -4366,9 +4389,16 @@ bool CheckBlock(const CBlock& block, CValidationState& state,
     // Skip all structural validation (size, coinbase, transactions, sigops) for pre-checkpoint blocks.
     if (fCheckSizeLimits) {
         // Size limits
-        // Allow larger blocks for historical chain variations - checkpoint validates correctness
-        const unsigned int GENEROUS_BLOCK_SIZE_LIMIT = 2000000; // 2MB to accommodate any historical forks
-        if (block.vtx.empty() || block.vtx.size() > GENEROUS_BLOCK_SIZE_LIMIT || ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION) > GENEROUS_BLOCK_SIZE_LIMIT)
+        // Allow larger blocks for historical chain variations - checkpoint validates correctness.
+        // The real mainnet history contains 1,272 blocks in (200000, 2000000] bytes (see
+        // audit-full.json + scripts/audit-mainnet-history.py). This generous limit (defined
+        // once in consensus/consensus.h) is also used by the -reindex/-loadblock path so that
+        // LoadExternalBlockFile can successfully import the canonical chain. See BLK-01.
+        // CON-05: the serialized-size check below is the real block-size guard.
+        // The former `block.vtx.size() > GENEROUS_BLOCK_SIZE_LIMIT` conjunct compared a
+        // transaction *count* against a *byte* constant (a no-op in practice, since vtx
+        // is already bounded by the serialized size) and was dropped for clarity.
+        if (block.vtx.empty() || ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION) > GENEROUS_BLOCK_SIZE_LIMIT)
             return state.DoS(100, error("CheckBlock(): size limits failed"),
                              REJECT_INVALID, "bad-blk-length");
 
@@ -4925,13 +4955,28 @@ bool static LoadBlockIndexDB()
             if (pindex->pprev) {
                 if (pindex->pprev->nChainTx) {
                     pindex->nChainTx = pindex->pprev->nChainTx + pindex->nTx;
+                    // CON-01: use overflow-safe CheckedAdd here, exactly as the live
+                    // ReceivedBlockTransactions path does. Raw signed '+' on CAmount
+                    // (int64_t) is undefined behaviour on overflow; on a corrupted
+                    // on-disk delta it could wrap the chain totals that the ZIP-209
+                    // turnstile reads. Fall back to boost::none (unknown) on overflow.
                     if (pindex->pprev->nChainSproutValue && pindex->nSproutValue) {
-                        pindex->nChainSproutValue = *pindex->pprev->nChainSproutValue + *pindex->nSproutValue;
+                        CAmount chainSprout;
+                        if (CheckedAdd(*pindex->pprev->nChainSproutValue, *pindex->nSproutValue, chainSprout)) {
+                            pindex->nChainSproutValue = chainSprout;
+                        } else {
+                            pindex->nChainSproutValue = boost::none;
+                        }
                     } else {
                         pindex->nChainSproutValue = boost::none;
                     }
                     if (pindex->pprev->nChainSaplingValue) {
-                        pindex->nChainSaplingValue = *pindex->pprev->nChainSaplingValue + pindex->nSaplingValue;
+                        CAmount chainSapling;
+                        if (CheckedAdd(*pindex->pprev->nChainSaplingValue, pindex->nSaplingValue, chainSapling)) {
+                            pindex->nChainSaplingValue = chainSapling;
+                        } else {
+                            pindex->nChainSaplingValue = boost::none;
+                        }
                     } else {
                         pindex->nChainSaplingValue = boost::none;
                     }
@@ -5447,8 +5492,16 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos *dbp)
 
     int nLoaded = 0;
     try {
-        // This takes over fileIn and calls fclose() on it in the CBufferedFile destructor
-        CBufferedFile blkdat(fileIn, 2*MAX_BLOCK_SIZE, MAX_BLOCK_SIZE+8, SER_DISK, CLIENT_VERSION);
+        // This takes over fileIn and calls fclose() on it in the CBufferedFile destructor.
+        //
+        // Use GENEROUS_BLOCK_SIZE_LIMIT (not MAX_BLOCK_SIZE) for both the buffer and the
+        // nSize filter. The canonical chain contains 1,272 blocks larger than the original
+        // 200000-byte MAX_BLOCK_SIZE (all < 2 MB). Without this, -reindex and -loadblock
+        // (see call sites in init.cpp) silently skip them via the size check or fail to
+        // buffer them, producing a divergent chainstate even though CheckBlock + checkpoints
+        // would have accepted them. ProcessNewBlock below will still validate each block
+        // through the normal (generous) CheckBlock path. See BLK-01 (Critical).
+        CBufferedFile blkdat(fileIn, 2*GENEROUS_BLOCK_SIZE_LIMIT, GENEROUS_BLOCK_SIZE_LIMIT+8, SER_DISK, CLIENT_VERSION);
         uint64_t nRewind = blkdat.GetPos();
         while (!blkdat.eof()) {
             boost::this_thread::interruption_point();
@@ -5467,7 +5520,7 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos *dbp)
                     continue;
                 // read size
                 blkdat >> nSize;
-                if (nSize < 80 || nSize > MAX_BLOCK_SIZE)
+                if (nSize < 80 || nSize > GENEROUS_BLOCK_SIZE_LIMIT)
                     continue;
             } catch (const std::exception&) {
                 // no valid block header found; don't complain
@@ -6458,9 +6511,14 @@ bool ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, int64_t
                     {
                         if (pnode->nVersion < CADDR_TIME_VERSION)
                             continue;
-                        unsigned int nPointer;
+                        // NET-01: use uintptr_t (not unsigned int) so the full pointer
+                        // value mixes into the relay-selection hash. On 64-bit a 4-byte
+                        // copy kept only the low 32 bits (ASLR randomizes the high bits),
+                        // letting distinct CNode* collide and biasing which peers receive
+                        // freshly-relayed addresses.
+                        uintptr_t nPointer;
                         memcpy(&nPointer, &pnode, sizeof(nPointer));
-                        uint256 hashKey = ArithToUint256(UintToArith256(hashRand) ^ nPointer);
+                        uint256 hashKey = ArithToUint256(UintToArith256(hashRand) ^ (uint64_t)nPointer);
                         hashKey = Hash(BEGIN(hashKey), END(hashKey));
                         mapMix.insert(make_pair(hashKey, pnode));
                     }

src/net.cpp

@@ -496,6 +496,17 @@ void CNode::Ban(const CSubNet& subNet, int64_t bantimeoffset, bool sinceUnixEpoc
         banTime = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;
 
     LOCK(cs_setBanned);
+    // NET-02: prune expired entries on each new ban so setBanned cannot grow
+    // unbounded. Without this, an attacker cycling distinct source IPs accumulates
+    // dead entries that IsBanned() linearly scans under cs_setBanned on every
+    // inbound connection, degrading into a lock-contention bottleneck over time.
+    int64_t nowPrune = GetTime();
+    for (std::map<CSubNet, int64_t>::iterator it = setBanned.begin(); it != setBanned.end(); ) {
+        if (it->second < nowPrune)
+            setBanned.erase(it++);
+        else
+            ++it;
+    }
     if (setBanned[subNet] < banTime)
         setBanned[subNet] = banTime;
 }
@@ -1883,6 +1894,17 @@ void RelayTransaction(const CTransaction& tx, const CDataStream& ss)
             vRelayExpiration.pop_front();
         }
 
+        // PERF-03: hard cap on relay entries. Time-based expiry alone lets a flood
+        // of unique-txid transactions grow mapRelay (full CDataStream per entry) to
+        // hundreds of MB before the 15-minute timer reclaims the oldest. Evict the
+        // oldest entries once the cap is reached.
+        static const size_t MAX_RELAY_ENTRIES = 100000;
+        while (mapRelay.size() >= MAX_RELAY_ENTRIES && !vRelayExpiration.empty())
+        {
+            mapRelay.erase(vRelayExpiration.front().second);
+            vRelayExpiration.pop_front();
+        }
+
         // Save original serialized message so newer versions are preserved
         mapRelay.insert(std::make_pair(inv, ss));
         vRelayExpiration.push_back(std::make_pair(GetTime() + 15 * 60, inv));

src/rpc/misc.cpp

@@ -100,7 +100,7 @@ UniValue getinfo(const UniValue& params, bool fHelp)
         obj.push_back(Pair("keypoolsize",   (int)pwalletMain->GetKeyPoolSize()));
     }
     if (pwalletMain && pwalletMain->IsCrypted())
-        obj.push_back(Pair("unlocked_until", nWalletUnlockTime));
+        obj.push_back(Pair("unlocked_until", nWalletUnlockTime.load())); // WAL-06
     obj.push_back(Pair("paytxfee",      ValueFromAmount(payTxFee.GetFeePerK())));
 #endif
     obj.push_back(Pair("relayfee",      ValueFromAmount(::minRelayTxFee.GetFeePerK())));

src/rpc/server.h

@@ -10,6 +10,7 @@
 #include "rpc/protocol.h"
 #include "uint256.h"
 
+#include <atomic>
 #include <list>
 #include <map>
 #include <stdint.h>
@@ -169,7 +170,9 @@ extern uint256 ParseHashO(const UniValue& o, std::string strKey);
 extern std::vector<unsigned char> ParseHexV(const UniValue& v, std::string strName);
 extern std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey);
 
-extern int64_t nWalletUnlockTime;
+// WAL-06: atomic so unlocked reads in getwalletinfo/getinfo cannot tear against
+// the relock timer's write (writes still occur under cs_nWalletUnlockTime).
+extern std::atomic<int64_t> nWalletUnlockTime;
 extern CAmount AmountFromValue(const UniValue& value);
 extern UniValue ValueFromAmount(const CAmount& amount);
 extern double GetDifficulty(const CBlockIndex* blockindex = NULL);