type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point #303

pull ryanofsky wants to merge 1 commits into bitcoin-core:master from ryanofsky:pr/timepoint changing 1 files +23 −0
  1. ryanofsky commented at 6:08 PM on July 8, 2026: collaborator

    Needed for bitcoin/bitcoin#10102 since bitcoin/bitcoin#34882 was merged, which uses NodeClock::time_point in the CNodeStats struct (m_last_send, m_last_recv, m_ping_start) returned by Node::getNodesStats.

  2. type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point
    Needed by bitcoin/bitcoin#34882 which uses NodeClock::time_point in the
    Node stats struct (m_last_send, m_last_recv, m_ping_start), requiring
    IPC serialization support for time_point types.
    
    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
    b37d1d71f1
  3. DrahtBot commented at 6:08 PM on July 8, 2026: none

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK maflcko

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. maflcko commented at 8:09 PM on July 8, 2026: contributor

    lgtm ACK b37d1d71f115f6b5a0000e09c7be063994387e1a

  5. Sjors commented at 11:55 AM on July 9, 2026: member

    Needed by https://github.com/bitcoin/bitcoin/pull/34882

    That's merged, by I assume you still need this?

  6. maflcko commented at 12:01 PM on July 9, 2026: contributor

    Needed by bitcoin/bitcoin#34882

    That's merged, by I assume you still need this?

    There was no rebase of https://github.com/bitcoin/bitcoin/pull/10102 in 4 months, so I don't think https://github.com/bitcoin/bitcoin/pull/10102 sees the changes from 34882 yet.

  7. ryanofsky commented at 12:42 PM on July 9, 2026: collaborator

    Sorry PR description was written in a confusing way, should hopefully be clearer now. In order for bitcoin/bitcoin#10102 to work after bitcoin/bitcoin#34882 it needs these ReadField / BuildField overloads. It could define them itself, but it makes sense for libmultiprocess to provide them since it already provides many similar ones including ones for std::chrono::duration

  8. in include/mp/type-chrono.h:42 in b37d1d71f1
      37 | +template <class Clock, class Duration, typename Value, typename Output>
      38 | +void CustomBuildField(TypeList<std::chrono::time_point<Clock, Duration>>, Priority<1>, InvokeContext& invoke_context,
      39 | +                      Value&& value, Output&& output)
      40 | +{
      41 | +    using Rep = typename Duration::rep;
      42 | +    static_assert(std::numeric_limits<decltype(output.get())>::lowest() <= std::numeric_limits<Rep>::lowest(),
    


    ViniciusCestarii commented at 1:17 PM on July 9, 2026:

    The range checks in both CustomBuildField overloads have a signedness bug:

    static_assert(std::numeric_limits<decltype(output.get())>::lowest() <= std::numeric_limits<Rep>::lowest(), ...);
    

    When the capnp field type is unsigned (e.g. UInt64) and Rep is signed (e.g. int64_t for std::chrono::nanoseconds), the usual arithmetic conversions convert Rep::lowest() (INT64_MIN) to the unsigned type before comparing, turning it into a huge positive number. So 0 <= INT64_MIN silently evaluates to true, and the assert passes for a field that can't actually represent negative tick counts (e.g. any pre-epoch system_clock::time_point).

    Concretely, this means a UInt64 field paired with a signed Duration::rep compiles today, and output.set(negative_count) wraps the value into UINT64_MAX. That's silently wrong for anything that reads the field as its declared (unsigned) type — a different language's capnp bindings, a JSON dump, or a raw comparison would see 18446744073709551615 instead of -1. A C++ round trip through the same chrono type can look fine, since the wrap is bit-for-bit invertible at equal width, which is what makes this easy to miss in testing.

    Fix: use std::cmp_less_equal/std::cmp_greater_equal (C++20, <utility>) instead of <=/>=, since they're designed to compare integers of differing signedness correctly:

    static_assert(std::cmp_less_equal(std::numeric_limits<decltype(output.get())>::lowest(), std::numeric_limits<Rep>::lowest()),
                  "capnp type does not have enough range to hold lowest std::chrono::time_point value");
    static_assert(std::cmp_greater_equal(std::numeric_limits<decltype(output.get())>::max(), std::numeric_limits<Rep>::max()),
                  "capnp type does not have enough range to hold highest std::chrono::time_point value");
    

    I verified this by temporarily declaring a test field as UInt64 against a signed Rep: it compiled before the fix and correctly fails to compile after.

    Happy to push chrono type tests as a follow-up PR if useful.

    Note: I had AI help me write this up clearly, apologies if the phrasing feels more polished or sloppy than my usual comments.


    ryanofsky commented at 2:09 PM on July 9, 2026:

    re: #303 (review)

    Good catch! I think this makes sense and it looks like this same problem also exists other places: for durations above and also in type-number.h. I'll see if it's possible to fix them all here without breaking anything.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/libmultiprocess. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-07-14 21:30 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me