util: remove Boost posix_time usage from GetTime* #21110
Conversation
|
Duplicate of #18282? |
Do you want to reopen that and include the epoch assertion/sanity check? Otherwise I can update this PR. |
|
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. |
|
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. |
|
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. |
|
Concept ACK; agree with adding the sanity check and not waiting for C++20. |
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>
e04da3e to
9266f74
Compare
|
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. |
|
Tested ACK 9266f74 Startup fails as expected if the sanity check doesn't pass: After this PR is merged the only remaining user of @elichai wrote a Boost-free replacement for |
|
Code review ACK 9266f74 |
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> Zcash: The first commit of bitcoin/bitcoin#21110; we intend to handle the changes made by the second commit of that PR in a separate fashion; see zcash#6042. (cherry picked from commit bitcoin/bitcoin@3c2e16b)
…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: `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> This is a backport of [[ bitcoin/bitcoin#21110 | core#21110]] [1/2] bitcoin/bitcoin@3c2e16b Note: see D5448 for usage of `#ifdef _WIN32` to determine `gmtime_s` availability. It this turns out to not work in the future, we can try to backport [[bitcoin/bitcoin#18358 | core#18358]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11789
Summary: remove Boost posix_time usage from GetTime* This is a backport of [[bitcoin/bitcoin#21110 | core#21110]] [2/2] bitcoin/bitcoin@9266f74 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11790
As |
I'm not sure if that is true? See discussion in #29081 (comment). Happy for you to submit a PR in any case. |
I have a followup that should remove the last of our
boost:posix_timeusage inParseISO8601DateTime, but that will likely need more cross-platform testing/discussion, so have just split them up as this change is straight forward.