diff --git a/src/node/handlers/mmp.rs b/src/node/handlers/mmp.rs index 43c0af8a..f6f6c723 100644 --- a/src/node/handlers/mmp.rs +++ b/src/node/handlers/mmp.rs @@ -15,7 +15,7 @@ use crate::proto::mmp::{ LinkReportKind, LinkReportSnapshot, MmpAction, PeerLivenessSnapshot, ReceiverReport, RrLog, SenderReport, }; -use crate::proto::stp::ParentEval; +use crate::proto::stp::{Stp, TreeDecision}; use crate::transport::{TransportAddr, TransportId}; use std::time::{Duration, Instant}; use tracing::{debug, info, trace, warn}; @@ -256,78 +256,93 @@ impl Node { // Compute the flap-dampening / hold-down veto at the edge; a mandatory // switch bypasses it, a discretionary one is taken only if not suppressed. let switch_suppressed = self.tree_state.is_switch_suppressed(mono_now_ms); - let new_parent = match self - .tree_state - .evaluate_parent(&peer_costs, &std::collections::BTreeSet::new()) - { - ParentEval::Mandatory(p) => Some(p), - ParentEval::Discretionary(p) if !switch_suppressed => Some(p), - ParentEval::Discretionary(_) | ParentEval::None => None, - }; - if let Some(new_parent) = new_parent { - let new_seq = self.tree_state.my_declaration().sequence() + 1; - let flap_dampened = - self.tree_state - .set_parent(new_parent, new_seq, now_secs, mono_now_ms); - self.tree_state.recompute_coords(); - // Clone identity once: sign_declaration borrows &mut tree_state while - // the identity() accessor borrows all of &self, so an owned copy avoids - // the split-borrow conflict on this infrequent parent-switch path. - let our_identity = self.identity().clone(); - if let Err(e) = - sign_declaration(self.tree_state.my_declaration_mut(), &our_identity) - { - warn!(error = %e, "Failed to sign declaration after first-RTT parent eval"); - self.metrics() - .tree - .record_reject(TreeReject::OutboundSignFailed); - return; + match Stp::classify_periodic( + &self.tree_state, + &peer_costs, + &std::collections::BTreeSet::new(), + switch_suppressed, + ) { + TreeDecision::Switch { + new_parent, + new_seq, + } => { + let flap_dampened = + self.tree_state + .set_parent(new_parent, new_seq, now_secs, mono_now_ms); + self.tree_state.recompute_coords(); + // Clone identity once: sign_declaration borrows &mut tree_state while + // the identity() accessor borrows all of &self, so an owned copy avoids + // the split-borrow conflict on this infrequent parent-switch path. + let our_identity = self.identity().clone(); + if let Err(e) = + sign_declaration(self.tree_state.my_declaration_mut(), &our_identity) + { + warn!(error = %e, "Failed to sign declaration after first-RTT parent eval"); + self.metrics() + .tree + .record_reject(TreeReject::OutboundSignFailed); + return; + } + // Surgical invalidation — see CoordCache::invalidate_via_node doc. + self.coord_cache + .invalidate_via_node(our_identity.node_addr()); + self.reset_lookup_backoff(); + self.metrics().tree.parent_switches.inc(); + info!( + new_parent = %self.peer_display_name(&new_parent), + new_seq = new_seq, + new_root = %self.tree_state.root(), + depth = self.tree_state.my_coords().depth(), + trigger = "first-rtt", + "Parent switched after first RTT measurement" + ); + if flap_dampened { + self.note_flap("first-rtt"); + } + self.send_tree_announce_to_all().await; + let all_peers: Vec = self.peers.keys().copied().collect(); + self.bloom_state.mark_all_updates_needed(all_peers); } - // Surgical invalidation — see CoordCache::invalidate_via_node doc. - self.coord_cache - .invalidate_via_node(our_identity.node_addr()); - self.reset_lookup_backoff(); - self.metrics().tree.parent_switches.inc(); - info!( - new_parent = %self.peer_display_name(&new_parent), - new_seq = new_seq, - new_root = %self.tree_state.root(), - depth = self.tree_state.my_coords().depth(), - trigger = "first-rtt", - "Parent switched after first RTT measurement" - ); - if flap_dampened { - self.note_flap("first-rtt"); + TreeDecision::SelfRoot => { + self.tree_state.become_root(now_secs); + // Clone identity once (see the parent-switch branch above for why). + let our_identity = self.identity().clone(); + if let Err(e) = + sign_declaration(self.tree_state.my_declaration_mut(), &our_identity) + { + warn!(error = %e, "Failed to sign self-root declaration after first-RTT"); + self.metrics() + .tree + .record_reject(TreeReject::OutboundSignFailed); + return; + } + // Surgical invalidation — see CoordCache::invalidate_other_roots doc. + self.coord_cache + .invalidate_other_roots(our_identity.node_addr()); + self.reset_lookup_backoff(); + self.metrics().tree.parent_switches.inc(); + info!( + new_root = %self.tree_state.root(), + trigger = "first-rtt", + "Self-promoted to root after first RTT: smallest visible NodeAddr" + ); + self.send_tree_announce_to_all().await; + let all_peers: Vec = self.peers.keys().copied().collect(); + self.bloom_state.mark_all_updates_needed(all_peers); } - self.send_tree_announce_to_all().await; - let all_peers: Vec = self.peers.keys().copied().collect(); - self.bloom_state.mark_all_updates_needed(all_peers); - } else if !self.tree_state.is_root() && self.tree_state.should_be_root() { - self.tree_state.become_root(now_secs); - // Clone identity once (see the parent-switch branch above for why). - let our_identity = self.identity().clone(); - if let Err(e) = - sign_declaration(self.tree_state.my_declaration_mut(), &our_identity) - { - warn!(error = %e, "Failed to sign self-root declaration after first-RTT"); - self.metrics() - .tree - .record_reject(TreeReject::OutboundSignFailed); - return; + // Nothing changed. The periodic tick rebroadcasts; this path does not. + TreeDecision::PeriodicRebroadcast => {} + // classify_periodic never yields these: there is no announcing + // peer, so the loop-drop / ancestry-update arms cannot arise, and + // ParentLost is the removal drive's outcome. + TreeDecision::LoopDrop + | TreeDecision::AncestryUpdate { .. } + | TreeDecision::ParentLost + | TreeDecision::NoChange => { + unreachable!( + "classify_periodic yields only Switch / SelfRoot / PeriodicRebroadcast" + ) } - // Surgical invalidation — see CoordCache::invalidate_other_roots doc. - self.coord_cache - .invalidate_other_roots(our_identity.node_addr()); - self.reset_lookup_backoff(); - self.metrics().tree.parent_switches.inc(); - info!( - new_root = %self.tree_state.root(), - trigger = "first-rtt", - "Self-promoted to root after first RTT: smallest visible NodeAddr" - ); - self.send_tree_announce_to_all().await; - let all_peers: Vec = self.peers.keys().copied().collect(); - self.bloom_state.mark_all_updates_needed(all_peers); } } } diff --git a/src/node/tests/mmp_chartests.rs b/src/node/tests/mmp_chartests.rs index efd8db80..99beeb68 100644 --- a/src/node/tests/mmp_chartests.rs +++ b/src/node/tests/mmp_chartests.rs @@ -33,7 +33,7 @@ use crate::node::session::{EndToEndState, SessionEntry}; use crate::noise::HandshakeState; use crate::peer::ActivePeer; use crate::proto::mmp::{MmpMode, ReceiverReport}; -use crate::proto::stp::{ParentDeclaration, TreeCoordinate}; +use crate::proto::stp::{CoordEntry, ParentDeclaration, TreeCoordinate}; // =========================================================================== // Helpers @@ -437,3 +437,130 @@ async fn non_first_receiver_report_does_not_retrigger_tree() { "a non-first ReceiverReport does not re-enter the first-RTT tree branch" ); } + +/// Insert a peer whose NodeAddr is strictly larger than the node's own, with +/// link MMP but no RTT yet, aged so a crafted ReceiverReport yields a first +/// RTT sample. The caller registers the peer's tree position. +fn insert_larger_unmeasured_peer(node: &mut Node) -> NodeAddr { + let my_addr = *node.node_addr(); + let (identity, addr) = loop { + let id = make_peer_identity(); + let a = *id.node_addr(); + if a > my_addr { + break (id, a); + } + }; + let mut peer = ActivePeer::new(identity, LinkId::new(1), 0); + peer.test_init_mmp(MmpMode::Full); + peer.test_backdate_session_start(std::time::Duration::from_secs(10)); + node.peers.insert(addr, peer); + addr +} + +/// Sum of the tree-announce fan-out counters. Every attempt to send an +/// announce to a peer moves exactly one of them. +fn tree_announce_attempts(node: &Node) -> u64 { + let tree = &node.metrics().tree; + tree.sent.get() + tree.send_failed.get() + tree.rate_limited.get() +} + +/// A root node whose only peer has a larger address has nothing to change on +/// the first RTT sample: it stays root, records no switch, and sends no +/// TreeAnnounce. The periodic tick rebroadcasts; the first-RTT path does not. +#[tokio::test] +async fn first_rtt_on_a_root_with_only_larger_peers_sends_no_tree_announce() { + let mut node = make_node(); + let addr = insert_larger_unmeasured_peer(&mut node); + node.tree_state_mut().update_peer( + ParentDeclaration::self_root(addr, 1, 0), + TreeCoordinate::root(addr), + ); + + assert!( + node.tree_state().is_root(), + "precondition: node starts as its own root" + ); + let switches_before = node.metrics().tree.parent_switches.get(); + let attempts_before = tree_announce_attempts(&node); + + node.handle_receiver_report(&addr, &craft_rr_payload(10, 5, 500)) + .await; + + assert!( + node.get_peer(&addr).unwrap().has_srtt(), + "precondition: this report was the peer's first RTT sample" + ); + assert!(node.tree_state().is_root(), "node remains its own root"); + assert_eq!( + node.metrics().tree.parent_switches.get(), + switches_before, + "no parent switch is recorded" + ); + assert_eq!( + tree_announce_attempts(&node), + attempts_before, + "the first-RTT path does not rebroadcast an unchanged declaration" + ); +} + +/// A node holding a parent whose tree has since re-rooted at a larger address +/// than its own promotes itself to root on the first RTT sample. +#[tokio::test] +async fn first_rtt_self_promotes_when_no_visible_root_is_smaller() { + let mut node = make_node(); + let addr = insert_larger_unmeasured_peer(&mut node); + + // The peer first sits under a root smaller than us, and we take it as + // parent, so our root is that smaller node. + let far_root = NodeAddr::from_bytes([0u8; 16]); + assert!( + far_root < *node.node_addr(), + "precondition: the far root is smaller than the node" + ); + node.tree_state_mut().update_peer( + ParentDeclaration::new(addr, far_root, 1, 0), + TreeCoordinate::new(vec![ + CoordEntry::new(addr, 1, 0), + CoordEntry::new(far_root, 1, 0), + ]) + .unwrap(), + ); + let seq = node.tree_state().my_declaration().sequence() + 1; + node.tree_state_mut() + .set_parent(addr, seq, 0, crate::time::mono_ms()); + node.tree_state_mut().recompute_coords(); + assert_eq!( + node.tree_state().root(), + &far_root, + "precondition: the node sits under the far root" + ); + + // The peer then re-roots at itself, larger than us: no visible root is + // smaller than the node any more. + node.tree_state_mut().update_peer( + ParentDeclaration::self_root(addr, 2, 0), + TreeCoordinate::root(addr), + ); + assert!( + !node.tree_state().is_root() && node.tree_state().should_be_root(), + "precondition: not root, but should be" + ); + let switches_before = node.metrics().tree.parent_switches.get(); + + node.handle_receiver_report(&addr, &craft_rr_payload(10, 5, 500)) + .await; + + assert!( + node.get_peer(&addr).unwrap().has_srtt(), + "precondition: this report was the peer's first RTT sample" + ); + assert!( + node.tree_state().is_root(), + "the first-RTT path promoted the node to root" + ); + assert_eq!( + node.metrics().tree.parent_switches.get(), + switches_before + 1, + "the self-promotion is recorded as one parent switch" + ); +} diff --git a/src/proto/stp/core.rs b/src/proto/stp/core.rs index 8181e94f..04e2fffe 100644 --- a/src/proto/stp/core.rs +++ b/src/proto/stp/core.rs @@ -165,6 +165,8 @@ impl Stp { /// `classify_announce`, the periodic path has no same-parent loop-drop / /// ancestry-update arms — a periodic tick has no announcing peer, so those cases /// never arise; the no-change tail is a re-broadcast rather than a true no-op. + /// The first-RTT re-evaluation in `node::handlers::mmp` is driven by it too, + /// and ignores `PeriodicRebroadcast`. pub(crate) fn classify_periodic( tree: &TreeState, peer_costs: &BTreeMap, diff --git a/src/proto/stp/mod.rs b/src/proto/stp/mod.rs index 2c679e24..677bfd96 100644 --- a/src/proto/stp/mod.rs +++ b/src/proto/stp/mod.rs @@ -34,7 +34,11 @@ pub use crate::proto::coord::{CoordEntry, CoordError, TreeCoordinate}; pub(crate) use crate::proto::coord::{ coords_wire_size, decode_coords, decode_optional_coords, encode_coords, encode_empty_coords, }; -pub(crate) use core::{ParentEval, Stp, TreeDecision}; +// Callers outside this module take the decision from `Stp`; only the tests +// name the parent evaluation itself. +#[cfg(test)] +pub(crate) use core::ParentEval; +pub(crate) use core::{Stp, TreeDecision}; pub use declaration::ParentDeclaration; pub use state::TreeState; pub use wire::TreeAnnounce;