refactor: Simplify GetTime #24871

maflcko · 2022-04-16T11:18:24Z

The implementation of GetTime is confusing:

The value returned by GetTime is assumed to be equal to GetTime<std::chrono::seconds>(). Both are mockable and the only difference is return type, the value itself is equal. However, the implementation does not support this assumption.
On some systems, time_t might be a signed 32-bit integer (https://en.cppreference.com/w/c/chrono/time), thus breaking in the year 2038, whereas GetTime<std::chrono::seconds> does not. Also, time_t might be -1 "on error", where "error" is unspecified.
GetTime<std::chrono::seconds> calls GetTimeMicros, which calls GetSystemTime, which calls std::chrono::system_clock::now, which doesn't have the above issues. See https://en.cppreference.com/w/cpp/chrono/system_clock/now
GetTimeMicros and the internal-only GetSystemTime will likely be renamed (to clarify they are the non-mockable non-monotonic system time) or removed in the future to be replaced by appropriate std::chrono::time_point<Clock> getters.

Fix all issues by:

making GetTime() an alias for GetTime<std::chrono::seconds>().count().
inlining the needed parts of GetSystemTime directly instead of needlessly increasing the function call stack with functions that are likely to be removed in the future.

laanwj · 2022-04-16T11:35:33Z

Concept ACK, GetTime should either use std::chrono or C time functions, not a mix for different types, that's really weird.

prusnak · 2022-04-16T12:41:32Z

src/util/time.cpp

@@ -129,6 +120,8 @@ int64_t GetTimeSeconds()
    return int64_t{GetSystemTime<std::chrono::seconds>().count()};
 }

+int64_t GetTime() { return GetTime<std::chrono::seconds>().count(); }


Nit: Why not using the same syntax/formatting as above in GetTimeSeconds?

I think the narrowing check is a bit overkill and probably doesn't make much sense. Mostly relevant isn't the maximum number of bits in the value, but the minimum, which is 35 bits according to https://en.cppreference.com/w/cpp/chrono/duration .

In either case the function is deprecated (see comment in the header file), and will likely be removed in the future anyway.

brunoerg

Strong Concept ACK

DrahtBot · 2022-04-17T05:01:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24697 (refactor address relay time by MarcoFalke)

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.

theStack · 2022-04-17T17:16:45Z

Concept ACK

martinus · 2022-04-18T12:56:59Z

Code review, untested ACK 0000a63. By the way strictly speaking std::chrono::system_clock is only guaranteed to be based on the unix epoch starting with C++20: https://en.cppreference.com/w/cpp/chrono/system_clock
But I have never seen a system where that is not already the case.

fanquake · 2022-04-18T13:13:32Z

But I have never seen a system where that is not already the case.

@martinus note that we have ChronoSanityCheck() which should fail in that case.

theStack

Code-review ACK 0000a63

I was first confused on why we don't have to std::chrono::duration_cast<T> the mock time, but C++ seems to be nice enough to implicitly convert between duration types if there is no precision loss (e.g. in our case seconds -> milliseconds and seconds -> microseconds). Note though that on master explicit instantiation with something less precise than seconds works:

template std::chrono::minutes GetTime();

while on the PR branch it doesn't. OTOH the chance that we ever need less precision than seconds is close to zero, so I think it's fine.

maflcko · 2022-04-19T07:35:39Z

Yes, the change was intentional. Given that we've never used std::chrono::minutes GetTime, it seems unlikely to ever be used in the future. Also, all of this code will likely be removed anyway soon(TM) and be replaced by a Now helper that returns time points (a type denoting a duration with a clock type embedded) instead of "flat" durations.

ajtowns · 2022-04-19T08:12:04Z

src/util/time.cpp

@@ -80,11 +70,12 @@ template <typename T>
 T GetTime()
 {
    const std::chrono::seconds mocktime{nMockTime.load(std::memory_order_relaxed)};
-
-    return std::chrono::duration_cast<T>(
+    const auto ret{


const T ret ?

Dropping the duration_cast around mocktime means you can't use GetTime<T>() with T set to minutes or hours or similar. Seems fine, just noting.

I think auto is fine for now. This will also reduce the diff in commit 8299fd2, where the type is changed from T to Clock::time_point::duration.

0000a63 Simplify GetTime (MarcoFalke) Pull request description: The implementation of `GetTime` is confusing: * The value returned by `GetTime` is assumed to be equal to `GetTime<std::chrono::seconds>()`. Both are mockable and the only difference is return type, the value itself is equal. However, the implementation does not support this assumption. * On some systems, `time_t` might be a signed 32-bit integer (https://en.cppreference.com/w/c/chrono/time), thus breaking in the year 2038, whereas `GetTime<std::chrono::seconds>` does not. Also, `time_t` might be `-1` "on error", where "error" is unspecified. * `GetTime<std::chrono::seconds>` calls `GetTimeMicros`, which calls `GetSystemTime`, which calls `std::chrono::system_clock::now`, which doesn't have the above issues. See https://en.cppreference.com/w/cpp/chrono/system_clock/now * `GetTimeMicros` and the internal-only `GetSystemTime` will likely be renamed (to clarify they are the non-mockable non-monotonic system time) or removed in the future to be replaced by appropriate `std::chrono::time_point<Clock>` getters. Fix all issues by: * making `GetTime()` an alias for `GetTime<std::chrono::seconds>().count()`. * inlining the needed parts of `GetSystemTime` directly instead of needlessly increasing the function call stack with functions that are likely to be removed in the future. ACKs for top commit: martinus: Code review, untested ACK 0000a63. By the way strictly speaking `std::chrono::system_clock` is only guaranteed to be based on the unix epoch starting with C++20: https://en.cppreference.com/w/cpp/chrono/system_clock theStack: Code-review ACK 0000a63 Tree-SHA512: f751ba740e0da65537be800e9414dd02282d9f04c0b0fb986a36546f257d0b888d8688653cdda5d355ec832c0e09d866922d9161b1ccd33485c1c92c5d1e802f

…ion time could be off by one Summary: util/time.* uses multiple clocks depending on circumstances like if mock time is set or if templated GetTime<>() is called. This can result in unexpected behavior where registerProof() uses a different clock than when SetMockTime(GetTime()...) is called in unit tests. Example failure: https://build.bitcoinabc.org/viewLog.html?buildId=416835&buildTypeId=BitcoinABC_BitcoinAbcStaging&tab=buildLog&_focus=1325 This patch makes this specific unit test predictable by setting mock time before any proofs are registered. The root cause may be fixed with additional backports to util/time.* that make the clock selection more consistent (see backport [[bitcoin/bitcoin#24871 | core#24871]] and [[bitcoin/bitcoin#21110 | core#21110]]). Test Plan: ninja check-avalanche Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11786

Summary: > The implementation of `GetTime` is confusing: > > * The value returned by `GetTime` is assumed to be equal to `GetTime<std::chrono::seconds>()`. Both are mockable and the only difference is return type, the value itself is equal. However, the implementation does not support this assumption. > > * On some systems, `time_t` might be a signed 32-bit integer (https://en.cppreference.com/w/c/chrono/time), thus breaking in the year 2038, whereas `GetTime<std::chrono::seconds>` does not. Also, `time_t` might be `-1` "on error", where "error" is unspecified. > > * `GetTime<std::chrono::seconds>` calls `GetTimeMicros`, which calls `GetSystemTime`, which calls `std::chrono::system_clock::now`, which doesn't have the above issues. See https://en.cppreference.com/w/cpp/chrono/system_clock/now > > * `GetTimeMicros` and the internal-only `GetSystemTime` will likely be renamed (to clarify they are the non-mockable non-monotonic system time) or removed in the future to be replaced by appropriate `std::chrono::time_point<Clock>` getters. > > > Fix all issues by: > > * making `GetTime()` an alias for `GetTime<std::chrono::seconds>().count()`. > > * inlining the needed parts of `GetSystemTime` directly instead of needlessly increasing the function call stack with functions that are likely to be removed in the future. This is a backport of [[bitcoin/bitcoin#24871 | core#24871]] Depends on D11790 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11791

Simplify GetTime

0000a63

maflcko added the Refactoring label Apr 16, 2022

prusnak reviewed Apr 16, 2022

View reviewed changes

brunoerg reviewed Apr 16, 2022

View reviewed changes

maflcko mentioned this pull request Apr 17, 2022

refactor address relay time #24697

Merged

martinus approved these changes Apr 18, 2022

View reviewed changes

theStack approved these changes Apr 18, 2022

View reviewed changes

fanquake requested a review from ajtowns April 19, 2022 07:50

ajtowns reviewed Apr 19, 2022

View reviewed changes

fanquake merged commit e0ff55a into bitcoin:master Apr 19, 2022

maflcko deleted the 2204-time-🖤 branch April 19, 2022 12:40

bitcoin locked and limited conversation to collaborators Apr 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Simplify GetTime #24871

refactor: Simplify GetTime #24871

Uh oh!

maflcko commented Apr 16, 2022

Uh oh!

laanwj commented Apr 16, 2022

Uh oh!

prusnak Apr 16, 2022

Uh oh!

maflcko Apr 16, 2022

Uh oh!

brunoerg left a comment

Uh oh!

DrahtBot commented Apr 17, 2022

Uh oh!

theStack commented Apr 17, 2022

Uh oh!

martinus commented Apr 18, 2022 •

edited

Loading

Uh oh!

fanquake commented Apr 18, 2022

Uh oh!

theStack left a comment •

edited

Loading

Uh oh!

maflcko commented Apr 19, 2022

Uh oh!

ajtowns Apr 19, 2022

Uh oh!

maflcko Apr 19, 2022

Uh oh!

Uh oh!

refactor: Simplify GetTime #24871

refactor: Simplify GetTime #24871

Uh oh!

Conversation

maflcko commented Apr 16, 2022

Uh oh!

laanwj commented Apr 16, 2022

Uh oh!

prusnak Apr 16, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 16, 2022

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 17, 2022

Conflicts

Uh oh!

theStack commented Apr 17, 2022

Uh oh!

martinus commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Apr 18, 2022

Uh oh!

theStack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 19, 2022

Uh oh!

ajtowns Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martinus commented Apr 18, 2022 •

edited

Loading

theStack left a comment •

edited

Loading