fuzz: Avoid time-based "non-determinism" in fuzzing harnesses by using mocked GetTime() #20437

Avoid time-based "non-determinism" in fuzzing harnesses by using mocked GetTime().

Prior to this commit the fuzzing harnesses banman, connman, net and rbf had time-based "non-determinism". addrman is fixed in #20425. process_message and process_messages are left to fix: simply using mock time is not enough for them due to interaction with IsInitialBlockDownload().

See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

utACK 892e7ddbd6f85653ee775c34f07b146c6f3b628e

Why pick a time that won't ever be hit in practice? Why pick a time that makes it impossible to go back in time?

@MarcoFalke

I choose SetMockTime(1) because SetMockTime(0) is a no-op, and it seemed to me that SetMockTime(1) is commonly use in src/test/:

$ git grep SetMockTime "src/test/" | sed 's/^.*: *//g' | grep ^SetMockTime | sort | uniq -c | sort -rn
      5 SetMockTime(0);
      3 SetMockTime(2);
      3 SetMockTime(1);
      1 SetMockTime(nStartTime); // Overrides future calls to GetTime()
      1 SetMockTime(nStartTime+24*60);
      1 SetMockTime(nStartTime+21*60);
      1 SetMockTime(GetTime() + 3 * chainparams.GetConsensus().nPowTargetSpacing + 1);
      1 SetMockTime(ConsumeTime(fuzzed_data_provider));
      1 SetMockTime(42);
      1 SetMockTime(111);

I see your point about using a constant that is more realistic (higher) and allows for going back in time. It seems like you had a number in mind: give me your suggested constant and I'll happily use it :)

SetMockTime(1) in existing tests in only used to check the difference between SetMockTime(2) and SetMockTime(1) is 1. I don't think one unit test case picking a random number should be a reason for all fuzz tests to pick the same number.

Ideally the fuzz engine picks the number, so that additional coverage can be achieved (if there is any). If there is no additional coverage to be achieved by mocktime, at worst all seeds will increase in size by a few bytes.

@MarcoFalke Oh, I should have clarified that the current patch was written to avoid time-based "non-determinism" without invalidating any seed corpora. Hence the constant rather than a fuzzer provided value.

I agree that if we are okay with invalidating seeds then SetMockTime(ConsumeTime(fuzzed_data_provider)); is obviously the superior choice. (FWIW I'm not opposed to invalidating seeds for these harnesses.)

I think we still need a constant for the -mocktime parameter passed to BasicTestingSetup though (for InitializeFuzzingContext). Did you have any number in mind that is better than 1? :)

I'll fix SetMockTime(ConsumeTime(fuzzed_data_provider)); and -mocktime=N when we've chosen the most appropriate default :)

I don't think we should be using -mocktime=N at all. This is just hiding the issue without actually improving anything.

@MarcoFalke Now using SetMockTime(ConsumeTime(fuzzed_data_provider)); and no -mocktime=N. Please re-review :)

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #20649 (refactor: Remove nMyStartingHeight from CNode/Connman by MarcoFalke)
• #20228 (addrman: Make addrman a top-level component by jnewbery)

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.

Anything left to do here? :)

review ACK 8c09c0c1d18885ef94f79b3f2d073f43269bc95d

Though, process_message* is still using non-mocked time. Also, is there a way to test? Locally the targets are non-deterministic before and after this change.

Though, process_message* is still using non-mocked time.

Thanks for reviewing! :)

process_message and process_messages are left to fix: simply using mock time is not enough for them due to interaction with IsInitialBlockDownload() (see OP description).

Also, is there a way to test? Locally the targets are non-deterministic before and after this change.

One way to test that time-based non-determinism is removed is probably to introduce if (g_fuzzing_started) { assert(false); } in the relevant part of GetTime, and set g_fuzzing_started = true; in test_one_input. Then compare the results before and after this patch :)

One way to test ...

That is what I did. I was hoping there is better tooling out there to detect non-determinism in fuzz tests. I guess we'll have to roll our own...

Btw, if you quote ACKs, it will look like you did a review in the merge commit

One way to test ...

That is what I did. I was hoping there is better tooling out there to detect non-determinism in fuzz tests. I guess we'll have to roll our own...

I've thought about modifying contrib/devtools/test_deterministic_coverage.sh to add support for running the fuzz tests instead, but I haven't had time for that yet :)