Rework serials in `domain::new` by withjannisNLnetLabs · Pull Request #691 · NLnetLabs/domain

withjannisNLnetLabs · 2026-07-07T09:12:28Z

TLDR; Improve existing Serial type with more declarative types; Timestamp, SoaSerial and SeqNumberU32, which represent the underlying data more appropriately.

In DNS the SOA Serial number and the RRSIG inception/expiration have a 32-bit number which represents them. The maximum number is around 4 billion. The number may overflow and therefore special consideration has to be taken to increment and compare such types. The arithmetical definition for comparison, addition, etc. is in RFC1982.

Timestamp Represents the time since epoch in seconds. If the number is larger than u32::MAX it will wrap around. The types 'unit' is seconds because it's in the nature of the represented data. This type should not be used for normale time and date operation because of its special properties for comparison and modification. For RRSIG the value may be represented in the YYYYMMDDHHmmSS format see RFC4034.

The Signature Expiration Time and Inception Time field values MUST be
represented either as an unsigned decimal integer indicating seconds
since 1 January 1970 00:00:00 UTC, or in the form YYYYMMDDHHmmSS in
UTC, where:

Careful consideration has to be taken when defining the Display implementation. Another usage of the type is the Cookie used for DNS messages. This type may be generated from an accurate Timestamp (jiff::Timestamp), for example after calculating the inception and expiration time of a RRSIG record.

what Display format should be used for the Timestamp? -> use the seconds representation, because this format is valid in all known cases.

SoaSerial Represents the version of a zone by being present in the SOA records serial field. The version number is mostly used in three different formats; a counter without special meaning, seconds since epoch or the date of the day including a two digit counter in the format YYYYMMDDxx. Additionally to the SOA record the type is used in the ZoneMD record. The Display format is simple because in any case the number representation matches the string representation.

SeqNumberU32 Represents the type described in RFC1982 with a 32-bit number. This type is used for the above types as the underlying, wrapped data structure. This type should not be used directly but may well be public for usage by the end user.

define SeqNumberU32 as public or private?

The types must be creatable from the primitive type u32. Timestamp and SoaSerial type have no logical connection and shall not be convertible. The use case for the conversion to the wrapped type is questionable.

SoaSerial / Timestamp convertible into SeqSequenceU32 and vise versa implemented?

Desired changes:

[new] Add SeqNumberU32, SoaSerial, Timestamp signatures with documentation.
[new] Implementation of SeqNumberU32, SoaSerial, Timestamp functionality.
[new] Add testing for SeqNumberU32, SoaSerial, Timestamp functionality.
[new] Migration of data types to new SoaSerial, Timestamp types.

Important

This PR requires Cascade to change, because the types in Rrisg and Soa change.

tertsdiepraam

Just some preliminary comments. Good work!

tertsdiepraam · 2026-07-07T14:49:16Z

+            Some(Ordering::Equal)
+        } else if lhs.abs_diff(rhs) == 1 << 31 {
+            None
+        } else if (lhs < rhs) ^ (lhs.abs_diff(rhs) > (1 << 31)) {


I think this could do with either some explanation or be written out more explicitly. Using the bitwise OR with booleans is a bit surprising, though I think it's correct! Maybe even just a small note about that you're aware that it's bitwise or and not exponentiation already helps 😄

I added some usage examples which should illustrate the underlying logic.

bal-e

This is really cool! Well done @withjannisNLnetLabs! I have a bunch of comments (of course :p), but overall, I really like the functionality and the way you've laid out the code. The commits were nicely organized and easy to review.

Minor nits:

You often refer to the [`SoaSerial`] or the [`SeqNumberU32`]; maybe you intend these to be read as "the SOA serial type" and "the SeqNumberU32 type", but since the word "type" does not appear in there, IMO it's better to omit "the" and just say e.g. [`SoaSerial`] commonly follows one of the following strategies ....
There are a few typos here and there, e.g. Comparision, huaman. I'm not sure if there's a good automated way to detect these, I suspect spellcheck will not deal well with Rust or Markdown.

Top-level feedback:

They Timestamp and SoaSerial type have no logical connection and shall not be convertible.

I'm not sure that's true; one of the common conventions for the SOA serial is as the number of seconds since the Unix epoch modulo 2^32, exactly like Timestamp. I can see some value in allowing conversions between the two, when users choose to use that convention. To avoid users relying on this convention in the wrong cases, maybe it should be implemented as inherent methods with clear names rather than From/Into.

define SeqNumberU32 as public or private?

SoaSerial / Timestamp convertible into SeqSequenceU32 and vise versa implemented?

I think SeqNumberU32 should be public; it is an implementation of an RFC and so I'd argue it's more than an implementation detail of SoaSerial / Timestamp. Following that, I think it makes sense to have conversions to and from the wrapped types. These can be simple From / Into impls (while inherent methods that are const would be nice, I don't think there's too much need for them; we can add them if we have a need to).

bal-e · 2026-07-14T12:38:06Z

+/// - Date including counter, on change the number gets set to the current
+///   date in the format (`YYYYMMDD00`) if the number is smaller than the
+///   previous version, the old serial gets increased by one.


if the number is smaller than the previous version, the old serial gets increased by one

I don't think this edge case needs to be mentioned in these high-level docs.

bal-e · 2026-07-14T12:43:24Z

+/// Construct [`SoaSerial`] from [`u32`].
+///
+/// Equivalent to [`SoaSerial::get()`].
+impl From<SoaSerial> for u32 {
+    fn from(_value: SoaSerial) -> u32 {
+        todo!()
+    }
+}
+
+/// The raw [`u32`] underlying this [`SoaSerial`].
+///
+/// Equivalent to [`SoaSerial::new()`].
+impl From<u32> for SoaSerial {
+    fn from(_value: u32) -> Self {
+        todo!()
+    }
+}


The top-level descriptions for these two impls appear to be swapped; the first one should say "the raw u32 underlying this SoaSerial." Note that the references to get() and new() do not need to be swapped.

bal-e · 2026-07-14T12:45:33Z

+/// The [`Timestamp`] stores the seconds since Unix Epoch modulo 2^32. It is
+/// used in the [`Rrsig`] to keep track of `inception` and `expiration` time
+/// and in the edns [`Cookie`] `timestamp`.


"edns" => "EDNS"?

I see you're referring to the timestamp field of Cookie; maybe refer to the method directly, i.e. [`Cookie::timestamp()`]?

The same could be done for the RRSIG fields.

Linking to the timestamp method is possible, but I think it is not possible to link to certain fields.

bal-e · 2026-07-14T12:48:23Z

+/// [RFC1982]: https://datatracker.ietf.org/doc/html/rfc1982
+/// [RFC4034]: https://datatracker.ietf.org/doc/html/rfc4034#section-3.1.5
+/// [RFC9018]: https://datatracker.ietf.org/doc/html/rfc9018#name-the-timestamp-sub-field


I'm worried docs added here in the future might reference [RFC4034] or [RFC9018] without realizing they point to specific sections of those documents. Maybe explicitly state them as [RFC 4034, section 3.1.5] and [RFC 9018, "The Timestamp Sub-Field"]?

bal-e · 2026-07-14T12:50:01Z

+    }
+
+    /// Underlying seconds since Unix Epoch modulo 2^32.
+    // This type has a specific unit, therefore the `get()` function was


Maybe add a blank // line before this comment so it is more clearly distinguished from the public docs?

I swapped the doc comment and the comment to have the doc comment closer to the function.

bal-e · 2026-07-14T13:39:45Z

+/// Basic operations performed with a [`SoaSerial`].
+/// ```
+/// # use domain::new::base::SoaSerial;
+/// let soa_serial: SoaSerial = SoaSerial::new(u32::MAX).increment();


Maybe separate the .new() and the .increment() into separate statements so it's easier for the user to see what they should focus on? With the line as it is right now, the user might miss new() and only see increment().

Also, I don't think the : SoaSerial type annotation is necessary. It would be if new() returned something weirder, but that is not the case here.

bal-e · 2026-07-14T13:40:39Z

    }

    /// Measure system time since Unix Epoch modulo 2^32.
+    /// ```


Maybe add a blank line before this doc test?

bal-e · 2026-07-14T13:43:44Z

+    /// let now = Timestamp::now();
+    /// assert_eq!(
+    ///     now.as_seconds(),
+    ///     (UNIX_EPOCH.elapsed().unwrap().as_secs() % 0x1_0000_0000) as u32


Since as u32 automatically performs the modulo operation, do you want to omit the % 0x1_0000_0000?

Also, this doc test is a little bit racy... is there some way to make it deterministically succeed? I really want to avoid spurious failures.

bal-e · 2026-07-14T13:45:28Z

    ///                         or
    ///                         this < other and (this - other) > 2^31
    ///                         // 10 < u32::MAX and u32::MAX - 1 > 2^31
+    ///```


There should be a blank line before the doc test, and a space between /// and ```.

bal-e · 2026-07-14T13:46:35Z

+    /// # // Get all the edge cases
+    /// # assert_eq!(
+    /// #     SeqNumberU32::new(0).partial_cmp(&SeqNumberU32::new((1 << 31) - 1)),
+    /// #     Some(Ordering::Less)
+    /// # );
+    ///
+    /// # assert_eq!(
+    /// #     SeqNumberU32::new(0).partial_cmp(&SeqNumberU32::new((1 << 31) + 1)),
+    /// #     Some(Ordering::Greater)
+    /// # );
+    ///
+    /// # assert_eq!(
+    /// #     SeqNumberU32::new(0).partial_cmp(&SeqNumberU32::new((1 << 31))),
+    /// #     None
+    /// # );


I think these belong in a dedicated test function, not in the doc test.

withjannisNLnetLabs · 2026-07-15T13:01:38Z

    UnsizedCopy,
 )]
 #[repr(transparent)]
 pub struct Timestamp(SeqNumberU32);


add from timestamp

…ity.

withjannisNLnetLabs force-pushed the cleanup-serials branch from 13d6f66 to 585424b Compare July 7, 2026 14:04

tertsdiepraam reviewed Jul 7, 2026

View reviewed changes

bal-e requested changes Jul 9, 2026

View reviewed changes

withjannisNLnetLabs added the new-api Relating to the new API, i.e. `domain::new::*`. label Jul 10, 2026

withjannisNLnetLabs self-assigned this Jul 10, 2026

withjannisNLnetLabs force-pushed the cleanup-serials branch 6 times, most recently from f146341 to 8393971 Compare July 14, 2026 10:03

withjannisNLnetLabs requested a review from bal-e July 14, 2026 10:08

withjannisNLnetLabs marked this pull request as ready for review July 14, 2026 11:15

withjannisNLnetLabs changed the title ~~rough structure of timestamp and soa serial~~ Rework serials in domain::new Jul 14, 2026

bal-e requested changes Jul 14, 2026

View reviewed changes

withjannisNLnetLabs force-pushed the cleanup-serials branch from 5fa3faf to 0c8c2e8 Compare July 15, 2026 12:41

withjannisNLnetLabs commented Jul 15, 2026

View reviewed changes

withjannisNLnetLabs force-pushed the cleanup-serials branch from 0c8c2e8 to 74d850f Compare July 16, 2026 09:58

withjannisNLnetLabs added 4 commits July 16, 2026 15:56

[new] Introduce new sequence number types

694db04

[new] Implementation of SeqNumberU32, SoaSerial, Timestamp functional…

e7ebdbb

…ity.

[new] Add testing for SeqNumberU32, SoaSerial, Timestamp functionality.

29eb6b5

[new] Migration of data types to new SoaSerial, Timestamp types.

95037df

withjannisNLnetLabs force-pushed the cleanup-serials branch 4 times, most recently from e13e2ec to 95a009f Compare July 16, 2026 14:15

PR review feedback

4d77fe0

withjannisNLnetLabs force-pushed the cleanup-serials branch from 95a009f to 4d77fe0 Compare July 16, 2026 14:53

withjannisNLnetLabs requested a review from bal-e July 16, 2026 14:57

withjannisNLnetLabs force-pushed the cleanup-serials branch from 306c96b to 21dc900 Compare July 17, 2026 15:03

withjannisNLnetLabs force-pushed the cleanup-serials branch 3 times, most recently from 49a1937 to 89c43a8 Compare July 17, 2026 16:15

Self review

67bb0d3

withjannisNLnetLabs force-pushed the cleanup-serials branch from 89c43a8 to 67bb0d3 Compare July 17, 2026 16:24

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

tertsdiepraam 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bal-e left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

withjannisNLnetLabs commented Jul 7, 2026 •

edited

Loading

tertsdiepraam left a comment •

edited

Loading