The private broadcast fuzz test does not use any randomness.
This was added in #35129 based on a draft created in #29415. That draft was created when the state of #29415 was still using SaltedTxidHashers, but this usage was removed before it was merged.
Setting the random state while not using it is causing issues with nightly https://github.com/maflcko/bitcoin-core-nightly/issues/25.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | kevkevinpal, vasild, maflcko, achow101 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK 1d19cca
Normally this wouldn't fail but in maflcko's nightly build there is this check that causes the test to fail
g_ever_used_g_prng |= g_used_g_prng; // after each input
if (tested && !g_ever_used_g_prng && g_seeded_g_prng_zero) {
Assert(false); // remove unused SeedRandomStateForTest(SeedRand::ZEROS)?
}
Nontheless this change makes sense because we do not need SeedRandomStateForTest(SeedRand::ZEROS);
ACK 1d19cca0761ab35eb46867df2aacdc9715fa5ae0
Is it possible to have this check run on PR CIs so that it is caught before the code is merged into master?
Is this harmful in any way more than being a useless code clutter?
Is there the reverse check either in master or elsewhere - to detect if a test does not set the random state and then uses random?
Is it possible to have this check run on PR CIs so that it is caught before the code is merged into master?
cc @maflcko
Is this harmful in any way more than being a useless code clutter?
I don't think it's harmful, but not sure.
Is there the reverse check either in master or elsewhere - to detect if a test does not set the random state and then uses random?
Yes, if this line is missing and the harness or code under test accesses the global RNG the CI fails loudly for all fuzz jobs.
Is it possible to have this check run on PR CIs so that it is caught before the code is merged into master?
cc @maflcko
No. Fuzz tests are similar to unit tests, because both describe a test case. However, unit tests have their input values hard-coded or hard-derived. Fuzz tests have their input values derived by a fuzz engine and mutable sets of inputs are stored in a folder: qa-assets.
Thus, to know that the fuzz test has "full" coverage and does not need randomness, the fuzz inputs have to be submitted before CI runs. However, usually the fuzz engine is only started after CI runs on a pull, usually even after a pull is merged.
Is this harmful in any way more than being a useless code clutter?
I don't think it's harmful, but not sure.
Right, it is just a stupid "linter". I think it makes sense to limit global setup, but no strong opinion.
lgtm ACK 1d19cca0761ab35eb46867df2aacdc9715fa5ae0
ACK 1d19cca0761ab35eb46867df2aacdc9715fa5ae0