After commit fae63bf13033adec80c7e6d73144a21ea3cfbc6d this must be placed even before test_setup. This is nice, because it makes the usage consistently appear in the first line.
The change is moving a SeedRandomForTest(SeedRand::ZEROS) to happen earlier. This is fine, because it will either have no effect, or make the code more deterministic, because after commit fae63bf, no other re-seeding other than ZEROS can happen in fuzz tests.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31521.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, brunoerg, hodlinator |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Also, if you’re already changing the message, small nit for this line:
0"Without a solution, fuzz stability and determinism can lead \n"
1"to non-reproducible bugs or inefficient fuzzing.\n\n"
Do you mean “stability and determinism issues” or something similar (instability and non-determinism)? The sentence is a bit unclear to me, as fuzz stability and determinism is what we’re aiming for.
After commit fae63bf13033adec80c7e6d73144a21ea3cfbc6d this must be
placed even before test_setup.
After commit fae63bf this must be placed even before test_setup.
I think I don’t understand what happens if SeedRandomStateForTest(SeedRand::ZEROS); is not called before the test setup. Is this causing the fuzz test to just crash on startup? Would be helpful to say what is happening in the PR description.
After commit fae63bf this must be placed even before test_setup.
I think I don’t understand what happens if
SeedRandomStateForTest(SeedRand::ZEROS);is not called before the test setup. Is this causing the fuzz test to just crash on startup? Would be helpful to say what is happening in the PR description.
Good question! There is no crash and I doubt that this change makes any difference in practise, so I used that as an excuse to not write a full explanation and instead opted to go with the “lazy explanation” (see last point below), to minimize the review burden.
Explanation of the background:
SeedRandomForTest(SeedRand::ZEROS) is called, which initializes the global PRNG with a constant state.utxo_total_supply), for every iteration, test_setup is constructed. The test setup constructor calls SeedRandomForTest(SeedRand::FIXED_SEED) before commit fae63bf and nothing after.SeedRandomForTest(SeedRand::FIXED_SEED) is actually fine, because fuzz tests are not supposed to receive an env var from outside to affect the fixed seed. Thus, the global PRNG would have been re-initialized with some constant state.Explanation of the fix:
SeedRandomForTest(SeedRand::ZEROS) is now called before test_setup, in every iteration of the fuzz target. This means in practise, that a constant PRNG initial state is picked again, just like before commit fae63bf. Also any salted hashers that are in test_setup should be deterministic.Alternative explanation for the fix (lazy version):
SeedRandomForTest(SeedRand::ZEROS) to happen earlier. This is fine, because it will either have no effect, or make the code more deterministic, because after commit fae63bf, no other re-seeding other than ZEROS can happen in fuzz tests.
I’ve added the lazy explanation to the pull request description as well.
If someone really wants to test the lengthy explanation, it is possible. I did that with FUZZ=utxo_total_supply ./bld-cmake/src/test/fuzz/fuzz -runs=2 and a small diff to peek into the global PRNG state. The expected result should be that a constant string is printed twice.
0diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
1index 3f8c6f41ba..771cf0e436 100644
2--- a/src/test/util/setup_common.cpp
3+++ b/src/test/util/setup_common.cpp
4@@ -148,7 +148,8 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
5 // tests, such as the fuzz tests to run in several processes at the
6 // same time, add a random element to the path. Keep it small enough to
7 // avoid a MAX_PATH violation on Windows.
8- const auto rand{HexStr(g_rng_temp_path.randbytes(10))};
9+ const auto rand{HexStr(m_rng.randbytes(10))};
10+ std::cout << rand << std::endl;
11 m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / rand;
12 TryCreateDirectories(m_path_root);
13 } else {
I was worried that g_rng_temp_path had somehow stopped working again, but I see you needed to replace it with m_rng in your diff to make paths deterministic.
It is a dilemma regarding the hash salts, one would like to exercise many values for them, but still get determinism. What would you think about having the fuzz input affect the random seed?
It is a dilemma regarding the hash salts, one would like to exercise many values for them, but still get determinism. What would you think about having the fuzz input affect the random seed?
Certainly, this is possible and can be done on a case-by-case basis for fuzz targets where it would be useful.
23@@ -24,12 +24,12 @@ struct CheckGlobalsImpl {
24 "The current fuzz target used the global random state.\n\n"
25
26 "This is acceptable, but requires the fuzz target to call \n"
27- "SeedRandomStateForTest(SeedRand::ZEROS) at the beginning \n"
28- "of processing the fuzz input.\n\n"
29+ "SeedRandomStateForTest(SeedRand::ZEROS) in the first line \n"
30+ "of the FUZZ_TARGET function.\n\n"
Was thinking there may creep up something else that also needs initializing really early and either is unaffected by randomness or somehow needs to affect randomness seeding (yes, contrived).
Then there is the idea to have a mixin seeding the randomness internally, which would change this part regardless.
ACK fadd568931a2d21e0f80e1efaf2281f5164fa20e
Prior to commit fae63bf13033adec80c7e6d73144a21ea3cfbc6d (#31486):
utxo_total_supply fuzz target was initializing ChainTestingSetup which would call SeedRandomStateForTest(SeedRand::FIXED_SEED).SeedRandomStateForTest(SeedRand::FIXED_SEED) would seed randomness using a value from a static variable initialized once during process startup.After fae63bf13033adec80c7e6d73144a21ea3cfbc6d:
if constexpr (!G_FUZZING)-guard inside of the ChainTestingSetup-initialization, we were no longer resetting the PRNG for every fuzz input (aside from the explicit SeedRandomStateForTest(SeedRand::ZEROS)-calls in the fuzz targets).utxo_total_supply itself explicitly called SeedRandomStateForTest(SeedRand::ZEROS).SeedRandomStateForTest(SeedRand::ZEROS) before ChainTestingSetup-initialization.Added this to the utxo_total_supply target:
0std::ofstream{"/tmp/fuzz_data", std::ios_base::app} << HexStr(FastRandomContext().rand256()) << std::endl;
When placed before SeedRandomStateForTest(SeedRand::ZEROS), /tmp/fuzz_data would contain this for 4 runs:
0b0fd14ff96a0bda154c329082c9c6533bb4c9473bf5dde138f82c9ac5553d958
1daaa4b425bfa3725e2b0f59f0fabf4ee3387123c569425cbcf8808296acc6f4e
2daaa4b425bfa3725e2b0f59f0fabf4ee3387123c569425cbcf8808296acc6f4e
3daaa4b425bfa3725e2b0f59f0fabf4ee3387123c569425cbcf8808296acc6f4e
- Meaning PRNG state would leak from the previous run, but it would be the same leaked state on the next run. This is not guaranteed if the fuzz target is complex enough though, as different fuzz inputs could lead to different amount of randomness being used.
When placed after SeedRandomStateForTest(SeedRand::ZEROS), one gets the same on each run:
0b0fd14ff96a0bda154c329082c9c6533bb4c9473bf5dde138f82c9ac5553d958
1b0fd14ff96a0bda154c329082c9c6533bb4c9473bf5dde138f82c9ac5553d958
2b0fd14ff96a0bda154c329082c9c6533bb4c9473bf5dde138f82c9ac5553d958
3b0fd14ff96a0bda154c329082c9c6533bb4c9473bf5dde138f82c9ac5553d958
Conceptually I think it would be an improvement to seed randomness using fuzz input instead of ZEROS, but that should be done in another PR. It would probably lead to marginally more test coverage while still retaining determinism.
The initialization-step would still use ZEROS I guess since it has no given fuzz input-data AFAIK.
Thanks for the explanations. The reason I was confused earlier is that I mistakenly thought CheckGlobalsImpl would crash if a fuzz test ever used the global RNG without seeding it with zeroes first.
But (unfortunately?) the checker doesn’t really check that. It only checks that if a fuzz test uses the RNG, then it must seed it with zeros at some point during the test. The checker doesn’t check that seeding happens before using the RNG.
So this PR does fix some nondeterminism, but the nondeterminism was undetected by the checker, so it was not causing any crash.
But (unfortunately?) the checker doesn’t really check that. It only checks that if a fuzz test uses the RNG, then it must seed it with zeros at some point during the test. The checker doesn’t check that seeding happens before using the RNG.
Sure, makes sense to add this assert as well.
Edit: done in https://github.com/bitcoin/bitcoin/pull/31548