mirror of
https://github.com/jmcorgan/fips.git
synced 2026-07-30 19:46:15 +00:00
Error recovery fixes and routing error rate limiting
- PathBroken handler: convert to async, trigger re-discovery via maybe_initiate_lookup(), reset COORDS_PRESENT warmup counter (was a stub that only invalidated coord_cache) - CoordsRequired recovery timing: reset warmup counter in handle_lookup_response() when discovery completes for an established session, so COORDS_PRESENT packets fire after fresh coords are available (not just on CoordsRequired receipt) - Routing error rate limiting: add RoutingErrorRateLimiter (100ms per-destination, matching ICMP PTB pattern) to gate send_routing_error() at transit nodes - Remove root refresh dead code: the 1800s periodic root re-announcement in check_tree_state() only propagated to depth 1 (sequence-only changes don't cascade). Root loss detection relies on link failure propagation which works correctly.
This commit is contained in:
@@ -122,13 +122,12 @@ Controls flood-based node discovery (LookupRequest/LookupResponse).
|
||||
|
||||
### Spanning Tree (`node.tree.*`)
|
||||
|
||||
Controls tree construction, root announcement, and parent selection.
|
||||
Controls tree construction and parent selection.
|
||||
|
||||
| Parameter | Type | Default | Description |
|
||||
|-----------|------|---------|-------------|
|
||||
| `node.tree.root_refresh_secs` | u64 | `1800` | Root self-announcement interval (30 minutes) |
|
||||
| `node.tree.announce_min_interval_ms` | u64 | `500` | Per-peer TreeAnnounce rate limit |
|
||||
| `node.tree.parent_switch_threshold` | usize | `1` | Min depth improvement required to switch parents |
|
||||
| Parameter | Type | Default | Description |
|
||||
|----------------------------------------|-------|---------|--------------------------------------------------|
|
||||
| `node.tree.announce_min_interval_ms` | u64 | `500` | Per-peer TreeAnnounce rate limit |
|
||||
| `node.tree.parent_switch_threshold` | usize | `1` | Min depth improvement required to switch parents |
|
||||
|
||||
### Bloom Filter (`node.bloom.*`)
|
||||
|
||||
@@ -293,7 +292,6 @@ node:
|
||||
timeout_secs: 10
|
||||
recent_expiry_secs: 10
|
||||
tree:
|
||||
root_refresh_secs: 1800
|
||||
announce_min_interval_ms: 500
|
||||
parent_switch_threshold: 1
|
||||
bloom:
|
||||
|
||||
@@ -170,9 +170,6 @@ impl DiscoveryConfig {
|
||||
/// Spanning tree (`node.tree.*`).
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
pub struct TreeConfig {
|
||||
/// Root self-announcement interval in seconds (`node.tree.root_refresh_secs`).
|
||||
#[serde(default = "TreeConfig::default_root_refresh_secs")]
|
||||
pub root_refresh_secs: u64,
|
||||
/// Per-peer TreeAnnounce rate limit in ms (`node.tree.announce_min_interval_ms`).
|
||||
#[serde(default = "TreeConfig::default_announce_min_interval_ms")]
|
||||
pub announce_min_interval_ms: u64,
|
||||
@@ -184,7 +181,6 @@ pub struct TreeConfig {
|
||||
impl Default for TreeConfig {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
root_refresh_secs: 1800,
|
||||
announce_min_interval_ms: 500,
|
||||
parent_switch_threshold: 1,
|
||||
}
|
||||
@@ -192,7 +188,6 @@ impl Default for TreeConfig {
|
||||
}
|
||||
|
||||
impl TreeConfig {
|
||||
fn default_root_refresh_secs() -> u64 { 1800 }
|
||||
fn default_announce_min_interval_ms() -> u64 { 500 }
|
||||
fn default_parent_switch_threshold() -> usize { 1 }
|
||||
}
|
||||
|
||||
@@ -148,6 +148,22 @@ impl Node {
|
||||
// Clean up pending lookup tracking
|
||||
self.pending_lookups.remove(&target);
|
||||
|
||||
// If an established session exists, reset the warmup counter.
|
||||
// Discovery has completed and transit nodes along the response
|
||||
// path now have fresh coords. Reset warmup so the next N
|
||||
// DataPackets include COORDS_PRESENT to re-warm the forward path.
|
||||
if let Some(entry) = self.sessions.get_mut(&target)
|
||||
&& entry.is_established()
|
||||
{
|
||||
let n = self.config.node.session.coords_warmup_packets;
|
||||
entry.set_coords_warmup_remaining(n);
|
||||
debug!(
|
||||
dest = %target,
|
||||
warmup_packets = n,
|
||||
"Reset coords warmup after discovery for existing session"
|
||||
);
|
||||
}
|
||||
|
||||
// If we have pending TUN packets for this target, retry session
|
||||
// initiation. The coord_cache now has coords, so find_next_hop()
|
||||
// should succeed.
|
||||
|
||||
@@ -181,6 +181,11 @@ impl Node {
|
||||
/// If we can't route the error back to the source either, drop silently.
|
||||
/// No cascading errors.
|
||||
async fn send_routing_error(&mut self, original: &SessionDatagram) {
|
||||
// Rate limit: one error signal per destination per 100ms
|
||||
if !self.routing_error_rate_limiter.should_send(&original.dest_addr) {
|
||||
return;
|
||||
}
|
||||
|
||||
let my_addr = *self.node_addr();
|
||||
|
||||
let now_ms = std::time::SystemTime::now()
|
||||
|
||||
@@ -48,7 +48,7 @@ impl Node {
|
||||
self.handle_coords_required(inner).await;
|
||||
}
|
||||
Some(SessionMessageType::PathBroken) => {
|
||||
self.handle_path_broken(inner);
|
||||
self.handle_path_broken(inner).await;
|
||||
}
|
||||
None => {
|
||||
debug!(msg_type, "Unknown session message type");
|
||||
@@ -360,8 +360,9 @@ impl Node {
|
||||
/// Handle a PathBroken error signal from a transit router.
|
||||
///
|
||||
/// The router has coordinates but still can't route to the destination.
|
||||
/// Invalidate cached coordinates and consider re-discovery.
|
||||
fn handle_path_broken(&mut self, inner: &[u8]) {
|
||||
/// Invalidate cached coordinates, trigger re-discovery, and reset the
|
||||
/// COORDS_PRESENT warmup counter so the new path gets warmed.
|
||||
async fn handle_path_broken(&mut self, inner: &[u8]) {
|
||||
let msg = match PathBroken::decode(inner) {
|
||||
Ok(m) => m,
|
||||
Err(e) => {
|
||||
@@ -378,6 +379,21 @@ impl Node {
|
||||
|
||||
// Invalidate stale cached coordinates
|
||||
self.coord_cache.remove(&msg.dest_addr);
|
||||
|
||||
// Trigger re-discovery to get fresh coordinates
|
||||
self.maybe_initiate_lookup(&msg.dest_addr).await;
|
||||
|
||||
// Reset coords warmup counter so the next N packets include
|
||||
// COORDS_PRESENT, re-warming transit caches along the new path.
|
||||
if let Some(entry) = self.sessions.get_mut(&msg.dest_addr) {
|
||||
let n = self.config.node.session.coords_warmup_packets;
|
||||
entry.set_coords_warmup_remaining(n);
|
||||
debug!(
|
||||
dest = %msg.dest_addr,
|
||||
warmup_packets = n,
|
||||
"Reset coords warmup counter after PathBroken"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// === Session Initiation (Send Path) ===
|
||||
|
||||
+6
-6
@@ -9,6 +9,7 @@ mod handlers;
|
||||
mod lifecycle;
|
||||
mod retry;
|
||||
mod rate_limit;
|
||||
mod routing_error_rate_limit;
|
||||
pub(crate) mod session;
|
||||
pub(crate) mod wire;
|
||||
mod tree;
|
||||
@@ -21,6 +22,7 @@ use crate::utils::index::IndexAllocator;
|
||||
use crate::node::session::SessionEntry;
|
||||
use crate::peer::{ActivePeer, PeerConnection};
|
||||
use self::rate_limit::HandshakeRateLimiter;
|
||||
use self::routing_error_rate_limit::RoutingErrorRateLimiter;
|
||||
use crate::transport::{
|
||||
Link, LinkId, PacketRx, PacketTx, TransportAddr, TransportHandle, TransportId,
|
||||
};
|
||||
@@ -314,10 +316,8 @@ pub struct Node {
|
||||
msg1_rate_limiter: HandshakeRateLimiter,
|
||||
/// Rate limiter for ICMP Packet Too Big messages.
|
||||
icmp_rate_limiter: IcmpRateLimiter,
|
||||
|
||||
// === Tree Announce Timing ===
|
||||
/// Last time we refreshed our root announcement (Unix seconds).
|
||||
last_root_refresh_secs: u64,
|
||||
/// Rate limiter for routing error signals (CoordsRequired / PathBroken).
|
||||
routing_error_rate_limiter: RoutingErrorRateLimiter,
|
||||
|
||||
// === Connection Retry ===
|
||||
/// Retry state for peers whose outbound connections have failed.
|
||||
@@ -406,7 +406,7 @@ impl Node {
|
||||
pending_outbound: HashMap::new(),
|
||||
msg1_rate_limiter,
|
||||
icmp_rate_limiter: IcmpRateLimiter::new(),
|
||||
last_root_refresh_secs: 0,
|
||||
routing_error_rate_limiter: RoutingErrorRateLimiter::new(),
|
||||
retry_pending: HashMap::new(),
|
||||
})
|
||||
}
|
||||
@@ -482,7 +482,7 @@ impl Node {
|
||||
pending_outbound: HashMap::new(),
|
||||
msg1_rate_limiter,
|
||||
icmp_rate_limiter: IcmpRateLimiter::new(),
|
||||
last_root_refresh_secs: 0,
|
||||
routing_error_rate_limiter: RoutingErrorRateLimiter::new(),
|
||||
retry_pending: HashMap::new(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,138 @@
|
||||
//! Routing error signal rate limiting.
|
||||
//!
|
||||
//! Prevents routing error floods (CoordsRequired / PathBroken) by
|
||||
//! rate-limiting error signals per destination address at transit nodes.
|
||||
|
||||
use crate::NodeAddr;
|
||||
use std::collections::HashMap;
|
||||
use std::time::{Duration, Instant};
|
||||
|
||||
/// Rate limiter for routing error signals (CoordsRequired / PathBroken).
|
||||
///
|
||||
/// Tracks the last time a routing error was sent for each destination
|
||||
/// address and enforces a minimum interval to prevent floods.
|
||||
pub struct RoutingErrorRateLimiter {
|
||||
/// Maps destination NodeAddr to the last time we sent an error about it.
|
||||
last_sent: HashMap<NodeAddr, Instant>,
|
||||
/// Minimum interval between error signals for the same destination.
|
||||
min_interval: Duration,
|
||||
/// Maximum age of entries before cleanup.
|
||||
max_age: Duration,
|
||||
}
|
||||
|
||||
impl RoutingErrorRateLimiter {
|
||||
/// Create a new rate limiter.
|
||||
///
|
||||
/// Default: max 10 errors/sec per destination (100ms interval).
|
||||
pub fn new() -> Self {
|
||||
Self {
|
||||
last_sent: HashMap::new(),
|
||||
min_interval: Duration::from_millis(100),
|
||||
max_age: Duration::from_secs(10),
|
||||
}
|
||||
}
|
||||
|
||||
/// Check if we should send a routing error for this destination.
|
||||
///
|
||||
/// Returns true if enough time has passed since the last error for
|
||||
/// this destination, or if this is the first error. Updates internal
|
||||
/// state when returning true.
|
||||
pub fn should_send(&mut self, dest_addr: &NodeAddr) -> bool {
|
||||
let now = Instant::now();
|
||||
|
||||
if let Some(&last) = self.last_sent.get(dest_addr)
|
||||
&& now.duration_since(last) < self.min_interval
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
self.last_sent.insert(*dest_addr, now);
|
||||
self.cleanup(now);
|
||||
true
|
||||
}
|
||||
|
||||
/// Remove entries older than max_age.
|
||||
fn cleanup(&mut self, now: Instant) {
|
||||
self.last_sent
|
||||
.retain(|_, &mut last| now.duration_since(last) < self.max_age);
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub fn len(&self) -> usize {
|
||||
self.last_sent.len()
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
impl Default for RoutingErrorRateLimiter {
|
||||
fn default() -> Self {
|
||||
Self::new()
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use std::thread;
|
||||
|
||||
fn addr(val: u8) -> NodeAddr {
|
||||
let mut bytes = [0u8; 16];
|
||||
bytes[0] = val;
|
||||
NodeAddr::from_bytes(bytes)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_first_send_allowed() {
|
||||
let mut limiter = RoutingErrorRateLimiter::new();
|
||||
assert!(limiter.should_send(&addr(1)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_rapid_sends_rate_limited() {
|
||||
let mut limiter = RoutingErrorRateLimiter::new();
|
||||
assert!(limiter.should_send(&addr(1)));
|
||||
assert!(!limiter.should_send(&addr(1)));
|
||||
assert!(!limiter.should_send(&addr(1)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_different_destinations_independent() {
|
||||
let mut limiter = RoutingErrorRateLimiter::new();
|
||||
assert!(limiter.should_send(&addr(1)));
|
||||
assert!(limiter.should_send(&addr(2)));
|
||||
assert!(!limiter.should_send(&addr(1)));
|
||||
assert!(!limiter.should_send(&addr(2)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_send_allowed_after_interval() {
|
||||
let mut limiter = RoutingErrorRateLimiter::new();
|
||||
assert!(limiter.should_send(&addr(1)));
|
||||
|
||||
thread::sleep(Duration::from_millis(110));
|
||||
|
||||
assert!(limiter.should_send(&addr(1)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_cleanup_removes_old_entries() {
|
||||
let mut limiter = RoutingErrorRateLimiter::new();
|
||||
assert!(limiter.should_send(&addr(1)));
|
||||
assert!(limiter.should_send(&addr(2)));
|
||||
assert_eq!(limiter.len(), 2);
|
||||
|
||||
let future = Instant::now() + Duration::from_secs(11);
|
||||
limiter.cleanup(future);
|
||||
assert_eq!(limiter.len(), 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_cleanup_preserves_recent_entries() {
|
||||
let mut limiter = RoutingErrorRateLimiter::new();
|
||||
assert!(limiter.should_send(&addr(1)));
|
||||
assert_eq!(limiter.len(), 1);
|
||||
|
||||
limiter.cleanup(Instant::now());
|
||||
assert_eq!(limiter.len(), 1);
|
||||
}
|
||||
}
|
||||
+1
-29
@@ -9,8 +9,6 @@ use crate::NodeAddr;
|
||||
use super::{Node, NodeError};
|
||||
use tracing::{debug, info, warn};
|
||||
|
||||
// Root refresh interval is configurable via `node.tree.root_refresh_secs`.
|
||||
|
||||
impl Node {
|
||||
/// Build a TreeAnnounce from our current tree state.
|
||||
fn build_tree_announce(&self) -> Result<TreeAnnounce, NodeError> {
|
||||
@@ -257,35 +255,9 @@ impl Node {
|
||||
|
||||
/// Periodic tree maintenance, called from the tick handler.
|
||||
///
|
||||
/// - Sends pending rate-limited announces
|
||||
/// - Refreshes root announcement every ROOT_REFRESH_INTERVAL_SECS (if root)
|
||||
/// Sends pending rate-limited announces.
|
||||
pub(super) async fn check_tree_state(&mut self) {
|
||||
// Send any pending rate-limited announces
|
||||
self.send_pending_tree_announces().await;
|
||||
|
||||
// Root refresh
|
||||
if self.tree_state.is_root() && !self.peers.is_empty() {
|
||||
let now_secs = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.map(|d| d.as_secs())
|
||||
.unwrap_or(0);
|
||||
|
||||
let root_refresh_secs = self.config.node.tree.root_refresh_secs;
|
||||
if now_secs.saturating_sub(self.last_root_refresh_secs) >= root_refresh_secs {
|
||||
let new_seq = self.tree_state.my_declaration().sequence() + 1;
|
||||
self.tree_state
|
||||
.set_parent(*self.identity.node_addr(), new_seq, now_secs);
|
||||
if let Err(e) = self.tree_state.sign_declaration(&self.identity) {
|
||||
warn!(error = %e, "Failed to sign root refresh declaration");
|
||||
return;
|
||||
}
|
||||
self.tree_state.recompute_coords();
|
||||
self.last_root_refresh_secs = now_secs;
|
||||
|
||||
debug!(seq = new_seq, "Root refresh: announcing to all peers");
|
||||
self.send_tree_announce_to_all().await;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Handle tree state cleanup when a peer is removed.
|
||||
|
||||
Reference in New Issue
Block a user