type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point by ryanofsky · Pull Request #303 · bitcoin-core/libmultiprocess

ryanofsky · 2026-07-08T18:08:19Z

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.

DrahtBot · 2026-07-08T18:08:29Z

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

Reviews

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

Type	Reviewers
ACK	maflcko, ViniciusCestarii

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

maflcko · 2026-07-08T20:09:17Z

lgtm ACK b37d1d7

Sjors · 2026-07-09T11:55:31Z

Needed by bitcoin/bitcoin#34882

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

maflcko · 2026-07-09T12:01:12Z

Needed by bitcoin/bitcoin#34882

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

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

ryanofsky · 2026-07-09T12:42:50Z

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

ViniciusCestarii · 2026-07-09T13:17:51Z

+                      Value&& value, Output&& output)
+{
+    using Rep = typename Duration::rep;
+    static_assert(std::numeric_limits<decltype(output.get())>::lowest() <= std::numeric_limits<Rep>::lowest(),


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, ) 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.

re: #303 (comment)

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.

re: #303 (comment)

Added a new commit to fix this problem. Using std::cmp functions directly didn't work for time_point comparisons because floating point time point types are used in some places so I had to add wrappers to handle floats.

Use std::cmp_less_equal/std::cmp_greater_equal (C++20) in the range static_asserts to avoid signed/unsigned comparison pitfall: when one side is unsigned and the other signed, the usual arithmetic conversions silently convert the signed lowest() to a huge positive number, making the assert pass when it should fire. Also fix the identical pattern in type-number.h BuildPrimitive. Problem was reported by ViniciusCestarii in bitcoin-core#303 (comment) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ryanofsky

Thanks for the reviews!

Updated b37d1d7 -> 978028f (pr/timepoint.1 -> pr/timepoint.2, compare) switching to std::cmp functions for more reliable static asserts

ryanofsky · 2026-07-30T00:45:02Z

+                      Value&& value, Output&& output)
+{
+    using Rep = typename Duration::rep;
+    static_assert(std::numeric_limits<decltype(output.get())>::lowest() <= std::numeric_limits<Rep>::lowest(),


re: #303 (comment)

Added a new commit to fix this problem. Using std::cmp functions directly didn't work for time_point comparisons because floating point time point types are used in some places so I had to add wrappers to handle floats.

maflcko · 2026-07-30T05:42:08Z

Looks like iwyu fails?

Use std::cmp_less_equal/std::cmp_greater_equal (C++20) in the range static_asserts to avoid signed/unsigned comparison pitfall: when one side is unsigned and the other signed, the usual arithmetic conversions silently convert the signed lowest() to a huge positive number, making the assert pass when it should fire. Also fix the identical pattern in type-number.h BuildPrimitive. Problem was reported by ViniciusCestarii in bitcoin-core#303 (comment) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ryanofsky

Updated 978028f -> 1e33111 (pr/timepoint.2 -> pr/timepoint.3, compare) to fix IWYU error https://github.com/bitcoin-core/libmultiprocess/actions/runs/30503532739/job/90748227074

Rebased 1e33111 -> 7a72df0 (pr/timepoint.3 -> pr/timepoint.4, compare) adding new commit and rebasing to fix incompatibility with bool and std::cmp_less_equal/std::cmp_greater_equal in libc++ https://github.com/bitcoin-core/libmultiprocess/actions/runs/30551079943/job/90899596238?pr=303

Exclude bool from the integral BuildPrimitive overload (bool is integral per std::is_integral_v but not a standard integer per std::cmp_*); add a dedicated bool overload that static_asserts the capnp LocalType is also bool, so a schema mismatch (e.g. Float64 used for a bool parameter) produces a clear error pointing at the schema. This change is also needed so the next commit can tighten the integer range changes in the BuildPrimitive integer overload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Use std::cmp_less_equal/std::cmp_greater_equal (C++20) in the range static_asserts to avoid signed/unsigned comparison pitfall: when one side is unsigned and the other signed, the usual arithmetic conversions silently convert the signed lowest() to a huge positive number, making the assert pass when it should fire. Also fix the identical pattern in type-number.h BuildPrimitive. Problem was reported by ViniciusCestarii in bitcoin-core#303 (comment) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…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>

uqlidi

looks good but there's a small suggestion

uqlidi · 2026-07-31T06:48:47Z

+    if constexpr (std::is_floating_point_v<A> || std::is_floating_point_v<B>) return a >= b;
+    else return std::cmp_greater_equal(a, b);


Suggested change

if constexpr (std::is_floating_point_v<A> || std::is_floating_point_v<B>) return a >= b;

else return std::cmp_greater_equal(a, b);

else return safe_less_equal(b, a);

re: #303 (comment)

This does seem like a good idea, but it's a minor cleanup so not planning to make the change here. Could be worth doing if this code changes again.

maflcko · 2026-07-31T08:41:06Z

lgtm ACK 7a72df0

ViniciusCestarii

ACK 7a72df0

ViniciusCestarii reviewed Jul 9, 2026

View reviewed changes

ryanofsky force-pushed the pr/timepoint branch from b37d1d7 to 978028f Compare July 30, 2026 00:43

ryanofsky commented Jul 30, 2026

View reviewed changes

ryanofsky force-pushed the pr/timepoint branch from 978028f to 1e33111 Compare July 30, 2026 14:18

ryanofsky commented Jul 30, 2026

View reviewed changes

ryanofsky and others added 3 commits July 30, 2026 11:19

ryanofsky force-pushed the pr/timepoint branch from 1e33111 to 7a72df0 Compare July 30, 2026 17:56

uqlidi suggested changes Jul 31, 2026

View reviewed changes

ViniciusCestarii reviewed Aug 1, 2026

View reviewed changes

ryanofsky merged commit f355108 into bitcoin-core:master Aug 3, 2026
13 checks passed

		if constexpr (std::is_floating_point_v<A> \|\| std::is_floating_point_v<B>) return a >= b;
		else return std::cmp_greater_equal(a, b);

	if constexpr (std::is_floating_point_v<A> \|\| std::is_floating_point_v<B>) return a >= b;
	else return std::cmp_greater_equal(a, b);
	else return safe_less_equal(b, a);

Uh oh!

Conversation

ryanofsky commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

maflcko commented Jul 8, 2026

Uh oh!

Sjors commented Jul 9, 2026

Uh oh!

maflcko commented Jul 9, 2026

Uh oh!

ryanofsky commented Jul 9, 2026

Uh oh!

ViniciusCestarii Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 30, 2026

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uqlidi left a comment

Choose a reason for hiding this comment

Uh oh!

uqlidi Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 31, 2026

Uh oh!

ViniciusCestarii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ryanofsky commented Jul 8, 2026 •

edited

Loading

DrahtBot commented Jul 8, 2026 •

edited

Loading

ryanofsky left a comment •

edited

Loading