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 ElapseTime helper, 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 react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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
This is less code and also required for the next commit.
This is a bit more type-safe than a raw i64.
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.
Also, it resets the global steady mock-time for the same reason.
This is required in the next commit.
src/test/util/time.h