ci: add fuzz regression testing and continuous fuzzing infrastructure#7173
ci: add fuzz regression testing and continuous fuzzing infrastructure#7173thepastaclaw wants to merge 21 commits into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds fuzzing infrastructure: a new reusable GitHub Actions job (test-linux64_fuzz) in .github/workflows/build.yml and a new workflow (.github/workflows/test-fuzz.yml) that downloads build artifacts, prepares corpora (inherited + synthetic), discovers fuzz targets in a fuzz binary, replays or exercises each target, collects crashes/logs, and reports a pass/fail summary (non-zero exit on failures). Adds contrib tools and docs: a continuous fuzzing daemon (contrib/fuzz/continuous_fuzz_daemon.sh), a corpus seeder that extracts real node data and creates synthetic seeds (contrib/fuzz/seed_corpus_from_chain.py), and documentation (contrib/fuzz/README.md). Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Artifact as Artifact Store
participant Container as Fuzz Container
participant FuzzBin as Fuzz Binary
participant Reporter as Report Summary
GHA->>Artifact: download build artifacts & seed corpus
Artifact-->>GHA: deliver artifacts and corpora
GHA->>Container: start container with inputs (bundle-key, build-target, container-path)
Container->>FuzzBin: discover fuzz targets
FuzzBin-->>Container: return target list
Container->>Container: prepare corpus (synthetic + inherited)
loop per fuzz target
Container->>FuzzBin: run target (replay or empty-corpus run)
FuzzBin-->>Container: execution result (pass/fail/crash)
Container->>Container: collect artifacts, logs, crashes
end
Container->>Reporter: produce summary (Passed/Failed/Total)
Reporter-->>GHA: return exit status / upload results
sequenceDiagram
participant Seeder as seed_corpus_from_chain.py
participant Node as Dash Node (RPC)
participant CorpusDir as Corpus Directory
Seeder->>Node: query recent blocks, txs, governance, masternode, quorum data
Node-->>Seeder: return JSON/hex data
Seeder->>CorpusDir: save serialized inputs (SHA256-named files) per target
Seeder->>CorpusDir: generate synthetic seeds for multiple targets
CorpusDir-->>Seeder: return per-target file counts and total size
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
.github/workflows/test-fuzz.yml (1)
127-138: No timeout guard on corpus replay — a hanging target can block the CI runner indefinitely.The empty-corpus branch wraps the fuzz binary with
timeout 30, but the corpus-replay branch (line 129) has no timeout. In libFuzzer, running a directory of test cases without fuzzing exits when all inputs are processed, but a target that hangs on a specific corpus input will block forever. Wrap the replay invocation withtimeout(e.g.,timeout $(($(find "$corpus_dir" -type f | wc -l) * 10 + 60))or a fixed ceiling like3600).⏱️ Proposed fix
- if FUZZ="$target" "$FUZZ_BIN" \ + if FUZZ="$target" timeout 3600 "$FUZZ_BIN" \ -rss_limit_mb=4000 \ "$corpus_dir" 2>&1; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-fuzz.yml around lines 127 - 138, The corpus-replay block currently runs the fuzz binary with FUZZ="$target" "$FUZZ_BIN" "$corpus_dir" without a timeout, so a hanging test can block CI; modify that invocation to be wrapped with timeout (e.g., prefix with timeout 3600 or compute timeout as $(($(find "$corpus_dir" -type f | wc -l) * 10 + 60))) so the command using FUZZ="$target" "$FUZZ_BIN" "$corpus_dir" is subject to a bounded runtime, and keep the existing success/failure handling (PASS/FAIL, PASSED/FAILED counters) unchanged.contrib/fuzz/continuous_fuzz_daemon.sh (1)
161-163:findwith-opredicates should be parenthesized for clarity and safety.Both crash-count expressions (
-name 'crash-*' -o -name 'timeout-*' -o -name 'oom-*') work correctly as written since no other predicates are ANDed in, but without explicit\( ... \)grouping the intent is ambiguous and adding-type flater would silently break the logic.🔧 Proposed fix
- crash_count=$(find "$target_crashes" -name 'crash-*' -o -name 'timeout-*' -o -name 'oom-*' 2>/dev/null | wc -l) + crash_count=$(find "$target_crashes" \( -name 'crash-*' -o -name 'timeout-*' -o -name 'oom-*' \) 2>/dev/null | wc -l)Apply the same fix to the
total_crashesexpression at line 233.Also applies to: 231-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/fuzz/continuous_fuzz_daemon.sh` around lines 161 - 163, The find invocations use multiple -name predicates combined with -o without explicit grouping, which is ambiguous and brittle; update both the crash_count and total_crashes find commands (the lines that set crash_count=$(find ... | wc -l) and total_crashes=...) to wrap the name predicates in escaped parentheses \( ... \) so the three -name tests are grouped (and if you later add -type f it won't change the logic); ensure any -type f predicate is placed outside the grouped parentheses or combined with -a as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-fuzz.yml:
- Around line 93-95: The SKIPPED counter is never incremented but is initialized
and printed ("Skipped: $SKIPPED"), so either remove the unused SKIPPED variable
and its summary output or wire it up where targets are actually skipped;
specifically update the logic that checks for missing corpus directories or
other early-continue paths to increment SKIPPED (increment SKIPPED with
SKIPPED=$((SKIPPED+1)) before continuing) or delete both the SKIPPED=0
initialization and the "Skipped: $SKIPPED" summary line to avoid a misleading
always-zero metric.
- Line 25: Remove the unused environment variable
CI_FAILFAST_TEST_LEAVE_DANGLING from the workflow: locate the env entry
CI_FAILFAST_TEST_LEAVE_DANGLING in the job/env block and delete that key (and
its value) so the workflow no longer sets this unused variable.
In `@contrib/fuzz/seed_corpus_from_chain.py`:
- Around line 211-219: The code is saving JSON text as hex bytes for the
"dash_quorum_snapshot_deserialize" target (qinfo = dash_cli(...); qinfo_hex =
qinfo.encode().hex()), which is wrong for a binary-deserialize fuzz target;
instead, call an RPC that returns the serialized quorum snapshot (or obtain the
serialized blob from the JSON response if it contains a hex/base64 field) and
pass that raw hex-serialized snapshot into save_corpus_input for the
"dash_quorum_snapshot_deserialize" target; update the logic around dash_cli,
qinfo and qinfo_hex (and the branch that calls save_corpus_input) to use the
actual serialized snapshot bytes (hex string) or skip this target if a
serialized form isn't available.
- Around line 158-168: The loop over objects uses an unused loop variable named
obj_hash; rename it to _obj_hash to satisfy Ruff B007 and indicate it's
intentionally unused; update the for statement in the block that calls
json.loads(result) and iterates "for obj_hash, obj_data in objects.items()" to
"for _obj_hash, obj_data in objects.items()" while leaving the body that calls
save_corpus_input (for "dash_governance_object_deserialize" and
"dash_governance_object_roundtrip") unchanged; no other logic changes are
required.
- Line 22: Remove the unused import by deleting the line that imports the os
module in contrib/fuzz/seed_corpus_from_chain.py (the standalone "import os"
statement) to resolve the Flake8 F401 unused-import warning; ensure no other
references to os remain in functions or top-level code before removing.
- Around line 137-144: The current block saves the entire raw_tx for special
transactions to non-existent fuzz targets and doesn't extract the payload;
either remove this code path or implement it properly: when extra_payload > 0
(variable extra_payload) and tx_type in type_map, slice out the payload bytes as
payload = raw_tx[-extra_payload:] (instead of saving raw_tx), then save payload
via save_corpus_input using a target name that actually exists (update type_map
values to real fuzz target names or add the corresponding fuzz targets), and/or
guard the save loop (for suffix in ["_deserialize","_roundtrip"]) by verifying
the targets exist (or simply drop the loop and block if you choose to remove
incomplete support).
---
Nitpick comments:
In @.github/workflows/test-fuzz.yml:
- Around line 127-138: The corpus-replay block currently runs the fuzz binary
with FUZZ="$target" "$FUZZ_BIN" "$corpus_dir" without a timeout, so a hanging
test can block CI; modify that invocation to be wrapped with timeout (e.g.,
prefix with timeout 3600 or compute timeout as $(($(find "$corpus_dir" -type f |
wc -l) * 10 + 60))) so the command using FUZZ="$target" "$FUZZ_BIN"
"$corpus_dir" is subject to a bounded runtime, and keep the existing
success/failure handling (PASS/FAIL, PASSED/FAILED counters) unchanged.
In `@contrib/fuzz/continuous_fuzz_daemon.sh`:
- Around line 161-163: The find invocations use multiple -name predicates
combined with -o without explicit grouping, which is ambiguous and brittle;
update both the crash_count and total_crashes find commands (the lines that set
crash_count=$(find ... | wc -l) and total_crashes=...) to wrap the name
predicates in escaped parentheses \( ... \) so the three -name tests are grouped
(and if you later add -type f it won't change the logic); ensure any -type f
predicate is placed outside the grouped parentheses or combined with -a as
intended.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/fuzz/seed_corpus_from_chain.py`:
- Line 41: The function save_corpus_input currently declares an unused parameter
label; remove the unused parameter from the function signature (change def
save_corpus_input(output_dir, target_name, data_hex):) and update any local
calls within this module that pass a label to instead pass only output_dir,
target_name, data_hex, or if callers elsewhere need labeling, alternatively wire
label into a debug/log/print inside save_corpus_input (e.g., include label in a
logging statement) so the parameter is actually used; focus your change on the
save_corpus_input definition and any direct callers to keep signatures
consistent.
- Line 46: The code calls bytes.fromhex(data_hex) which can raise ValueError for
malformed/non-hex RPC output; wrap this conversion in a try/except ValueError
around the bytes.fromhex(data_hex) call (the expression that assigns raw_bytes)
and handle failures by skipping the bad entry (e.g., continue) and optionally
logging the offending data_hex so the extractor doesn’t abort the whole
run—ensure you reference the same symbols (data_hex and raw_bytes) when adding
the try/except and any log message.
---
Duplicate comments:
In `@contrib/fuzz/seed_corpus_from_chain.py`:
- Around line 136-143: The loop is saving the full raw_tx to fuzz targets that
don't exist and ignores the comment that the payload is the last extra_payload
bytes; update the block in seed_corpus_from_chain.py to slice out only the last
extra_payload bytes (e.g., payload = raw_tx[-extra_payload:]) and pass that
payload to save_corpus_input instead of raw_tx, and also ensure the constructed
target names (the values in type_map combined with suffixes like "_deserialize"
and "_roundtrip") match actual registered fuzz target names in src/test/fuzz (or
adjust type_map to the correct target names) so save_corpus_input(output_dir,
f"{target}{suffix}", payload) writes to real targets.
- Around line 188-193: extract_masternode_list is saving the full
getrawtransaction bytes into non-existent/incorrect fuzz targets
(dash_proreg_tx_deserialize, dash_proreg_tx_roundtrip,
dash_deterministic_mn_deserialize); change the logic around
raw_tx/dash_cli/save_corpus_input so you extract the ProReg payload bytes (the
isolated ProReg payload parsed from the transaction) instead of the full
serialized transaction, and save those payload bytes to the correct existing
fuzz target names used under src/test/fuzz (match the target names used by
extract_special_txs); locate the raw_tx assignment and the loop over targets and
replace the raw_tx writes with the extracted proreg_payload variable (or re-use
the extraction routine from extract_special_txs) and ensure the target strings
passed to save_corpus_input correspond to real fuzzers.
|
Follow-up fix pushed in commit 5cac8c1:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/fuzz/continuous_fuzz_daemon.sh`:
- Around line 171-173: The pipeline using grep against "$target_log" can return
exit status 1 when no matches are found which, combined with set -o pipefail and
set -e, will abort the daemon; modify the crash-logging pipeline (the grep ... |
while IFS= read -r line; do log "CRASH" " $line"; done) to prevent a non-zero
exit from grep from failing the script — for example wrap that pipeline so
failures are ignored (append a harmless "|| true" to the pipeline) or
temporarily disable pipefail around this block, ensuring you still reference the
same variables and the log function and keep the existing crash_count guard.
- Around line 150-151: In continuous_fuzz_daemon.sh the inline environment
assignment for ASAN_OPTIONS overwrites any caller-provided options; change it to
preserve existing ASAN_OPTIONS and append (or prepend) the required option
(detect_leaks=0) using the standard colon-separated form so caller values like
abort_on_error=1 or symbolize=1 are retained, while still setting FUZZ="$target"
as before; update the assignment where ASAN_OPTIONS is set to merge with the
existing ASAN_OPTIONS rather than replacing it.
|
Issues and Suggestions High Priority
The 30s timeout wrapper around a 10s max_total_time is redundant but benign. However, libFuzzer may not always honor max_total_time if stuck in initialization — the external timeout is a good safety net. Consider aligning them or adding a comment explaining the intentional gap.
The comment says "The extra payload is the last N bytes of the raw tx", but the code saves the entire raw transaction (raw_tx), not just the payload bytes. If the fuzz targets expect only the payload (as the target names like dash_proreg_tx_deserialize suggest), this gives them wrong input. If they expect the full transaction, that's fine — but the comment is misleading. Clarify the intent or actually extract just the payload bytes.
Medium Priority
|
|
Thanks for the detailed review — I pushed follow-up fixes in
Validation run locally:
Commit: thepastaclaw@ef6525050e |
|
@UdjinM6 thanks for the detailed review — I addressed all 8 items in 3700fcc5562154f5f3b26d5af0cbffcfcb6b9968. Point-by-point:
Validation run locally:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
contrib/fuzz/seed_corpus_from_chain.py (1)
41-41:⚠️ Potential issue | 🟡 MinorUnused
labelparameter still present (Ruff ARG001).The
label=""parameter is declared but never referenced in the function body. Ruff still fires ARG001 on this line.🔧 Proposed fix (drop the parameter)
-def save_corpus_input(output_dir, target_name, data_hex, label=""): +def save_corpus_input(output_dir, target_name, data_hex):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/fuzz/seed_corpus_from_chain.py` at line 41, The function save_corpus_input declares an unused parameter label="" which triggers Ruff ARG001; remove the label parameter from the function signature in save_corpus_input and update any callers that pass a label argument to stop supplying it (or adjust them to only pass output_dir, target_name, data_hex), and run tests/lint to ensure no remaining references to label remain..github/workflows/test-fuzz.yml (1)
25-25:⚠️ Potential issue | 🟡 Minor
CI_FAILFAST_TEST_LEAVE_DANGLINGis set but never referenced.This env var is defined at the workflow level but is not consumed anywhere in this workflow or in any script it calls. It can be safely removed.
🔧 Proposed fix
-env: - CI_FAILFAST_TEST_LEAVE_DANGLING: 1 - jobs:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-fuzz.yml at line 25, Remove the unused workflow environment variable CI_FAILFAST_TEST_LEAVE_DANGLING from the workflow definition: locate the environment block where CI_FAILFAST_TEST_LEAVE_DANGLING is set and delete that key (the variable name CI_FAILFAST_TEST_LEAVE_DANGLING) so the workflow no longer defines an unused env var; before removing, grep for CI_FAILFAST_TEST_LEAVE_DANGLING across the repo to confirm nothing references it and then commit the simplified workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-fuzz.yml:
- Around line 93-95: The TARGETS enumeration swallows real startup failures due
to the "|| true" and yields an empty string which makes TARGET_COUNT incorrectly
report 1; after populating TARGETS (from PRINT_ALL_FUZZ_TARGETS_AND_ABORT via
FUZZ_BIN) add an explicit guard that checks whether TARGETS is empty or contains
an error marker and aborts with a non-zero exit and an explanatory error log if
so; use the variables TARGETS, TARGET_COUNT, and FUZZ_BIN in this check (e.g.,
test for empty string or for common loader/architecture errors in TARGETS) so
the workflow fails fast on genuine startup problems rather than reporting a
vacuous pass.
In `@contrib/fuzz/seed_corpus_from_chain.py`:
- Around line 244-250: The extract_masternode_list function is saving the full
raw ProRegTx to targets that expect only the extra payload bytes; change it to
call extract_extra_payload_hex(raw_tx) and pass that payload hex to
save_corpus_input for the dash_proreg_tx_deserialize and
dash_proreg_tx_roundtrip targets (mirroring extract_special_txs), and do not
feed the full transaction there. For dash_deterministic_mn_deserialize,
determine the expected serialized CDeterministicMN format and extract/convert
the appropriate bytes from the ProRegTx payload (or use an existing helper that
converts a ProRegTx payload into a CDeterministicMN serialization) and pass that
serialized data to save_corpus_input instead of the full raw_tx; update
extract_masternode_list to reference extract_extra_payload_hex,
save_corpus_input, extract_masternode_list, and
dash_deterministic_mn_deserialize so the correct payload bytes are saved for
each target.
---
Duplicate comments:
In @.github/workflows/test-fuzz.yml:
- Line 25: Remove the unused workflow environment variable
CI_FAILFAST_TEST_LEAVE_DANGLING from the workflow definition: locate the
environment block where CI_FAILFAST_TEST_LEAVE_DANGLING is set and delete that
key (the variable name CI_FAILFAST_TEST_LEAVE_DANGLING) so the workflow no
longer defines an unused env var; before removing, grep for
CI_FAILFAST_TEST_LEAVE_DANGLING across the repo to confirm nothing references it
and then commit the simplified workflow.
In `@contrib/fuzz/seed_corpus_from_chain.py`:
- Line 41: The function save_corpus_input declares an unused parameter label=""
which triggers Ruff ARG001; remove the label parameter from the function
signature in save_corpus_input and update any callers that pass a label argument
to stop supplying it (or adjust them to only pass output_dir, target_name,
data_hex), and run tests/lint to ensure no remaining references to label remain.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test-fuzz.ymlcontrib/fuzz/continuous_fuzz_daemon.shcontrib/fuzz/seed_corpus_from_chain.py
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/fuzz/continuous_fuzz_daemon.sh
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
.github/workflows/test-fuzz.yml (2)
25-25:⚠️ Potential issue | 🟡 MinorUnused environment variable
CI_FAILFAST_TEST_LEAVE_DANGLING.This variable is set but never referenced anywhere in the workflow or any called scripts.
🔧 Proposed fix
-env: - CI_FAILFAST_TEST_LEAVE_DANGLING: 1 -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-fuzz.yml at line 25, The environment variable CI_FAILFAST_TEST_LEAVE_DANGLING is defined in the workflow but never used; either remove the CI_FAILFAST_TEST_LEAVE_DANGLING entry from the workflow file or wire it into the code path that needs it (e.g., pass it into steps as env or reference it in called scripts/commands), ensuring references use the exact name CI_FAILFAST_TEST_LEAVE_DANGLING so there are no unused vars left in the workflow.
93-95:⚠️ Potential issue | 🟡 MinorBinary startup failure produces a vacuous pass.
|| truesuppresses genuine startup failures (missing shared libraries, wrong architecture, etc.). In that caseTARGETSis empty,TARGET_COUNTbecomes1(off-by-one fromecho "" | wc -l), the loop skips the single empty line, and the step exits 0 — reporting success with zero targets tested.🛡️ Proposed fix
TARGETS=$(PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 "$FUZZ_BIN" 2>&1 || true) + if [ -z "$TARGETS" ]; then + echo "ERROR: No fuzz targets found; fuzz binary may not have started correctly" + exit 1 + fi TARGET_COUNT=$(echo "$TARGETS" | wc -l)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-fuzz.yml around lines 93 - 95, The current line uses "TARGETS=$(PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 "$FUZZ_BIN" 2>&1 || true)" which masks startup failures and causes an empty output to be treated as a successful run; remove the "|| true" and instead check the command exit status and fail the step on non-zero to surface missing libraries/arch issues. Also compute TARGET_COUNT by counting non-empty lines in TARGETS (e.g., count lines that are not blank) so an empty output yields 0 targets rather than 1; refer to the variables/flags TARGETS, TARGET_COUNT, PRINT_ALL_FUZZ_TARGETS_AND_ABORT and the executable "$FUZZ_BIN" when making these changes.contrib/fuzz/continuous_fuzz_daemon.sh (2)
174-184:⚠️ Potential issue | 🟡 Minor
ASAN_OPTIONSstill overwrites any caller-supplied value.The inline assignment always sets a fixed string; users running with custom options like
abort_on_error=1orsymbolize=1will silently lose them.🛡️ Proposed fix
- ASAN_OPTIONS="detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1:detect_leaks=0" \ + ASAN_OPTIONS="${ASAN_OPTIONS:+${ASAN_OPTIONS}:}detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1:detect_leaks=0" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/fuzz/continuous_fuzz_daemon.sh` around lines 174 - 184, The inline assignment for ASAN_OPTIONS overwrites any caller-supplied value; change the invocation around "$FUZZ_BIN" so it preserves existing ASAN_OPTIONS by concatenating them with the defaults instead of replacing them (e.g. build ASAN_OPTIONS using the existing ASAN_OPTIONS variable if set, then append the defaults like detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1:detect_leaks=0), and use that combined variable in the inline env assignment before invoking "$FUZZ_BIN" so options such as abort_on_error or symbolize provided by callers are retained.
195-197:⚠️ Potential issue | 🟠 Major
set -o pipefail+ zerogrepmatches aborts the daemon.When the log contains no matching lines,
grepexits with status 1. Withset -euo pipefail, the pipelinegrep ... | while IFS= read -r line; ...propagates that status 1, whichset -ethen turns into a daemon abort — even though the intent is simply "skip if nothing matches".🐛 Proposed fix
- grep -E "SUMMARY|ERROR|BINGO|crash-|timeout-|oom-" "$target_log" 2>/dev/null | while IFS= read -r line; do - log "CRASH" " $line" - done + grep -E "SUMMARY|ERROR|BINGO|crash-|timeout-|oom-" "$target_log" 2>/dev/null | while IFS= read -r line; do + log "CRASH" " $line" + done || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/fuzz/continuous_fuzz_daemon.sh` around lines 195 - 197, The pipeline using grep on "$target_log" can return exit status 1 when there are no matches, which with set -euo pipefail aborts the daemon; fix by testing for matches first and only running the pipeline when matches exist: use grep -Eq "SUMMARY|ERROR|BINGO|crash-|timeout-|oom-" "$target_log" 2>/dev/null && grep -E "SUMMARY|ERROR|BINGO|crash-|timeout-|oom-" "$target_log" 2>/dev/null | while IFS= read -r line; do log "CRASH" " $line"; done, so the while loop only runs when grep finds matches (references: "$target_log" and the grep ... | while IFS= read -r line; do log "CRASH" ... done pipeline).contrib/fuzz/seed_corpus_from_chain.py (2)
41-41:⚠️ Potential issue | 🟡 MinorUnused
labelparameter insave_corpus_input.
label=""is declared but never referenced in the function body. Either use it (e.g., include it in a debug🔧 Proposed fix (drop the parameter)
-def save_corpus_input(output_dir, target_name, data_hex, label=""): +def save_corpus_input(output_dir, target_name, data_hex):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/fuzz/seed_corpus_from_chain.py` at line 41, The function save_corpus_input declares an unused parameter label="" which should be removed to avoid dead API surface; update the function signature in save_corpus_input to drop the label parameter and remove it from all call sites (or alternatively, if label is intended, thread it into the filename/logging within save_corpus_input such as appending to the output filename or including in debug output) so there are no unused parameters left; ensure references to save_corpus_input elsewhere match the new signature (or use the label inside the function if you choose that route).
274-299:⚠️ Potential issue | 🟠 Major
extract_masternode_listsaves the full raw transaction to payload-specific targets.
extract_special_txscorrectly callsextract_extra_payload_hex()to strip the extra-payload bytes before saving todash_proreg_tx_deserialize/dash_proreg_tx_roundtrip.extract_masternode_listbypasses that extraction and writes the entire raw transaction to the same targets, producing structurally mismatched corpus inputs that will generate only spurious parse errors at fuzz time.dash_deterministic_mn_deserializealmost certainly expectsCDeterministicMNbytes, not a full ProRegTx.🐛 Proposed fix
raw_tx = dash_cli("getrawtransaction", protx_hash, datadir=datadir) if raw_tx: - for target in ["dash_proreg_tx_deserialize", "dash_proreg_tx_roundtrip", - "dash_deterministic_mn_deserialize"]: - if save_corpus_input(output_dir, target, raw_tx): - saved += 1 + # Extract payload for proreg payload-specific targets + # Fetch full tx info to get extraPayloadSize + tx_info_str = dash_cli("getrawtransaction", protx_hash, "1", datadir=datadir) + extra_payload_size = 0 + if tx_info_str: + try: + tx_info = json.loads(tx_info_str) + extra_payload_size = int(tx_info.get("extraPayloadSize", 0)) + except (json.JSONDecodeError, TypeError, ValueError): + pass + proreg_payload_hex, err = extract_extra_payload_hex(raw_tx, extra_payload_size) + if proreg_payload_hex: + for target in ["dash_proreg_tx_deserialize", "dash_proreg_tx_roundtrip"]: + if save_corpus_input(output_dir, target, proreg_payload_hex): + saved += 1 + # NOTE: dash_deterministic_mn_deserialize expects CDeterministicMN bytes; + # verify the correct binary format and implement extraction separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/fuzz/seed_corpus_from_chain.py` around lines 274 - 299, The function extract_masternode_list currently writes the entire raw transaction to payload-specific fuzz targets, causing malformed inputs; instead, call the existing extract_extra_payload_hex(raw_tx) and save its returned extra-payload hex to the dash_proreg_tx_deserialize and dash_proreg_tx_roundtrip targets, and do NOT save the full raw_tx there; for dash_deterministic_mn_deserialize, do not save the full tx either—either extract the CDeterministicMN bytes (using the same extractor if it returns the deterministic-MN blob) or skip saving to that target until a proper extractor exists, and update the loop in extract_masternode_list to use the extracted payload variable for the appropriate targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-fuzz.yml:
- Around line 134-136: The corpus replay invocation using FUZZ="$target"
"$FUZZ_BIN" -rss_limit_mb=4000 "$corpus_dir" can run indefinitely because it
lacks a termination flag; modify the call that constructs the fuzzer command
(the line invoking FUZZ, "$FUZZ_BIN", and "$corpus_dir") to include a
termination option such as -runs=0 (or alternatively -max_total_time=<seconds>)
so libFuzzer will only replay the corpus and exit instead of entering unlimited
mutation mode.
---
Duplicate comments:
In @.github/workflows/test-fuzz.yml:
- Line 25: The environment variable CI_FAILFAST_TEST_LEAVE_DANGLING is defined
in the workflow but never used; either remove the
CI_FAILFAST_TEST_LEAVE_DANGLING entry from the workflow file or wire it into the
code path that needs it (e.g., pass it into steps as env or reference it in
called scripts/commands), ensuring references use the exact name
CI_FAILFAST_TEST_LEAVE_DANGLING so there are no unused vars left in the
workflow.
- Around line 93-95: The current line uses
"TARGETS=$(PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 "$FUZZ_BIN" 2>&1 || true)" which
masks startup failures and causes an empty output to be treated as a successful
run; remove the "|| true" and instead check the command exit status and fail the
step on non-zero to surface missing libraries/arch issues. Also compute
TARGET_COUNT by counting non-empty lines in TARGETS (e.g., count lines that are
not blank) so an empty output yields 0 targets rather than 1; refer to the
variables/flags TARGETS, TARGET_COUNT, PRINT_ALL_FUZZ_TARGETS_AND_ABORT and the
executable "$FUZZ_BIN" when making these changes.
In `@contrib/fuzz/continuous_fuzz_daemon.sh`:
- Around line 174-184: The inline assignment for ASAN_OPTIONS overwrites any
caller-supplied value; change the invocation around "$FUZZ_BIN" so it preserves
existing ASAN_OPTIONS by concatenating them with the defaults instead of
replacing them (e.g. build ASAN_OPTIONS using the existing ASAN_OPTIONS variable
if set, then append the defaults like
detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1:detect_leaks=0),
and use that combined variable in the inline env assignment before invoking
"$FUZZ_BIN" so options such as abort_on_error or symbolize provided by callers
are retained.
- Around line 195-197: The pipeline using grep on "$target_log" can return exit
status 1 when there are no matches, which with set -euo pipefail aborts the
daemon; fix by testing for matches first and only running the pipeline when
matches exist: use grep -Eq "SUMMARY|ERROR|BINGO|crash-|timeout-|oom-"
"$target_log" 2>/dev/null && grep -E "SUMMARY|ERROR|BINGO|crash-|timeout-|oom-"
"$target_log" 2>/dev/null | while IFS= read -r line; do log "CRASH" " $line";
done, so the while loop only runs when grep finds matches (references:
"$target_log" and the grep ... | while IFS= read -r line; do log "CRASH" ...
done pipeline).
In `@contrib/fuzz/seed_corpus_from_chain.py`:
- Line 41: The function save_corpus_input declares an unused parameter label=""
which should be removed to avoid dead API surface; update the function signature
in save_corpus_input to drop the label parameter and remove it from all call
sites (or alternatively, if label is intended, thread it into the
filename/logging within save_corpus_input such as appending to the output
filename or including in debug output) so there are no unused parameters left;
ensure references to save_corpus_input elsewhere match the new signature (or use
the label inside the function if you choose that route).
- Around line 274-299: The function extract_masternode_list currently writes the
entire raw transaction to payload-specific fuzz targets, causing malformed
inputs; instead, call the existing extract_extra_payload_hex(raw_tx) and save
its returned extra-payload hex to the dash_proreg_tx_deserialize and
dash_proreg_tx_roundtrip targets, and do NOT save the full raw_tx there; for
dash_deterministic_mn_deserialize, do not save the full tx either—either extract
the CDeterministicMN bytes (using the same extractor if it returns the
deterministic-MN blob) or skip saving to that target until a proper extractor
exists, and update the loop in extract_masternode_list to use the extracted
payload variable for the appropriate targets.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test-fuzz.ymlcontrib/fuzz/continuous_fuzz_daemon.shcontrib/fuzz/seed_corpus_from_chain.py
|
Consolidation update: I merged the open fuzz-target branches into this PR branch ( Included/superseded PRs:
Those PRs were closed as superseded, and all relevant fuzz sources are now on this branch. |
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
pls squash fixes into original commits and rebase to get rid of "Merge remote...."
|
Good catch — you're right. I’m cleaning this up now by squashing the fixup commits into their originals and rebasing to remove the merge commits (including the "Merge remote..." ones). I’ll push a cleaned history shortly. |
Consolidate fuzz regression workflow, continuous fuzz daemon tooling, and Dash-specific fuzz target additions into a clean commit history for PR dashpay#7173 cleanup.
thepastaclaw
force-pushed
the
ci/fuzz-regression
branch
from
c80135d to
2c28e19
Compare
Btw, you squashed everything into one commit instead of squashing the fixup commits into their originals. |
thepastaclaw
force-pushed
the
ci/fuzz-regression
branch
from
61a2e13 to
c194d87
Compare
Consolidate fuzz regression workflow, continuous fuzz daemon tooling, and Dash-specific fuzz target additions into a clean commit history for PR dashpay#7173 cleanup.
thepastaclaw
force-pushed
the
ci/fuzz-regression
branch
from
505247c to
93ff82d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93ff82d3f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I re-validated the prior and current findings against the checked-out head 93ff82d3f7e2689df745623b041ebdcaaacd6c15. Two blocking issues still apply on this head: the fuzz regression workflow still runs open-ended libFuzzer sessions instead of a deterministic corpus replay, and createwallet still rejects explicit legacy-wallet requests when descriptors=false is passed without load_on_startup. The earlier governance corpus-seeding issue is resolved on this head because the generator now serializes full Governance::Object instances instead of writing raw DataHex payloads.
Reviewed commit: 93ff82d
🔴 2 blocking
1 additional finding
🔴 blocking: `createwallet` rejects explicit legacy-wallet calls whenever `descriptors` is set without `load_on_startup`
src/wallet/rpc/wallet.cpp (lines 679-682)
The new guard throws as soon as parameter 5 is present and parameter 6 is omitted, before checking whether descriptors was actually set to true. As a result, createwallet ... descriptors=false now fails even though the RPC help still documents descriptors=false as the way to request a legacy wallet and that call is otherwise unambiguous. The functional test framework masks this by supplying load_on_startup, but direct RPC and CLI callers still hit an unnecessary API break.
💡 Suggested change
if (!request.params[5].isNull() && request.params[5].get_bool() && request.params[6].isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires specifying the 'load_on_startup' flag when creating descriptor wallets. Dash Core v21 introduced this requirement due to breaking changes in the createwallet RPC.");
}
if (request.params[5].isNull() || request.params[5].get_bool()) {
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/test-fuzz.yml`:
- [BLOCKING] lines 184-190: Fuzz regression step still starts unbounded fuzzing instead of replaying the saved corpus once
This block is labeled as a corpus regression replay, but it invokes libFuzzer with the corpus directory and `-runs=0`. In libFuzzer that means there is no execution cap, so after replaying the existing inputs it immediately switches into normal mutation fuzzing until the outer `timeout 3600` kills it. That makes this CI job nondeterministic and much slower than intended, and it stops serving as a reliable regression check tied to the committed corpus.
In `src/wallet/rpc/wallet.cpp`:
- [BLOCKING] lines 679-682: `createwallet` rejects explicit legacy-wallet calls whenever `descriptors` is set without `load_on_startup`
The new guard throws as soon as parameter 5 is present and parameter 6 is omitted, before checking whether `descriptors` was actually set to `true`. As a result, `createwallet ... descriptors=false` now fails even though the RPC help still documents `descriptors=false` as the way to request a legacy wallet and that call is otherwise unambiguous. The functional test framework masks this by supplying `load_on_startup`, but direct RPC and CLI callers still hit an unnecessary API break.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds substantial Dash-specific fuzz target coverage and a new regression-replay CI workflow. The fuzz harnesses are well-structured and production-code changes are minimal. Several legitimate suggestions remain: the regression workflow disables LSan globally (defeating the purpose of the suppressions file present alongside it), new Dash-only fuzz sources are not registered in non-backported.txt (excluding them from Dash-specific cppcheck), and several newly registered targets receive no seed coverage. Codex's blocking claim that -runs=0 does not perform corpus replay is incorrect — libFuzzer replays seed corpora unconditionally before checking MaxNumberOfRuns, so -runs=0 exits immediately after replay (still worth aligning with the project's existing -runs=1 convention).
Reviewed commit: 93ff82d
🟡 5 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
🟡 suggestion: New Dash-specific fuzz sources are missing from non-backported.txt
test/util/data/non-backported.txt (lines 56-74)
This PR adds many Dash-only fuzz sources (asset_lock_unlock.cpp, bls_operations.cpp, coinjoin.cpp, deserialize_dash.cpp, deterministic_mn_list_diff.cpp, governance_proposal_validator.cpp, llmq_messages.cpp, process_message_dash.cpp, roundtrip_dash.cpp, simplified_mn_list_diff.cpp, special_tx_validation.cpp) under src/test/fuzz/, but test/util/data/non-backported.txt has no pattern matching them. test/lint/lint-cppcheck-dash.py (line 69) builds its file list exclusively from this allowlist, so the new harnesses are silently excluded from the Dash-specific cppcheck pass. Add an src/test/fuzz/<file>.cpp entry per new file (or a glob if appropriate) so the lint covers these new entry points.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/util/data/non-backported.txt`:
- [SUGGESTION] lines 56-74: New Dash-specific fuzz sources are missing from non-backported.txt
This PR adds many Dash-only fuzz sources (`asset_lock_unlock.cpp`, `bls_operations.cpp`, `coinjoin.cpp`, `deserialize_dash.cpp`, `deterministic_mn_list_diff.cpp`, `governance_proposal_validator.cpp`, `llmq_messages.cpp`, `process_message_dash.cpp`, `roundtrip_dash.cpp`, `simplified_mn_list_diff.cpp`, `special_tx_validation.cpp`) under `src/test/fuzz/`, but `test/util/data/non-backported.txt` has no pattern matching them. `test/lint/lint-cppcheck-dash.py` (line 69) builds its file list exclusively from this allowlist, so the new harnesses are silently excluded from the Dash-specific cppcheck pass. Add an `src/test/fuzz/<file>.cpp` entry per new file (or a glob if appropriate) so the lint covers these new entry points.
In `.github/workflows/test-fuzz.yml`:
- [SUGGESTION] lines 129-132: detect_leaks=0 silently masks the BLS leak this PR explicitly acknowledges
`ASAN_OPTIONS` sets `detect_leaks=0`, which disables LSan for every replayed corpus run. The PR description lists a known `libdashbls` leak surfaced by `bls_operations` campaigns, and this configuration ensures CI will never catch a regression of that leak — or any new leak — again. The line below already wires up `LSAN_OPTIONS=suppressions=...`, so the cleaner approach is to leave leak detection on and add a suppression for the known noisy frame in `test/sanitizer_suppressions/lsan`, rather than disabling the whole detector globally. As written, the suppressions file is dead code while `detect_leaks=0`.
- [SUGGESTION] lines 184-190: Use `-runs=1` to match the project's existing replay convention
The project's existing `test/fuzz/test_runner.py:289` uses `-runs=1` for corpus replay; this workflow uses `-runs=0`. In current libFuzzer both effectively exit after seed corpus replay (the seed corpora are read unconditionally and the main loop's `TotalNumberOfRuns >= MaxNumberOfRuns` check then trips immediately), so this is not a correctness bug — Codex's blocking claim that `-runs=0` becomes unbounded fuzzing is overstated. However, `-runs=1` is the project convention and is safer against future libFuzzer changes. Worth aligning for consistency.
In `contrib/fuzz/seed_corpus_from_chain.py`:
- [SUGGESTION] lines 1140-1210: Several registered fuzz targets receive no synthetic seeds and fall back to the workflow's empty-corpus smoke run
The workflow falls back to a 10-second empty-corpus run for any target without a corpus directory (test-fuzz.yml:159-181), so the seed generator needs to cover every target meant to participate in regression replay. The synthetic_seeds map omits `dash_dkg_justification_deserialize`, `dash_sig_shares_inv_deserialize`, `dash_batched_sig_shares_deserialize`, and `dash_mnhf_tx_deserialize` (only `dash_mnhf_tx_payload_deserialize` is generated indirectly via `extract_special_txs` type_map[7]). Unless these targets exist in the external dashpay/qa-assets corpus, they'll only ever take the smoke path instead of true replay, weakening the advertised regression gate.
In `src/test/fuzz/util.cpp`:
- [SUGGESTION] lines 332-341: Narrowing nVersion to int16_t drops upstream nVersion fuzz coverage
`ConsumeTransaction` previously fed `nVersion` an arbitrary `int32_t`. After this change a non-CURRENT_VERSION transaction always has its top 16 bits zeroed, which means every shape where the high half of `nVersion` is non-zero (including all SPECIAL_VERSION + non-zero `nType` combinations) is now unreachable through this helper for every consumer (`block.cpp`, primitives_transaction targets, etc.). The comment correctly identifies the symptom (special-tx-type crashes swamping signal), but the right fix is to gate that branch — produce either a normal version with high bits zero, or a fuzzed full int32 with `tx_mut.nType` set explicitly — not to permanently narrow the range for all callers. As written, regressions in upstream `nVersion` handling are no longer reachable through this helper.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 441bbff3d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior-review fixes have all landed: non-backported.txt now lists the new Dash fuzz cpps, the regression workflow uses -runs=1 with LSan suppressions, synthetic seeds were added for the previously-missing targets, and the daemon rejects zero target time. Remaining feedback is minor infrastructure-quality concerns: the long-running daemon still hard-disables LSan (the very thing the workflow stopped doing), the seed script aborts at import time outside a checkout, the corpus-replay step swallows exit codes so timeouts look like crashes, the MNAUTH skip TODO in process_message is already covered by process_message_dash, and util_dash.h is not pattern-matched by the cppcheck allowlist.
Reviewed commit: c613ff5
🟡 1 suggestion(s) | 💬 4 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `contrib/fuzz/continuous_fuzz_daemon.sh`:
- [SUGGESTION] lines 39-51: Continuous fuzz daemon hard-disables leak detection
`DAEMON_ASAN_OPTIONS` sets `detect_leaks=0`, so the long-running campaigns added by this PR cannot catch leak regressions at all — including in the `libdashbls` area called out in the PR description. The CI workflow has already moved away from a global LSan disable in favour of `LSAN_OPTIONS=suppressions=test/sanitizer_suppressions/lsan`; the daemon should adopt the same approach to preserve leak signal. `count_crash_artifacts()` at line 51 also only matches `crash-*`/`timeout-*`/`oom-*`, so even if leak detection is re-enabled later, leak findings (`leak-*` artifacts) will not be counted as new crashes by the daemon's reporting path.
Reject zero --time-per-target so the daemon never spins on no-op cycles, and align ASAN/LSAN options with the fuzz regression workflow: keep leak detection on, source suppressions from test/sanitizer_suppressions/lsan, and count/report leak-* artifacts alongside crash/timeout/oom. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
force-pushed
the
ci/fuzz-regression
branch
from
c613ff5 to
ecb73ac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6e01dbca6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
force-pushed
the
ci/fuzz-regression
branch
from
e6e01db to
f68353c
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All five prior findings are resolved at f68353c (LSan re-enabled with suppressions, timeout/kill exits now classified separately from crashes, deferred PROTOCOL_VERSION lookup with --stream-version override, MNAUTH comment redirect, and util_dash.h listed in non-backported.txt). Codex flags two remaining gaps in the new fuzz infrastructure: synthetic-seed generation failures are downgraded to warnings (allowing Dash-only targets to silently fall back to 10s empty-input runs), and _run_helper_self_checks is dead code despite being the only deterministic guard for the new helpers. Both verified; neither is blocking.
Reviewed commit: f68353c
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/test-fuzz.yml`:
- [SUGGESTION] lines 107-112: Synthetic seed generation failures silently demote targets to empty-input smoke tests
`seed_corpus_from_chain.py --synthetic-only` is invoked with `|| echo "::warning::..."`, so any failure in the synthetic generator is non-fatal. Targets whose corpus directory ends up empty fall through to the empty-corpus branch at L171-195, which only runs the harness for 10s with no replay inputs. That means a regression in the seed generator can remove the only structured inputs for several Dash-only deserialize/roundtrip targets (governance vote files, BLS-IES blobs, LLMQ messages, etc.) without failing CI, and the job will still go green as long as those harnesses don't crash on empty input. For a workflow whose purpose is corpus-replay regression testing, this masks exactly the failure mode the synthetic generator was added to prevent. Either fail the step on non-zero exit, or assert post-generation that the expected per-target seed directories are non-empty.
f68353c to
c11cf3c
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior review's two carry-forward findings are partially addressed at c11cf3c: the synthetic-seed failure mask has been replaced with a hard-fail loaded_external gate, but two issues remain. The narrower coverage gap is that the new gate only checks that some external corpus loaded — Dash-only targets not in the synthetic generator can still silently fall through to the empty-input 10s smoke branch if the pinned dashpay/qa-assets snapshot omits them. The dead _run_helper_self_checks() carry-over is unchanged.
Reviewed commit: c11cf3c
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/test-fuzz.yml`:
- [SUGGESTION] lines 170-193: Dash-only targets without synthetic seeds can silently fall through to empty-input smoke runs
The new `loaded_external` gate (L62, L102-105) correctly prevents a global fall-back to synthetic-only/empty-corpus runs, but it only verifies that *some* external corpus loaded — it does not assert per-target presence. The synthetic generator at `contrib/fuzz/seed_corpus_from_chain.py:1169-1264` covers a fixed subset (CoinJoin, LLMQ messages, DKG, MNAuth, governance vote/instance/rec/file, BLS IES, MNHF). Other newly added Dash-only targets — `dash_governance_object_common_deserialize`, `dash_proreg_tx_deserialize`, `dash_quorum_data_request_deserialize`, `dash_quorum_snapshot_deserialize`, `dash_get_quorum_rotation_info_deserialize` — are populated only by the live-chain extract paths (`seed_corpus_from_chain.py:699, 851, 979, 1039, 1106`) or by the external `dashpay/qa-assets` snapshot. If the pinned snapshot ever omits or renames one of those directories, the per-target check at L170 sees an empty `corpus_dir`, runs the harness with no inputs for 10s, and the job goes green without ever replaying a structured input for that harness. For a regression workflow this is a coverage hole — the fix is to assert that targets known to require external/live-chain inputs have a non-empty corpus directory, and fail the job otherwise.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-review at c11cf3c confirms two non-blocking gaps in the fuzz regression infrastructure. _run_helper_self_checks() remains unreachable, and the per-target replay loop still allows empty-corpus 10s smoke runs for Dash-only targets that are absent from both the pinned external snapshot and the synthetic seed generator. No blocking issues observed in the workflow, daemon, or harness sources.
Reviewed commit: c11cf3c
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/test-fuzz.yml`:
- [SUGGESTION] lines 170-194: Dash-only targets without seeds still fall back to empty-corpus smoke runs
The new `loaded_external` gate (L102-L105) only proves that at least one external corpus source was fetched — it does not verify that each registered target actually has corpus inputs. The per-target replay loop at L170-L194 still treats a missing or empty `/tmp/fuzz_corpus/${target}` as a passing 10-second empty-input run, masking the absence of real regression coverage. I checked the pinned `dashpay/qa-assets` snapshot (`b4d1a7ce41a9a21d522381944ba84b60b1ad5b60`) together with the synthetic seed generator output: several Dash-only deserialize/roundtrip targets still have no inputs, including `dash_credit_pool_deserialize`, `dash_dmn_state_deserialize`, `dash_get_quorum_rotation_info_deserialize`, `dash_quorum_snapshot_deserialize`, and `dash_smn_list_entry_deserialize` plus related roundtrip variants. Those targets therefore get only smoke-only execution while CI still goes green. Consider failing (or maintaining an explicit allowlist for) targets whose corpus directory is empty after all three layers have been applied, so newly added Dash targets cannot silently degrade.
c11cf3c to
9df19ad
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9df19ad170
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
force-pushed
the
ci/fuzz-regression
branch
from
9df19ad to
e1d88f6
Compare
|
Addressed the latest fuzz regression review feedback in
Validation run locally:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1d88f6c3f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Workflow corpus replay now classifies timeout/kill exits (124/137/143) distinctly from generic crashes in both log and FAILED_TARGETS, and captures the exit code in the non-empty-corpus branch like the empty branch does. - Defer src/version.h lookup in seed_corpus_from_chain.py until a stream version prefix is actually needed; --help no longer requires an in-tree checkout. Add --stream-version CLI flag and DASH_FUZZ_STREAM_VERSION env override. - Correct synthetic LLMQ seeds to match C++ serialization: CRecoveredSig now includes msgHash; CSigSesAnn uses VARINT(sessionId) + llmqType + quorumHash + id + msgHash; CSigShare adds quorumMember and 96-byte sigShare; CDKGComplaint includes a full 96-byte BLS signature after both DYNBITSETs. Self-checks assert the corrected byte sizes. - Replace stale TODO above the MNAUTH skip in process_message.cpp with a pointer to process_message_dash, which already exercises the Dash-aware MNAUTH setup. - Add src/test/fuzz/util_dash.h to non-backported.txt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
force-pushed
the
ci/fuzz-regression
branch
from
e1d88f6 to
fc87319
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-review at 9df19ad. The prior Dash-only smoke-fallback concern is resolved (the workflow now fails closed for dash_* targets lacking corpus). One carry-forward nit remains: _run_helper_self_checks() in contrib/fuzz/seed_corpus_from_chain.py is still unreachable, so its 'regression guard' assertions on governance helpers and LLMQ synthetic seed sizes never run in CI. No blocking issues.
Reviewed commit: 9df19ad
💬 1 nitpick(s)
| def _run_helper_self_checks(): | ||
| """Deterministic checks for new governance/final-commitment helpers.""" | ||
| parsed_vote = parse_governance_vote_record( | ||
| "11" * 32 + "-2:1700000000:yes:funding:1", | ||
| "22" * 32, | ||
| ) | ||
| assert parsed_vote["masternode_txid"] == "11" * 32 | ||
| assert parsed_vote["masternode_n"] == 2 | ||
| assert parsed_vote["signal"] == 1 | ||
| assert parsed_vote["outcome"] == 1 | ||
| vote_bytes = serialize_governance_vote(parsed_vote) | ||
| assert len(vote_bytes) == 32 + 4 + 32 + 4 + 4 + 8 + 1 | ||
|
|
||
| vote_instance = serialize_vote_instance(1, 1700000000, 1690000000) | ||
| assert len(vote_instance) == 20 | ||
| vote_rec = serialize_vote_rec({1: vote_instance, 2: serialize_vote_instance(2, 1700000100, 1690000000)}) | ||
| assert vote_rec.startswith(b"\x02") | ||
| vote_file = serialize_governance_vote_file([vote_bytes]) | ||
| assert vote_file[:4] == _serialize_int32(1) | ||
|
|
||
| payload = bytes.fromhex("0100") + _serialize_uint32(42) + b"\xaa\xbb\xcc" | ||
| assert _final_commitment_from_tx_payload(payload.hex()) == b"\xaa\xbb\xcc" | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| tmp_path = Path(tmpdir) | ||
| create_synthetic_seeds(tmp_path) | ||
| required_targets = [ | ||
| "dash_final_commitment_deserialize", | ||
| "dash_final_commitment_roundtrip", | ||
| "dash_governance_vote_deserialize", | ||
| "dash_vote_instance_deserialize", | ||
| "dash_vote_rec_deserialize", | ||
| "dash_governance_vote_file_deserialize", | ||
| "dash_bls_ies_encrypted_blob_deserialize", | ||
| "dash_bls_ies_encrypted_blob_roundtrip", | ||
| "dash_bls_ies_multi_recipient_blobs_deserialize", | ||
| "dash_bls_ies_multi_recipient_blobs_roundtrip", | ||
| "dash_coinjoin_entry_deserialize", | ||
| "dash_coinjoin_entry_roundtrip", | ||
| "dash_dkg_complaint_deserialize", | ||
| "dash_dkg_complaint_roundtrip", | ||
| "dash_dkg_justification_deserialize", | ||
| "dash_dkg_justification_roundtrip", | ||
| "dash_sig_shares_inv_deserialize", | ||
| "dash_sig_shares_inv_roundtrip", | ||
| "dash_batched_sig_shares_deserialize", | ||
| "dash_batched_sig_shares_roundtrip", | ||
| "dash_recovered_sig_deserialize", | ||
| "dash_recovered_sig_roundtrip", | ||
| "dash_sig_ses_ann_deserialize", | ||
| "dash_sig_ses_ann_roundtrip", | ||
| "dash_sig_share_deserialize", | ||
| "dash_sig_share_roundtrip", | ||
| "dash_mnauth_deserialize", | ||
| "dash_mnauth_roundtrip", | ||
| "dash_mnhf_tx_deserialize", | ||
| ] | ||
| missing = [target for target in required_targets if not any((tmp_path / target).iterdir())] | ||
| assert not missing, f"missing synthetic seeds for: {', '.join(missing)}" | ||
|
|
||
| # Assert the LLMQ seed sizes match the C++ serialization layouts so the | ||
| # synthetic seeds aren't silently truncated again (regression guard). | ||
| def _seed_bytes(target): | ||
| files = list((tmp_path / target).iterdir()) | ||
| assert len(files) == 1, f"{target}: expected one synthetic seed, got {len(files)}" | ||
| return files[0].read_bytes() | ||
|
|
||
| # The Dash deserialize/roundtrip wrappers prepend a 4-byte stream version. | ||
| prefix = len(_stream_version_prefix()) | ||
| # CRecoveredSig: 1 + 32 + 32 + 32 + 96 = 193 | ||
| assert len(_seed_bytes("dash_recovered_sig_deserialize")) == prefix + 193 | ||
| # CSigSesAnn (VARINT(0)=1 byte): 1 + 1 + 32 + 32 + 32 = 98 | ||
| assert len(_seed_bytes("dash_sig_ses_ann_deserialize")) == prefix + 98 | ||
| # CSigShare: 1 + 32 + 2 + 32 + 32 + 96 = 195 | ||
| assert len(_seed_bytes("dash_sig_share_deserialize")) == prefix + 195 | ||
| # CDKGComplaint with empty bitsets: 1 + 32 + 32 + 1 + 1 + 96 = 163 | ||
| assert len(_seed_bytes("dash_dkg_complaint_deserialize")) == prefix + 163 | ||
|
|
||
| # --stream-version override path: setting the override must not require | ||
| # src/version.h, and _stream_version_prefix must round-trip the value. | ||
| global _STREAM_VERSION_OVERRIDE, _STREAM_VERSION_CACHE | ||
| saved_override, saved_cache = _STREAM_VERSION_OVERRIDE, _STREAM_VERSION_CACHE | ||
| try: | ||
| _STREAM_VERSION_OVERRIDE = 0x12345678 | ||
| _STREAM_VERSION_CACHE = None | ||
| assert _stream_version_prefix() == b"\x78\x56\x34\x12" | ||
| finally: | ||
| _STREAM_VERSION_OVERRIDE = saved_override | ||
| _STREAM_VERSION_CACHE = saved_cache |
There was a problem hiding this comment.
💬 Nitpick: _run_helper_self_checks() is dead code — its 'regression guard' assertions never execute
_run_helper_self_checks() (L1281) is the only place that asserts the new governance vote serializers (parse_governance_vote_record, serialize_governance_vote, serialize_vote_instance, serialize_vote_rec, serialize_governance_vote_file), the _final_commitment_from_tx_payload() extractor, the --stream-version override round-trip, and — explicitly labelled as a regression guard at L1341–L1357 — the LLMQ synthetic-seed byte-size invariants (CRecoveredSig=193, CSigSesAnn=98, CSigShare=195, CDKGComplaint=163). However, main() (L1372) never calls it, the argparse block (L1394–L1403) exposes no --self-check flag, and a repo-wide grep shows no other call site. As-written, the regression-guard comment is misleading: a future change that breaks any of those layouts will not be caught by CI or by --synthetic-only runs. Wire it into main() (e.g., behind a --self-check flag, or unconditionally as part of --synthetic-only) or migrate the assertions into a unit test that actually executes in CI.
source: ['claude', 'codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all three Claude findings against the worktree at fc87319. All are valid non-blocking observations about fuzz infrastructure: the new --self-check entrypoint exists but no CI step invokes it; six dash_* deserialize/roundtrip targets have no synthetic seeds and depend entirely on pinned external corpus; and the cumulative MoneyRange assert in asset_lock_unlock cannot fire under the harness's own bounds. Codex reported no findings on this SHA. No blockers.
Reviewed commit: fc87319
Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.
Code Review
Reviewed commit: fc87319
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `contrib/fuzz/seed_corpus_from_chain.py`:
- [SUGGESTION] lines 1297-1427: Several dash_* deserialize/roundtrip targets have no synthetic seeds
The CI workflow runs `seed_corpus_from_chain.py --synthetic-only` at .github/workflows/test-fuzz.yml:110, which skips the live `extract_special_txs()` / `extract_masternode_list()` extractors. Since the workflow now hard-fails for any `dash_*` target without corpus inputs (test-fuzz.yml:194-201, with `DASH_SMOKE_ONLY_ALLOWLIST` empty), these registered Dash-only targets must come from the pinned `dashpay/qa-assets` snapshot — they are absent from the `synthetic_seeds` dict at lines 1297-1427:
- `dash_proreg_tx_deserialize` / `_roundtrip` (only seeded live by extract_masternode_list at line 851)
- `dash_proupserv_tx_*`, `dash_cbtx_*`, `dash_asset_lock_payload_*`, `dash_asset_unlock_payload_*` (only seeded live by extract_special_txs type_map at lines 594-604)
- `dash_dmn_state_diff_deserialize` / `_roundtrip` (no extractor, no synthetic seed — `dmn_state_diff` does not appear anywhere in the script)
If a future bump of `DASH_QA_ASSETS_SHA` (test-fuzz.yml:56) drops or renames these directories, the regression job breaks with a missing-corpus failure even though no Dash code changed. Adding minimal synthetic entries — the wire layouts for ProRegTx/ProUpServTx/CbTx/AssetLockPayload/AssetUnlockPayload/CDeterministicMNStateDiff are already used by adjacent synthetic builders — would make the workflow self-contained and match how the rest of the dash_* targets are now seeded.
2 participants
Summary
Consolidates all active Dash Core fuzzing work into a single branch/PR
so we avoid cross-PR merge conflicts.
This PR now contains both:
(deserialization, roundtrip, and functional validators)
(CI regression workflow, corpus tooling, and continuous fuzz daemon)
This is the end-to-end delivery for the Dash Core fuzzing initiative
(tracker#108).
Included work (folded into this PR)
The following previously separate fuzz PRs were merged into this branch
and closed in favor of this consolidated PR:
— Dash-specific deserialization fuzz targets
— roundtrip serialize/deserialize fuzz targets
— governance proposal validator fuzz target
— BLS operations + BLS IES fuzz targets
— asset lock/unlock validation fuzz targets
— special transaction validation fuzz target
— LLMQ message handler fuzz target
— CoinJoin validation fuzz targets
What this PR now delivers
1) Dash-specific fuzz target coverage
Adds broad fuzz coverage for Dash-specific serialization and functional
validation paths across:
evo/(ProTx, asset lock/unlock, deterministic MN structures)llmq/(commitments, DKG/session message handling, quorum data)governance/(objects/votes/proposal validation)coinjoin/(queue/broadcast/denomination/status/entry logic)bls/(operations, aggregation, threshold ops, IES encryption)2) CI fuzz regression workflow (
test-fuzz.yml)linux64_fuzzbitcoin-core/qa-assetsSKIP_LINUX64_FUZZvariable3) Continuous fuzzing daemon
contrib/fuzz/continuous_fuzz_daemon.sh:~/fuzz_corpus/<target>/~/fuzz_crashes/<target>/--single-cycle4) Seed corpus generator
contrib/fuzz/seed_corpus_from_chain.py:--synthetic-onlyfor offline corpus generationValidation
Build verification
Built fuzz binary with
--enable-fuzz-binary(PROVIDE_FUZZ_MAIN_FUNCTIONdeterministic mode) on macOS arm64 (Apple Silicon, clang, 16 cores).
Binary compiled successfully with all 95 Dash-specific targets registered
(252 total targets including inherited Bitcoin Core targets).
Target registration check
All 95 new Dash-specific fuzz targets confirmed registered in the binary:
deterministic MN, credit pool), llmq (commitments, DKG messages, quorum
snapshots, sig shares), governance (objects, votes), coinjoin (queue,
broadcast, entry, status, accept), bls (IES encrypted blobs, multi-recipient)
consistency for all major Dash protocol structures
asset_lock_tx,asset_lock_tx_raw,asset_unlock_fee,bls_operations,bls_ies,coinjoin_broadcasttx,coinjoin_denominations,coinjoin_entry_addscriptsig,coinjoin_queue,coinjoin_status_update,deterministic_mn_list_diff,governance_proposal_validator,llmq_messages,special_tx_validation,process_message_dashConcrete run results
Each of the 95 Dash-specific targets was exercised with 21 inputs
(20 random seed inputs of varying sizes 1-256 bytes + 1 empty input).
All targets processed every input without crashes or hangs.
Summary: 95/95 targets PASS, 0 crashes, 0 timeouts.
Total inputs processed: 1,995 (95 targets x 21 inputs)
Continuous daemon validation
Daemon dry-run confirmed on Guix VM — target enumeration, corpus dir
creation, and crash dir creation all verified:
Seed corpus generation
Synthetic corpus generation validated (offline mode, no running node):
CI behavior
linux64_fuzzartifactsNotes on validation scope
This validation confirms all targets build, register, and handle arbitrary
input without crashing in deterministic mode. Extended fuzzing campaigns
(with libFuzzer engine + coverage feedback) are tracked separately under
the continuous fuzzing deployment plan.
Bugs surfaced during fuzzing (pre-existing issues)
(
CCoinJoinQueue::IsTimeOutOfBoundssigned overflow case)bls_operationscampaigns(libdashbls internals)
CQuorumSnapshot::UnserializemissingmnSkipList.clear()before read loop
Notes
review/merge unit for fuzzing.
while review is in progress.