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

1. 
   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.

2. 
   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.

3. 
   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 :)
4. 
   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.

5. 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
6. 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
7. theuni force-pushed on Jul 16, 2024
8. DrahtBot added the label CI failed on Jul 16, 2024
9. 
   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.

10. 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

11. 
    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?

12. 
    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.

13. 
    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?

14. 
    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?

15. fanquake marked this as a draft on Jul 25, 2024
16. 
    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.
17. 
    theuni commented at 3:03 pm on August 29, 2024: member

    @maflcko All yours :)
18. fanquake closed this on Aug 29, 2024

    ---

19. fanquake added the label Up for grabs on Aug 29, 2024
20. maflcko removed the label Up for grabs on Nov 28, 2024
21. 
    maflcko commented at 12:47 pm on November 28, 2024: member

    should do everything in about 20 lines of code?

    Ok, it is only 18 lines of code, see https://github.com/bitcoin/bitcoin/pull/31391

Contributors
theuni DrahtBot hebasto maflcko

Labels
CI failed