fix(fuzz): apply follow-up review bundle

thepastaclaw · thepastaclaw · commit 0a92dc61ef3f · 2026-04-22T19:34:47.000-05:00
diff --git a/.github/workflows/test-fuzz.yml b/.github/workflows/test-fuzz.yml
@@ -52,29 +52,44 @@ jobs:
         run: |
           mkdir -p /tmp/fuzz_corpus
 
+          # Fail the job if neither external qa-assets source is reachable — otherwise we
+          # silently degrade to synthetic-only/empty-corpus smoke runs and CI goes green on
+          # no signal. Synthetic seeds are additive only; they do not substitute for the
+          # curated external corpora.
+          loaded_external=0
+
           # Layer 1: bitcoin-core inherited corpus
           if git clone --depth=1 https://github.com/bitcoin-core/qa-assets /tmp/qa-assets; then
             if [ -d "/tmp/qa-assets/fuzz_seed_corpus" ]; then
               cp -r /tmp/qa-assets/fuzz_seed_corpus/. /tmp/fuzz_corpus/
               echo "Loaded bitcoin-core corpus"
+              loaded_external=1
             fi
           else
-            echo "WARNING: Failed to clone bitcoin-core/qa-assets (non-fatal)"
+            echo "::warning::Failed to clone bitcoin-core/qa-assets"
           fi
 
           # Layer 2: Dash-specific corpus (overlays on top)
           if git clone --depth=1 https://github.com/dashpay/qa-assets /tmp/dash-qa-assets; then
             if [ -d "/tmp/dash-qa-assets/fuzz/corpora" ]; then
               cp -r /tmp/dash-qa-assets/fuzz/corpora/. /tmp/fuzz_corpus/
               echo "Loaded Dash-specific corpus"
+              loaded_external=1
             fi
           else
-            echo "WARNING: Failed to clone dashpay/qa-assets (non-fatal)"
+            echo "::warning::Failed to clone dashpay/qa-assets"
+          fi
+
+          if [ "$loaded_external" -eq 0 ]; then
+            echo "::error::No external corpus sources reachable - refusing to run synthetic-only/empty-corpus smoke tests that produce no signal"
+            exit 1
           fi
 
-          # Layer 3: Generate synthetic seeds for Dash-specific targets
+          # Layer 3: Generate synthetic seeds for Dash-specific targets (additive only —
+          # gated on external corpus being present so we never pass on synthetic-only runs).
           if [ -f "contrib/fuzz/seed_corpus_from_chain.py" ]; then
-            python3 contrib/fuzz/seed_corpus_from_chain.py --synthetic-only -o /tmp/fuzz_corpus
+            python3 contrib/fuzz/seed_corpus_from_chain.py --synthetic-only -o /tmp/fuzz_corpus || \
+              echo "::warning::Synthetic seed generation failed; continuing with external corpus only"
           fi
         shell: bash
 
@@ -112,10 +127,15 @@ jobs:
           FAILED=0
           PASSED=0
           FAILED_TARGETS=""
+          # libFuzzer writes crash-/leak-/oom-/timeout- files to this directory on failure
+          # via -artifact_prefix, so the "Upload crash artifacts" step below can collect them.
+          ARTIFACT_DIR=/tmp/fuzz_artifacts
+          mkdir -p "$ARTIFACT_DIR"
 
           while IFS= read -r target; do
             [ -z "$target" ] && continue
             corpus_dir="/tmp/fuzz_corpus/${target}"
+            artifact_prefix="${ARTIFACT_DIR}/${target}-"
 
             if [ ! -d "$corpus_dir" ] || [ -z "$(ls -A "$corpus_dir" 2>/dev/null)" ]; then
               # No corpus for this target — run with empty input for 10s
@@ -128,6 +148,7 @@ jobs:
                   -rss_limit_mb=4000 \
                   -max_total_time=10 \
                   -reload=0 \
+                  -artifact_prefix="$artifact_prefix" \
                   "$corpus_dir" 2>&1; then
                 echo "PASS: $target (empty corpus)"
                 PASSED=$((PASSED + 1))
@@ -146,6 +167,7 @@ jobs:
             if FUZZ="$target" timeout 3600 "$FUZZ_BIN" \
                 -rss_limit_mb=4000 \
                 -runs=0 \
+                -artifact_prefix="$artifact_prefix" \
                 "$corpus_dir" 2>&1; then
               echo "PASS: $target"
               PASSED=$((PASSED + 1))
@@ -171,3 +193,12 @@ jobs:
             exit 1
           fi
         shell: bash
+
+      - name: Upload crash artifacts
+        if: failure() && steps.fuzz-test.conclusion == 'failure'
+        uses: actions/upload-artifact@v4
+        with:
+          name: fuzz-crashes-${{ inputs.build-target }}
+          path: /tmp/fuzz_artifacts
+          if-no-files-found: ignore
+          retention-days: 30
diff --git a/contrib/fuzz/seed_corpus_from_chain.py b/contrib/fuzz/seed_corpus_from_chain.py
@@ -38,24 +38,40 @@ def dash_cli(*args, datadir=None):
         return None
 
 
-# Must match src/version.h PROTOCOL_VERSION. The Dash-specific deserialize/
-# roundtrip fuzz harnesses (src/test/fuzz/deserialize_dash.cpp,
-# src/test/fuzz/roundtrip_dash.cpp) read a 4-byte little-endian int from the
-# start of the buffer and use it as the stream version before deserializing
-# the object. Chain data we extract is serialized at PROTOCOL_VERSION, so we
-# prepend that value to seeds for those targets.
-DASH_STREAM_VERSION = 70240
-DASH_STREAM_VERSION_PREFIX = DASH_STREAM_VERSION.to_bytes(4, byteorder="little", signed=False)
+# Must match src/version.h PROTOCOL_VERSION. Several fuzz harnesses read a
+# 4-byte little-endian int from the start of the buffer and use it as the
+# stream version before deserializing the object:
+#   * The Dash-specific helpers DashDeserializeFromFuzzingInput /
+#     DashRoundtripFromFuzzingInput (src/test/fuzz/deserialize_dash.cpp,
+#     src/test/fuzz/roundtrip_dash.cpp), used by dash_*_deserialize and
+#     dash_*_roundtrip targets.
+#   * The upstream block target (src/test/fuzz/block.cpp), which reads the
+#     version int inline before deserializing CBlock.
+#   * The upstream block_deserialize target (src/test/fuzz/deserialize.cpp),
+#     via DeserializeFromFuzzingInput when no explicit protocol_version is
+#     passed.
+# Chain data we extract is serialized at PROTOCOL_VERSION, so we prepend
+# that value to seeds for those targets.
+STREAM_VERSION = 70240
+STREAM_VERSION_PREFIX = STREAM_VERSION.to_bytes(4, byteorder="little", signed=False)
+
+# Non-Dash targets (outside the dash_* naming convention) whose harnesses
+# also consume the 4-byte stream version prefix described above.
+_NON_DASH_STREAM_VERSION_TARGETS = frozenset({"block", "block_deserialize"})
 
 
 def _needs_stream_version_prefix(target_name):
     """Return True if this target's harness consumes a 4-byte stream version prefix.
 
     Matches the DashDeserializeFromFuzzingInput / DashRoundtripFromFuzzingInput
-    helpers used by every dash_*_deserialize and dash_*_roundtrip target. Non-Dash
-    targets (block_deserialize, block, decode_tx, ...) don't use that helper and
-    must be left untouched.
+    helpers used by every dash_*_deserialize and dash_*_roundtrip target, plus
+    the upstream ``block`` and ``block_deserialize`` targets which do the same
+    (see src/test/fuzz/block.cpp and src/test/fuzz/deserialize.cpp). Other
+    non-Dash targets (decode_tx, ...) don't consume such a prefix and must be
+    left untouched.
     """
+    if target_name in _NON_DASH_STREAM_VERSION_TARGETS:
+        return True
     return target_name.startswith("dash_") and (
         target_name.endswith("_deserialize") or target_name.endswith("_roundtrip")
     )
@@ -73,7 +89,7 @@ def save_corpus_input(output_dir, target_name, data_hex):
         return False
 
     if _needs_stream_version_prefix(target_name):
-        raw_bytes = DASH_STREAM_VERSION_PREFIX + raw_bytes
+        raw_bytes = STREAM_VERSION_PREFIX + raw_bytes
 
     filename = hashlib.sha256(raw_bytes).hexdigest()[:16]
     filepath = target_dir / filename
@@ -315,7 +331,42 @@ def extract_masternode_list(output_dir, datadir=None):
             if not protx_hash:
                 continue
 
-            raw_tx = dash_cli("getrawtransaction", protx_hash, datadir=datadir)
+            state = mn.get("state")
+            if not isinstance(state, dict):
+                print(f"WARNING: Missing state for protx {protx_hash}, skipping", file=sys.stderr)
+                continue
+
+            registered_height = state.get("registeredHeight")
+            try:
+                registered_height = int(registered_height)
+            except (TypeError, ValueError):
+                print(
+                    f"WARNING: Invalid registeredHeight for protx {protx_hash}: {registered_height!r}",
+                    file=sys.stderr,
+                )
+                continue
+
+            block_hash = dash_cli("getblockhash", str(registered_height), datadir=datadir)
+            if not block_hash:
+                continue
+
+            block_json = dash_cli("getblock", block_hash, "1", datadir=datadir)
+            if not block_json:
+                continue
+
+            try:
+                block = json.loads(block_json)
+            except json.JSONDecodeError:
+                continue
+
+            if protx_hash not in block.get("tx", []):
+                print(
+                    f"WARNING: ProRegTx {protx_hash} not found in registeredHeight block {registered_height} ({block_hash}), skipping",
+                    file=sys.stderr,
+                )
+                continue
+
+            raw_tx = dash_cli("getrawtransaction", protx_hash, "false", block_hash, datadir=datadir)
             if not raw_tx:
                 continue
 
@@ -325,7 +376,7 @@ def extract_masternode_list(output_dir, datadir=None):
 
             # Extract the special payload for payload-specific targets
             # ProRegTx type is 1, get extraPayloadSize from verbose tx
-            verbose_tx = dash_cli("getrawtransaction", protx_hash, "true", datadir=datadir)
+            verbose_tx = dash_cli("getrawtransaction", protx_hash, "true", block_hash, datadir=datadir)
             if not verbose_tx:
                 continue
             try:
diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp
@@ -57,10 +57,10 @@ bool CCoinJoinQueue::CheckSignature(const CBLSPublicKey& blsPubKey) const
 
 bool CCoinJoinQueue::IsTimeOutOfBounds(int64_t current_time) const
 {
+    // Both operands are gated to be non-negative, so the differences cannot overflow int64_t.
     if (current_time < 0 || nTime < 0) return true;
-    const uint64_t diff = (current_time > nTime) ? static_cast<uint64_t>(current_time) - static_cast<uint64_t>(nTime)
-                                                 : static_cast<uint64_t>(nTime) - static_cast<uint64_t>(current_time);
-    return diff > static_cast<uint64_t>(COINJOIN_QUEUE_TIMEOUT);
+    return current_time - nTime > COINJOIN_QUEUE_TIMEOUT ||
+           nTime - current_time > COINJOIN_QUEUE_TIMEOUT;
 }
 
 [[nodiscard]] std::string CCoinJoinQueue::ToString() const
diff --git a/src/coinjoin/common.h b/src/coinjoin/common.h
@@ -10,9 +10,7 @@
 #include <consensus/amount.h>
 #include <primitives/transaction.h>
 
-#include <algorithm>
 #include <array>
-#include <limits>
 #include <string>
 
 /** Holds a mixing input
@@ -130,8 +128,9 @@ constexpr int CalculateAmountPriority(CAmount nInputAmount)
     }
 
     //nondenom return largest first
-    const int64_t val = -(nInputAmount / COIN);
-    return int(std::clamp<int64_t>(val, std::numeric_limits<int>::min(), std::numeric_limits<int>::max()));
+    // nInputAmount is bounded to [0, MAX_MONEY] by the guard at the top of this function,
+    // so (nInputAmount / COIN) fits comfortably in int.
+    return -1 * (nInputAmount / COIN);
 }
 
 } // namespace CoinJoin
diff --git a/src/llmq/snapshot.h b/src/llmq/snapshot.h
@@ -78,8 +78,12 @@ class CQuorumSnapshot
         size_t cnt = ReadCompactSize(s);
         ReadFixedBitSet(s, activeQuorumMembers, cnt);
         cnt = ReadCompactSize(s);
+        // Reset the destination so in-place re-deserialization (e.g. via CDBWrapper::Read
+        // reusing an existing snapshot object) doesn't append onto stale entries.
+        // Note: intentionally no reserve(cnt) — cnt comes from an unbounded ReadCompactSize
+        // on a P2P-reachable path (QRINFO), and eager reservation would let a peer force
+        // large allocations from a tiny message.
         mnSkipList.clear();
-        mnSkipList.reserve(cnt);
         for ([[maybe_unused]] const auto _ : util::irange(cnt)) {
             int obj;
             s >> obj;
diff --git a/src/test/fuzz/decode_tx.cpp b/src/test/fuzz/decode_tx.cpp
@@ -14,6 +14,9 @@
 
 FUZZ_TARGET(decode_tx)
 {
+    // Dash's DecodeHexTx takes no try_no_witness/try_witness flags (no witness support),
+    // so unlike upstream we can't assert all decode attempts fail — a fuzzer-generated
+    // buffer can coincidentally be a valid serialized transaction.
     const std::string tx_hex = HexStr(std::string{buffer.begin(), buffer.end()});
     CMutableTransaction mtx;
     (void)DecodeHexTx(mtx, tx_hex);
diff --git a/src/test/fuzz/governance_proposal_validator.cpp b/src/test/fuzz/governance_proposal_validator.cpp
@@ -21,7 +21,10 @@ std::string HexEncodeString(const std::string& input)
     static constexpr char HEX_DIGITS[] = "0123456789abcdef";
     std::string out;
     out.reserve(input.size() * 2);
-    for (const unsigned char ch : input) {
+    // std::string holds char, which may be signed. Cast explicitly to avoid UBSan's
+    // implicit-integer-sign-change on the char->unsigned char conversion.
+    for (const char raw : input) {
+        const auto ch = static_cast<unsigned char>(raw);
         out.push_back(HEX_DIGITS[ch >> 4]);
         out.push_back(HEX_DIGITS[ch & 0x0f]);
     }
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
@@ -17,10 +17,11 @@
 #include <validationinterface.h>
 #include <version.h>
 
+#include <algorithm>
+#include <array>
 #include <cstdlib>
 #include <iostream>
 #include <memory>
-#include <set>
 #include <string>
 #include <string_view>
 #include <vector>
@@ -62,9 +63,11 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
     if (!LIMIT_TO_MESSAGE_TYPE.empty() && random_message_type != LIMIT_TO_MESSAGE_TYPE) {
         return;
     }
-    // Skip Dash message types that require subsystem initialization not present in the fuzz harness
-    static const std::set<std::string> skip_message_types{"mnauth"};
-    if (skip_message_types.count(random_message_type)) {
+    // Skip Dash message types that require subsystem initialization not present in the fuzz harness.
+    // TODO: initialize the masternode/LLMQ contexts here (see process_message_dash.cpp) and remove
+    // this list so these handlers get real coverage.
+    static constexpr std::array<std::string_view, 1> skip_message_types{"mnauth"};
+    if (std::find(skip_message_types.begin(), skip_message_types.end(), random_message_type) != skip_message_types.end()) {
         return;
     }
     CNode& p2p_node = *ConsumeNodeAsUniquePtr(fuzzed_data_provider).release();
diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp
@@ -97,13 +97,17 @@ const std::vector<std::string> RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{
     "getaddressmempool",     // Dash: requires address index
     "getaddresstxids",       // Dash: requires address index
     "getaddressutxos",       // Dash: requires address index
+    "getassetunlockstatuses", // Dash: requires chainstate
     "getbestchainlock",      // Dash: requires LLMQ subsystem
+    "getblockfrompeer",      // avoid network activity
     "getblockhashes",        // Dash: requires timestamp index
     "getblockheaders",       // Dash: requires block index
     "getcoinjoininfo",       // Dash: requires MN subsystem
     "getgovernanceinfo",     // Dash: requires governance subsystem
     "getislocks",            // Dash: requires LLMQ subsystem
     "getmerkleblocks",       // Dash: requires block index
+    "getrawtransactionmulti", // Dash: requires chainstate
+    "gettxchainlocks",       // Dash: requires LLMQ subsystem
     "getspentinfo",          // Dash: requires spent index
     "getspecialtxes",        // Dash: requires block index
     "getsuperblockbudget",   // Dash: requires governance subsystem
@@ -112,8 +116,10 @@ const std::vector<std::string> RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{
     "gobject check",         // Dash: requires governance subsystem
     "gobject count",         // Dash: requires governance subsystem
     "gobject deserialize",   // Dash: requires governance subsystem
+    "gobject diff",          // Dash: requires governance subsystem
     "gobject get",           // Dash: requires governance subsystem
     "gobject getcurrentvotes", // Dash: requires governance subsystem
+    "gobject list",          // Dash: requires governance subsystem
     "gobject list-prepared", // Dash: requires governance subsystem
     "gobject prepare",       // Dash: requires governance subsystem
     "gobject submit",        // Dash: requires governance subsystem
@@ -124,20 +130,24 @@ const std::vector<std::string> RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{
     "masternode",            // Dash: requires MN subsystem
     "masternode connect",    // Dash: requires MN subsystem
     "masternode count",      // Dash: requires MN subsystem
+    "masternode list",       // Dash: requires MN subsystem
     "masternode outputs",    // Dash: requires MN subsystem
     "masternode payments",   // Dash: requires MN subsystem
     "masternode status",     // Dash: requires MN subsystem
     "masternode winners",    // Dash: requires MN subsystem
     "masternodelist",        // Dash: requires MN subsystem
     "mnauth",                // Dash: requires MN subsystem
     "mnsync",                // Dash: requires MN subsystem
+    "protx",                 // Dash: requires MN subsystem
     "protx diff",            // Dash: requires MN subsystem
     "protx info",            // Dash: requires MN subsystem
     "protx list",            // Dash: requires MN subsystem
     "protx listdiff",       // Dash: requires MN subsystem
     "protx register_submit", // Dash: requires MN subsystem
     "protx revoke",          // Dash: requires MN subsystem
     "protx update_service",  // Dash: requires MN subsystem
+    "quorum",                // Dash: requires LLMQ subsystem
+    "quorum dkginfo",        // Dash: requires LLMQ subsystem
     "quorum dkgsimerror",    // Dash: requires LLMQ subsystem
     "quorum dkgstatus",      // Dash: requires LLMQ subsystem
     "quorum getdata",        // Dash: requires LLMQ subsystem
@@ -149,6 +159,7 @@ const std::vector<std::string> RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{
     "quorum listextended",   // Dash: requires LLMQ subsystem
     "quorum memberof",       // Dash: requires LLMQ subsystem
     "quorum platformsign",   // Dash: requires LLMQ subsystem
+    "quorum rotationinfo",   // Dash: requires LLMQ subsystem
     "quorum selectquorum",   // Dash: requires LLMQ subsystem
     "quorum sign",           // Dash: requires LLMQ subsystem
     "quorum verify",         // Dash: requires LLMQ subsystem
diff --git a/src/test/fuzz/simplified_mn_list_diff.cpp b/src/test/fuzz/simplified_mn_list_diff.cpp
@@ -126,10 +126,18 @@ FUZZ_TARGET(simplified_mn_list_diff, .init = initialize_simplified_mn_list_diff)
             new_state->BanIfNotBanned(fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 100000));
             break;
         case 7:
-            new_state->platformNodeID = Uint160FromTag(next_unique_tag++ ^ 0x12121212ULL);
+            // platformNodeID is only serialized in CSimplifiedMNListEntry when nType == Evo.
+            // Mutating it on a Regular MN would break the round-trip invariant below.
+            if (old_mn->nType == MnType::Evo) {
+                new_state->platformNodeID = Uint160FromTag(next_unique_tag++ ^ 0x12121212ULL);
+            }
             break;
         case 8:
-            new_state->platformHTTPPort = fuzzed_data_provider.ConsumeIntegral<uint16_t>();
+            // platformHTTPPort is only serialized when nType == Evo && nVersion < ExtAddr;
+            // MakeMasternode/case 1 never set nVersion >= ExtAddr, so guarding on Evo suffices.
+            if (old_mn->nType == MnType::Evo) {
+                new_state->platformHTTPPort = fuzzed_data_provider.ConsumeIntegral<uint16_t>();
+            }
             break;
         case 9:
             new_state->nRegisteredHeight = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 10000);
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
diff --git a/src/test/llmq_snapshot_tests.cpp b/src/test/llmq_snapshot_tests.cpp

Original file line number	Diff line number	Diff line change
`@@ -57,10 +57,10 @@ bool CCoinJoinQueue::CheckSignature(const CBLSPublicKey& blsPubKey) const`
`57`	`57`
`58`	`58`	`bool CCoinJoinQueue::IsTimeOutOfBounds(int64_t current_time) const`
`59`	`59`	`{`
	`60`	`+ // Both operands are gated to be non-negative, so the differences cannot overflow int64_t.`
`60`	`61`	`if (current_time < 0 \|\| nTime < 0) return true;`
`61`		`- const uint64_t diff = (current_time > nTime) ? static_cast<uint64_t>(current_time) - static_cast<uint64_t>(nTime)`
`62`		`- : static_cast<uint64_t>(nTime) - static_cast<uint64_t>(current_time);`
`63`		`- return diff > static_cast<uint64_t>(COINJOIN_QUEUE_TIMEOUT);`
	`62`	`+ return current_time - nTime > COINJOIN_QUEUE_TIMEOUT \|\|`
	`63`	`+ nTime - current_time > COINJOIN_QUEUE_TIMEOUT;`
`64`	`64`	`}`
`65`	`65`
`66`	`66`	`[[nodiscard]] std::string CCoinJoinQueue::ToString() const`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,9 @@`
`14`	`14`
`15`	`15`	`FUZZ_TARGET(decode_tx)`
`16`	`16`	`{`
	`17`	`+ // Dash's DecodeHexTx takes no try_no_witness/try_witness flags (no witness support),`
	`18`	`+ // so unlike upstream we can't assert all decode attempts fail — a fuzzer-generated`
	`19`	`+ // buffer can coincidentally be a valid serialized transaction.`
`17`	`20`	`const std::string tx_hex = HexStr(std::string{buffer.begin(), buffer.end()});`
`18`	`21`	`CMutableTransaction mtx;`
`19`	`22`	`(void)DecodeHexTx(mtx, tx_hex);`