utils: replace boost::date_time usage with c++20 std::chrono

theuni commented at 7:04 pm on July 16, 2024: member

This would be a very straightforward and uncontroversial change if not for the sad state of std lib implementations which implement this functionality.

As of now, std::chrono::parse and friends (c++20 additions) are only available as of gcc 14 and (unreleased) clang 19.

Formatting in chrono is quite useful, so I don’t think it makes sense to wait years until we require those compilers.

Instead of waiting around, this PR takes the sledgehammer approach of adding a fully-conformant implementation of the new c++20 chrono features (from the author of std::chrono himself). More info available here: https://howardhinnant.github.io/date/date.html

A convenience header adds the chrono_parse namespace, which uses the std lib if possible, and date.h otherwise.

This allows us to get rid of our last use of boost::date_time, and we can begin using more modern c++20-isms as well.

DrahtBot commented at 7:04 pm on July 16, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	hebasto

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:

#30664 (build: Remove Autotools-based build system by hebasto)

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.

theuni commented at 7:06 pm on July 16, 2024: member

This was noticed while working on #30434, which requires us to install boost::date_time. Though vendoring date.h here may seem heavy, keep in mind that the status quo is a forward-incompatible mess of boost headers :)

hebasto commented at 7:06 pm on July 16, 2024: member

This allows us to get rid of our last use of boost::date_time

Concept ACK on that.

util: add date.h from Howard Hinnant's date lib

Taken from commit 8a932116795f4046ba8ed79ce33717c7331a1490

See documentation here: https://howardhinnant.github.io/date/date.html

This is meant to be a full implementation of the c++20 additions to
std::chrono.

This functionality is only available in libstd++ >= 14, and libc++ >= 19,
meaning that it will be years before we can require availability in the std
namespace.

Use of this lib will allow us to drop our current dependency on
boost::date_time, as well as to begin using (at least) formatting with
std::chrono.

a4084db4b0

util: add date_parse convenience header and use it for ParseISO8601DateTime

Because the functions in date.h are intended to line up exactly with the c++20
implementation in the std namespace, attempt to use std::chrono if a conforming
version is found.

While testing, I noticed that libstdc++ breaks for very old timestamps, so
guard against that and add an additional test.

b3f5c292d3

theuni force-pushed on Jul 16, 2024

DrahtBot added the label CI failed on Jul 16, 2024

DrahtBot commented at 7:11 pm on July 16, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27526907840

Make sure to run all tests locally, according to the documentation.

The failure may 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.

in src/wallet/rpc/util.cpp:36 in b3f5c292d3

41         return 0;
42-    return (ptime - epoch).total_seconds();
43+
44+    // Guard against broken implementations which can't handle very old time
45+    // values. Speficically, libstdc++ (as of v14) is broken at least on x86_64
46+    // with times before 1677-09-21T00:12:44Z .

maflcko commented at 7:18 pm on July 16, 2024:

Can you add more details, whether this is a crash/UB, or just wrong behavior? Is there a bug?

theuni commented at 7:32 pm on July 16, 2024:

Specifically the comparison against the epoch (0) fails. So, if the timestamp is old enough, this should fail but doesn’t:

0assert tp < epoch

You can play around here: https://godbolt.org/z/G8r837oGf

I’ll add more specific info. I should probably submit a bug report upstream as well.

maflcko commented at 7:37 pm on July 16, 2024:

Ah, so it is UB/crash, according to ubsan:

0/opt/compiler-explorer/gcc-14.1.0/include/c++/14.1.0/bits/chrono.h:227:38: runtime error: signed integer overflow: -9223372037 * 1000000000 cannot be represented in type 'long int'

maflcko commented at 7:42 pm on July 16, 2024:

Would be good to add a fuzz target as well to https://github.com/google/oss-fuzz/tree/master/projects/libstdcpp

Edit: https://www.github.com/pauldreik/stdfuzz/issues/8

maflcko commented at 7:26 pm on July 16, 2024: member

Formatting in chrono is quite useful, so I don’t think it makes sense to wait years until we require those compilers.

Can you give some examples where it would be useful? Right now it is only used in a single place in the wallet.

This was noticed while working on #30434, which requires us to install boost::date_time. Though vendoring date.h here may seem heavy, keep in mind that the status quo is a forward-incompatible mess of boost headers :)

Is the boost header footprint larger than this header? Also, by adding this header the locale linter alerts, so I wonder if the new stuff changes the behavior for users, or if the linter just alerts about unused code. Finally, ParseISO8601DateTime can be done in much less than 8k lines of C++ code, so I wonder if just the required parts can be extracted?

theuni commented at 7:51 pm on July 16, 2024: member

Formatting in chrono is quite useful, so I don’t think it makes sense to wait years until we require those compilers.

Can you give some examples where it would be useful? Right now it is only used in a single place in the wallet.

I was assuming that it’d be useful in other places where it’s simply cumbersome to deal with these conversions like logging. Or as a simplification for FormatISO8601DateTime in util/time.cpp. But sure, if we’re not going to end up using any more of this formatting in the future, I agree it’s not worth bothering with for this single function.

Is the boost header footprint larger than this header?

Heh. I modified #30434 to only install the headers required by boost::date_time (and its dependencies, recursively). The result:

0cory@nowhere:19:48:40:~/dev/bitcoin (cmake-boost-depends)
1 $ find depends/x86_64-pc-linux-gnu/include/boost/ -type f -name "*.hpp" | wc -l
24268

I won’t even bother adding up the lines of code, but I’m quite sure it’s less than 8k :)

Also, by adding this header the locale linter alerts, so I wonder if the new stuff changes the behavior for users, or if the linter just alerts about unused code.

Hmm, will have a look at this.

Finally, ParseISO8601DateTime can be done in much less than 8k lines of C++ code, so I wonder if just the required parts can be extracted?

I started down this rabbit hole intending to simply rewrite this function, but ended up afraid of introducing bugs due to the locale footguns. Maybe I’m over-complicating it?

I think if you look at this as: “use c++20 functionality with a bridge for compilers which don’t support it yet”, this PR makes perfect sense. But given the size of that bridge, I agree it’s maybe not worth it.

I suppose I was also curious if anyone else was waiting on this functionality for other code.

maflcko commented at 8:06 pm on July 16, 2024: member

I started down this rabbit hole intending to simply rewrite this function, but ended up afraid of introducing bugs due to the locale footguns. Maybe I’m over-complicating it?

Happy to try myself, but https://en.cppreference.com/w/cpp/chrono/year_month_day (which can convert to system clock) is available, so using Split + ToIntegral + year_month_day, then adding the hours, minutes and seconds should do everything in about 20 lines of code?

maflcko commented at 9:24 am on July 25, 2024: member

Maybe I’m over-complicating it?

And if there were any issues that I am missing, commit fa72dcbfa56177ca878375bae7c7bca6ca6a1f40 would be wrong as well, no?

fanquake marked this as a draft on Jul 25, 2024

maflcko commented at 11:00 am on August 29, 2024: member

@theuni Are you still working on this? My suggestion from #30462 (comment) should be easy to implement, unless I am missing something. Happy to pick that up, if this pull is up-for-grabs.

theuni commented at 3:03 pm on August 29, 2024: member

@maflcko All yours :)

fanquake closed this on Aug 29, 2024

fanquake added the label Up for grabs on Aug 29, 2024

utils: replace boost::date_time usage with c++20 std::chrono #30462

Code Coverage

Reviews

Conflicts