contrib: Add deterministic-unittest-coverage

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).

gitignore: target/

Needed for the recently added
contrib/devtools/deterministic-fuzz-coverage/target/

fa1e0a7228

DrahtBot commented at 8:30 pm on February 18, 2025: 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/31901.

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.

Conflicts

No conflicts as of last run.

DrahtBot renamed this:
~~contrib: Add deterministic-unittest-coverage~~
contrib: Add deterministic-unittest-coverage
on Feb 18, 2025

DrahtBot added the label Scripts and tools on Feb 18, 2025

dergoegge commented at 10:10 am on February 19, 2025: member

Concept ACK

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:

0# llvm-profdata-19 --version 
1Debian LLVM version 19.1.4
2  Optimized build.
3# llvm-profdata-16 --version
4llvm-profdata-16: Unknown command!
5USAGE: 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.

0# llvm-profdata-17 --version 
1llvm-profdata-17
2Ubuntu LLVM version 17.0.6
3  Optimized build.

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

thx, fixed

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:

0    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

theStack commented at 1:36 pm on February 21, 2025: contributor

Concept ACK

maflcko force-pushed on Feb 21, 2025

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.

0            .expect("Failed to execute git command")

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

thx, done

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).

0    eprintln!(
1        "Error: {err}\n\
2         \n\
3         Usage: program ./build_dir ./qa-assets/fuzz_corpora fuzz_target_name\n\
4         \n\
5         Refer to the devtools/README.md for more details."
6    );

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.

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:

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

With suggestion:

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

 0        if !Command::new(test_exe)
 1            .env("LLVM_PROFILE_FILE", &profraw_file)
 2            .env("BOOST_TEST_RUN_FILTERS", filter)
 3            .env("RANDOM_CTX_SEED", "21")
 4            .status()
 5            .expect("test failed")
 6            .success()
 7        {
 8            eprintln!("{} failed", test_exe.display());
 9            std::process::exit(1)
10        }

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:

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

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

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

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:

 0₿ RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build boost_unittest_filter
 1    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
 2     Running `contrib/devtools/deterministic-unittest-coverage/target/debug/deterministic-unittest-coverage /home/hodlinator/b2/build boost_unittest_filter`
 3Test setup error: no test cases matching filter or all test cases were disabled
 4thread 'main' panicked at src/main.rs:73:9:
 5assertion failed: Command::new(test_exe).env("LLVM_PROFILE_FILE",
 6                        &profraw_file).env("BOOST_TEST_RUN_FILTERS",
 7                    filter).env("RANDOM_CTX_SEED",
 8                "21").status().expect("test failed").success()
 9stack backtrace:
10   0: rust_begin_unwind
11   1: core::panicking::panic_fmt
12   2: core::panicking::panic
13   3: deterministic_unittest_coverage::deterministic_coverage::{{closure}}
14             at ./contrib/devtools/deterministic-unittest-coverage/src/main.rs:73:9
15   4: deterministic_unittest_coverage::deterministic_coverage
16             at ./contrib/devtools/deterministic-unittest-coverage/src/main.rs:120:14
17   5: deterministic_unittest_coverage::main
18             at ./contrib/devtools/deterministic-unittest-coverage/src/main.rs:65:5
19   6: core::ops::function::FnOnce::call_once
20             at /build/rustc-1.84.1-src/library/core/src/ops/function.rs:250:5
21note: 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

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:

0To execute the tool, compilation has to be done with the build options:
1```
2-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
3```
4Both 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 \.

0To execute the tool, compilation has to be done with the build options
1`-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' \
2-DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'`. Both
3llvm-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

hodlinator commented at 3:09 pm on February 22, 2025: contributor

Concept ACK fafce12a4871ed66a868b460b779e794a89218eb

doc: Remove unused -fPIC

This is harmless, but no longer needed after commit
06b9236f4327525875768af5fc554c651c5ec3cf.

faf905b9b6

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

maflcko force-pushed on Feb 24, 2025

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?

0cargo 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

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.

0    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

hodlinator approved

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

 0diff --git a/src/random.cpp b/src/random.cpp
 1index 5b605e988d..087894ffcd 100644
 2--- a/src/random.cpp
 3+++ b/src/random.cpp
 4@@ -441,6 +441,8 @@ public:
 5 
 6     ~RNGState() = default;
 7 
 8+    bool IsDeterministic() noexcept EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { LOCK(m_mutex); return m_deterministic_prng.has_value(); }
 9+
10     void AddEvent(uint32_t event_info) noexcept EXCLUSIVE_LOCKS_REQUIRED(!m_events_mutex)
11     {
12         LOCK(m_events_mutex);
13@@ -642,6 +644,7 @@ void ProcRand(unsigned char* out, int num, RNGLevel level, bool always_use_real_
14     assert(num <= 32);
15 
16     CSHA512 hasher;
17+    if (!rng.IsDeterministic() || always_use_real_rng) {
18     switch (level) {
19     case RNGLevel::FAST:
20         SeedFast(hasher);
21@@ -653,6 +656,7 @@ void ProcRand(unsigned char* out, int num, RNGLevel level, bool always_use_real_
22         SeedPeriodic(hasher, rng);
23         break;
24     }
25+    }
26 
27     // Combine with and update state
28     if (!rng.MixExtract(out, num, std::move(hasher), false, always_use_real_rng)) {
29@@ -771,7 +775,7 @@ FastRandomContext::FastRandomContext(bool fDeterministic) noexcept : requires_se
30 void RandomInit()
31 {
32     // Invoke RNG code to trigger initialization (if not already performed)
33-    ProcRand(nullptr, 0, RNGLevel::FAST, /*always_use_real_rng=*/true);
34+    ProcRand(nullptr, 0, RNGLevel::FAST, /*always_use_real_rng=*/false);
35 
36     ReportHardwareRand();
37 }
38diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
39index bf26997c07..a6349f3280 100644
40--- a/src/test/util/setup_common.cpp
41+++ b/src/test/util/setup_common.cpp
42@@ -78,7 +78,6 @@ static FastRandomContext g_rng_temp_path;
43 static const bool g_rng_temp_path_init{[] {
44     // Must be initialized before any SeedRandomForTest
45     Assert(!g_used_g_prng);
46-    (void)g_rng_temp_path.rand64();
47     g_used_g_prng = false;
48     return true;
49 }()};

DrahtBot requested review from theStack on Feb 24, 2025

DrahtBot requested review from dergoegge on Feb 24, 2025

maflcko force-pushed on Feb 25, 2025

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.

contrib: Add deterministic-unittest-coverage

This replaces the bash script with a tool based on clang/llvm tools.

fa579d663d

test: Exclude SeedStartup from coverage counts fa99c3b544

maflcko force-pushed on Feb 25, 2025

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.

hodlinator approved

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.

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.

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

then I tested it for 3 tests:

0RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_31901 coinselector_tests
1RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_31901 addrman_tests
2RUST_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.

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.

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:

 0 /Users/brunogarcia/projects/bitcoin-core-dev/src/crypto/sha512.h:
 1     1|       |// Copyright (c) 2014-2022 The Bitcoin Core developers
 2@@ -138933,7 +138933,7 @@
 3    62|      0|}
 4    63|       |
 5    64|       |inline int64_t GetPerformanceCounter() noexcept
 6-   65|    958|{
 7+   65|    966|{
 8    66|       |    // Read the hardware time stamp counter when available.
 9    67|       |    // See https://en.wikipedia.org/wiki/Time_Stamp_Counter for more information.
10    68|       |#if defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))
11@@ -138948,9 +138948,9 @@
12    77|       |    return (r2 << 32) | r1;
13    78|       |#else
14    79|       |    // Fall back to using standard library clock (usually microsecond or nanosecond precision)
15-   80|    958|    return std::chrono::high_resolution_clock::now().time_since_epoch().count();
16-   81|    958|#endif
17-   82|    958|}
18+   80|    966|    return std::chrono::high_resolution_clock::now().time_since_epoch().count();
19+   81|    966|#endif
20+   82|    966|}
21    83|       |
22    84|       |#ifdef HAVE_GETCPUID
23    85|       |bool g_rdrand_supported = false;
24@@ -139185,22 +139185,22 @@
25   314|       |    // Hash loop
26   315|      2|    unsigned char buffer[64];
27   316|      2|    const auto stop{SteadyClock::now() + dur};
28-  317|    780|    do {
29-  318|   780k|        for (int i = 0; i < 1000; ++i) {
30-                                                ^780k
31+  317|    788|    do {
32+  318|   788k|        for (int i = 0; i < 1000; ++i) {
33+                                                ^788k
34   ------------------
35-  |  Branch (318:25): [True: 780k, False: 780]
36+  |  Branch (318:25): [True: 788k, False: 788]
37   ------------------
38-  319|   780k|            inner_hasher.Finalize(buffer);
39-  320|   780k|            inner_hasher.Reset();
40-  321|   780k|            inner_hasher.Write(buffer, sizeof(buffer));
41-  322|   780k|        }
42+  319|   788k|            inner_hasher.Finalize(buffer);
43+  320|   788k|            inner_hasher.Reset();
44+  321|   788k|            inner_hasher.Write(buffer, sizeof(buffer));
45+  322|   788k|        }
46   323|       |        // Benchmark operation and feed it into outer hasher.
47-  324|    780|        int64_t perf = GetPerformanceCounter();
48-  325|    780|        hasher.Write((const unsigned char*)&perf, sizeof(perf));
49-  326|    780|    } while (SteadyClock::now() < stop);
50+  324|    788|        int64_t perf = GetPerformanceCounter();
51+  325|    788|        hasher.Write((const unsigned char*)&perf, sizeof(perf));
52+  326|    788|    } while (SteadyClock::now() < stop);
53   ------------------
54-  |  Branch (326:14): [True: 778, False: 2]
55+  |  Branch (326:14): [True: 786, False: 2]
56   ------------------
57   327|       |
58   328|       |    // Produce output from inner state and feed it to outer hasher.

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.

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.

dergoegge approved

dergoegge commented at 2:23 pm on February 26, 2025: member

tACK fa99c3b544b631cfe34d52fb5e71636aedb1b423

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:

0In 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.

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:

0In addition to git, both llvm-profdata and llvm-cov must be installed.

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

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.

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

achow101 added the label Has ACKs on Mar 4, 2025

achow101 removed the label Has ACKs on Mar 4, 2025

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.

fanquake merged this on Mar 13, 2025

fanquake closed this on Mar 13, 2025

maflcko deleted the branch on Mar 13, 2025

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)

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 )

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.

janb84 commented at 11:16 am on March 13, 2025: contributor

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

hebasto referenced this in commit 72c150dfe7 on Mar 13, 2025

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)

 0#if defined(__clang__) && defined(__linux__)
 1extern "C" void __llvm_profile_reset_counters(void) __attribute__((weak));
 2extern "C" void __gcov_reset(void) __attribute__((weak));
 3
 4void ResetCoverageCounters()
 5{
 6    if (__llvm_profile_reset_counters) {
 7        __llvm_profile_reset_counters();
 8    }
 9
10    if (__gcov_reset) {
11        __gcov_reset();
12    }
13}
14#else
15void ResetCoverageCounters() {}
16#endif

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

0#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

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:

 0diff --git a/src/test/util/coverage.cpp b/src/test/util/coverage.cpp
 1index bbf068a6fa..ac0fb00e36 100644
 2--- a/src/test/util/coverage.cpp
 3+++ b/src/test/util/coverage.cpp
 4@@ -4,7 +4,7 @@
 5
 6 #include <test/util/coverage.h>
 7
 8-#if defined(__clang__) && defined(__linux__)
 9+#if defined(__clang__)
10 extern "C" void __llvm_profile_reset_counters(void) __attribute__((weak));
11 extern "C" void __gcov_reset(void) __attribute__((weak));

 0➜  bitcoin-core-dev git:(master) ✗ RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_string_tests
 1    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
 2     Running `contrib/devtools/deterministic-unittest-coverage/target/debug/deterministic-unittest-coverage /Users/brunogarcia/projects/bitcoin-core-dev/build_31901 util_string_tests`
 3Running 1 test case...
 4
 5*** No errors detected
 6Running 1 test case...
 7
 8*** No errors detected
 9The coverage was deterministic across two runs.
10➜  bitcoin-core-dev git:(master) ✗ RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_string_tests
11    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
12     Running `contrib/devtools/deterministic-unittest-coverage/target/debug/deterministic-unittest-coverage /Users/brunogarcia/projects/bitcoin-core-dev/build_31901 util_string_tests`
13Running 1 test case...
14
15*** No errors detected
16Running 1 test case...
17
18*** No errors detected
19The coverage was deterministic across two runs.

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

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

I tested on the latest master branch with following commands .

0cmake -B build -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \ 
1   -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
2   -DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \
3   -DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping"
4
5cmake --build build -j$(sysctl -n hw.ncpu) 
6RUST_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

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

without change

 0@@ -331406,7 +331406,7 @@
 1    60|       |     *
 2    61|       |     * Called on a background thread. Only called for the active chainstate.
 3    62|       |     */
 4-   63|    158|    virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {}
 5+   63|    166|    virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {}
 6    64|       |    /**
 7    65|       |     * Notifies listeners any time the block chain tip changes, synchronously.
 8    66|       |     */
 9@@ -331458,14 +331458,14 @@
10   112|       |     *
11   113|       |     * Called on a background thread.
12   114|       |     */
13-  115|    183|    virtual void MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) {}
14+  115|    191|    virtual void MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) {}
15   116|       |    /**
16   117|       |     * Notifies listeners of a block being connected.
17   118|       |     * Provides a vector of transactions evicted from the mempool as a result.
18   119|       |     *
19   120|       |     * Called on a background thread.
20   121|       |     */
21-  122|    162|    virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {}
22+  122|    169|    virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {}
23   123|       |    /**
24   124|       |     * Notifies listeners of a block being disconnected
25   125|       |     * Provides the block that was disconnected.
26
27The coverage was not deterministic between runs.
28The fuzz target input was /Users/prabhatverma/projects/bitcoin/qa-assets/fuzz_corpora/process_message/00023ad97f6f03d0899bfa81d8ff7d095d8b4ee9.
29Exiting.

with the change

 0Check if using libFuzzer ...
 1process_message: succeeded against 1 files in 0s.
 2process_message: succeeded against 1 files in 0s.
 3process_message: succeeded against 1 files in 0s.
 4process_message: succeeded against 1 files in 0s.
 5process_message: succeeded against 1 files in 0s.
 6process_message: succeeded against 1 files in 0s.
 7process_message: succeeded against 1 files in 0s.
 8process_message: succeeded against 1 files in 0s.
 9process_message: succeeded against 1 files in 0s.
10process_message: succeeded against 1 files in 0s.
11process_message: succeeded against 1 files in 0s.
12process_message: succeeded against 1 files in 0s.
13process_message: succeeded against 1 files in 0s.
14process_message: succeeded against 1 files in 0s.
15process_message: succeeded against 1 files in 0s.
16process_message: succeeded against 1 files in 0s.
17process_message: succeeded against 1 files in 0s.
18process_message: succeeded against 1 files in 0s.
19process_message: succeeded against 1 files in 0s.
20process_message: succeeded against 1 files in 0s.
21process_message: succeeded against 1 files in 0s.
22process_message: succeeded against 1 files in 0s.
23process_message: succeeded against 1 files in 0s.
24process_message: succeeded against 1 files in 0s.
25process_message: succeeded against 1 files in 0s.
26process_message: succeeded against 1 files in 0s.
27process_message: succeeded against 1 files in 0s.
28process_message: succeeded against 1 files in 0s.
29process_message: succeeded against 1 files in 0s.
30process_message: succeeded against 1 files in 0s.
31process_message: succeeded against 1 files in 0s.
32process_message: succeeded against 1 files in 0s.
33process_message: succeeded against 1 files in 0s.
34process_message: succeeded against 1 files in 0s.
35process_message: succeeded against 1 files in 0s.
36process_message: succeeded against 1 files in 0s.
37process_message: succeeded against 1 files in 0s.
38process_message: succeeded against 1 files in 0s.
39# exited manually

contrib: Add deterministic-unittest-coverage #31901

Code Coverage & Benchmarks

Reviews

Conflicts