test: Add and use ElapseTime helper #32430

1. 
   maflcko commented at 10:39 pm on May 6, 2025: member

   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.

2. 
   DrahtBot commented at 10:39 pm on May 6, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32430.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	w0xlt
   Concept ACK	l0rinc

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32526 (fuzz: Delete wallet_notifications by maflcko)
   • #32414 (validation: periodically flush dbcache during reindex-chainstate by andrewtoth)
   • #29680 (wallet: fix unrelated parent conflict doesn’t cause child tx to be marked as conflict by Eunovo)
   • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

   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.

3. DrahtBot added the label Tests on May 6, 2025
4. maflcko force-pushed on May 6, 2025
5. DrahtBot added the label CI failed on May 6, 2025
6. 
   DrahtBot commented at 10:54 pm on May 6, 2025: contributor

   🚧 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.

7. maflcko force-pushed on May 6, 2025
8. maflcko force-pushed on May 7, 2025
9. test: Allow time_point in boost checks fa656be5ba
10. maflcko force-pushed on May 7, 2025
11. maflcko force-pushed on May 7, 2025
12. test: Avoid resetting mocktime in testing setup

    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.

    fa3662d41b
13. fuzz: Return chrono duration from ConsumeTime()

    This is a bit more type-safe than a raw i64.

    faf004e8db
14. test: Add ElapseTime context

    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.

    aaaaebe961
15. fuzz: Use ElapseTime fa0c70bd0e
16. test: Use ElapseTime in more tests fae29c5dd7
17. maflcko force-pushed on May 7, 2025
18. l0rinc approved
19. 
    l0rinc commented at 9:44 am on May 7, 2025: contributor

    Concept ACK - was thinking of something similar when reviewing 41479ed (#32414)
20. 
    maflcko commented at 9:47 am on May 7, 2025: member

    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 :sweat_smile:
21. DrahtBot removed the label CI failed on May 7, 2025
22. in src/test/util/setup_common.h:291 in fae29c5dd7

    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)
    

    ---

    ---

    

    w0xlt commented at 6:24 pm on May 7, 2025:

    Where is this being used ?

    ---

    

    l0rinc commented at 6:32 pm on May 7, 2025:

    If you remove it and try to compile, you’ll notice that Boost requires these for the test to be able to print out the the values when there’s a mismatch, see: https://github.com/bitcoin/bitcoin/blob/fae29c5dd71b263fb3bda8d554c9e6a4908175db/src/test/testnet4_miner_tests.cpp#L49
23. 
    w0xlt commented at 6:39 pm on May 7, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/32430/commits/fae29c5dd71b263fb3bda8d554c9e6a4908175db
24. DrahtBot requested review from l0rinc on May 7, 2025
25. DrahtBot added the label Needs rebase on May 16, 2025
26. 
    DrahtBot commented at 2:04 pm on May 16, 2025: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

Contributors
maflcko DrahtBot l0rinc w0xlt

Review Requested
l0rinc

Labels
Tests Needs rebase