contrib: add deterministic fuzz coverage mode #35809

HowHsu commented at 2:17 PM on July 26, 2026: contributor

Adds an optional integer coverage_check argument to deterministic-fuzz-coverage. This makes it easier to isolate per-input nondeterminism from cross-input state leakage. Good for debugging.

DrahtBot added the label Scripts and tools on Jul 26, 2026

DrahtBot commented at 2:17 PM on July 26, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35809.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	maflcko, Crypt-iQ
Stale ACK	jeanpablojp, marcofleon

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35608 (contrib: Skip llvm-cov rendering for deterministic fuzz inputs by HowHsu)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

HowHsu commented at 8:02 AM on August 13, 2026: contributor

Hi @maflcko , any chacne to have a look at this one when you're available. Thanks.

jeanpablojp commented at 10:16 PM on August 18, 2026: contributor

tACK e01ca9bacdeb860291b1824cac7fb099a3b41111

Built the tool and ran all three modes against an instrumented binary I put together here, all green, and the check catches nondeterminism on both paths. Mode 0 came out identical to master.

Silly nit: could the motivation go into the commit message?

HowHsu force-pushed on Aug 24, 2026

HowHsu commented at 11:57 AM on August 24, 2026: contributor

tACK e01ca9b

Built the tool and ran all three modes against an instrumented binary I put together here, all green, and the check catches nondeterminism on both paths. Mode 0 came out identical to master.

Silly nit: could the motivation go into the commit message?

Sorry for the delay, updated.

in contrib/devtools/deterministic-fuzz-coverage/src/main.rs:41 in 635b1cccb6

  36 | +                ))
  37 | +            })?,
  38 | +            None => 0,
  39 | +        };
  40 | +        match mode {
  41 | +            0 => Ok(Self::Both),

maflcko commented at 5:12 PM on September 1, 2026:

Would be cleaner to use properly named strings for this enum? E.g:

            "both" => Ok(Self::Both),
            "single" => Ok(Self::IndividualInputs),
            "combined" => Ok(Self::AllInputs),
            other => Err(exit_help(&format!(
                "Invalid coverage check mode '{other}'. Expected 'both', 'single', or 'combined'"
            ))),

Possibly this could just be a single bool without any extra class and with proper named args:

let mut check_individual = true; // default is "both"

// skip the pos args (everything after is a named arg)
for arg in env::args().skip(5) {
   if let Some(value) = arg.strip_prefix("--mode=") {
        check_individual = match value {
            "both" => true,
            "combined" => false,
            other => return Err(help(&format!(
                "Invalid mode '{other}'. Expected 'both' or 'combined'"
            ))),
        };
    }else {
        return Err(help(&format!("Too many args, or unknown named arg: {arg}")));
    }
}

The rationale being that there should be no reason to skip the full combined check, only the expensive single check. Otherwise, two boolean flags are still simpler than a full enum class for this?

HowHsu commented at 2:01 PM on September 3, 2026:

The rationale being that there should be no reason to skip the full combined check, only the expensive single check. Otherwise, two boolean flags are still simpler than a full enum class for this?

The motivation to propose this PR is when I fixed a coverage issue, I run the tool to verify if it worked, and it's really time-consuming. Forgot which target I used, but I recall the combined run itself is time-consuming too, let's go wtih the former one?

            "both" => Ok(Self::Both),
            "single" => Ok(Self::IndividualInputs),
            "combined" => Ok(Self::AllInputs),
            other => Err(exit_help(&format!(
                "Invalid coverage check mode '{other}'. Expected 'both', 'single', or 'combined'"
            ))),

maflcko approved

maflcko commented at 5:15 PM on September 1, 2026: member

Seems fine, but would be good to use named args at some point 😅

fanquake commented at 8:53 AM on September 2, 2026: member

cc @marcofleon @Crypt-iQ

HowHsu force-pushed on Sep 3, 2026

in contrib/devtools/README.md:29 in 754a9e908f

  21 | @@ -22,9 +22,13 @@ repository must have been cloned. Finally, a fuzz target has to be picked
  22 |  before running the tool:
  23 |  
  24 |  ```
  25 | -cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/fuzz_corpora fuzz_target_name
  26 | +cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/fuzz_corpora fuzz_target_name [parallelism] [coverage_check]
  27 |  ```
  28 |  
  29 | +The optional `coverage_check` argument controls which checks are run: `both`
  30 | +runs both checks, `single` only checks each input individually, and `combined`

Crypt-iQ commented at 5:31 PM on September 4, 2026:

nit: could be more descriptive for somebody reading this and say: both runs each input individually and all inputs in one go, or put it at the end after combined

maflcko commented at 8:51 AM on September 18, 2026:

I don't think there is the need to repeat the help here. One can call --help, or more on the source code to read it.

Could be removed, but no strong opinion.

HowHsu commented at 1:05 PM on September 21, 2026:

updated.

Crypt-iQ commented at 7:06 PM on September 4, 2026: contributor

crACK 754a9e908ff828341d8eee657584b8ebd31f58e3

marcofleon commented at 3:57 PM on September 10, 2026: contributor

tACK 754a9e908ff828341d8eee657584b8ebd31f58e3

in contrib/devtools/deterministic-fuzz-coverage/src/main.rs:102 in 754a9e908f

  98 | @@ -74,7 +99,8 @@ fn app() -> AppResult {
  99 |          None => DEFAULT_PAR,
 100 |      }
 101 |      .max(1);
 102 | -    if args.get(5).is_some() {
 103 | +    let coverage_check = CoverageCheck::from_arg(args.get(5).map(String::as_str))?;

maflcko commented at 8:54 AM on September 18, 2026:

Again, would be nice to start using proper named args instead of the forced named+positional hack:

E.g.:

// skip the pos args (everything after is a named arg)
for arg in env::args().skip(5) {
   if let Some(value) = arg.strip_prefix("--mode=") {
           let coverage_check = CoverageCheck::from_arg(value)?;
    }else {
        return Err(help(&format!("Too many args, or unknown named arg: {arg}")));
    }
}

HowHsu commented at 1:06 PM on September 21, 2026:

Updated.

in contrib/devtools/deterministic-fuzz-coverage/src/main.rs:247 in 754a9e908f

 274 | -                        Err(_e) => Err("A scoped thread panicked".to_string()),
 275 | -                        Ok(r) => r,
 276 | -                    };
 277 | -                    if thread_result.is_err() {
 278 | -                        res = thread_result;
 279 | +    if coverage_check != CoverageCheck::AllInputs {

maflcko commented at 9:01 AM on September 18, 2026:

Again, using two plain booleans to toggle the two paths independently seems simpler than an exclusive enum class with inversion?

Its less logic, less inversion, and less code?

Above:

            "both" => run_combined=true;run_single=true,
            "single" => run_single=true,
            "combined" => run_combined=true,
            other => Err(exit_help(&format!(
                "Invalid coverage check mode '{other}'. Expected 'both', 'single', or 'combined'"
            ))),

HowHsu commented at 1:06 PM on September 21, 2026:

Updated.

maflcko approved

maflcko commented at 9:02 AM on September 18, 2026: member

lgtm

maflcko commented at 9:02 AM on September 18, 2026: member

Also, the pull description is outdated? Maybe just remove the bullet list from it?

contrib: add deterministic fuzz coverage mode

Add an optional coverage_check argument to deterministic-fuzz-coverage.
The `both` mode runs both checks and preserves the current default
behavior. The `single` mode checks each corpus input individually.
The `combined` mode checks all corpus inputs in a single process.

This makes it easier to distinguish per-input nondeterminism from
cross-input state leakage. Good for debugging.

Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>

5945cb83dd

HowHsu force-pushed on Sep 21, 2026

HowHsu commented at 1:06 PM on September 21, 2026: contributor

Also, the pull description is outdated? Maybe just remove the bullet list from it?

Updated.

maflcko commented at 1:23 PM on September 21, 2026: member

review ACK 5945cb83ddcf5df841d1bcb3c5fea641e9e24cde 🌯

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 5945cb83ddcf5df841d1bcb3c5fea641e9e24cde 🌯
3Hr/ExyoZR2PqhigX7ccQAwaa8oV+lbI7IORj2h7Wwrj/g4bPKnhaul/JiGzkarPIfwWxRTwAwgGNOXukVssCQ==

</details>

DrahtBot requested review from marcofleon on Sep 21, 2026

DrahtBot requested review from Crypt-iQ on Sep 21, 2026

Crypt-iQ commented at 9:55 PM on September 22, 2026: contributor

crACK 5945cb83ddcf5df841d1bcb3c5fea641e9e24cde

maflcko added this to the milestone 33.0 on Sep 23, 2026

fanquake merged this on Sep 23, 2026

fanquake closed this on Sep 23, 2026