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:
SetMockTime for determinism by maflcko)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)
This is in symmetry to MockableSteadyClock.
This allows to set the mocktime before the testing setup.
Also, in some fuzz tests the mocktime was reset to 0 before this change,
so set it.
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 it leaked between test cases.
Also, it resets the global steady mock-time for the same reason.
🚧 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.