contrib: Add deterministic-unittest-coverage #31901

pull maflcko wants to merge 5 commits into bitcoin:master from maflcko:2502-deterministic-unittest-coverage changing 13 files +227 −195
  1. maflcko commented at 8:30 PM on February 18, 2025: member

    The contrib/devtools/test_deterministic_coverage.sh script is problematic:

    • It is written in bash. This can lead to issues when running with the ancient bash version shipped by macOS by default, or can lead to other compatibility issues, such as #31588 (review). Also, pipefail isn't set, so IO errors may be silently ignored.
    • It is based on gcov. This can lead to issues, such as #31588#pullrequestreview-2602169248 (possibly due to prefix-map), or #31588 (comment) (gcovr processing error), or #31588#pullrequestreview-2605954001 (gcovr assertion error).
    • The script is severely outdated, with the last update to NON_DETERMINISTIC_TESTS being in the prior decade.

    Instead of patching around all issues one-by-one, just provide a fresh rewrite, based on the recently added deterministic-fuzz-coverage tool based on clang, llvm-cov, and llvm-profdata. (Initial feedback indicates that this is a more promising attempt: #31588 (comment) and #31588 (comment)).

    The new tool also sets RANDOM_CTX_SEED=21 as suggested by hodlinator in #31588 (comment).

  2. gitignore: target/
    Needed for the recently added
contrib/devtools/deterministic-fuzz-coverage/target/
    fa1e0a7228
  3. DrahtBot commented at 8:30 PM on February 18, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator, brunoerg, dergoegge
    Concept ACK theStack, janb84, Prabhat1308

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  4. DrahtBot renamed this:
    contrib: Add deterministic-unittest-coverage
    contrib: Add deterministic-unittest-coverage
    on Feb 18, 2025
  5. DrahtBot added the label Scripts and tools on Feb 18, 2025
  6. dergoegge commented at 10:10 AM on February 19, 2025: member

    Concept ACK

  7. in contrib/devtools/deterministic-unittest-coverage/src/main.rs:26 in bbbb6998f9 outdated 
      21 | +    exit(1)
  22 | +}
  23 | +
  24 | +fn sanity_check(test_exe: &Path) {
  25 | +    for tool in [LLVM_PROFDATA, LLVM_COV, DIFF] {
  26 | +        let output = Command::new(tool).arg("--version").output();
    theStack commented at 1:32 PM on February 21, 2025:

    On the machine where I tried this yesterday quickly (OpenBSD 7.6, with LLVM 16.0.6 included in the base system), neither llvm-profdata nor diff know a --version argument, so the sanity check failed even though the binaries were there. Will try on a Linux machine with Ubuntu in a bit.

    maflcko commented at 1:51 PM on February 21, 2025:

    It should be possible to drop diff and use git diff --no-index (or something else). As for the llvm tool, it looks like they added the arg later:

    # llvm-profdata-19 --version 
Debian LLVM version 19.1.4
  Optimized build.
# llvm-profdata-16 --version
llvm-profdata-16: Unknown command!
USAGE: llvm-profdata-16 <merge|show|overlap> [args...]
    maflcko commented at 1:53 PM on February 21, 2025:

    Looks like they added it in 17, so I could require that, or remove the --version check.

    # llvm-profdata-17 --version 
llvm-profdata-17
Ubuntu LLVM version 17.0.6
  Optimized build.
    maflcko commented at 10:58 AM on February 24, 2025:

    thx, fixed

  8. in contrib/devtools/deterministic-unittest-coverage/src/main.rs:18 in bbbb6998f9 outdated 
      13 | +const DIFF: &str = "diff";
  14 | +
  15 | +fn exit_help(err: &str) -> ! {
  16 | +    eprintln!("Error: {}", err);
  17 | +    eprintln!();
  18 | +    eprintln!("Usage: program ./build_dir [custom test filter (default: all but known non-deterministic tests)]");
    theStack commented at 1:34 PM on February 21, 2025: 
        eprintln!("Usage: program ./build_dir <custom test filter>");

    as there is no default yet and specifying a filter is required by now?

    maflcko commented at 10:58 AM on February 24, 2025:

    thx, fixed

  9. theStack commented at 1:36 PM on February 21, 2025: contributor

    Concept ACK

  10. maflcko force-pushed on Feb 21, 2025
  11. in contrib/devtools/deterministic-unittest-coverage/src/main.rs:111 in fafce12a48 outdated 
     106 | +        let same = Command::new(GIT)
 107 | +            .args(["--no-pager", "diff", "--no-index"])
 108 | +            .arg(a)
 109 | +            .arg(b)
 110 | +            .status()
 111 | +            .expect("Failed to execute diff command")
    hodlinator commented at 10:43 PM on February 21, 2025:

    Detected diff between fuzz and unit test.

                .expect("Failed to execute git command")
    maflcko commented at 10:58 AM on February 24, 2025:

    thx, done

  12. in contrib/devtools/deterministic-unittest-coverage/src/main.rs:20 in fafce12a48 outdated 
      15 | +fn exit_help(err: &str) -> ! {
  16 | +    eprintln!("Error: {}", err);
  17 | +    eprintln!();
  18 | +    eprintln!("Usage: program ./build_dir boost_unittest_filter");
  19 | +    eprintln!();
  20 | +    eprintln!("Refer to the devtools/README.md for more details.");
    hodlinator commented at 11:17 PM on February 21, 2025:

    One write to stderr should suffice? Also {err} when we can. (Think I've seen discouragement of std::endl in C++ land in favor of \n, this felt similar).

        eprintln!(
        "Error: {err}\n\
         \n\
         Usage: program ./build_dir ./qa-assets/fuzz_corpora fuzz_target_name\n\
         \n\
         Refer to the devtools/README.md for more details."
    );
    maflcko commented at 10:58 AM on February 24, 2025:

    I don't like the manual \n handling and the manual \ handling. An alternative would be a string which natively contains newlines. However, I don't think https://clang.llvm.org/extra/clang-tidy/checks/performance/avoid-endl.html applies, because this isn't performance critical. Leaving as-is for now.

  13. in contrib/devtools/deterministic-unittest-coverage/src/main.rs:79 in fafce12a48 outdated 
      74 | +            .env("LLVM_PROFILE_FILE", &profraw_file)
  75 | +            .env("BOOST_TEST_RUN_FILTERS", filter)
  76 | +            .env("RANDOM_CTX_SEED", "21")
  77 | +            .status()
  78 | +            .expect("test failed")
  79 | +            .success());
    hodlinator commented at 11:49 PM on February 21, 2025:

    Current output on !success() due to use of assert! for other things than logical invariants:

    â‚¿ cargo run --release ~/b2/build miniscript
   Compiling deterministic-unittest-coverage v0.1.0 (/home/hodlinator/b2/contrib/devtools/deterministic-unittest-coverage)
    Finished `release` profile [optimized] target(s) in 0.36s
     Running `target/release/deterministic-unittest-coverage /home/hodlinator/b2/build miniscript`
Test setup error: no test cases matching filter or all test cases were disabled
thread 'main' panicked at src/main.rs:73:9:
assertion failed: Command::new(test_exe).env("LLVM_PROFILE_FILE",
                        &profraw_file).env("BOOST_TEST_RUN_FILTERS",
                    filter).env("RANDOM_CTX_SEED",
                "21").status().expect("test failed").success()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

    With suggestion:

    â‚¿ cargo run --release ~/b2/build miniscript                                                                                                                                  
   Compiling deterministic-unittest-coverage v0.1.0 (/home/hodlinator/b2/contrib/devtools/deterministic-unittest-coverage)
    Finished `release` profile [optimized] target(s) in 0.36s
     Running `target/release/deterministic-unittest-coverage /home/hodlinator/b2/build miniscript`
Test setup error: no test cases matching filter or all test cases were disabled
/home/hodlinator/b2/build/src/test/test_bitcoin failed

            if !Command::new(test_exe)
            .env("LLVM_PROFILE_FILE", &profraw_file)
            .env("BOOST_TEST_RUN_FILTERS", filter)
            .env("RANDOM_CTX_SEED", "21")
            .status()
            .expect("test failed")
            .success()
        {
            eprintln!("{} failed", test_exe.display());
            std::process::exit(1)
        }
    maflcko commented at 10:58 AM on February 24, 2025:

    You are right that the use of assert here may be a bit wrong to sanitize an input string. However, in the context of tests (or dev-only test-only scripts), the distinction doesn't matter, as long as any failure happens. (This is also similar of how the functional tests simply do an assert on the return code)

    In any case, if this was changed, it should be applied to almost all unwrap,expect,or panic as well? For example, I could imagine that expect("test failed") could fail when the exe was missing the executable bit?

    Leaving as-is for now.

    hodlinator commented at 12:50 PM on February 24, 2025:

    In any case, if this was changed, it should be applied to almost all unwrap,expect,or panic as well? For example, I could imagine that expect("test failed") could fail when the exe was missing the executable bit?

    The expect()s here are fine since we've already run sanity_check() which has proper exit(1) behavior via exit_help().

    So I really do think since we are passing the user-input filter value, we should not be triggering an assert!. I think for the other commands where we are using fixed inputs, asserts are more okay.

    maflcko commented at 1:19 PM on February 24, 2025:

    I don't think the other asserts are any different. For example, LLVM_PROFDATA may fail, because the executable was not compiled with coverage:

    error: test_det_cov.profraw: No such file or directory
thread 'main' panicked at src/main.rs:80:9:
assertion failed: Command::new(LLVM_PROFDATA).arg("merge").arg("--sparse").arg(&profraw_file).arg("-o").arg(&profdata_file).status().expect("merge failed").success()
stack backtrace:
...

    Similarly, as already mentioned, the expect("test failed") can also fail:

    thread 'main' panicked at src/main.rs:78:14:
test failed: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }
stack backtrace:
...

    No strong opinion, but I think this should either be done for all places, or for none. What is more important to me is the raw functionality this offers and I don't want to spend too much time on adjusting error messages one way or the other.

    hodlinator commented at 1:30 PM on February 24, 2025:

    No strong opinion, but I think this should either be done for all places, or for none.

    Note that you are using exit(1) for some cases, so you are already drawing an arbitrary line.

    A user using something like miniscript as input is more likely than an permission failure IMO. Agree that not compiling with coverage is somewhere in between though.

    If we're going to introduce more Rust code in the project, and use it in an interactive way, I think we should spend some time to make the user experience on par or better than Bash in every way.

    maflcko commented at 2:01 PM on February 24, 2025:

    I don't think this has something to do with the language it is written in. I am sure it is possible to write a check for the exit code in any language and print a different error message than an assertion failure.

    Note that you are using exit(1) for some cases, so you are already drawing an arbitrary line.

    Yes, currently the line here is for any sanity check that can be cheaply done while parsing the args and print the help on failure.

    I understand your concern, and I'd be happy to address it wholesale in a follow-up for all places. I expect the follow-up to be trivial to review.

    However, for now, my preference would be to keep this script as close to the existing fuzz coverage script as possible. For now, I want to focus the review on the core functionality.

    Also, I'd be happy to push your suggested change, if this was your only blocker. However, I don't want to spend time on shaping error messages, when not a single reviewer thinks the code is otherwise acceptable to merge (it has zero code acks right now).

    hodlinator commented at 2:34 PM on February 24, 2025:

    Responding to #31901 (review):

    I don't think this has something to do with the language it is written in. I am sure it is possible to write a check for the exit code in any language and print a different error message than an assertion failure.

    Bash can't spew stacktraces AFAIK. And the "noise" is even larger than I posted initially, if one runs with the recommended args from contrib/devtools/README.md:

    â‚¿ RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build boost_unittest_filter
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running `contrib/devtools/deterministic-unittest-coverage/target/debug/deterministic-unittest-coverage /home/hodlinator/b2/build boost_unittest_filter`
Test setup error: no test cases matching filter or all test cases were disabled
thread 'main' panicked at src/main.rs:73:9:
assertion failed: Command::new(test_exe).env("LLVM_PROFILE_FILE",
                        &profraw_file).env("BOOST_TEST_RUN_FILTERS",
                    filter).env("RANDOM_CTX_SEED",
                "21").status().expect("test failed").success()
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic
   3: deterministic_unittest_coverage::deterministic_coverage::{{closure}}
             at ./contrib/devtools/deterministic-unittest-coverage/src/main.rs:73:9
   4: deterministic_unittest_coverage::deterministic_coverage
             at ./contrib/devtools/deterministic-unittest-coverage/src/main.rs:120:14
   5: deterministic_unittest_coverage::main
             at ./contrib/devtools/deterministic-unittest-coverage/src/main.rs:65:5
   6: core::ops::function::FnOnce::call_once
             at /build/rustc-1.84.1-src/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

    I understand your concern, and I'd be happy to address it wholesale in a follow-up for all places. I expect the follow-up to be trivial to review.

    Also, I'd be happy to push your suggested change, if this was your only blocker. However, I don't want to spend time on shaping error messages, when not a single reviewer thinks the code is otherwise acceptable to merge (it has zero code acks right now).

    I agree it can be done in a follow-up.

    maflcko commented at 10:35 AM on February 25, 2025:

    I agree it can be done in a follow-up.

    Closing thread for now, but I'll keep the follow-up in mind

    maflcko commented at 9:03 AM on March 15, 2025:

    :nail_care: Applied nail polish to the messages in https://github.com/bitcoin/bitcoin/pull/32074

  14. in contrib/devtools/README.md:37 in fafce12a48 outdated 
      34 | +```
  35 | +
  36 | +To execute the tool, compilation has to be done with the build options
  37 | +`-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++'
  38 | +-DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'`. Both
  39 | +llvm-profdata and llvm-cov must be installed.
    hodlinator commented at 11:56 PM on February 21, 2025:

    For the sake of easy copy-paste when viewing the .md file directly within editor:

    To execute the tool, compilation has to be done with the build options:
```
-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
```
Both llvm-profdata and llvm-cov must be installed.

    Same for the fuzz-version.

    hodlinator commented at 10:50 AM on February 24, 2025:

    A minor improvement could be to at least add \.

    To execute the tool, compilation has to be done with the build options
`-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' \
-DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'`. Both
llvm-profdata and llvm-cov must be installed.

    But the beginning/end of lines would then still contain text one doesn't want to copy, so I prefer the first suggestion.

    maflcko commented at 10:58 AM on February 24, 2025:

    Thx, done

  15. hodlinator commented at 3:09 PM on February 22, 2025: contributor

    Concept ACK fafce12a4871ed66a868b460b779e794a89218eb

  16. doc: Remove unused -fPIC
    This is harmless, but no longer needed after commit
06b9236f4327525875768af5fc554c651c5ec3cf.
    faf905b9b6
  17. contrib: deterministic-fuzz-coverage fixups
    * Name the fuzz_corpora dir after its real name.
* Add missing cargo lock file.
* Use git instead of diff command to increase compatibility
* Use --help instead of --version to increase compatibility
* Use assert consistently for unexpected errors.
* Remove redundant Stdio::from.
* Fix typos.
    fa3940b1cb
  18. maflcko force-pushed on Feb 24, 2025
  19. in contrib/devtools/README.md:46 in fafa592a8a outdated 
      48 | +```
  49 | +
  50 | +Both llvm-profdata and llvm-cov must be installed.
  51 | +
  52 | +```
  53 | +RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_dir boost_unittest_filter
    hodlinator commented at 3:37 PM on February 24, 2025:

    Curious why you include RUST_BACKTRACE=1 in the recommended command line?

    Also could maybe use ~angle~arrow-brackets or something to denote input?

    cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_dir <boost_unittest_filter>
    maflcko commented at 9:10 AM on February 25, 2025:

    Curious why you include RUST_BACKTRACE=1 in the recommended command line?

    I just wanted the tool to behave similar to python when an assertion error is hit or when an "exception" is thrown. However, this can go away in the patch that polishes the error messages, which I plan to do in a follow-up.

    maflcko commented at 10:35 AM on February 25, 2025:

    Closing thread for now, but I'll keep the follow-up in mind

  20. in contrib/devtools/deterministic-unittest-coverage/src/main.rs:123 in fafa592a8a outdated 
     118 | +        }
 119 | +    };
 120 | +    let r0 = run_single(0);
 121 | +    let r1 = run_single(1);
 122 | +    check_diff(&r0, &r1);
 123 | +    println!("Coverage test passed.");
    hodlinator commented at 4:00 PM on February 24, 2025:

    nit: Could provide a more satisfying message.

        println!("Coverage test passed - determinism held for given conditions in 2 consecutive runs!");
    maflcko commented at 9:12 AM on February 25, 2025:

    thx, pushed something

  21. hodlinator approved
  22. hodlinator commented at 4:47 PM on February 24, 2025: contributor

    ACK fafa592a8ad95fcb64db3d342cdc7192ff7ef2cd

    Was able to use this to diagnose and disable non-determinism issues for: cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_tests

    <details><summary>Hacky WIP pro-determinism patch resulting in pass</summary>

    diff --git a/src/random.cpp b/src/random.cpp
index 5b605e988d..087894ffcd 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -441,6 +441,8 @@ public:
 
     ~RNGState() = default;
 
+    bool IsDeterministic() noexcept EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { LOCK(m_mutex); return m_deterministic_prng.has_value(); }
+
     void AddEvent(uint32_t event_info) noexcept EXCLUSIVE_LOCKS_REQUIRED(!m_events_mutex)
     {
         LOCK(m_events_mutex);
@@ -642,6 +644,7 @@ void ProcRand(unsigned char* out, int num, RNGLevel level, bool always_use_real_
     assert(num <= 32);
 
     CSHA512 hasher;
+    if (!rng.IsDeterministic() || always_use_real_rng) {
     switch (level) {
     case RNGLevel::FAST:
         SeedFast(hasher);
@@ -653,6 +656,7 @@ void ProcRand(unsigned char* out, int num, RNGLevel level, bool always_use_real_
         SeedPeriodic(hasher, rng);
         break;
     }
+    }
 
     // Combine with and update state
     if (!rng.MixExtract(out, num, std::move(hasher), false, always_use_real_rng)) {
@@ -771,7 +775,7 @@ FastRandomContext::FastRandomContext(bool fDeterministic) noexcept : requires_se
 void RandomInit()
 {
     // Invoke RNG code to trigger initialization (if not already performed)
-    ProcRand(nullptr, 0, RNGLevel::FAST, /*always_use_real_rng=*/true);
+    ProcRand(nullptr, 0, RNGLevel::FAST, /*always_use_real_rng=*/false);
 
     ReportHardwareRand();
 }
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index bf26997c07..a6349f3280 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -78,7 +78,6 @@ static FastRandomContext g_rng_temp_path;
 static const bool g_rng_temp_path_init{[] {
     // Must be initialized before any SeedRandomForTest
     Assert(!g_used_g_prng);
-    (void)g_rng_temp_path.rand64();
     g_used_g_prng = false;
     return true;
 }()};

    </details>

  23. DrahtBot requested review from theStack on Feb 24, 2025
  24. DrahtBot requested review from dergoegge on Feb 24, 2025
  25. maflcko force-pushed on Feb 25, 2025
  26. maflcko commented at 9:12 AM on February 25, 2025: member

    cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_tests Hacky WIP pro-determinism patch resulting in pass

    Thx, pushed a different fix/hack for this. I tested it on util_string_tests.

  27. contrib: Add deterministic-unittest-coverage
    This replaces the bash script with a tool based on clang/llvm tools.
    fa579d663d
  28. test: Exclude SeedStartup from coverage counts fa99c3b544
  29. maflcko force-pushed on Feb 25, 2025
  30. in src/test/util/setup_common.cpp:84 in fa99c3b544 
      80 | @@ -80,6 +81,7 @@ static const bool g_rng_temp_path_init{[] {
  81 |      Assert(!g_used_g_prng);
  82 |      (void)g_rng_temp_path.rand64();
  83 |      g_used_g_prng = false;
  84 | +    ResetCoverageCounters(); // The seed strengthen in SeedStartup is not deterministic, so exclude it from coverage counts
    hodlinator commented at 9:59 AM on February 25, 2025:

    Static initialization should logically run first for linked libraries, but I just tested it and that was not the case. Code run during static initialization in tests was run before this lambda.

    This means we could be accidentally hiding obscure cases/sources of non-determinism, but:

    • If such early non-deterministic code has downstream effects on determinism of tests we will hopefully realize ResetCoverageCounters may be hiding something.
    • If early code doesn't have downstream effects on determinism of tests, then we probably don't care.

    At least test fixture code like the BasicTestingSetup-ctor in BOOST_FIXTURE_TEST_SUITE(util_tests, BasicTestingSetup) is just registered and actually run later (otherwise g_rng_temp_path_init hadn't worked to begin with).

    maflcko commented at 10:34 AM on February 25, 2025:

    Generally, tests shouldn't be using globals. When global state is used nonetheless, at least the (de-)initialization should happen in a well-defined order after main. Thus, I agree with your comment.

  31. hodlinator approved
  32. hodlinator commented at 10:26 AM on February 25, 2025: contributor

    re-ACK fa99c3b544b631cfe34d52fb5e71636aedb1b423

    Minor text-only changes + Fix for source of differing coverage-counters.

    I'm fine with or without the ResetCoverageCounters()-change. Only meant to demonstrate that this tool was useful by testing it out in my last ACK.

  33. brunoerg commented at 1:11 PM on February 26, 2025: contributor

    light ACK fa99c3b544b631cfe34d52fb5e71636aedb1b423

    I have not reviewed the code yet, only checked that deterministic-unittest-coverage is working as expected. I tested on MacOS 14.3.

    cmake -B build_31901 -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'

    then I tested it for 3 tests:

    RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_31901 coinselector_tests
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_31901 addrman_tests
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_31901 net_tests

    All of them returned "The coverage was not deterministic between runs.". I noticed that the reason is basically locks and crypto functions as expected.

  34. maflcko commented at 1:16 PM on February 26, 2025: member

    All of them returned "The coverage was not deterministic between runs.". I noticed that the reason is basically locks and crypto functions as expected.

    I'd say that non-determinism shouldn't be expected, though fixing them can be done in a follow-up. If you want to check a deterministic test case you can try util_string_tests or util_tests.

  35. brunoerg commented at 1:21 PM on February 26, 2025: contributor

    If you want to check a deterministic test case you can try util_string_tests or util_tests.

    Both returned "The coverage was not deterministic between runs.". One of the differences:

     /Users/brunogarcia/projects/bitcoin-core-dev/src/crypto/sha512.h:
     1|       |// Copyright (c) 2014-2022 The Bitcoin Core developers
@@ -138933,7 +138933,7 @@
    62|      0|}
    63|       |
    64|       |inline int64_t GetPerformanceCounter() noexcept
-   65|    958|{
+   65|    966|{
    66|       |    // Read the hardware time stamp counter when available.
    67|       |    // See https://en.wikipedia.org/wiki/Time_Stamp_Counter for more information.
    68|       |#if defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))
@@ -138948,9 +138948,9 @@
    77|       |    return (r2 << 32) | r1;
    78|       |#else
    79|       |    // Fall back to using standard library clock (usually microsecond or nanosecond precision)
-   80|    958|    return std::chrono::high_resolution_clock::now().time_since_epoch().count();
-   81|    958|#endif
-   82|    958|}
+   80|    966|    return std::chrono::high_resolution_clock::now().time_since_epoch().count();
+   81|    966|#endif
+   82|    966|}
    83|       |
    84|       |#ifdef HAVE_GETCPUID
    85|       |bool g_rdrand_supported = false;
@@ -139185,22 +139185,22 @@
   314|       |    // Hash loop
   315|      2|    unsigned char buffer[64];
   316|      2|    const auto stop{SteadyClock::now() + dur};
-  317|    780|    do {
-  318|   780k|        for (int i = 0; i < 1000; ++i) {
-                                                ^780k
+  317|    788|    do {
+  318|   788k|        for (int i = 0; i < 1000; ++i) {
+                                                ^788k
   ------------------
-  |  Branch (318:25): [True: 780k, False: 780]
+  |  Branch (318:25): [True: 788k, False: 788]
   ------------------
-  319|   780k|            inner_hasher.Finalize(buffer);
-  320|   780k|            inner_hasher.Reset();
-  321|   780k|            inner_hasher.Write(buffer, sizeof(buffer));
-  322|   780k|        }
+  319|   788k|            inner_hasher.Finalize(buffer);
+  320|   788k|            inner_hasher.Reset();
+  321|   788k|            inner_hasher.Write(buffer, sizeof(buffer));
+  322|   788k|        }
   323|       |        // Benchmark operation and feed it into outer hasher.
-  324|    780|        int64_t perf = GetPerformanceCounter();
-  325|    780|        hasher.Write((const unsigned char*)&perf, sizeof(perf));
-  326|    780|    } while (SteadyClock::now() < stop);
+  324|    788|        int64_t perf = GetPerformanceCounter();
+  325|    788|        hasher.Write((const unsigned char*)&perf, sizeof(perf));
+  326|    788|    } while (SteadyClock::now() < stop);
   ------------------
-  |  Branch (326:14): [True: 778, False: 2]
+  |  Branch (326:14): [True: 786, False: 2]
   ------------------
   327|       |
   328|       |    // Produce output from inner state and feed it to outer hasher.
  36. maflcko commented at 1:31 PM on February 26, 2025: member

    Both returned "The coverage was not deterministic between runs.". One of the differences:

    The last commit is supposed to fix that and it seemingly did for hodlinator and myself. Are you sure you compiled the right commit? Though, even if it persists, my recommendation would be to address it in a follow-up. The goal here is mostly to add the contrib script itself.

  37. brunoerg commented at 1:45 PM on February 26, 2025: contributor

    The last commit is supposed to fix that and it seemingly did for hodlinator and myself. Are you sure you compiled the right commit? Though, even if it persists, my recommendation would be to address it in a follow-up. The goal here is mostly to add the contrib script itself.

    Yes, I just verified it is the right commit, I re-built it again and I'm getting the same. Anyway, I'm fine with the script, we can address it in a follow-up.

  38. dergoegge approved
  39. dergoegge commented at 2:23 PM on February 26, 2025: member

    tACK fa99c3b544b631cfe34d52fb5e71636aedb1b423

  40. in contrib/devtools/README.md:20 in fa99c3b544 
      22 |  ```
  23 | -RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/corpora-dir fuzz_target_name
  24 | +-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DBUILD_FOR_FUZZING=ON -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
  25 | +```
  26 | +
  27 | +Both llvm-profdata and llvm-cov must be installed. Also, the qa-assets
    janb84 commented at 3:54 PM on February 27, 2025:

    Because git is now a hard dependency, I would suggest to change the readme to:

    In addition to git, both llvm-profdata and llvm-cov must be installed. Also, the qa-assets
    maflcko commented at 4:12 PM on February 27, 2025:

    Not sure. If someone has the source code, they likely got it via git. If someone has compiled with clang, they likely have llvm tools. So it would be better to just remove the sentence.

    In any case, I'll leave this nit as-is for now, to not invalidate the three acks.

  41. in contrib/devtools/README.md:43 in fa99c3b544 
      45 | +
  46 | +```
  47 | +-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
  48 | +```
  49 | +
  50 | +Both llvm-profdata and llvm-cov must be installed.
    janb84 commented at 3:56 PM on February 27, 2025:

    Also add it here:

    In addition to git, both llvm-profdata and llvm-cov must be installed.
  42. janb84 commented at 4:07 PM on February 27, 2025: contributor

    Concept ACK fa99c3b

    • Cloned & Build
    • Runned the tooling, as instructed per Readme
    • Reviewed the code

    As a side note: I have run into the same non-deterministic issues with util_string_tests or util_tests as brunoerg although not important for this PR

  43. Prabhat1308 commented at 2:46 PM on March 1, 2025: none

    Concept ACK fa99c3b Fully agree with removing the outdated bash script.

    I have only checked the deterministic-unittest-coverage and not the deterministic-fuzz-coverage on MacOS 15.3.1 with clang 19 . It has been giving me some flaky results . On the first run both util_string_tests or util_tests were giving me result as deterministic coverage.

    <details>

    <summary>First run</summary>

    <img width="1035" alt="Screenshot 2025-03-01 at 7 58 12 PM" src="https://github.com/user-attachments/assets/5d0d6e85-c478-4377-a71c-e12f8942cd04" />

    </details>

    I ran them again and they started giving non-deterministic coverage. No change was made between the runs.

    <details>

    <summary>Second run</summary>

    <img width="1055" alt="Screenshot 2025-03-01 at 7 59 38 PM" src="https://github.com/user-attachments/assets/3e7db7ab-07c7-4dd5-8632-8d94ecf337cd" /> <img width="1046" alt="Screenshot 2025-03-01 at 8 00 04 PM" src="https://github.com/user-attachments/assets/83f63e15-b191-4e30-8ffe-c52d76360a89" />

    </details>

  44. achow101 added the label Has ACKs on Mar 4, 2025
  45. achow101 removed the label Has ACKs on Mar 4, 2025
  46. maflcko commented at 3:26 PM on March 12, 2025: member

    Anything left to do here?

    The goal here is to add the contrib tool, not to fix all non-determinism in all tests. Especially, if they happen only on macOS, I can't really fix it myself anyway, since I don't have macOS. Ideally this is done in a follow-up.

    There are three acks, two of which indicate to have tested the changes.

  47. fanquake merged this on Mar 13, 2025
  48. fanquake closed this on Mar 13, 2025
  49. maflcko deleted the branch on Mar 13, 2025
  50. maflcko commented at 8:12 AM on March 13, 2025: member

    @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:

    • Is your result different with or without the last commit?
    • Does ResetCoverageCounters work?
    • What is the coverage result for src/test/util/setup_common.cpp, especially g_rng_temp_path_init? (You can find it in fuzz_det_cov.show.{run_id}.txt)
  51. janb84 commented at 10:24 AM on March 13, 2025: contributor

    @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:

    • Is your result different with or without the last commit?
    • Does ResetCoverageCounters work?
    • What is the coverage result for src/test/util/setup_common.cpp, especially g_rng_temp_path_init? (You can find it in fuzz_det_cov.show.{run_id}.txt)

    In checking this issue I discovered that due to the change in 31161 the tooling is broken. (the path has changed from src/test/ to /bin )

  52. maflcko commented at 10:27 AM on March 13, 2025: member

    I discovered that due to the change in 31161 the tooling is broken. (the path has changed from src/test/ to /bin )

    Should be trivial to fix up. I am happy to review a tiny pull to fix the silent merge conflict here.

  53. janb84 commented at 11:16 AM on March 13, 2025: contributor

    Fix created in https://github.com/bitcoin/bitcoin/pull/32055

  54. hebasto referenced this in commit 72c150dfe7 on Mar 13, 2025
  55. janb84 commented at 3:35 PM on March 13, 2025: contributor

    @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:

    • Is your result different with or without the last commit?
    • Does ResetCoverageCounters work?
    • What is the coverage result for src/test/util/setup_common.cpp, especially g_rng_temp_path_init? (You can find it in fuzz_det_cov.show.{run_id}.txt)

    In checking this issue I discovered that due to the change in 31161 the tooling is broken. (the path has changed from src/test/ to /bin )

    The trouble is in this code/macro (test/util/coverage.ccp)

    #if defined(__clang__) && defined(__linux__)
extern "C" void __llvm_profile_reset_counters(void) __attribute__((weak));
extern "C" void __gcov_reset(void) __attribute__((weak));

void ResetCoverageCounters()
{
    if (__llvm_profile_reset_counters) {
        __llvm_profile_reset_counters();
    }

    if (__gcov_reset) {
        __gcov_reset();
    }
}
#else
void ResetCoverageCounters() {}
#endif

    linux is undefined, therefor it falls in the else branch. When changing the macro to the following:

    #if defined(__clang__)  && (defined(__linux__) || defined(__APPLE__))

    The test will report as deterministic.

    Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue

  56. brunoerg commented at 4:00 PM on March 13, 2025: contributor

    Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue

    Confirmed. I still have the issue on master but I just tested with the following diff and seems to work:

    diff --git a/src/test/util/coverage.cpp b/src/test/util/coverage.cpp
index bbf068a6fa..ac0fb00e36 100644
--- a/src/test/util/coverage.cpp
+++ b/src/test/util/coverage.cpp
@@ -4,7 +4,7 @@

 #include <test/util/coverage.h>

-#if defined(__clang__) && defined(__linux__)
+#if defined(__clang__)
 extern "C" void __llvm_profile_reset_counters(void) __attribute__((weak));
 extern "C" void __gcov_reset(void) __attribute__((weak));

    ➜  bitcoin-core-dev git:(master) ✗ RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_string_tests
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
     Running `contrib/devtools/deterministic-unittest-coverage/target/debug/deterministic-unittest-coverage /Users/brunogarcia/projects/bitcoin-core-dev/build_31901 util_string_tests`
Running 1 test case...

*** No errors detected
Running 1 test case...

*** No errors detected
The coverage was deterministic across two runs.
➜  bitcoin-core-dev git:(master) ✗ RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_string_tests
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
     Running `contrib/devtools/deterministic-unittest-coverage/target/debug/deterministic-unittest-coverage /Users/brunogarcia/projects/bitcoin-core-dev/build_31901 util_string_tests`
Running 1 test case...

*** No errors detected
Running 1 test case...

*** No errors detected
The coverage was deterministic across two runs.
  57. Prabhat1308 commented at 4:05 PM on March 13, 2025: none

    Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue

    Applied this change and I get deterministic results on all runs

    #if defined(__clang__)  && (defined(__linux__) || defined(__APPLE__))

    <img width="1512" alt="Screenshot 2025-03-13 at 9 31 40 PM" src="https://github.com/user-attachments/assets/fb622440-8634-47ba-b21a-54fc567803df" />

    I tested on the latest master branch with following commands .

    cmake -B build -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \ 
   -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
   -DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \
   -DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping"

cmake --build build -j$(sysctl -n hw.ncpu) 
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_tests

    Also with the same change suggested by @janb84 . I get deterministic results on fuzz-coverage too. Running commands

    RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build $PWD/qa-assets/fuzz_corpora process_message

    without change

    @@ -331406,7 +331406,7 @@
    60|       |     *
    61|       |     * Called on a background thread. Only called for the active chainstate.
    62|       |     */
-   63|    158|    virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {}
+   63|    166|    virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {}
    64|       |    /**
    65|       |     * Notifies listeners any time the block chain tip changes, synchronously.
    66|       |     */
@@ -331458,14 +331458,14 @@
   112|       |     *
   113|       |     * Called on a background thread.
   114|       |     */
-  115|    183|    virtual void MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) {}
+  115|    191|    virtual void MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) {}
   116|       |    /**
   117|       |     * Notifies listeners of a block being connected.
   118|       |     * Provides a vector of transactions evicted from the mempool as a result.
   119|       |     *
   120|       |     * Called on a background thread.
   121|       |     */
-  122|    162|    virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {}
+  122|    169|    virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {}
   123|       |    /**
   124|       |     * Notifies listeners of a block being disconnected
   125|       |     * Provides the block that was disconnected.

The coverage was not deterministic between runs.
The fuzz target input was /Users/prabhatverma/projects/bitcoin/qa-assets/fuzz_corpora/process_message/00023ad97f6f03d0899bfa81d8ff7d095d8b4ee9.
Exiting.

    with the change

    Check if using libFuzzer ...
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
process_message: succeeded against 1 files in 0s.
# exited manually
Contributors
maflckoDrahtBotdergoeggetheStackhodlinatorbrunoergjanb84Prabhat1308


theStack

Labels
Scripts and tools
Linked (view graph)
#31588 contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds#32055 contrib: Fix deterministic-unittest-coverage tool path#32059 test: Update coverage.cpp to drop linux restriction#32074 contrib: Make deterministic-coverage error messages more readable
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-24 09:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me