mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 19:18:25 +00:00
Drive the first-RTT parent re-evaluation from the spanning-tree core's periodic classification
The first-RTT branch of the ReceiverReport handler carried its own copy of the parent-switch / self-root ladder, which already lives in Stp::classify_periodic and drives the periodic re-evaluation. The handler now matches on that classification, so the two paths cannot drift apart. The arm bodies, log lines and metrics are unchanged. The first-RTT path ignores the periodic rebroadcast outcome: the periodic tick is what rebroadcasts an unchanged declaration. Two characterization tests pin the paths no test reached before: a root whose only peer is larger sends no TreeAnnounce on the first RTT sample, and a node whose visible roots are all larger than itself promotes itself to root.
This commit is contained in:
Johnathan Corgan
+85
-70
@@ -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<crate::NodeAddr> = 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<crate::NodeAddr> = 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<crate::NodeAddr> = 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<crate::NodeAddr> = self.peers.keys().copied().collect();
|
||||
self.bloom_state.mark_all_updates_needed(all_peers);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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<NodeAddr, f64>,
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user