threading: use std::chrono for timestamps #9566

theuni · 2017-01-16T22:58:35Z

Obviously not for 0.14.

Though boost's chrono hides the racy gmtime() issue nicely, it's really just using gmtime_r internally. This switches to the same thing, but without the boost indirection.

PRing this separately from other boost removals because it's not a 1:1 replacement.

fanquake · 2017-01-17T02:28:58Z

Failed on both Windows builds with:

utiltime.cpp: In function ‘const tm gmtime_int(time_t)’:
utiltime.cpp:31:25: error: cannot convert ‘time_t* {aka long int*}’ to ‘tm*’ for argument ‘1’ to ‘errno_t gmtime_s(tm*, const time_t*)’
     gmtime_s(&time, &out);

Unfortunately, there's still no standard way of printing the current time in a threadsafe way. Digging down into boost's approach, they simply use gmtime_r when possible, as guessed by availability macros. We now use the same approach, but use autotools to detect whether gmtime_r or gmtime_s can be used, or as a fallback, the racy gmtime. Note that MilliSleep was not replaced because it is an interruption point. That can be done once boost threads are all gone.

std::chrono::system_clock.time_since_epoch and time_t(0) are not guaranteed to use the Unix epoch timestamp, but in practice they almost certainly will. Any differing behavior will be assumed to be an error, unless certain platforms prove to consistently deviate, at which point we'll cope with it by adding offsets. Do a quick runtime check to verify that time_t(0) == std::chrono::system_clock's epoch time == unix epoch.

theuni · 2017-01-17T03:58:04Z

Of course windows has reversed arguments. Why wouldn't it?

Pushed the reversal.

laanwj · 2017-01-17T06:28:34Z

src/utiltime.cpp

+    return out;
+}
+
+bool ChronoSanityCheck()


Needing something like this makes me wonder if we're not better off just using the C time functions, which guarantee being based on the UNIX epoch.

Especially as we're already probing for gmtime_int anyway. This seems double.

Do we need something that is only offered by std::chrono? (or needs much less code with std::chrono)

@laanwj Best I can tell from google and reading the c spec, it's not guaranteed to be based on the UNIX epoch either :(. Of course, I've never seen an implementation that isn't, as that would likely break years worth of assumptions. And based on that, I would assume that c++ implementations will be using the underlying c implementation.

So my logic was: if we can run a quick sanity check here to be sure that c/c++/unix epoch are all aligned, then we can use c apis and std::chrono interchangeably throughout the codebase without worry.

As for why chrono here, it was to avoid gettimeofday portability issues. If you'd prefer to work around those instead, no problem.

You're right though that DateTimeStrFormat is kinda clunky. I only used c++ there to avoid c string size guessing. I'm also happy to switch that if you'd prefer.

Best I can tell from google and reading the c spec, it's not guaranteed to be based on the UNIX epoch either :(

Which ones? The man page of "gmtime" tells me the following:

The ctime(), gmtime() and localtime() functions all take an argument of data type time_t, which represents calendar time. When interpreted as an absolute time value, it represents the number of seconds elapsed since the Epoch, 1970-01-01 00:00:00 +0000 (UTC).

Same for gettimeofday:

and gives the number of seconds and microseconds since the Epoch (see time(2)). The tz argument is a struct timezone:

If using std::chrono makes anything less hassle I'm fine with using it. It just seems like a lot of code necessary to work around issues (and that's just to get started using it!), but you're right that gettimeofday and strftime have their own issues.

@laanwj Posix uses a value since the Unix epoch, but time_t is implementation-defined according to the c spec. But it really doesn't matter, realistically it'll be the Unix epoch everywhere we run. Sorry for the distraction.

I only brought it up because the c++ system_clock's epoch is technically implementation-defined as well, I was just making the case that it's probably just as safe to assume as with c.

zstardust · 2017-01-17T14:44:43Z

There's a few places in Bitcoin Core where non monotonic time stamps can cause the node to act strangely, for example if NTP tweaks the time backwards between a p2p PING and PONG, the int64 for node latency can overflow. Without much review, does this change any of the behavior in that respect? chrono::steady_clock assures monotone time where required.

theuni · 2017-01-17T16:53:57Z

@zstardust There is (should be) no behavioral change here. steady_clock would indeed make sense for some of those cases.

laanwj · 2017-01-17T20:31:36Z

Agreed, using monotonic clock makes sense where only differences in time are important, when there is no need to print the time (as it will have an arbitrary starting offset). That's orthogonal to the changes here, though.

TheBlueMatt · 2017-01-23T16:43:40Z

I think we need to think more about how we use time everywhere, see eg the issues turned up in #9606 - do we want our own types for time to enforce that we dont compare mock and non-mock time? Do we want multiple types of mock times? etc.

theuni · 2017-01-23T18:22:38Z

@TheBlueMatt This is intended to just be a replacement of what we have now. The behavioral changes can come next.

I have an impl of a clock/time_point that I've made that is explicitly non-convertable from one type to another. Happy to PR that for discussion as well. I expect that it would replace our current timestamp usage, but I figured it'd be best to get rid of boost first, since the boost condvars use boost::chrono.

jtimon · 2017-02-01T01:48:37Z

Concept ACK. With C++ std or with C time functions. Seems like removing boost's dependency here is better irrespective of next steps.
Needs rebase.

laanwj · 2017-02-01T07:56:29Z

src/utiltime.cpp

+
+    // Check that the above zero time is actually equal to the known unix timestamp.
+    tm epoch = gmtime_int(nTime);
+    if ((epoch.tm_sec != 0)  || \


Why the \ at the end of the line?

laanwj · 2017-02-01T07:57:41Z

src/utiltime.cpp

+    // to a time_t and verify that it's the same as before.
+    const time_t zeroTime{};
+    auto clock = std::chrono::system_clock::from_time_t(zeroTime);
+    if (std::chrono::duration_cast<std::chrono::seconds>(clock.time_since_epoch()).count() != 0)


nit: bracing style: we decided to always use braces unless the if and the statement can be on one line: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

laanwj · 2017-03-09T09:05:05Z

Needs rebase/nit fix.

laanwj · 2017-04-19T08:27:19Z

Is this still in your plans @theuni?

theuni · 2017-06-27T20:10:41Z

Thanks for the reminder, I'll circle back to this.

sipa · 2017-07-13T18:10:43Z

What is the status here? Going to make it for 0.15?

theuni · 2017-07-13T19:00:29Z

I'll go ahead and rebase/clean this up now, but it's not a priority for 0.15.

danra · 2017-09-30T15:34:25Z

Needs rebase

laanwj · 2018-02-08T18:32:45Z

@theuni Ping

theuni · 2018-03-06T19:44:54Z

After discussion with @laanwj, closing this in favor of a different approach.

maflcko · 2018-12-03T20:01:55Z

Anyone remembers what the "different approach" was?

maflcko · 2020-03-06T21:43:52Z

ping @theuni 😬

maflcko · 2020-03-07T15:09:40Z

After discussion with @laanwj, closing this in favor of a different approach.

Or maybe, ping @laanwj 😬

fanquake added the Refactoring label Jan 16, 2017

theuni added 2 commits January 16, 2017 22:56

theuni force-pushed the nuke-boost-chrono2 branch from 6523d07 to 9bd32ce Compare January 17, 2017 03:56

laanwj reviewed Jan 17, 2017

View reviewed changes

fanquake added this to the 0.15.0 milestone Jan 18, 2017

This was referenced Jan 20, 2017

bip68-sequence.py failing on master after recent net changes, due to mocktime interaction #9586

Closed

Use CScheduler for wallet flushing, remove ThreadFlushWalletDB #9605

Merged

laanwj reviewed Feb 1, 2017

View reviewed changes

laanwj mentioned this pull request Feb 10, 2017

Bump minimum required Boost version #8875

Closed

laanwj assigned theuni Jun 26, 2017

laanwj modified the milestones: 0.16.0, 0.15.0 Jul 13, 2017

maflcko modified the milestones: 0.16.0, 0.17.0 Nov 22, 2017

theuni closed this Mar 6, 2018

theuni mentioned this pull request Apr 19, 2018

Fix for utiltime to compile with msvc. #13031

Merged

maflcko mentioned this pull request Dec 3, 2018

util: Don't throw in GetTime{Millis,Micros}(). Mark as noexcept. #13382

Closed

maflcko mentioned this pull request Jul 15, 2019

Set minimum required Boost to 1.53.0 #16381

Closed

maflcko mentioned this pull request Mar 6, 2020

util: Use std::chrono for time getters #18282

Closed

practicalswift mentioned this pull request Feb 11, 2021

util: remove Boost posix_time usage from GetTime* #21110

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

threading: use std::chrono for timestamps #9566

threading: use std::chrono for timestamps #9566

Uh oh!

Conversation

theuni commented Jan 16, 2017

Uh oh!

fanquake commented Jan 17, 2017

Uh oh!

theuni commented Jan 17, 2017

Uh oh!

laanwj Jan 17, 2017 • 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!

theuni Jan 17, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Jan 17, 2017 • 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!

theuni Jan 17, 2017

Choose a reason for hiding this comment

Uh oh!

zstardust commented Jan 17, 2017

Uh oh!

theuni commented Jan 17, 2017

Uh oh!

laanwj commented Jan 17, 2017

Uh oh!

TheBlueMatt commented Jan 23, 2017

Uh oh!

theuni commented Jan 23, 2017

Uh oh!

jtimon commented Feb 1, 2017

Uh oh!

laanwj Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 9, 2017

Uh oh!

laanwj commented Apr 19, 2017

Uh oh!

theuni commented Jun 27, 2017

Uh oh!

sipa commented Jul 13, 2017

Uh oh!

theuni commented Jul 13, 2017

Uh oh!

danra commented Sep 30, 2017

Uh oh!

laanwj commented Feb 8, 2018

Uh oh!

theuni commented Mar 6, 2018

Uh oh!

maflcko commented Dec 3, 2018

Uh oh!

maflcko commented Mar 6, 2020

Uh oh!

maflcko commented Mar 7, 2020

Uh oh!

Uh oh!

laanwj Jan 17, 2017 •

edited

Loading

laanwj Jan 17, 2017 •

edited

Loading