process_message(s) are the least stable fuzz targets, according to OSS-Fuzz.
Tracking issue: #29018.
Needs coverage compilation, as explained in ./contrib/devtools/README.md. And then, using 32 threads:
0cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ process_messages 32
Each commit can be reverted to see more non-determinism re-appear.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32822.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, dergoegge |
| Stale ACK | brunoerg |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
for testing, the single-check doesn’t hit here and can be quite expensive, so it could be disabled via:
0diff --git a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
1index 3eeb121db0..f729740a3a 100644
2--- a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
3+++ b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
4@@ -219,13 +219,6 @@ The coverage was not deterministic between runs.
5 if !entry.is_file() {
6 Err(format!("{} should be a file", entry.display()))?;
7 }
8- let cov_txt_base = run_single('a', &entry, thread_id)?;
9- let cov_txt_repeat = run_single('b', &entry, thread_id)?;
10- check_diff(
11- &cov_txt_base,
12- &cov_txt_repeat,
13- &format!("The fuzz target input was {}.", entry.display()),
14- )?;
15 Ok(())
16 };
17 thread::scope(|s| -> AppResult {
Apologies for the force push. However, it seems that the fuzz target actually produces valid block headers, so the chainman state is dirty and needs to be reset as well for those fuzz inputs. I’ve pushed a new commit doing this (after a rebase).
There is still non-determinism in operator==<transaction_identifier>, however I fail to see the source. I may take a look in the future.
tACK fac673d434b4d32d8c44dcc50a3655ba4a1816de
Running the stability script, there’s significantly less coverage differences with this PR. My only question is why does ResetChainman set mock time in process_messages but not in process_message? Didn’t see an obvious reason, but I could be missing something.
mock time
My bad and nice catch. Added it there as well. should be trivial to re-review with range-diff
utACK fa501ea5ed874ffcba2b606806460e61a1204bd2
As a follow up (if someone is interested), we could add the block hashes for the blocks with mature coinbases (as well as the mature coinbase prevouts) to the dictionary in qa-assets.
The PeerManager has several members, such as the FastRandomContext,
which need to be reset before every run to avoid leaking state from one
run into the next.
Also, style fixups in p2p_handshake.cpp, where this code is copied from.
This is required in the process_message(s) fuzz targets to avoid leaking
the next write time from one run to the next. Also, disable it
completely because it is not needed and due to leveldb-internal
non-determinism.
ReACK fa1a14a13a15ecfb7587a94ee86b4ace7c819519
range-diff 33332cf86a..fa501ea5ed fa11eea405..fa1a14a13a lgtm