From 1d8c22a91b24c94d53424066be2feef8e18a9d2a Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Fri, 2 Oct 2026 16:44:05 +0000 Subject: [PATCH] Share one hop-limit rule between the forwarding pre-check and the routing core The forwarding handler resolves a next hop only for datagrams the routing core can forward, which keeps the coordinate-cache touch in that resolution scoped to genuine forwards. Its TTL test (ttl > 1) restated the core's drop (decrement, then drop at zero) in a different form, and no test could see the two disagree: reverting the pre-check to its older ttl != 0 form passed every unit test. ttl_after_hop now holds the rule. The routing core drops on it, SessionDatagram::can_forward uses it, and a new SessionDatagramRef::can_forward is what the handler calls. A unit test pins the function, a routing test checks the core and can_forward agree for every TTL, and a handler test checks that a last-hop transit datagram does not refresh the destination's cached coordinates while a ttl=2 one does. --- src/node/dataplane/forwarding.rs | 13 ++++---- src/node/tests/forwarding.rs | 47 +++++++++++++++++++++++++++++ src/proto/link.rs | 52 +++++++++++++++++++++++++++++++- src/proto/routing/core.rs | 16 +++++----- src/proto/routing/tests/core.rs | 44 ++++++++++++++++++++++++++- 5 files changed, 155 insertions(+), 17 deletions(-) diff --git a/src/node/dataplane/forwarding.rs b/src/node/dataplane/forwarding.rs index 76164e66..2af800f7 100644 --- a/src/node/dataplane/forwarding.rs +++ b/src/node/dataplane/forwarding.rs @@ -55,13 +55,12 @@ impl Node { self.try_warm_coord_cache_ref(&datagram_ref, payload.len()); // Pre-resolve the next hop only for datagrams the core can actually - // forward: not locally destined, and carrying a TTL that survives the - // decrement (`ttl > 1` — the shell-side mirror of the core's - // would-leave-zero drop). This keeps `find_next_hop`'s coord-cache - // LRU-touch side effect scoped to genuine forwards, as it was when the - // TTL test ran inline ahead of it. Warming above has already run, so - // the resolution observes freshly cached coords. - let next_hop = if datagram_ref.dest_addr != my_addr && datagram_ref.ttl > 1 { + // forward: not locally destined, and passing `can_forward`, which is + // the core's own hop-limit rule. This keeps `find_next_hop`'s + // coord-cache LRU-touch side effect scoped to genuine forwards, as it + // was when the TTL test ran inline ahead of it. Warming above has + // already run, so the resolution observes freshly cached coords. + let next_hop = if datagram_ref.dest_addr != my_addr && datagram_ref.can_forward() { self.resolve_next_hop(&datagram_ref.dest_addr) } else { None diff --git a/src/node/tests/forwarding.rs b/src/node/tests/forwarding.rs index c092e2d8..80023fc9 100644 --- a/src/node/tests/forwarding.rs +++ b/src/node/tests/forwarding.rs @@ -155,6 +155,53 @@ async fn test_forwarding_ttl_two_transit_clears_the_gate() { ); } +/// The next hop is resolved only for a datagram the core can forward, and that +/// resolution refreshes the destination's cached coordinates. A last-hop +/// transit datagram (ttl=1) is dropped by the core, so it must not refresh +/// them; a ttl=2 datagram reaches the resolution and does. +#[tokio::test] +async fn test_forwarding_last_hop_transit_does_not_refresh_destination_coords() { + let mut node = make_node(); + let from = make_node_addr(0xAA); + let src = make_node_addr(0x01); + let dest = make_node_addr(0x02); + let root = make_node_addr(0xF0); + let coords = TreeCoordinate::from_addrs(vec![dest, root]).unwrap(); + + let now_ms = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis() as u64; + let stamped_ms = now_ms - node.coord_cache().default_ttl_ms() / 2; + node.coord_cache_mut() + .insert_verified(dest, coords, stamped_ms); + let last_used = |node: &Node| node.coord_cache().get_entry(&dest).unwrap().last_used(); + assert_eq!( + last_used(&node), + stamped_ms, + "precondition: the entry carries the past stamp" + ); + + for ttl in [1u8, 2] { + let dg = SessionDatagram::new(src, dest, vec![0x10, 0x00, 0x00, 0x00]).with_ttl(ttl); + let encoded = dg.encode(); + node.handle_session_datagram(&from, &encoded[1..], false) + .await; + if ttl == 1 { + assert_eq!( + last_used(&node), + stamped_ms, + "a transit ttl=1 datagram is dropped, so it must not refresh the destination's coords" + ); + } else { + assert!( + last_used(&node) > stamped_ms, + "a transit ttl=2 datagram reaches next-hop resolution, which refreshes the coords" + ); + } + } +} + // --- Local delivery --- #[tokio::test] diff --git a/src/proto/link.rs b/src/proto/link.rs index ebc4d26b..7c982fa4 100644 --- a/src/proto/link.rs +++ b/src/proto/link.rs @@ -144,6 +144,20 @@ pub struct SessionDatagramRef<'a> { pub payload: &'a [u8], } +/// The TTL a transit datagram leaves this node with, or `None` when it may not +/// be transmitted because it would leave with zero. +/// +/// Follows IP semantics: the decrement comes first, and `saturating_sub` folds +/// an already-exhausted arrival (TTL 0) into the same outcome as a last-hop +/// arrival (TTL 1). This is the one rule behind the routing core's hop-limit +/// drop and both `can_forward`s. +pub(crate) fn ttl_after_hop(ttl: u8) -> Option { + match ttl.saturating_sub(1) { + 0 => None, + left => Some(left), + } +} + /// SessionDatagram fixed header size: msg_type(1) + ttl(1) + path_mtu(2) + src_addr(16) + dest_addr(16). pub const SESSION_DATAGRAM_HEADER_SIZE: usize = 36; @@ -191,7 +205,7 @@ impl SessionDatagram { /// True only at TTL 2 or more: at TTL 1 the decrement leaves zero, so the /// datagram is dropped rather than forwarded. pub fn can_forward(&self) -> bool { - self.ttl > 1 + ttl_after_hop(self.ttl).is_some() } /// Encode as link-layer message (msg_type + ttl + path_mtu + src_addr + dest_addr + payload). @@ -239,6 +253,12 @@ impl<'a> SessionDatagramRef<'a> { }) } + /// Check whether this datagram would survive a transit hop, by the same + /// rule the routing core drops on (`ttl_after_hop`). + pub fn can_forward(&self) -> bool { + ttl_after_hop(self.ttl).is_some() + } + /// Materialize an owned datagram for forwarding/re-encoding paths. pub fn into_owned(self) -> SessionDatagram { SessionDatagram { @@ -424,6 +444,36 @@ mod tests { assert!(dg.with_ttl(255).can_forward()); } + #[test] + fn ttl_after_hop_drops_only_what_would_leave_at_zero() { + assert_eq!( + ttl_after_hop(0), + None, + "an exhausted arrival leaves at zero" + ); + assert_eq!(ttl_after_hop(1), None, "a last-hop arrival leaves at zero"); + assert_eq!(ttl_after_hop(2), Some(1)); + assert_eq!(ttl_after_hop(255), Some(254)); + + let dg = SessionDatagram::new(make_node_addr(1), make_node_addr(2), vec![0x42]); + for ttl in 0..=u8::MAX { + assert_eq!( + ttl_after_hop(ttl).is_some(), + ttl > 1, + "ttl={ttl}: only a TTL of 2 or more survives the hop" + ); + let owned = dg.clone().with_ttl(ttl); + let encoded = owned.encode(); + let view = SessionDatagramRef::decode(&encoded[1..]).unwrap(); + assert_eq!( + view.can_forward(), + owned.can_forward(), + "ttl={ttl}: the borrowed and owned views must agree" + ); + assert_eq!(view.can_forward(), ttl_after_hop(ttl).is_some()); + } + } + #[test] fn test_session_datagram_decrement_ttl() { let base = SessionDatagram::new(make_node_addr(1), make_node_addr(2), vec![0x42]); diff --git a/src/proto/routing/core.rs b/src/proto/routing/core.rs index e73caa72..44215a25 100644 --- a/src/proto/routing/core.rs +++ b/src/proto/routing/core.rs @@ -17,7 +17,7 @@ use super::limits::LimitVerdict; use super::state::Router; use super::wire::{CoordsRequired, MtuExceeded, PathBroken}; -use crate::proto::link::{SessionDatagram, SessionDatagramRef}; +use crate::proto::link::{SessionDatagram, SessionDatagramRef, ttl_after_hop}; use crate::{NodeAddr, TreeCoordinate}; /// Read-only view of routing state the routing core needs. @@ -112,7 +112,8 @@ impl Router { /// datagram that would leave with a TTL of zero is not transmitted. /// /// The shell pre-resolves `next_hop` only for datagrams this can actually - /// forward (dest not local and TTL surviving the decrement), so + /// forward (dest not local, and `SessionDatagramRef::can_forward`, which + /// applies the same [`ttl_after_hop`] rule this drops on), so /// `find_next_hop`'s LRU-touch side effect stays scoped to genuine /// forwards. `route` still re-checks local delivery and the TTL /// authoritatively. @@ -132,15 +133,14 @@ impl Router { } // TTL enforcement on the transit path: decrement first, then drop if - // the datagram would leave with a TTL of zero. `saturating_sub` folds - // the already-exhausted arrival (ttl=0) into the same test as the - // last-hop arrival (ttl=1); neither is transmitted. - let forwarded_ttl = dg.ttl.saturating_sub(1); - if forwarded_ttl == 0 { + // the datagram would leave with a TTL of zero. The already-exhausted + // arrival (ttl=0) and the last-hop arrival (ttl=1) are both dropped; + // neither is transmitted. + let Some(forwarded_ttl) = ttl_after_hop(dg.ttl) else { return RouteOutcome::Drop { reason: DropReason::TtlExhausted, }; - } + }; let nh = match next_hop { Some(nh) => nh, diff --git a/src/proto/routing/tests/core.rs b/src/proto/routing/tests/core.rs index 75d25bab..7c9f3174 100644 --- a/src/proto/routing/tests/core.rs +++ b/src/proto/routing/tests/core.rs @@ -1,7 +1,7 @@ //! Tests for the sans-IO routing decision core. use super::util::{MockPeer, MockRoutingView, make_coords, make_datagram_ref, make_next_hop}; -use crate::proto::link::SessionDatagramRef; +use crate::proto::link::{SessionDatagramRef, ttl_after_hop}; use crate::proto::routing::RoutingSignalType; use crate::proto::routing::{ DropReason, LimitVerdict, RouteAction, RouteOutcome, Router, RoutingView, select_best_candidate, @@ -514,3 +514,45 @@ fn synth_mtu_exceeded_rate_limit_gate_suppresses_second_call() { assert!(third.action.is_some()); assert_eq!(third.verdict, LimitVerdict::Admit); } + +/// The routing core's hop-limit drop and the shell's `can_forward` pre-check +/// agree for every TTL: a transit datagram with a next hop is dropped as +/// TTL-exhausted exactly when `can_forward` is false, and otherwise leaves +/// with the TTL `ttl_after_hop` gives. +#[test] +fn route_drops_for_hop_limit_exactly_when_can_forward_is_false() { + let my_addr = make_node_addr(0x10); + let nh_addr = make_node_addr(0x30); + let rv = MockRoutingView::new(false); + for ttl in 0..=u8::MAX { + let mut router = Router::new(); + let dg = make_datagram_ref(ttl, make_node_addr(0x20)); + let out = router.route( + &dg, + &my_addr, + false, + Some(make_next_hop(nh_addr, 1400)), + &rv, + ); + match out { + RouteOutcome::Drop { + reason: DropReason::TtlExhausted, + } => assert!( + !dg.can_forward(), + "ttl={ttl}: the core dropped a datagram the pre-check would forward" + ), + RouteOutcome::Forward { bytes, .. } => { + assert!( + dg.can_forward(), + "ttl={ttl}: the core forwarded a datagram the pre-check would not" + ); + assert_eq!( + Some(decode_forward(&bytes).ttl), + ttl_after_hop(ttl), + "ttl={ttl}: the forwarded TTL must be the shared rule's" + ); + } + _ => panic!("ttl={ttl}: expected Drop(TtlExhausted) or Forward"), + } + } +}