Iterating over fuzz inputs will usually be done in the same process. As the mocktime is global, it can theoretically leak from one fuzz input run into the next run, making it less deterministic.
Fix this issue, by adding and using a context manager to handle the mocktime and reset it before the end.
This refactor should not change any behavior.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
BlockTemplateCache by ismaelsadeeq)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.
SetMockTime to NodeClockContext) in CheckGlobalsImpl for when the target accesses system time?
Approach ACK 666643be8fcdbf587f8fd18edea064ee3c547936
grep -rn 'SetMockTime' src/test/fuzz/ src/wallet/test/fuzz/ shows several remaining calls. Of those, these two follow the same ConsumeTime pattern and could be included:
src/test/fuzz/block_index_tree.cpp:44src/wallet/test/fuzz/fees.cpp:65Built with libfuzzer, verified both targets pass with this diff applied:
0bitcoin pr-34479 !❄ ❯ git diff -U1
1diff --git a/src/test/fuzz/block_index_tree.cpp b/src/test/fuzz/block_index_tree.cpp
2index a4bbe95840..bff8eafa92 100644
3--- a/src/test/fuzz/block_index_tree.cpp
4+++ b/src/test/fuzz/block_index_tree.cpp
5@@ -14,2 +14,3 @@
6 #include <test/util/setup_common.h>
7+#include <test/util/time.h>
8 #include <test/util/validation.h>
9@@ -43,3 +44,3 @@ FUZZ_TARGET(block_index_tree, .init = initialize_block_index_tree)
10 FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
11- SetMockTime(ConsumeTime(fuzzed_data_provider));
12+ NodeClockContext clock_ctx{ConsumeTime(fuzzed_data_provider)};
13 auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
14diff --git a/src/wallet/test/fuzz/fees.cpp b/src/wallet/test/fuzz/fees.cpp
15index 6d9de0cc0f..03a5e2abc9 100644
16--- a/src/wallet/test/fuzz/fees.cpp
17+++ b/src/wallet/test/fuzz/fees.cpp
18@@ -8,2 +8,3 @@
19 #include <test/util/setup_common.h>
20+#include <test/util/time.h>
21 #include <test/util/txmempool.h>
22@@ -64,3 +65,3 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup)
23 FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
24- SetMockTime(ConsumeTime(fuzzed_data_provider));
25+ NodeClockContext clock_ctx{ConsumeTime(fuzzed_data_provider)};
26 auto& node{g_setup->m_node};
This makes it easier to use mock-time in tests. Also, it resets the
global mocktime, so that no state is leaked between test cases.
This refactor does not change any behavior.
However, it is nice to know that no global mocktime leaks from the fuzz
init step to the first fuzz input, or from one fuzz input execution to
the next.
With the clock context, the global is re-set at the end of the context.
git range-diff bitcoin-core/master 666643be8f fa1f69d02d
31@@ -31,10 +32,9 @@ using SteadyMicroseconds = std::chrono::time_point<std::chrono::steady_clock, st
32
33 using SystemClock = std::chrono::system_clock;
34
35-/**
36- * Version of SteadyClock that is mockable in the context of tests (set the
37- * current value with SetMockTime), otherwise the system steady clock.
38- */
39+/// Version of SteadyClock that is mockable in the context of tests (via
40+/// SteadyClockContext, or self.SetMockTime), otherwise the system steady
self.SetMockTime in the doc comment, however the suggested fix seems off since it shouldn’t use the global ::SetMockTime. Not blocking, but if you need another push you can update to MockableSteadyClock::SetMockTime.
self. makes less sense than Self::, so I switched to that for now.
ACK fa1f69d02d80aedabc94a4de33091deb511440a0
git range-diff bitcoin-core/master 666643be8f fa1f69d02d that the only changes from the prior version are the two additional targets (block_index_tree.cpp, fees.cpp) and rebase contextCheckGlobalsImpl is handled and the warning message now references NodeClockContext and SteadyClockContext
