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!(), } } }