util: remove Boost posix_time usage from GetTime* #21110

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:no_boost_gettime changing 4 files +63 −9

fanquake commented at 9:46 AM on February 8, 2021: member

I have a followup that should remove the last of our boost:posix_time usage in ParseISO8601DateTime, but that will likely need more cross-platform testing/discussion, so have just split them up as this change is straight forward.
fanquake added the label Refactoring on Feb 8, 2021
fanquake added the label Utils/log/libs on Feb 8, 2021
maflcko commented at 9:53 AM on February 8, 2021: member

Duplicate of #18282?
fanquake commented at 3:07 AM on February 10, 2021: member

Duplicate of #18282?

Do you want to reopen that and include the epoch assertion/sanity check? Otherwise I can update this PR.
maflcko commented at 7:27 AM on February 10, 2021: member

My plan was to wait till c++20, because it means less code to write.
fanquake commented at 7:39 AM on February 10, 2021: member

My plan was to wait till c++20, because it means less code to write.

I'm not sure that waiting the many years until the project will actually be able to use C++20, while preventing the removal of Boost, and use of std library features is worth not having to write a few additional lines of code. Especially since the assumption we have to make here is just a Unix time epoch. If it was something more controversial, or involved having to write hundreds of lines of additional code, which would incur a high maintenance overhead, I might agree, but that shouldn't be the case here.
maflcko commented at 7:50 AM on February 10, 2021: member

Obviously no objection to adding the sanity check now and removing it when we switch to c++20. I just meant to say that I am not going to write (rebase) it.
laanwj commented at 8:59 AM on February 10, 2021: member

Concept ACK

Let's go with a sanity check. I know I disagreed on this in https://github.com/bitcoin/bitcoin/pull/9566/files#r96350569 but as we age we get tired of the same old things and just want to move forward.
practicalswift commented at 9:29 AM on February 11, 2021: contributor

With the simple runtime sanity check from #9566 (https://github.com/bitcoin/bitcoin/pull/9566/commits/9bd32ce6aa50df95fd5aeaece92b8b908c582c73) we're guaranteed that we get UTC.

Concept ACK (assuming the sanity check is added of course)
ajtowns commented at 4:35 AM on February 12, 2021: contributor

Concept ACK; agree with adding the sanity check and not waiting for C++20.
laanwj added the label Waiting for author on Feb 15, 2021

time: add runtime sanity check

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.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

3c2e16be22

util: Use std::chrono for time getters 9266f7497f
fanquake removed the label Waiting for author on Feb 17, 2021
fanquake force-pushed on Feb 17, 2021
fanquake commented at 5:49 AM on February 17, 2021: member

I've changed this to pull in @theuni's sanity check (with some adjustments by @ajtowns , and some review comments from #9566 addressed). I've also dropped my commit in favour of @MarcoFalke change from #18282.
practicalswift commented at 9:59 AM on February 17, 2021: contributor
Tested ACK 9266f7497f256d780178829e0f3a29ddaeb794ba

Startup fails as expected if the sanity check doesn't pass:
```
$ src/bitcoind
Error: Clock epoch mismatch. Aborting.
Error: Initialization sanity check failed. Bitcoin Core is shutting down.
```
After this PR is merged the only remaining user of boost::posix_time is ParseISO8601DateTime. @elichai wrote a Boost-free replacement for ParseISO8601DateTime (then called DecodeDumpTime) two years ago in #17245. That replacement could be re-submitted as a new PR to allow for permanent removal of the boost/date_time/posix_time/posix_time.hpp dependency 🎉
laanwj commented at 7:33 PM on February 17, 2021: member

Code review ACK 9266f7497f256d780178829e0f3a29ddaeb794ba
laanwj merged this on Feb 17, 2021
laanwj closed this on Feb 17, 2021
sidhujag referenced this in commit 958325fd58 on Feb 18, 2021
fanquake deleted the branch on Feb 18, 2021
nuttycom referenced this in commit a423c2af8c on Jul 7, 2022
Fabcien referenced this in commit 06a488e349 on Jul 22, 2022
Fabcien referenced this in commit 7ba2b08340 on Jul 22, 2022
Fabcien referenced this in commit a208aeb229 on Jul 22, 2022
bitcoin locked this on Aug 16, 2022
hebasto commented at 8:47 PM on January 4, 2024: member

@fanquake

I have a followup that should remove the last of our boost:posix_time usage in ParseISO8601DateTime...

I'm not sure that waiting the many years until the project will actually be able to use C++20...

As std::chrono::parse is available now, do you mind submitting your follow up? If not, I can do it :)
fanquake commented at 10:32 AM on January 5, 2024: member

As std::chrono::parse is available now,

I'm not sure if that is true? See discussion in #29081 (comment). Happy for you to submit a PR in any case.

Contributors

Labels

Refactoring Utils/log/libs

Linked (view graph)

#23363 Discussion: Upgrading to C++20