This PR introduces g_mock_get_rand to allow tests to mock GetRand and makes g_mock_get_rand consume from the fuzzed data in the process_message(s) targets.
Mocking GetRand reduces the non-determinism in these targets and results in more stable fuzzing.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | brunoerg |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
62 | @@ -62,6 +63,11 @@ FUZZ_TARGET(process_message, .init = initialize_process_message) 63 | { 64 | FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); 65 | 66 | + g_mock_get_rand = [&](uint64_t max) -> uint64_t { 67 | + if (fuzzed_data_provider.remaining_bytes() == 0) return 1;
Not having this exception leads to an endless loop here: https://github.com/bitcoin/bitcoin/blob/c9f288244b8d183e09a917025922b99e3368ef78/src/net_processing.cpp#L5225-L5227
Shouldn't this return max - 1, otherwise it may return an invalid value when GetRand(1) is called?
563 | @@ -564,9 +564,11 @@ void RandAddPeriodic() noexcept { ProcRand(nullptr, 0, RNGLevel::PERIODIC); }
564 | void RandAddEvent(const uint32_t event_info) noexcept { GetRNGState().AddEvent(event_info); }
565 |
566 | bool g_mock_deterministic_tests{false};
Setting g_mock_deterministic_tests is not a great alternative as it makes GetRand always return the same value.
nit: It is bad enough that there is a single test-only mock in here. Now, having two, it would make sense to remove one of them again. (Maybe in a follow-up)
Concept ACK
Can you add examples where this matters?
I presume one example would be ping-pong handling?
Longer term it would be nice to nuke the global GetRand and use a mockable instance everywhere.
Can you add examples where this matters?
https://github.com/bitcoin/bitcoin/blob/c9f288244b8d183e09a917025922b99e3368ef78/src/net_processing.cpp#L5369 https://github.com/bitcoin/bitcoin/blob/c9f288244b8d183e09a917025922b99e3368ef78/src/net_processing.cpp#L1106 https://github.com/bitcoin/bitcoin/blob/c9f288244b8d183e09a917025922b99e3368ef78/src/net_processing.cpp#L5270 https://github.com/bitcoin/bitcoin/blob/c9f288244b8d183e09a917025922b99e3368ef78/src/net_processing.cpp#L5264
The examples you linked to should still be non-deterministic, because the feefilter rounder is random, as well as the address relay, no?
Also, no transactions are ever relayed, so while this makes the coverage data more deterministic, the benefits should be minimal? Considering that this may affect all fuzz inputs, I'd prefer to go the full and complete route instead.
Would be nice to start using one FastRandomContext in PeerManager (locked by the message thread mutex), or more, if needed. The FRC would be deterministic in tests.
No need to fix everything, but it shouldn't be too hard to fix one instance (e.g. ping-pong handling) with the added benefit of not breaking the fuzz input format. Happy to review that.
In the future, the deterministic seed can be read from the fuzz input (in a breaking fuzz change), and more places can be converted.
Would be nice to start using one FastRandomContext in PeerManager
This doesn't solve our problem in the fuzz tests since the peerman is global, so this will only help in fuzz tests that create a new peerman each iteration.