From 93491e776234f76e4a46a6b5decc21e25d2a43d7 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 00:21:59 +0700 Subject: [PATCH] refactor(proto): preserve open parameter presence `Params::decode` seeded `Params::default()` and overwrote only the tags it saw, so an omitted tag and one explicitly encoded as zero were indistinguishable afterwards. The protocol gives those the same value but not the same meaning, and a server cannot apply its open admission rules without telling them apart: an absent Duration or Interval is accepted as the wire default zero, while one explicitly encoded as zero is invalid. Add `Params::decode_with_presence`, returning the decoded values alongside a `ParamPresence` recording which known tags appeared. `Params::decode` now delegates to it, so there is still exactly one parser and the two cannot diverge. Unknown tags stay ignored and untracked, and repeated known tags keep last-value-wins. Add `Clock::Unspecified = 0` as the default, which is the wire default for an absent Clock tag and what an empty parameter payload decodes to. An explicit Clock tag encoding zero remains an invalid enum value, so `Unspecified` is only ever reached by omission; `encode` omits the tag for it rather than emitting an explicit zero. It selects no clock, so `has_wall`/`has_mono` are false and no timestamp field is laid out. `ClientConfig::clock` is public and therefore now constructible as `Unspecified`, which is a wire default rather than a client mode. Reject it during config validation. The client default stays `Clock::Both`, so normal client-generated params and wire vectors are unchanged. --- crates/irtt-client/src/client/tests/config.rs | 10 + crates/irtt-client/src/config.rs | 4 + crates/irtt-client/src/session/machine.rs | 7 +- crates/irtt-client/tests/protocol_options.rs | 1 + .../tests/protocol_options/backend_smoke.rs | 3 + crates/irtt-proto/src/echo.rs | 9 +- crates/irtt-proto/src/layout.rs | 1 + crates/irtt-proto/src/lib.rs | 11 +- crates/irtt-proto/src/params.rs | 291 +++++++++++++++++- crates/irtt-proto/tests/directional_codecs.rs | 4 +- 10 files changed, 325 insertions(+), 16 deletions(-) diff --git a/crates/irtt-client/src/client/tests/config.rs b/crates/irtt-client/src/client/tests/config.rs index dcdcbee..07fe6d4 100644 --- a/crates/irtt-client/src/client/tests/config.rs +++ b/crates/irtt-client/src/client/tests/config.rs @@ -139,6 +139,16 @@ fn params_from_config_rejects_invalid_values() { }, "dscp", ), + ( + // The unspecified clock is the absent-tag wire default, not a + // client mode; a client that needs timestamps must name a clock. + "unspecified clock", + ClientConfig { + clock: Clock::Unspecified, + ..ClientConfig::default() + }, + "clock must be wall, monotonic, or both", + ), ( "empty server fill", ClientConfig { diff --git a/crates/irtt-client/src/config.rs b/crates/irtt-client/src/config.rs index cd69eb3..ca4505d 100644 --- a/crates/irtt-client/src/config.rs +++ b/crates/irtt-client/src/config.rs @@ -77,6 +77,10 @@ pub struct ClientConfig { /// Requested timestamp placement in echo replies. pub stamp_at: StampAt, /// Requested server clock sources for timestamp fields. + /// + /// [`Clock::Unspecified`] is the wire default meaning "no clock tag was + /// sent" and is not a client mode; opening a session with it is rejected. + /// Request [`Clock::Wall`], [`Clock::Monotonic`], or [`Clock::Both`]. pub clock: Clock, /// Requested DSCP codepoint. /// diff --git a/crates/irtt-client/src/session/machine.rs b/crates/irtt-client/src/session/machine.rs index 91c0255..a7a7775 100644 --- a/crates/irtt-client/src/session/machine.rs +++ b/crates/irtt-client/src/session/machine.rs @@ -1,7 +1,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use irtt_proto::{ - decode_echo_reply, echo_packet_len, encode_request, flags, EchoReply, OpenReply, Params, + decode_echo_reply, echo_packet_len, encode_request, flags, Clock, EchoReply, OpenReply, Params, RequestToEncode, ServerFill, TimestampFields, PROTOCOL_VERSION, }; @@ -989,6 +989,11 @@ fn validate_protocol_config(config: &ClientConfig) -> Result<(), ClientError> { reason: "interval must be greater than zero".to_owned(), }); } + if config.clock == Clock::Unspecified { + return Err(ClientError::InvalidConfig { + reason: "clock must be wall, monotonic, or both".to_owned(), + }); + } if config.dscp > MAX_DSCP_CODEPOINT { return Err(ClientError::InvalidConfig { reason: format!("dscp must be <= {MAX_DSCP_CODEPOINT}"), diff --git a/crates/irtt-client/tests/protocol_options.rs b/crates/irtt-client/tests/protocol_options.rs index 18b987b..c43ca36 100644 --- a/crates/irtt-client/tests/protocol_options.rs +++ b/crates/irtt-client/tests/protocol_options.rs @@ -121,6 +121,7 @@ fn clock_modes_drive_reply_event_clock_fields() { assert_timing_fields(reply.server_timing, true, true, false, false, true, true); assert_one_way_presence(reply.one_way, true, true); } + Clock::Unspecified => unreachable!("clients never request an unspecified clock"), } assert_processing_subtracted(reply); } diff --git a/crates/irtt-client/tests/protocol_options/backend_smoke.rs b/crates/irtt-client/tests/protocol_options/backend_smoke.rs index 3607b9e..ea89459 100644 --- a/crates/irtt-client/tests/protocol_options/backend_smoke.rs +++ b/crates/irtt-client/tests/protocol_options/backend_smoke.rs @@ -127,6 +127,9 @@ fn backend_clock_smoke() { assert!(st.send_wall_ns.is_some()); assert!(st.send_mono_ns.is_some()); } + Clock::Unspecified => { + unreachable!("clients never request an unspecified clock") + } } } other => panic!("expected EchoReply, got {other:?}"), diff --git a/crates/irtt-proto/src/echo.rs b/crates/irtt-proto/src/echo.rs index bcedbbe..a17731d 100644 --- a/crates/irtt-proto/src/echo.rs +++ b/crates/irtt-proto/src/echo.rs @@ -13,8 +13,13 @@ use crate::{ /// header. const MIDPOINT_COMPAT_EXTRA: usize = TIMESTAMP_SIZE; +/// Whether the negotiation selects a midpoint timestamp from exactly one clock, +/// which is the only case the upstream dual-field compatibility form applies to. +/// +/// [`Clock::Unspecified`] selects no clock and therefore lays out no midpoint +/// field at all, so it is not a single-clock negotiation. fn is_midpoint_single_clock(params: &Params) -> bool { - params.stamp_at == StampAt::Midpoint && !matches!(params.clock, Clock::Both) + params.stamp_at == StampAt::Midpoint && matches!(params.clock, Clock::Wall | Clock::Monotonic) } /// Total datagram length of the upstream dual-midpoint compatibility form. @@ -135,7 +140,7 @@ pub fn decode_echo_reply( match params.clock { Clock::Wall => (Some(wall), None), Clock::Monotonic => (None, Some(mono)), - Clock::Both => { + Clock::Unspecified | Clock::Both => { unreachable!("midpoint compat only applies to single-clock negotiations") } } diff --git a/crates/irtt-proto/src/layout.rs b/crates/irtt-proto/src/layout.rs index 02b41e3..e712f18 100644 --- a/crates/irtt-proto/src/layout.rs +++ b/crates/irtt-proto/src/layout.rs @@ -142,6 +142,7 @@ mod tests { ReceivedStats::Both => RECV_COUNT_SIZE + RECV_WINDOW_SIZE, }; let clock_count = match clock { + Clock::Unspecified => 0, Clock::Wall | Clock::Monotonic => 1, Clock::Both => 2, }; diff --git a/crates/irtt-proto/src/lib.rs b/crates/irtt-proto/src/lib.rs index 646c579..b1ebc17 100644 --- a/crates/irtt-proto/src/lib.rs +++ b/crates/irtt-proto/src/lib.rs @@ -12,6 +12,12 @@ //! performs no additional validation, so callers that build `Params` manually //! are responsible for validating those values before sending them. //! +//! Parameters are optional on the wire, and an absent tag takes its wire +//! default — zero for every integer field, and [`Clock::Unspecified`] for the +//! clock. A receiver that must distinguish an omitted tag from one explicitly +//! encoded as zero uses [`Params::decode_with_presence`], which runs the same +//! parser and additionally reports a [`ParamPresence`]. +//! //! The crate also provides packet layout calculation, packet encoding and //! decoding, and optional HMAC placement, computation, and verification //! helpers. @@ -51,7 +57,10 @@ pub use flags::*; pub use hmac::{compute_hmac, compute_hmac_in_place, verify_hmac, verify_packet_hmac}; pub use layout::{echo_header_len, echo_packet_len, PacketLayout}; pub use open::{decode_open_reply, encode_open_reply, OpenReply}; -pub use params::{Clock, Params, ReceivedStats, ServerFill, StampAt, MAX_SERVER_FILL_BYTES}; +pub use params::{ + Clock, DecodedParams, ParamPresence, Params, ReceivedStats, ServerFill, StampAt, + MAX_SERVER_FILL_BYTES, +}; pub use request::{ decode_request, encode_request, DecodedRequest, DecodedRequestKind, RequestToEncode, }; diff --git a/crates/irtt-proto/src/params.rs b/crates/irtt-proto/src/params.rs index 86ddcb3..6ead82d 100644 --- a/crates/irtt-proto/src/params.rs +++ b/crates/irtt-proto/src/params.rs @@ -11,6 +11,11 @@ pub const MAX_SERVER_FILL_BYTES: usize = 32; /// Higher-level callers should validate user or configuration input before /// encoding. The normal `irtt-client` configuration path enforces /// [`MAX_SERVER_FILL_BYTES`] for `server_fill`. +/// +/// [`Params::default`] is the **wire** default: every integer field is zero and +/// [`clock`](Params::clock) is [`Clock::Unspecified`], which is what an open +/// request with an empty parameter payload means. It is not a set of sensible +/// client settings; a client builds its request from its own configuration. #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct Params { pub protocol_version: i64, @@ -63,21 +68,67 @@ impl Params { /// /// This includes invalid enum values, malformed UTF-8, and `server_fill` /// values longer than [`MAX_SERVER_FILL_BYTES`]. + /// + /// Absent parameters take their wire default, so a caller cannot tell an + /// omitted tag from one explicitly encoded as zero. Use + /// [`decode_with_presence`](Params::decode_with_presence) when that + /// distinction matters; both share one parser. pub fn decode(input: &[u8]) -> Result { + Self::decode_with_presence(input).map(|decoded| decoded.params) + } + + /// Decodes parameters and additionally reports which known tags appeared. + /// + /// This is the same parser and the same validation as [`decode`], with the + /// presence of each known tag retained. A receiver needs it because the + /// protocol gives an omitted tag and an explicit zero the same value but + /// not the same meaning: an absent Duration or Interval is accepted as the + /// wire default zero, while one explicitly encoded as zero is invalid. + /// + /// Presence means the tag appeared at least once. Repeated known tags keep + /// last-value-wins, and unknown tags remain ignored and untracked. + /// + /// [`decode`]: Params::decode + pub fn decode_with_presence(input: &[u8]) -> Result { let mut params = Self::default(); + let mut presence = ParamPresence::default(); let mut pos = 0; while pos < input.len() { let (tag, used) = varint::decode_uvarint(&input[pos..])?; pos += used; match tag { - 1 => params.protocol_version = read_int(input, &mut pos)?, - 2 => params.duration_ns = read_int(input, &mut pos)?, - 3 => params.interval_ns = read_int(input, &mut pos)?, - 4 => params.length = read_int(input, &mut pos)?, - 5 => params.received_stats = ReceivedStats::try_from(read_int(input, &mut pos)?)?, - 6 => params.stamp_at = StampAt::try_from(read_int(input, &mut pos)?)?, - 7 => params.clock = Clock::try_from(read_int(input, &mut pos)?)?, - 8 => params.dscp = read_int(input, &mut pos)?, + 1 => { + params.protocol_version = read_int(input, &mut pos)?; + presence.protocol_version = true; + } + 2 => { + params.duration_ns = read_int(input, &mut pos)?; + presence.duration_ns = true; + } + 3 => { + params.interval_ns = read_int(input, &mut pos)?; + presence.interval_ns = true; + } + 4 => { + params.length = read_int(input, &mut pos)?; + presence.length = true; + } + 5 => { + params.received_stats = ReceivedStats::try_from(read_int(input, &mut pos)?)?; + presence.received_stats = true; + } + 6 => { + params.stamp_at = StampAt::try_from(read_int(input, &mut pos)?)?; + presence.stamp_at = true; + } + 7 => { + params.clock = Clock::try_from(read_int(input, &mut pos)?)?; + presence.clock = true; + } + 8 => { + params.dscp = read_int(input, &mut pos)?; + presence.dscp = true; + } 9 => { let (len, used) = varint::decode_uvarint(&input[pos..])?; pos += used; @@ -97,6 +148,7 @@ impl Params { .to_owned(); pos += len; params.server_fill = Some(ServerFill { value }); + presence.server_fill = true; } _ => { let (_, used) = varint::decode_uvarint(&input[pos..])?; @@ -104,10 +156,38 @@ impl Params { } } } - Ok(params) + Ok(DecodedParams { params, presence }) } } +/// Decoded parameters together with which known tags the payload carried. +/// +/// Produced by [`Params::decode_with_presence`]. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct DecodedParams { + /// The decoded values. Absent parameters hold their wire default. + pub params: Params, + /// Which known tags appeared in the payload. + pub presence: ParamPresence, +} + +/// Which known parameter tags a decoded payload carried. +/// +/// A field is `true` when its tag appeared at least once, whatever value it +/// carried. Unknown tags are ignored and are not represented here. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct ParamPresence { + pub protocol_version: bool, + pub duration_ns: bool, + pub interval_ns: bool, + pub length: bool, + pub received_stats: bool, + pub stamp_at: bool, + pub clock: bool, + pub dscp: bool, + pub server_fill: bool, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct ServerFill { pub value: String, @@ -179,12 +259,31 @@ impl TryFrom for StampAt { } } +/// Which server clock sources supply timestamp fields. +/// +/// # The zero value +/// +/// [`Clock::Unspecified`] is the **wire default**, meaning the Clock tag was +/// absent from an open parameter payload — which is valid, and is what an empty +/// payload decodes to. It does not mean a peer may send an explicit Clock tag +/// encoding zero: [`TryFrom`](Clock::try_from) still rejects an explicit +/// zero as an invalid enum value, so this state is only ever reached by +/// omission. [`encode`](Params::encode) omits the tag for `Unspecified` rather +/// than emitting an explicit zero, which round-trips absence faithfully. +/// +/// `Unspecified` selects no clock, so [`has_wall`](Clock::has_wall) and +/// [`has_mono`](Clock::has_mono) are both false for it and no timestamp field +/// is laid out. A client that wants timestamps must request a real clock; the +/// `irtt-client` configuration path rejects `Unspecified`. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] #[repr(i64)] pub enum Clock { + /// Wire default for an absent Clock tag. Never produced from an explicit + /// encoded value. + #[default] + Unspecified = 0, Wall = 1, Monotonic = 2, - #[default] Both = 3, } @@ -201,6 +300,11 @@ impl Clock { impl TryFrom for Clock { type Error = ProtoError; + /// Converts an **explicitly encoded** Clock value. + /// + /// Zero is rejected: it is only valid as an absent tag, never as an encoded + /// one. [`Clock::Unspecified`] is therefore unreachable through this + /// conversion by design. fn try_from(value: i64) -> Result { match value { 1 => Ok(Self::Wall), @@ -244,6 +348,13 @@ mod tests { encoded } + fn encode_int(tag: u64, value: i64) -> Vec { + let mut encoded = Vec::new(); + varint::encode_uvarint(tag, &mut encoded); + varint::encode_varint(value, &mut encoded); + encoded + } + #[test] fn params_round_trip() { let params = Params { @@ -452,6 +563,166 @@ mod tests { value: 0, }) ); + assert_eq!( + Params::decode_with_presence(&encoded), + Err(ProtoError::InvalidEnum { + name: "Clock", + value: 0, + }) + ); + } + + #[test] + fn explicit_clock_values_one_to_three_are_accepted() { + for (value, expected) in [(1, Clock::Wall), (2, Clock::Monotonic), (3, Clock::Both)] { + let decoded = Params::decode_with_presence(&encode_int(7, value)).unwrap(); + assert_eq!(decoded.params.clock, expected); + assert!(decoded.presence.clock); + } + } + + #[test] + fn empty_payload_decodes_to_wire_defaults_with_nothing_present() { + let decoded = Params::decode_with_presence(&[]).unwrap(); + + assert_eq!(decoded.params, Params::default()); + assert_eq!(decoded.params.protocol_version, 0); + assert_eq!(decoded.params.duration_ns, 0); + assert_eq!(decoded.params.interval_ns, 0); + assert_eq!(decoded.params.length, 0); + assert_eq!(decoded.params.dscp, 0); + assert_eq!(decoded.params.received_stats, ReceivedStats::None); + assert_eq!(decoded.params.stamp_at, StampAt::None); + // The wire default clock is zero, not `Both`. + assert_eq!(decoded.params.clock, Clock::Unspecified); + assert!(!decoded.params.clock.has_wall()); + assert!(!decoded.params.clock.has_mono()); + assert_eq!(decoded.params.server_fill, None); + + assert_eq!(decoded.presence, ParamPresence::default()); + assert_eq!( + decoded.presence, + ParamPresence { + protocol_version: false, + duration_ns: false, + interval_ns: false, + length: false, + received_stats: false, + stamp_at: false, + clock: false, + dscp: false, + server_fill: false, + } + ); + } + + #[test] + fn absent_and_explicitly_zero_parameters_share_a_value_but_not_a_presence() { + // Duration and Interval are the pair a server must tell apart: absent is + // accepted, explicitly zero is not. + let absent = Params::decode_with_presence(&encode_int(1, 1)).unwrap(); + assert_eq!(absent.params.duration_ns, 0); + assert!(!absent.presence.duration_ns); + assert_eq!(absent.params.interval_ns, 0); + assert!(!absent.presence.interval_ns); + assert_eq!(absent.params.clock, Clock::Unspecified); + assert!(!absent.presence.clock); + + let explicit_duration = Params::decode_with_presence(&encode_int(2, 0)).unwrap(); + assert_eq!(explicit_duration.params.duration_ns, 0); + assert!(explicit_duration.presence.duration_ns); + + let explicit_interval = Params::decode_with_presence(&encode_int(3, 0)).unwrap(); + assert_eq!(explicit_interval.params.interval_ns, 0); + assert!(explicit_interval.presence.interval_ns); + } + + #[test] + fn every_known_tag_reports_its_own_presence() { + let mut encoded = Vec::new(); + for (tag, value) in [ + (1, 1), + (2, 5), + (3, 6), + (4, 7), + (5, 3), + (6, 4), + (7, 3), + (8, 184), + ] { + encoded.extend_from_slice(&encode_int(tag, value)); + } + encoded.extend_from_slice(&encode_server_fill_value(b"rand")); + + let decoded = Params::decode_with_presence(&encoded).unwrap(); + assert_eq!( + decoded.presence, + ParamPresence { + protocol_version: true, + duration_ns: true, + interval_ns: true, + length: true, + received_stats: true, + stamp_at: true, + clock: true, + dscp: true, + server_fill: true, + } + ); + } + + #[test] + fn unspecified_clock_is_encoded_by_omission() { + let params = Params { + protocol_version: 1, + clock: Clock::Unspecified, + ..Params::default() + }; + let encoded = params.encode(); + + assert_eq!(encoded, encode_int(1, 1)); + assert!( + !encoded.windows(2).any(|bytes| bytes[0] == 7), + "an unspecified clock must omit the tag rather than encode an explicit zero" + ); + assert_round_trip(params); + } + + #[test] + fn decode_and_decode_with_presence_agree() { + let mut payloads = vec![ + Vec::new(), + encode_int(1, 1), + encode_int(2, 0), + encode_server_fill_value(b"rand"), + ]; + payloads.push( + Params { + protocol_version: 1, + duration_ns: 3_000_000_000, + interval_ns: 1_000_000_000, + length: 1472, + received_stats: ReceivedStats::Both, + stamp_at: StampAt::Both, + clock: Clock::Both, + dscp: 184, + server_fill: Some(ServerFill { + value: "rand".to_owned(), + }), + } + .encode(), + ); + // Malformed payloads must fail identically through both entry points. + payloads.push(vec![1, 0x80]); + payloads.push(encode_int(7, 0)); + + for payload in payloads { + assert_eq!( + Params::decode(&payload), + Params::decode_with_presence(&payload).map(|decoded| decoded.params), + "decode and decode_with_presence disagreed on {payload:02x?}" + ); + } } #[test] diff --git a/crates/irtt-proto/tests/directional_codecs.rs b/crates/irtt-proto/tests/directional_codecs.rs index b0cbd71..cfdd467 100644 --- a/crates/irtt-proto/tests/directional_codecs.rs +++ b/crates/irtt-proto/tests/directional_codecs.rs @@ -413,7 +413,7 @@ fn midpoint_dual_field_reply_is_accepted_wherever_length_identifies_it() { // The first wire field must not be mistaken for the // negotiated monotonic value. Clock::Monotonic => (None, Some(222)), - Clock::Both => unreachable!(), + Clock::Unspecified | Clock::Both => unreachable!(), }; assert_eq!( reply.timestamps.midpoint_wall, expected_wall, @@ -463,7 +463,7 @@ fn midpoint_dual_field_reply_of_equal_length_is_accepted_but_not_correctable() { assert_eq!(reply.timestamps.midpoint_wall, None); assert_eq!(reply.timestamps.midpoint_mono, Some(111)); } - Clock::Both => unreachable!(), + Clock::Unspecified | Clock::Both => unreachable!(), } } }