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.
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-
ryanofsky commented at 6:08 PM on July 8, 2026: collaborator
-
b37d1d71f1
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>
-
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><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
-
maflcko commented at 8:09 PM on July 8, 2026: contributor
lgtm ACK b37d1d71f115f6b5a0000e09c7be063994387e1a
-
Sjors commented at 11:55 AM on July 9, 2026: member
That's merged, by I assume you still need this?
-
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.
-
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/BuildFieldoverloads. It could define them itself, but it makes sense for libmultiprocess to provide them since it already provides many similar ones including ones forstd::chrono::duration -
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.
Contributors
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
More mirrored repositories can be found on mirror.b10c.me