Currently mocktime is written to a global, which may leak between sub-tests (albeit some tests try to reset the mocktime on a best-effort basis). Also, when advancing it, one has to keep a counter variable around.
Fix both issues by a new NodeClockContext, which resets the mocktime once it goes out of scope. Also, it has a method to advance the mocktime by a delta.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32430.
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.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/41759458930
LLM reason (✨ experimental): (empty)
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
41479ed (#32414)
285@@ -286,6 +286,12 @@ inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
286 return v ? os << *v
287 : os << "std::nullopt";
288 }
289+
290+template <typename Clock, typename Duration>
291+inline std::ostream& operator<<(std::ostream& os, const std::chrono::time_point<Clock, Duration>& tp)
🚧 At least one of the CI tasks failed.
Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/45588216821
LLM reason (✨ experimental): The CI failure is caused by a compilation error in src/util/time.cpp due to incorrect usage of std::chrono::duration, which prevents building.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Sorry for the force-pushes, but this uncovered pre-existing issues in the fuzz tests that were not using mocktime during init. So I went ahead and fixed those as well in a separate commit. Should be good now, hopefully 😅
Went ahead and cherry-picked out the fuzz-fixes into #32927. Should be easy to (re-)review that one.
59 CBlockHeader genesis_header{Params().GenesisBlock()};
60 CBlockIndex start_index(genesis_header);
61
62- if (mock_time < start_index.GetMedianTimePast()) return;
63- SetMockTime(mock_time);
64+ SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/start_index.GetMedianTimePast()));
👍 for using the built-in min instead.
Could we add it to the commit message that this isn’t just a simple refactor (and not just about the return value), but as @hodlinator stated in a different PR where he did the same:
That way we can always proceed instead of just exiting the fuzz target, wasting cycles. Also reduces code complexity.
typo in commit message:
no state is leaked between test cases
30+ ElapseSteady& operator=(const ElapseSteady&) = delete;
31+
32+ /** Change mocktime by the given duration delta */
33 void operator()(std::chrono::milliseconds d)
34 {
35+ Assert(d >= 0s); // Steady time can only move forward.
nit: the code isn’t in exact alignment with the comment - and since we are calling it with 0, maybe something like:
0 Assert(d >= 0ms); // Steady time cannot move backward.
(nit2: I know it’s the same, but to clarify that we don’t just mean that time-jumps cannot be 0.5 seconds, we might want to use 0ms instead)
41+class ElapseTime
42+{
43+ NodeSeconds m_t{std::chrono::seconds::max()};
44+
45+public:
46+ /** Initialize with initial time */
ElapseSteady, but I don’t find the comment helpful as it is.
58+ /** Set mocktime */
59+ void set(NodeSeconds t) { SetMockTime(m_t = t); }
60+ void set(std::chrono::seconds t) { set(NodeSeconds{t}); }
61+
62+ /** Change mocktime by the given duration delta */
63+ void operator()(std::chrono::seconds d) { SetMockTime(m_t += d); }
maybe we could reuse the previous setters like we do in other methods:
0 void operator()(std::chrono::seconds d) { set(m_t + d); }
or maybe even:
0 void operator()(std::chrono::seconds d)
1 {
2 Assert(d >= 0s); // Elapse time cannot move backward.
3 set(m_t + d);
4 }
nit: I don’t find the operator to be intuitive here, maybe we could call it Advance instead?
nit: I don’t find the operator to be intuitive here, maybe we could call it
Advanceinstead?
I think elapse_time(4h) or elapse_steady(4h) is self-explanatory. So leaving as-is for now.
48+ ElapseTime(std::chrono::seconds init_time) { set(init_time); }
49+ /** Initialize with current time, using the next tick to avoid going back by rounding to seconds */
50+ ElapseTime() { set(++Now<NodeSeconds>().time_since_epoch()); }
51+
52+ /** Unset mocktime */
53+ ~ElapseTime() { SetMockTime(0s); }
In the constructors we’re using the setters, can we use it here, too, to minimize the places where we’re adjusting the local/global state?
0 ~ElapseTime() { set(0s); }
18- {
19- (*this)(0s); // init
20- }
21+
22+public:
23+ /** Initialize with INITIAL_MOCK_TIME. */
54+
55+ ElapseTime(const ElapseTime&) = delete;
56+ ElapseTime& operator=(const ElapseTime&) = delete;
57+
58+ /** Set mocktime */
59+ void set(NodeSeconds t) { SetMockTime(m_t = t); }
I think the part that a setter modifies both the local and global state should be made more obvious, it’s not intuitive:
0 void Set(NodeSeconds t) noexcept
1 {
2 m_t = t;
3 SetMockTime(m_t); // Set the global mock time.
4 }
nit: capitalized + noexcept
22+public:
23+ /** Initialize with INITIAL_MOCK_TIME. */
24+ ElapseSteady() { (*this)(0s); }
25+
26+ /** Unset mocktime */
27+ ~ElapseSteady() { MockableSteadyClock::SetMockTime(0s); }
MockableSteadyClock::ClearMockTime(); here
19- (*this)(0s); // init
20- }
21+
22+public:
23+ /** Initialize with INITIAL_MOCK_TIME. */
24+ ElapseSteady() { (*this)(0s); }
What would happen if we ran multiple such tests on different threads? Could we guard against that via something like:
0 ElapseSteady()
1 {
2 assert(GetMockTime() == 0s); // Ensure that the steady clock is not mocked yet.
3 MockableSteadyClock::SetMockTime(0s);
4 }
nit: (*this)(0s); is not very intuitive in my opinion
116@@ -116,7 +117,7 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
117 {
118 SeedRandomStateForTest(SeedRand::ZEROS);
119 FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
120- SetMockTime(ConsumeTime(fuzzed_data_provider));
121+ ElapseTime elapse_time{ConsumeTime(fuzzed_data_provider)};
time_mock or something to obviate why it’s unused?
are we planning on using the new declared object?
Yes, it can trivially be used, if there is need to. Possibly it isn’t used in this specific instance any further, but it seems beneficial to allow it and also beneficial to be consistent in the naming and usage.
17 BOOST_FIXTURE_TEST_SUITE(banman_tests, BasicTestingSetup)
18
19 BOOST_AUTO_TEST_CASE(file)
20 {
21- SetMockTime(777s);
22+ ElapseTime elapse_time{777s};
I find it non-intuitive that we have constructor and operator doing different things, i.e. why is the absolute setter:
0 ElapseTime elapse_time{777s};
different from the relative one:
0 ElapseTime elapse_time{};
1 elapse_time(777s);
Looking at the code I understand it of course, but maybe something like:
0 ElapseTime elapse_time{};
1 elapse_time.Advance(777s);
would be easier to read
Looking at the code I understand it of course, but maybe something like:
0 ElapseTime elapse_time{}; 1 elapse_time.Advance(777s);
Not sure. This is doing something else, as explained above by yourself? So this replacement is not correct to apply here.
32 m_node.validation_signals->SyncWithValidationInterfaceQueue();
33 BOOST_CHECK(!sub->m_did_flush);
34
35 // The periodic flush interval is between 50 and 70 minutes (inclusive)
36- SetMockTime(GetTime<std::chrono::minutes>() + 49min);
37+ elapse_time(49min);
167@@ -167,7 +168,7 @@ BOOST_FIXTURE_TEST_CASE(stale_tip_peer_management, OutboundTest)
168 BOOST_CHECK(node->fDisconnect == false);
169 }
170
171- SetMockTime(time_later);
172+ elapse_time(delta);
any reason for defining the delta so far away from first usage?
0 const auto delta{3 * std::chrono::seconds{m_node.chainman->GetConsensus().nPowTargetSpacing} + 1s};
1 elapse_time(delta);
579- SetMockTime(42 + 2*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
580+ elapse_time(HALFLIFE / 4);
581 BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK(), llround((maxFeeRateRemoved.GetFeePerK() + 1000)/8.0));
582 // ... with a 1/4 halflife when mempool is < 1/4 its target size
583
584- SetMockTime(42 + 7*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
44@@ -44,7 +45,7 @@ FUZZ_TARGET(p2p_handshake, .init = ::initialize)
45
46 auto& connman = static_cast<ConnmanTestMsg&>(*g_setup->m_node.connman);
47 auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
48- SetMockTime(1610000000); // any time to successfully reset ibd
49+ ElapseTime elapse_time{1610000000s}; // any time to successfully reset ibd
42+{
43+ NodeSeconds m_t{std::chrono::seconds::max()};
44+
45+public:
46+ /** Initialize with initial time */
47+ ElapseTime(NodeSeconds init_time) { set(init_time); }
explicit
src/test/util/time.h
git range-diff bitcoin-core/master fa80e750e7 fa053c2e32. The only changes were (1) dropping hunks of deleted code, (2) two in-line conflicts, (3) some adjacent lines modified in unrelated changes.
diff reACK fa053c2e3237c46642d9e231eaea14cb62926c4f
mini_miner_selection and LimitOrphans fuzz targets were removed during rebase and the refactored chainstate_write_interval refactoring was adjusted.
80@@ -80,10 +81,11 @@ FUZZ_TARGET(p2p_handshake, .init = ::initialize)
81 continue;
82 }
83
84- SetMockTime(GetTime() +
85+ elapse_time(std::chrono::seconds{
ElapseSteady -> SteadyClockContext and instead of operator() -> operator+= (https://github.com/bitcoin/bitcoin/pull/34432). Same for ElapseTime -> NodeClockContext.
Code re-review and rebase diff ACK 58ca20a0d1e4ae2b037eaeb10d2f0d6830a4c737
Redid the rebase locally, ended up with the same, except for some comment changes.
0diff --git a/src/test/util/time.h b/src/test/util/time.h
1--- a/src/test/util/time.h (revision 4f2a0dc6404fe8a2fd9d42954ca883eed459f120)
2+++ b/src/test/util/time.h (date 1769431359007)
3@@ -10,8 +10,8 @@
4
5 #include <optional>
6
7-/// Helper to initialize the global MockableSteadyClock, elapse it, and reset
8-/// it after use in a test.
9+/// Helper to initialize the global MockableSteadyClock, let a duration elapse,
10+/// and reset it after use in a test.
11 class ElapseSteady
12 {
13 MockableSteadyClock::mock_time_point::duration t{MockableSteadyClock::INITIAL_MOCK_TIME};
14@@ -35,8 +35,8 @@
15 }
16 };
17
18-/// Helper to initialize the global NodeClock, elapse it, and reset
19-/// it after use in a test.
20+/// Helper to initialize the global NodeClock, let a duration elapse,
21+/// and reset it after use in a test.
22 class ElapseTime
23 {
24 NodeSeconds m_t{std::chrono::seconds::max()};
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.
This is required in the next commit.
🚧 At least one of the CI tasks failed.
Task macOS-cross to x86_64: https://github.com/bitcoin/bitcoin/actions/runs/21588670919/job/62203167324
LLM reason (✨ experimental): Compilation failed: unknown type ElapseTime in bench/util_time.cpp during building bench_bitcoin.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.