From a382b1793142f1fdeff5b5f0e184f7cb39fe9f20 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 18 Jul 2026 21:24:03 +0000 Subject: [PATCH 1/7] node: seed control machines directly in tests, ahead of removing the leg Tests built a free-standing PeerConnection, mutated it, and handed it to Node::add_connection by value. None of those sites survives the removal of PeerConnection, so converting them afterwards would mean one enormous commit that cannot be reviewed honestly. Convert them now, while the struct still exists and the conversion can be validated against a green tree. Adds a cfg(test) Node::seed_handshake_machine plus a HandshakeSeed builder, and rewrites make_completed_connection (now seed_completed_connection) and the twenty inline builders onto it. add_connection keeps its body and loses its test callers. The builder's carrier seeding is a verbatim copy of add_connection's: the two conditional writes for their_index and transport_id, then set_leg, through the same entry().or_insert_with() so an existing leg-less machine keeps its constructor-side fields. Nothing else reaches the carrier -- our_index, source_addr, post-construction started_at and the stored handshake bytes stay leg-only. Seeding more than that would let these tests observe a carrier richer than production's and keep passing even if a later production write-lift were missed. The Noise exchange now runs on the already-seeded leg rather than before the hand-over. That is neutral because the only read of expected_identity is guarded by is_outbound, and no crypto method allocates a session index. Also adds a compile-time check that PeerAction is Clone + Eq, which is what keeps a runtime handle from being smuggled into an action payload. --- src/node/mod.rs | 142 ++++++++++++++++++++++++++ src/node/tests/acl.rs | 21 ++-- src/node/tests/bootstrap.rs | 3 +- src/node/tests/decrypt_failure.rs | 3 +- src/node/tests/establish_chartests.rs | 97 +++++++++++------- src/node/tests/handshake.rs | 125 +++++++++++++++-------- src/node/tests/mod.rs | 54 ++++++---- src/node/tests/routing.rs | 37 +++---- src/node/tests/session.rs | 4 +- src/node/tests/spanning_tree.rs | 24 +++-- src/node/tests/unit.rs | 113 ++++++++++---------- src/peer/machine.rs | 19 ++++ 12 files changed, 436 insertions(+), 206 deletions(-) diff --git a/src/node/mod.rs b/src/node/mod.rs index ea038c9..9799b02 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -2463,6 +2463,77 @@ impl Node { Ok(()) } + /// Test-support: seed a control machine for `seed.link_id` the way + /// [`Node::add_connection`] does, without the caller having to build a + /// free-standing leg first. + /// + /// The carrier seeding below is a verbatim copy of `add_connection`'s: the + /// two conditional writes (`their_index`, `transport_id`) and `set_leg`, + /// built through the same `entry(..).or_insert_with(..)` so an existing + /// leg-less machine keeps its constructor-side fields. Nothing else is + /// written to the carrier — `our_index`, `source_addr`, post-construction + /// `started_at`, and the stored handshake bytes stay leg-only, exactly as + /// they do for a test that goes through `add_connection` today. + /// + /// The duplication is deliberate: keeping the carrier writes visible here + /// is what lets each later step of the leg dissolution revise them at a + /// single reviewable site. The two bodies must stay in sync until + /// `add_connection` itself is removed; any drift surfaces as a test + /// failure while both paths still exist. + #[cfg(test)] + pub(crate) fn seed_handshake_machine(&mut self, seed: HandshakeSeed) -> Result<(), NodeError> { + let link_id = seed.link_id; + + let mut connection = match seed.expected_identity { + Some(identity) => PeerConnection::outbound(link_id, identity, seed.started_at_ms), + None => PeerConnection::inbound(link_id, seed.started_at_ms), + }; + if let Some(id) = seed.transport_id { + connection.set_transport_id(id); + } + if let Some(addr) = seed.source_addr { + connection.set_source_addr(addr); + } + if let Some(index) = seed.our_index { + connection.set_our_index(index); + } + if let Some(index) = seed.their_index { + connection.set_their_index(index); + } + + if self + .peer_machines + .get(&link_id) + .is_some_and(|machine| machine.leg().is_some()) + { + return Err(NodeError::ConnectionAlreadyExists(link_id)); + } + + if self.max_connections() > 0 && self.connection_count() >= self.max_connections() { + return Err(NodeError::MaxConnectionsExceeded { + max: self.max_connections(), + }); + } + + let machine = self.peer_machines.entry(link_id).or_insert_with(|| { + let now = connection.started_at(); + match connection.expected_identity() { + Some(identity) if connection.is_outbound() => { + PeerMachine::new_outbound(link_id, *identity, now) + } + _ => PeerMachine::new_inbound(link_id, now), + } + }); + if let Some(their) = connection.their_index() { + machine.set_conn_their_index(their); + } + if let Some(tid) = connection.transport_id() { + machine.set_conn_transport_id(tid); + } + machine.set_leg(connection); + Ok(()) + } + /// Get a connection by LinkId. pub fn get_connection(&self, link_id: &LinkId) -> Option<&PeerConnection> { self.leg(link_id) @@ -3183,3 +3254,74 @@ impl fmt::Debug for Node { .finish() } } + +/// Test-support seed spec for [`Node::seed_handshake_machine`]. +/// +/// Mirrors the leg constructors plus the setters tests apply to a connection +/// *before* handing it to `add_connection`. Only fields that exist on the leg +/// at add time belong here; crypto is run afterwards through +/// `get_connection_mut`, which `add_connection` never reads. +#[cfg(test)] +#[derive(Debug, Clone)] +pub(crate) struct HandshakeSeed { + link_id: LinkId, + expected_identity: Option, + started_at_ms: u64, + transport_id: Option, + source_addr: Option, + our_index: Option, + their_index: Option, +} + +#[cfg(test)] +impl HandshakeSeed { + /// Outbound leg: we know who we are dialing. + pub(crate) fn outbound( + link_id: LinkId, + expected_identity: PeerIdentity, + started_at_ms: u64, + ) -> Self { + Self { + link_id, + expected_identity: Some(expected_identity), + started_at_ms, + transport_id: None, + source_addr: None, + our_index: None, + their_index: None, + } + } + + /// Inbound leg: identity is unknown until msg1 decrypts. + pub(crate) fn inbound(link_id: LinkId, started_at_ms: u64) -> Self { + Self { + link_id, + expected_identity: None, + started_at_ms, + transport_id: None, + source_addr: None, + our_index: None, + their_index: None, + } + } + + pub(crate) fn with_transport_id(mut self, transport_id: TransportId) -> Self { + self.transport_id = Some(transport_id); + self + } + + pub(crate) fn with_source_addr(mut self, source_addr: TransportAddr) -> Self { + self.source_addr = Some(source_addr); + self + } + + pub(crate) fn with_our_index(mut self, index: crate::utils::index::SessionIndex) -> Self { + self.our_index = Some(index); + self + } + + pub(crate) fn with_their_index(mut self, index: crate::utils::index::SessionIndex) -> Self { + self.their_index = Some(index); + self + } +} diff --git a/src/node/tests/acl.rs b/src/node/tests/acl.rs index b28c77a..1ce5dbc 100644 --- a/src/node/tests/acl.rs +++ b/src/node/tests/acl.rs @@ -84,14 +84,22 @@ async fn test_outbound_msg2_denied_after_acl_reload() { let peer_b_identity = PeerIdentity::from_pubkey_full(node_b.identity().pubkey_full()); let link_id_a = node_a.allocate_link_id(); - let mut conn_a = PeerConnection::outbound(link_id_a, peer_b_identity, 1000); let our_index_a = node_a.index_allocator.allocate().unwrap(); - let noise_msg1 = conn_a - .start_handshake(node_a.identity().keypair(), node_a.startup_epoch(), 1000) + node_a + .seed_handshake_machine( + HandshakeSeed::outbound(link_id_a, peer_b_identity, 1000) + .with_our_index(our_index_a) + .with_transport_id(transport_id) + .with_source_addr(remote_addr.clone()), + ) + .unwrap(); + let keypair_a = node_a.identity().keypair(); + let epoch_a = node_a.startup_epoch(); + let noise_msg1 = node_a + .get_connection_mut(&link_id_a) + .unwrap() + .start_handshake(keypair_a, epoch_a, 1000) .unwrap(); - conn_a.set_our_index(our_index_a); - conn_a.set_transport_id(transport_id); - conn_a.set_source_addr(remote_addr.clone()); let link_a = Link::connectionless( link_id_a, @@ -104,7 +112,6 @@ async fn test_outbound_msg2_denied_after_acl_reload() { node_a .addr_to_link .insert((transport_id, remote_addr.clone()), link_id_a); - node_a.add_connection(conn_a).unwrap(); node_a .pending_outbound .insert((transport_id, our_index_a.as_u32()), link_id_a); diff --git a/src/node/tests/bootstrap.rs b/src/node/tests/bootstrap.rs index 69d3b20..459181b 100644 --- a/src/node/tests/bootstrap.rs +++ b/src/node/tests/bootstrap.rs @@ -127,9 +127,8 @@ async fn test_adopted_traversal_skips_already_connected_peer() { let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, peer_identity) = make_completed_connection(&mut node, link_id, transport_id, 1_000); + let peer_identity = seed_completed_connection(&mut node, link_id, transport_id, 1_000); let peer_node_addr = *peer_identity.node_addr(); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, peer_identity, 2_000) .unwrap(); diff --git a/src/node/tests/decrypt_failure.rs b/src/node/tests/decrypt_failure.rs index 8509339..b590be3 100644 --- a/src/node/tests/decrypt_failure.rs +++ b/src/node/tests/decrypt_failure.rs @@ -28,10 +28,9 @@ fn test_decrypt_failure_threshold_removes_peer() { // Build a fully-promoted active peer with our_index/transport_id set // so peers_by_index is populated by promote_connection. - let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 1_000); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 1_000); let node_addr = *identity.node_addr(); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, identity, 2_000).unwrap(); // Sanity: peer is registered and indexed. diff --git a/src/node/tests/establish_chartests.rs b/src/node/tests/establish_chartests.rs index 3acc53e..995e887 100644 --- a/src/node/tests/establish_chartests.rs +++ b/src/node/tests/establish_chartests.rs @@ -192,8 +192,6 @@ async fn chartest_msg1_duplicate_pending_resends_stored_msg2() { // A pending inbound connection with a stored msg2, keyed in addr_to_link, // NOT promoted to an active peer. let link_id = node.allocate_link_id(); - let conn = - PeerConnection::inbound_with_transport(link_id, transport_id, peer_addr.clone(), 1000); let stored_msg2 = vec![0xC1, 0xC2, 0xC3, 0xC4, 0xC5]; let link = Link::connectionless( link_id, @@ -205,7 +203,12 @@ async fn chartest_msg1_duplicate_pending_resends_stored_msg2() { node.links.insert(link_id, link); node.addr_to_link .insert((transport_id, peer_addr.clone()), link_id); - node.add_connection(conn).unwrap(); + node.seed_handshake_machine( + HandshakeSeed::inbound(link_id, 1000) + .with_transport_id(transport_id) + .with_source_addr(peer_addr.clone()), + ) + .unwrap(); // The stored msg2 lives on the control machine's carrier (the resend source // for a duplicate msg1 while pending), mirroring the inbound establish path. node.peer_machines @@ -327,15 +330,21 @@ async fn chartest_msg1_inbound_promote_defers_pending_outbound_to_same_identity( // different source address. let out_link = node.allocate_link_id(); let out_addr = TransportAddr::from_string("10.0.0.9:2121"); - let mut out_conn = PeerConnection::outbound(out_link, sender_pid, 1000); - let our_keypair = node.identity().keypair(); - let _ = out_conn - .start_handshake(our_keypair, node.startup_epoch(), 1000) - .unwrap(); let out_index = node.index_allocator.allocate().unwrap(); - out_conn.set_our_index(out_index); - out_conn.set_transport_id(transport_id); - out_conn.set_source_addr(out_addr.clone()); + node.seed_handshake_machine( + HandshakeSeed::outbound(out_link, sender_pid, 1000) + .with_our_index(out_index) + .with_transport_id(transport_id) + .with_source_addr(out_addr.clone()), + ) + .unwrap(); + let our_keypair = node.identity().keypair(); + let startup_epoch = node.startup_epoch(); + let _ = node + .get_connection_mut(&out_link) + .unwrap() + .start_handshake(our_keypair, startup_epoch, 1000) + .unwrap(); let out_l = Link::connectionless( out_link, transport_id, @@ -346,7 +355,6 @@ async fn chartest_msg1_inbound_promote_defers_pending_outbound_to_same_identity( node.links.insert(out_link, out_l); node.addr_to_link .insert((transport_id, out_addr.clone()), out_link); - node.add_connection(out_conn).unwrap(); node.pending_outbound .insert((transport_id, out_index.as_u32()), out_link); assert_eq!(node.peer_count(), 0); @@ -402,16 +410,21 @@ async fn chartest_msg1_at_cap_with_pending_outbound_bypasses_early_gate() { // `has_pending_outbound_to_peer`, which turns off the early silent-drop. let out_link = node.allocate_link_id(); let out_addr = TransportAddr::from_string("10.0.0.9:2121"); - let mut out_conn = PeerConnection::outbound(out_link, sender_pid, 1000); - let our_keypair = node.identity().keypair(); - let _ = out_conn - .start_handshake(our_keypair, node.startup_epoch(), 1000) - .unwrap(); let out_index = node.index_allocator.allocate().unwrap(); - out_conn.set_our_index(out_index); - out_conn.set_transport_id(transport_id); - out_conn.set_source_addr(out_addr.clone()); - node.add_connection(out_conn).unwrap(); + node.seed_handshake_machine( + HandshakeSeed::outbound(out_link, sender_pid, 1000) + .with_our_index(out_index) + .with_transport_id(transport_id) + .with_source_addr(out_addr.clone()), + ) + .unwrap(); + let our_keypair = node.identity().keypair(); + let startup_epoch = node.startup_epoch(); + let _ = node + .get_connection_mut(&out_link) + .unwrap() + .start_handshake(our_keypair, startup_epoch, 1000) + .unwrap(); node.pending_outbound .insert((transport_id, out_index.as_u32()), out_link); @@ -499,14 +512,22 @@ async fn chartest_cross_connection_tiebreak_winner_and_loser() { // A initiates to B. let link_a_out = node_a.allocate_link_id(); - let mut conn_a = PeerConnection::outbound(link_a_out, peer_b_identity, 1000); let out_index_a = node_a.index_allocator.allocate().unwrap(); - let noise_msg1_a = conn_a - .start_handshake(node_a.identity().keypair(), node_a.startup_epoch(), 1000) + node_a + .seed_handshake_machine( + HandshakeSeed::outbound(link_a_out, peer_b_identity, 1000) + .with_our_index(out_index_a) + .with_transport_id(transport_id_a) + .with_source_addr(remote_addr_b.clone()), + ) + .unwrap(); + let keypair_a = node_a.identity().keypair(); + let epoch_a = node_a.startup_epoch(); + let noise_msg1_a = node_a + .get_connection_mut(&link_a_out) + .unwrap() + .start_handshake(keypair_a, epoch_a, 1000) .unwrap(); - conn_a.set_our_index(out_index_a); - conn_a.set_transport_id(transport_id_a); - conn_a.set_source_addr(remote_addr_b.clone()); let wire_msg1_a = build_msg1(out_index_a, &noise_msg1_a); node_a.links.insert( link_a_out, @@ -521,21 +542,28 @@ async fn chartest_cross_connection_tiebreak_winner_and_loser() { node_a .addr_to_link .insert((transport_id_a, remote_addr_b.clone()), link_a_out); - node_a.add_connection(conn_a).unwrap(); node_a .pending_outbound .insert((transport_id_a, out_index_a.as_u32()), link_a_out); // B initiates to A. let link_b_out = node_b.allocate_link_id(); - let mut conn_b = PeerConnection::outbound(link_b_out, peer_a_identity, 1000); let out_index_b = node_b.index_allocator.allocate().unwrap(); - let noise_msg1_b = conn_b - .start_handshake(node_b.identity().keypair(), node_b.startup_epoch(), 1000) + node_b + .seed_handshake_machine( + HandshakeSeed::outbound(link_b_out, peer_a_identity, 1000) + .with_our_index(out_index_b) + .with_transport_id(transport_id_b) + .with_source_addr(remote_addr_a.clone()), + ) + .unwrap(); + let keypair_b = node_b.identity().keypair(); + let epoch_b = node_b.startup_epoch(); + let noise_msg1_b = node_b + .get_connection_mut(&link_b_out) + .unwrap() + .start_handshake(keypair_b, epoch_b, 1000) .unwrap(); - conn_b.set_our_index(out_index_b); - conn_b.set_transport_id(transport_id_b); - conn_b.set_source_addr(remote_addr_a.clone()); let wire_msg1_b = build_msg1(out_index_b, &noise_msg1_b); node_b.links.insert( link_b_out, @@ -550,7 +578,6 @@ async fn chartest_cross_connection_tiebreak_winner_and_loser() { node_b .addr_to_link .insert((transport_id_b, remote_addr_a.clone()), link_b_out); - node_b.add_connection(conn_b).unwrap(); node_b .pending_outbound .insert((transport_id_b, out_index_b.as_u32()), link_b_out); diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index e5438bc..da5e087 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -53,19 +53,27 @@ async fn test_two_node_handshake_udp() { let peer_b_node_addr = *peer_b_identity.node_addr(); let link_id_a = node_a.allocate_link_id(); - let mut conn_a = PeerConnection::outbound(link_id_a, peer_b_identity, 1000); // Allocate session index for A's outbound let our_index_a = node_a.index_allocator.allocate().unwrap(); + node_a + .seed_handshake_machine( + HandshakeSeed::outbound(link_id_a, peer_b_identity, 1000) + .with_our_index(our_index_a) + .with_transport_id(transport_id_a) + .with_source_addr(remote_addr_b.clone()), + ) + .unwrap(); + // Start handshake (generates Noise IK msg1) let our_keypair_a = node_a.identity().keypair(); - let noise_msg1 = conn_a - .start_handshake(our_keypair_a, node_a.startup_epoch(), 1000) + let startup_epoch_a = node_a.startup_epoch(); + let noise_msg1 = node_a + .get_connection_mut(&link_id_a) + .unwrap() + .start_handshake(our_keypair_a, startup_epoch_a, 1000) .unwrap(); - conn_a.set_our_index(our_index_a); - conn_a.set_transport_id(transport_id_a); - conn_a.set_source_addr(remote_addr_b.clone()); // Build wire msg1 and track in node state let wire_msg1 = build_msg1(our_index_a, &noise_msg1); @@ -78,7 +86,6 @@ async fn test_two_node_handshake_udp() { Duration::from_millis(100), ); node_a.links.insert(link_id_a, link_a); - node_a.add_connection(conn_a).unwrap(); node_a .pending_outbound .insert((transport_id_a, our_index_a.as_u32()), link_id_a); @@ -294,16 +301,23 @@ async fn test_run_rx_loop_handshake() { let peer_b_node_addr = *peer_b_identity.node_addr(); let link_id_a = node_a.allocate_link_id(); - let mut conn_a = PeerConnection::outbound(link_id_a, peer_b_identity, 1000); let our_index_a = node_a.index_allocator.allocate().unwrap(); - let our_keypair_a = node_a.identity().keypair(); - let noise_msg1 = conn_a - .start_handshake(our_keypair_a, node_a.startup_epoch(), 1000) + node_a + .seed_handshake_machine( + HandshakeSeed::outbound(link_id_a, peer_b_identity, 1000) + .with_our_index(our_index_a) + .with_transport_id(transport_id_a) + .with_source_addr(remote_addr_b.clone()), + ) + .unwrap(); + let our_keypair_a = node_a.identity().keypair(); + let startup_epoch_a = node_a.startup_epoch(); + let noise_msg1 = node_a + .get_connection_mut(&link_id_a) + .unwrap() + .start_handshake(our_keypair_a, startup_epoch_a, 1000) .unwrap(); - conn_a.set_our_index(our_index_a); - conn_a.set_transport_id(transport_id_a); - conn_a.set_source_addr(remote_addr_b.clone()); let wire_msg1 = build_msg1(our_index_a, &noise_msg1); @@ -315,7 +329,6 @@ async fn test_run_rx_loop_handshake() { Duration::from_millis(100), ); node_a.links.insert(link_id_a, link_a); - node_a.add_connection(conn_a).unwrap(); node_a .pending_outbound .insert((transport_id_a, our_index_a.as_u32()), link_id_a); @@ -483,15 +496,22 @@ async fn test_cross_connection_both_initiate() { // Node A initiates to Node B let link_id_a_out = node_a.allocate_link_id(); - let mut conn_a = PeerConnection::outbound(link_id_a_out, peer_b_identity, 1000); let our_index_a = node_a.index_allocator.allocate().unwrap(); - let our_keypair_a = node_a.identity().keypair(); - let noise_msg1_a = conn_a - .start_handshake(our_keypair_a, node_a.startup_epoch(), 1000) + node_a + .seed_handshake_machine( + HandshakeSeed::outbound(link_id_a_out, peer_b_identity, 1000) + .with_our_index(our_index_a) + .with_transport_id(transport_id_a) + .with_source_addr(remote_addr_b.clone()), + ) + .unwrap(); + let our_keypair_a = node_a.identity().keypair(); + let startup_epoch_a = node_a.startup_epoch(); + let noise_msg1_a = node_a + .get_connection_mut(&link_id_a_out) + .unwrap() + .start_handshake(our_keypair_a, startup_epoch_a, 1000) .unwrap(); - conn_a.set_our_index(our_index_a); - conn_a.set_transport_id(transport_id_a); - conn_a.set_source_addr(remote_addr_b.clone()); let wire_msg1_a = build_msg1(our_index_a, &noise_msg1_a); @@ -506,22 +526,28 @@ async fn test_cross_connection_both_initiate() { node_a .addr_to_link .insert((transport_id_a, remote_addr_b.clone()), link_id_a_out); - node_a.add_connection(conn_a).unwrap(); node_a .pending_outbound .insert((transport_id_a, our_index_a.as_u32()), link_id_a_out); // Node B initiates to Node A let link_id_b_out = node_b.allocate_link_id(); - let mut conn_b = PeerConnection::outbound(link_id_b_out, peer_a_identity, 1000); let our_index_b = node_b.index_allocator.allocate().unwrap(); - let our_keypair_b = node_b.identity().keypair(); - let noise_msg1_b = conn_b - .start_handshake(our_keypair_b, node_b.startup_epoch(), 1000) + node_b + .seed_handshake_machine( + HandshakeSeed::outbound(link_id_b_out, peer_a_identity, 1000) + .with_our_index(our_index_b) + .with_transport_id(transport_id_b) + .with_source_addr(remote_addr_a.clone()), + ) + .unwrap(); + let our_keypair_b = node_b.identity().keypair(); + let startup_epoch_b = node_b.startup_epoch(); + let noise_msg1_b = node_b + .get_connection_mut(&link_id_b_out) + .unwrap() + .start_handshake(our_keypair_b, startup_epoch_b, 1000) .unwrap(); - conn_b.set_our_index(our_index_b); - conn_b.set_transport_id(transport_id_b); - conn_b.set_source_addr(remote_addr_a.clone()); let wire_msg1_b = build_msg1(our_index_b, &noise_msg1_b); @@ -536,7 +562,6 @@ async fn test_cross_connection_both_initiate() { node_b .addr_to_link .insert((transport_id_b, remote_addr_a.clone()), link_id_b_out); - node_b.add_connection(conn_b).unwrap(); node_b .pending_outbound .insert((transport_id_b, our_index_b.as_u32()), link_id_b_out); @@ -662,17 +687,23 @@ async fn test_stale_connection_cleanup() { // Create outbound connection with a timestamp far in the past let past_time_ms = 1000; // A very early timestamp let link_id = node.allocate_link_id(); - let mut conn = PeerConnection::outbound(link_id, peer_identity, past_time_ms); // Allocate session index and set transport info let our_index = node.index_allocator.allocate().unwrap(); + node.seed_handshake_machine( + HandshakeSeed::outbound(link_id, peer_identity, past_time_ms) + .with_our_index(our_index) + .with_transport_id(transport_id) + .with_source_addr(remote_addr.clone()), + ) + .unwrap(); let our_keypair = node.identity().keypair(); - let _noise_msg1 = conn - .start_handshake(our_keypair, node.startup_epoch(), past_time_ms) + let startup_epoch = node.startup_epoch(); + let _noise_msg1 = node + .get_connection_mut(&link_id) + .unwrap() + .start_handshake(our_keypair, startup_epoch, past_time_ms) .unwrap(); - conn.set_our_index(our_index); - conn.set_transport_id(transport_id); - conn.set_source_addr(remote_addr.clone()); // Set up all the state that initiate_peer_connection would create let link = Link::connectionless( @@ -685,7 +716,6 @@ async fn test_stale_connection_cleanup() { node.links.insert(link_id, link); node.addr_to_link .insert((transport_id, remote_addr.clone()), link_id); - node.add_connection(conn).unwrap(); node.pending_outbound .insert((transport_id, our_index.as_u32()), link_id); @@ -741,16 +771,22 @@ async fn test_failed_connection_cleanup() { .map(|d| d.as_millis() as u64) .unwrap_or(0); let link_id = node.allocate_link_id(); - let mut conn = PeerConnection::outbound(link_id, peer_identity, now_ms); let our_index = node.index_allocator.allocate().unwrap(); + node.seed_handshake_machine( + HandshakeSeed::outbound(link_id, peer_identity, now_ms) + .with_our_index(our_index) + .with_transport_id(transport_id) + .with_source_addr(remote_addr.clone()), + ) + .unwrap(); let our_keypair = node.identity().keypair(); - let _noise_msg1 = conn - .start_handshake(our_keypair, node.startup_epoch(), now_ms) + let startup_epoch = node.startup_epoch(); + let _noise_msg1 = node + .get_connection_mut(&link_id) + .unwrap() + .start_handshake(our_keypair, startup_epoch, now_ms) .unwrap(); - conn.set_our_index(our_index); - conn.set_transport_id(transport_id); - conn.set_source_addr(remote_addr.clone()); let link = Link::connectionless( link_id, @@ -762,7 +798,6 @@ async fn test_failed_connection_cleanup() { node.links.insert(link_id, link); node.addr_to_link .insert((transport_id, remote_addr.clone()), link_id); - node.add_connection(conn).unwrap(); node.pending_outbound .insert((transport_id, our_index.as_u32()), link_id); diff --git a/src/node/tests/mod.rs b/src/node/tests/mod.rs index 8754f50..57d2500 100644 --- a/src/node/tests/mod.rs +++ b/src/node/tests/mod.rs @@ -84,27 +84,49 @@ pub(super) fn make_peer_identity() -> PeerIdentity { PeerIdentity::from_pubkey(identity.pubkey()) } -/// Create a PeerConnection with a completed Noise IK handshake. +/// Seed a control machine whose leg carries a completed Noise IK handshake. /// -/// Returns (connection, peer_identity) where the connection is outbound, -/// in Complete state, with session, indices, and transport info set. -pub(super) fn make_completed_connection( +/// Returns the peer identity. The leg is outbound, in Complete state, with +/// session, indices, and transport info set, and is installed on the node +/// through [`Node::seed_handshake_machine`] — the test-surface twin of +/// `Node::add_connection`. +/// +/// The Noise exchange runs on the already-seeded leg, where it used to run +/// before the leg was handed over. That reordering is neutral, but not +/// because the handshake leaves the seeded fields alone — +/// `receive_handshake_init` does write `expected_identity`. It is neutral +/// because the only read of `expected_identity` is guarded by `is_outbound`: +/// an inbound leg takes the `new_inbound` arm whether or not the identity has +/// been learned, and an outbound leg never runs that method. The remaining +/// reads (`link_id`, `started_at`, `is_outbound`, `their_index`, +/// `transport_id`) are genuinely untouched by the handshake. +pub(super) fn seed_completed_connection( node: &mut Node, link_id: LinkId, transport_id: TransportId, current_time_ms: u64, -) -> (PeerConnection, PeerIdentity) { +) -> PeerIdentity { let peer_identity_full = Identity::generate(); // Must use from_pubkey_full to preserve parity for ECDH let peer_identity = PeerIdentity::from_pubkey_full(peer_identity_full.pubkey_full()); - // Create outbound connection - let mut conn = PeerConnection::outbound(link_id, peer_identity, current_time_ms); + let our_index = node.index_allocator.allocate().unwrap(); + node.seed_handshake_machine( + HandshakeSeed::outbound(link_id, peer_identity, current_time_ms) + .with_our_index(our_index) + .with_their_index(SessionIndex::new(42)) + .with_transport_id(transport_id) + .with_source_addr(TransportAddr::from_string("127.0.0.1:5000")), + ) + .unwrap(); // Run initiator side of handshake let our_keypair = node.identity().keypair(); - let msg1 = conn - .start_handshake(our_keypair, node.startup_epoch(), current_time_ms) + let startup_epoch = node.startup_epoch(); + let msg1 = node + .get_connection_mut(&link_id) + .unwrap() + .start_handshake(our_keypair, startup_epoch, current_time_ms) .unwrap(); // Run responder side to generate msg2 @@ -117,14 +139,10 @@ pub(super) fn make_completed_connection( .unwrap(); // Complete initiator handshake - conn.complete_handshake(&msg2, current_time_ms).unwrap(); + node.get_connection_mut(&link_id) + .unwrap() + .complete_handshake(&msg2, current_time_ms) + .unwrap(); - // Set indices and transport info - let our_index = node.index_allocator.allocate().unwrap(); - conn.set_our_index(our_index); - conn.set_their_index(SessionIndex::new(42)); - conn.set_transport_id(transport_id); - conn.set_source_addr(TransportAddr::from_string("127.0.0.1:5000")); - - (conn, peer_identity) + peer_identity } diff --git a/src/node/tests/routing.rs b/src/node/tests/routing.rs index 5123581..cd6b1c2 100644 --- a/src/node/tests/routing.rs +++ b/src/node/tests/routing.rs @@ -29,9 +29,8 @@ fn test_routing_direct_peer() { let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); let peer_addr = *identity.node_addr(); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, identity, 2000).unwrap(); let result = node.find_next_hop(&peer_addr); @@ -58,15 +57,13 @@ fn test_routing_bloom_filter_hit() { // Create two peers let link_id1 = LinkId::new(1); - let (conn1, id1) = make_completed_connection(&mut node, link_id1, transport_id, 1000); + let id1 = seed_completed_connection(&mut node, link_id1, transport_id, 1000); let peer1_addr = *id1.node_addr(); - node.add_connection(conn1).unwrap(); node.promote_connection(link_id1, id1, 2000).unwrap(); let link_id2 = LinkId::new(2); - let (conn2, id2) = make_completed_connection(&mut node, link_id2, transport_id, 1000); + let id2 = seed_completed_connection(&mut node, link_id2, transport_id, 1000); let peer2_addr = *id2.node_addr(); - node.add_connection(conn2).unwrap(); node.promote_connection(link_id2, id2, 2000).unwrap(); // Set up tree: we are root, both peers are our children @@ -115,10 +112,9 @@ fn test_routing_bloom_filter_multiple_hits_tiebreak() { let mut peer_addrs = Vec::new(); for i in 1..=3 { let link_id = LinkId::new(i); - let (conn, id) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let id = seed_completed_connection(&mut node, link_id, transport_id, 1000); let addr = *id.node_addr(); peer_addrs.push(addr); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, id, 2000).unwrap(); } @@ -166,9 +162,8 @@ fn test_routing_tree_fallback() { // Create a peer let link_id = LinkId::new(1); - let (conn, id) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let id = seed_completed_connection(&mut node, link_id, transport_id, 1000); let peer_addr = *id.node_addr(); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, id, 2000).unwrap(); // Set up tree state through the public API. @@ -220,9 +215,8 @@ fn test_routing_bloom_hit_not_closer_falls_through_to_tree() { // tree_peer: child of self, on the path to dest (greedy tree pick). let tree_link = LinkId::new(1); - let (tree_conn, tree_id) = make_completed_connection(&mut node, tree_link, transport_id, 1000); + let tree_id = seed_completed_connection(&mut node, tree_link, transport_id, 1000); let tree_peer_addr = *tree_id.node_addr(); - node.add_connection(tree_conn).unwrap(); node.promote_connection(tree_link, tree_id, 2000).unwrap(); // bloom_peer: also a child of self, but with a stale/false-positive @@ -230,10 +224,8 @@ fn test_routing_bloom_hit_not_closer_falls_through_to_tree() { // ours, so the self-distance check in select_best_candidate excludes // it — leaving zero viable bloom candidates. let bloom_link = LinkId::new(2); - let (bloom_conn, bloom_id) = - make_completed_connection(&mut node, bloom_link, transport_id, 1000); + let bloom_id = seed_completed_connection(&mut node, bloom_link, transport_id, 1000); let bloom_peer_addr = *bloom_id.node_addr(); - node.add_connection(bloom_conn).unwrap(); node.promote_connection(bloom_link, bloom_id, 2000).unwrap(); // Tree topology (we are root): @@ -300,8 +292,7 @@ fn test_routing_tree_no_coords_in_cache() { // Create a peer let link_id = LinkId::new(1); - let (conn, id) = make_completed_connection(&mut node, link_id, transport_id, 1000); - node.add_connection(conn).unwrap(); + let id = seed_completed_connection(&mut node, link_id, transport_id, 1000); node.promote_connection(link_id, id, 2000).unwrap(); // Destination not in bloom filters and not in coord cache @@ -319,9 +310,8 @@ fn test_routing_refreshes_coord_cache_ttl() { // Create a peer let link_id = LinkId::new(1); - let (conn, id) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let id = seed_completed_connection(&mut node, link_id, transport_id, 1000); let peer_addr = *id.node_addr(); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, id, 2000).unwrap(); // Set up tree coordinates @@ -364,15 +354,13 @@ fn test_routing_bloom_hit_without_coords_returns_none() { // Create two peers let link_id1 = LinkId::new(1); - let (conn1, id1) = make_completed_connection(&mut node, link_id1, transport_id, 1000); + let id1 = seed_completed_connection(&mut node, link_id1, transport_id, 1000); let peer1_addr = *id1.node_addr(); - node.add_connection(conn1).unwrap(); node.promote_connection(link_id1, id1, 2000).unwrap(); let link_id2 = LinkId::new(2); - let (conn2, id2) = make_completed_connection(&mut node, link_id2, transport_id, 1000); + let id2 = seed_completed_connection(&mut node, link_id2, transport_id, 1000); let peer2_addr = *id2.node_addr(); - node.add_connection(conn2).unwrap(); node.promote_connection(link_id2, id2, 2000).unwrap(); let dest = make_node_addr(99); @@ -404,9 +392,8 @@ fn test_routing_discovery_coord_cache() { // Create a peer let link_id = LinkId::new(1); - let (conn, id) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let id = seed_completed_connection(&mut node, link_id, transport_id, 1000); let peer_addr = *id.node_addr(); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, id, 2000).unwrap(); // Set up tree: we are root, peer is our child diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index 8d50d1f..4ad7c65 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -1008,9 +1008,7 @@ fn test_identity_cache_populated_on_promote() { let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, peer_identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); - - node.add_connection(conn).unwrap(); + let peer_identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); // Promote let result = node diff --git a/src/node/tests/spanning_tree.rs b/src/node/tests/spanning_tree.rs index 12b323e..cb6e8eb 100644 --- a/src/node/tests/spanning_tree.rs +++ b/src/node/tests/spanning_tree.rs @@ -116,16 +116,25 @@ pub(super) async fn initiate_handshake(nodes: &mut [TestNode], i: usize, j: usiz let transport_id = initiator.transport_id; let link_id = initiator.node.allocate_link_id(); - let mut conn = PeerConnection::outbound(link_id, peer_identity, 1000); let our_index = initiator.node.index_allocator.allocate().unwrap(); - let our_keypair = initiator.node.identity().keypair(); - let noise_msg1 = conn - .start_handshake(our_keypair, initiator.node.startup_epoch(), 1000) + initiator + .node + .seed_handshake_machine( + HandshakeSeed::outbound(link_id, peer_identity, 1000) + .with_our_index(our_index) + .with_transport_id(transport_id) + .with_source_addr(responder_addr.clone()), + ) + .unwrap(); + let our_keypair = initiator.node.identity().keypair(); + let startup_epoch = initiator.node.startup_epoch(); + let noise_msg1 = initiator + .node + .get_connection_mut(&link_id) + .unwrap() + .start_handshake(our_keypair, startup_epoch, 1000) .unwrap(); - conn.set_our_index(our_index); - conn.set_transport_id(transport_id); - conn.set_source_addr(responder_addr.clone()); let wire_msg1 = build_msg1(our_index, &noise_msg1); @@ -141,7 +150,6 @@ pub(super) async fn initiate_handshake(nodes: &mut [TestNode], i: usize, j: usiz .node .addr_to_link .insert((transport_id, responder_addr.clone()), link_id); - initiator.node.add_connection(conn).unwrap(); initiator .node .pending_outbound diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 13731c3..3185ba8 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -337,9 +337,9 @@ fn test_node_connection_management() { let identity = make_peer_identity(); let link_id = LinkId::new(1); - let conn = PeerConnection::outbound(link_id, identity, 1000); + node.seed_handshake_machine(HandshakeSeed::outbound(link_id, identity, 1000)) + .unwrap(); - node.add_connection(conn).unwrap(); assert_eq!(node.connection_count(), 1); assert!(node.get_connection(&link_id).is_some()); @@ -354,11 +354,10 @@ fn test_node_connection_duplicate() { let identity = make_peer_identity(); let link_id = LinkId::new(1); - let conn1 = PeerConnection::outbound(link_id, identity, 1000); - let conn2 = PeerConnection::outbound(link_id, identity, 2000); + node.seed_handshake_machine(HandshakeSeed::outbound(link_id, identity, 1000)) + .unwrap(); - node.add_connection(conn1).unwrap(); - let result = node.add_connection(conn2); + let result = node.seed_handshake_machine(HandshakeSeed::outbound(link_id, identity, 2000)); assert!(matches!(result, Err(NodeError::ConnectionAlreadyExists(_)))); } @@ -370,9 +369,8 @@ fn test_peer_maps_coherent_after_add_connection() { let identity = make_peer_identity(); let link_id = LinkId::new(1); - let conn = PeerConnection::outbound(link_id, identity, 1000); - - node.add_connection(conn).unwrap(); + node.seed_handshake_machine(HandshakeSeed::outbound(link_id, identity, 1000)) + .unwrap(); assert!( node.peer_machines.contains_key(&link_id), @@ -388,9 +386,8 @@ fn test_peer_maps_coherent_through_establish() { let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); - node.add_connection(conn).unwrap(); node.debug_assert_peer_maps_coherent(); let result = node.promote_connection(link_id, identity, 2000).unwrap(); @@ -427,10 +424,9 @@ fn test_node_promote_connection() { let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); let node_addr = *identity.node_addr(); - node.add_connection(conn).unwrap(); assert_eq!(node.connection_count(), 1); assert_eq!(node.peer_count(), 0); @@ -467,10 +463,9 @@ fn test_node_cross_connection_resolution() { // First connection and promotion (becomes active peer) let link_id1 = LinkId::new(1); - let (conn1, identity) = make_completed_connection(&mut node, link_id1, transport_id, 1000); + let identity = seed_completed_connection(&mut node, link_id1, transport_id, 1000); let node_addr = *identity.node_addr(); - node.add_connection(conn1).unwrap(); node.promote_connection(link_id1, identity, 1500).unwrap(); assert_eq!(node.peer_count(), 1); @@ -500,8 +495,7 @@ fn test_node_peer_limit() { // Add two peers via promotion for i in 0..2 { let link_id = LinkId::new(i as u64 + 1); - let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); - node.add_connection(conn).unwrap(); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); node.promote_connection(link_id, identity, 2000).unwrap(); } @@ -509,8 +503,7 @@ fn test_node_peer_limit() { // Third should fail let link_id = LinkId::new(3); - let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 3000); - node.add_connection(conn).unwrap(); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 3000); let result = node.promote_connection(link_id, identity, 4000); assert!(matches!(result, Err(NodeError::MaxPeersExceeded { .. }))); @@ -558,22 +551,19 @@ fn test_node_sendable_peers() { // Add a healthy peer let link_id1 = LinkId::new(1); - let (conn1, identity1) = make_completed_connection(&mut node, link_id1, transport_id, 1000); + let identity1 = seed_completed_connection(&mut node, link_id1, transport_id, 1000); let node_addr1 = *identity1.node_addr(); - node.add_connection(conn1).unwrap(); node.promote_connection(link_id1, identity1, 2000).unwrap(); // Add another peer and mark it stale (still sendable) let link_id2 = LinkId::new(2); - let (conn2, identity2) = make_completed_connection(&mut node, link_id2, transport_id, 1000); - node.add_connection(conn2).unwrap(); + let identity2 = seed_completed_connection(&mut node, link_id2, transport_id, 1000); node.promote_connection(link_id2, identity2, 2000).unwrap(); // Add a third peer and mark it disconnected (not sendable) let link_id3 = LinkId::new(3); - let (conn3, identity3) = make_completed_connection(&mut node, link_id3, transport_id, 1000); + let identity3 = seed_completed_connection(&mut node, link_id3, transport_id, 1000); let node_addr3 = *identity3.node_addr(); - node.add_connection(conn3).unwrap(); node.promote_connection(link_id3, identity3, 2000).unwrap(); node.get_peer_mut(&node_addr3).unwrap().mark_disconnected(); @@ -708,20 +698,24 @@ fn test_promote_cleans_up_pending_outbound_to_same_peer() { // This simulates A having sent msg1 to B before B was running. let pending_link_id = LinkId::new(1); let pending_time_ms = 1000; - let mut pending_conn = - PeerConnection::outbound(pending_link_id, peer_b_identity, pending_time_ms); + let pending_index = node.index_allocator.allocate().unwrap(); + let pending_addr = TransportAddr::from_string("10.0.0.2:2121"); + node.seed_handshake_machine( + HandshakeSeed::outbound(pending_link_id, peer_b_identity, pending_time_ms) + .with_our_index(pending_index) + .with_transport_id(transport_id) + .with_source_addr(pending_addr.clone()), + ) + .unwrap(); let our_keypair = node.identity().keypair(); - let _msg1 = pending_conn - .start_handshake(our_keypair, node.startup_epoch(), pending_time_ms) + let startup_epoch = node.startup_epoch(); + let _msg1 = node + .get_connection_mut(&pending_link_id) + .unwrap() + .start_handshake(our_keypair, startup_epoch, pending_time_ms) .unwrap(); - let pending_index = node.index_allocator.allocate().unwrap(); - pending_conn.set_our_index(pending_index); - pending_conn.set_transport_id(transport_id); - let pending_addr = TransportAddr::from_string("10.0.0.2:2121"); - pending_conn.set_source_addr(pending_addr.clone()); - let pending_link = Link::connectionless( pending_link_id, transport_id, @@ -732,7 +726,6 @@ fn test_promote_cleans_up_pending_outbound_to_same_peer() { node.links.insert(pending_link_id, pending_link); node.addr_to_link .insert((transport_id, pending_addr.clone()), pending_link_id); - node.add_connection(pending_conn).unwrap(); node.pending_outbound .insert((transport_id, pending_index.as_u32()), pending_link_id); @@ -747,12 +740,22 @@ fn test_promote_cleans_up_pending_outbound_to_same_peer() { let completing_link_id = LinkId::new(2); let completing_time_ms = 2000; - let mut completing_conn = - PeerConnection::outbound(completing_link_id, peer_b_identity, completing_time_ms); + let completing_index = node.index_allocator.allocate().unwrap(); + node.seed_handshake_machine( + HandshakeSeed::outbound(completing_link_id, peer_b_identity, completing_time_ms) + .with_our_index(completing_index) + .with_their_index(SessionIndex::new(99)) + .with_transport_id(transport_id) + .with_source_addr(TransportAddr::from_string("10.0.0.2:4001")), + ) + .unwrap(); let our_keypair = node.identity().keypair(); - let msg1 = completing_conn - .start_handshake(our_keypair, node.startup_epoch(), completing_time_ms) + let startup_epoch = node.startup_epoch(); + let msg1 = node + .get_connection_mut(&completing_link_id) + .unwrap() + .start_handshake(our_keypair, startup_epoch, completing_time_ms) .unwrap(); // B responds @@ -764,18 +767,11 @@ fn test_promote_cleans_up_pending_outbound_to_same_peer() { .receive_handshake_init(peer_keypair, resp_epoch, &msg1, completing_time_ms) .unwrap(); - completing_conn + node.get_connection_mut(&completing_link_id) + .unwrap() .complete_handshake(&msg2, completing_time_ms) .unwrap(); - let completing_index = node.index_allocator.allocate().unwrap(); - completing_conn.set_our_index(completing_index); - completing_conn.set_their_index(SessionIndex::new(99)); - completing_conn.set_transport_id(transport_id); - completing_conn.set_source_addr(TransportAddr::from_string("10.0.0.2:4001")); - - node.add_connection(completing_conn).unwrap(); - // Now 2 connections, 1 link (pending has link, completing doesn't yet need one for this test) assert_eq!(node.connection_count(), 2); assert_eq!(node.index_allocator.count(), 2); @@ -1039,9 +1035,8 @@ fn test_schedule_retry_skips_connected_peer() { // Promote a peer so it's in the peers map let link_id = LinkId::new(1); - let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); let node_addr = *identity.node_addr(); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, identity, 2000).unwrap(); assert_eq!(node.peer_count(), 1); @@ -1058,10 +1053,9 @@ async fn test_try_peer_addresses_skips_connected_peer() { let mut node = make_node(); let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, peer_identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let peer_identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); let peer_config = crate::config::PeerConfig::new(peer_identity.npub(), "udp", "127.0.0.1:9"); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, peer_identity, 2000) .unwrap(); let link_count = node.link_count(); @@ -1088,8 +1082,8 @@ async fn test_try_peer_addresses_skips_connecting_peer() { let mut node = make_node(); let peer_identity = make_peer_identity(); let peer_config = crate::config::PeerConfig::new(peer_identity.npub(), "udp", "127.0.0.1:9"); - let pending = PeerConnection::outbound(LinkId::new(1), peer_identity, 1000); - node.add_connection(pending).unwrap(); + node.seed_handshake_machine(HandshakeSeed::outbound(LinkId::new(1), peer_identity, 1000)) + .unwrap(); node.try_peer_addresses(&peer_config, peer_identity, true) .await @@ -1270,8 +1264,7 @@ async fn test_nostr_traversal_failure_skips_connected_peer() { let mut node = make_node(); let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, peer_identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); - node.add_connection(conn).unwrap(); + let peer_identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); node.promote_connection(link_id, peer_identity, 2000) .unwrap(); @@ -1304,8 +1297,7 @@ async fn test_nostr_traversal_established_skips_connected_peer() { let mut node = make_node(); let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, peer_identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); - node.add_connection(conn).unwrap(); + let peer_identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); node.promote_connection(link_id, peer_identity, 2000) .unwrap(); let link_count = node.link_count(); @@ -1525,7 +1517,7 @@ fn test_promote_clears_retry_pending() { let transport_id = TransportId::new(1); let link_id = LinkId::new(1); - let (conn, identity) = make_completed_connection(&mut node, link_id, transport_id, 1000); + let identity = seed_completed_connection(&mut node, link_id, transport_id, 1000); let node_addr = *identity.node_addr(); // Simulate a retry entry existing for this peer @@ -1535,7 +1527,6 @@ fn test_promote_clears_retry_pending() { ); assert_eq!(node.peering.reconciler.retry_pending.len(), 1); - node.add_connection(conn).unwrap(); node.promote_connection(link_id, identity, 2000).unwrap(); assert!( diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 1834917..91941ea 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -2971,3 +2971,22 @@ mod tests { assert_eq!(alloc.count(), 0); } } + +/// T-SANSIO: the action vocabulary must stay plain, comparable data. +/// +/// `PeerAction` is the sans-IO boundary between the pure reducer and its +/// driver. Requiring `Clone + Eq` is a compile-time statement that no variant +/// may carry a runtime handle (a socket, a `JoinHandle`, a channel sender), +/// since none of those are `Clone + Eq`. If a future variant smuggles one in, +/// this bound stops compiling. +#[cfg(test)] +mod action_contract { + use super::PeerAction; + + fn assert_clone_eq() {} + + #[test] + fn peer_action_is_plain_comparable_data() { + assert_clone_eq::(); + } +} From 5b09e22956c6c36f211cd90383ab6e702de76b5e Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 18 Jul 2026 21:45:09 +0000 Subject: [PATCH 2/7] node: read promotion's carrier fields in one borrow `promote_connection` detached the pending connection and then interleaved reads off that detached value with three separate lookups of the same control machine. Gather the machine-side fields (`their_index`, `transport_id`, link stats) in the single `get_mut` that takes the connection, so the machine is borrowed once. Behaviour is unchanged. The connection is still taken before anything is validated, so a rejected promotion leaves the machine with no pending connection, and the prelude only gathers options: the checks below it still report the first missing field in the same order (`our_index`, `their_index`, `transport_id`, `source_addr`). Link stats move to the prelude, ahead of those checks. The value is the same either way: they live on the surviving carrier rather than the detached connection, and nothing between the two points touches the machine map. The lookup could not fail at the old site either, since the function had already reached it through a successful lookup on the same key, so dropping the defaulting arm changes nothing. Add a test covering the error order and the detach, driving promotion with connections missing each required field in turn plus every later one, so an implementation that validated during the prelude would report the wrong field. Test seeding grows a variant that lets the caller shape the seed. --- src/node/handlers/handshake.rs | 47 ++++++++--------- src/node/tests/mod.rs | 34 ++++++++---- src/node/tests/unit.rs | 96 ++++++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+), 34 deletions(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 20f2ae6..4bc9ad7 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -1366,14 +1366,26 @@ impl Node { verified_identity: PeerIdentity, current_time_ms: u64, ) -> Result { - // Take the pending connection off its control machine. The machine + // Take the pending connection off its control machine, and read the + // carrier fields the promotion needs in the same borrow. The machine // survives the promotion (it becomes the active peer's control // machine), left with no pending connection. - let mut connection = self + // + // The connection is detached before anything is validated, so every + // error return below leaves the machine leg-less — the caller disposes + // of it. Gathering the carrier reads up front is only a borrow shape: + // they are infallible, so the order in which the missing-field errors + // are reported below is unchanged. + let machine = self .peer_machines .get_mut(&link_id) - .and_then(|machine| machine.take_leg()) .ok_or(NodeError::ConnectionNotFound(link_id))?; + let mut connection = machine + .take_leg() + .ok_or(NodeError::ConnectionNotFound(link_id))?; + let carrier_their_index = machine.conn_their_index(); + let carrier_transport_id = machine.conn_transport_id(); + let link_stats = machine.conn_link_stats().clone(); // Verify handshake is complete and extract session if !connection.has_session() { @@ -1390,22 +1402,14 @@ impl Node { link_id, reason: "missing our_index".into(), })?; - let their_index = self - .peer_machines - .get(&link_id) - .and_then(|machine| machine.conn_their_index()) - .ok_or_else(|| NodeError::PromotionFailed { - link_id, - reason: "missing their_index".into(), - })?; - let transport_id = self - .peer_machines - .get(&link_id) - .and_then(|machine| machine.conn_transport_id()) - .ok_or_else(|| NodeError::PromotionFailed { - link_id, - reason: "missing transport_id".into(), - })?; + let their_index = carrier_their_index.ok_or_else(|| NodeError::PromotionFailed { + link_id, + reason: "missing their_index".into(), + })?; + let transport_id = carrier_transport_id.ok_or_else(|| NodeError::PromotionFailed { + link_id, + reason: "missing transport_id".into(), + })?; let current_addr = connection .source_addr() .ok_or_else(|| NodeError::PromotionFailed { @@ -1413,11 +1417,6 @@ impl Node { reason: "missing source_addr".into(), })? .clone(); - let link_stats = self - .peer_machines - .get(&link_id) - .map(|machine| machine.conn_link_stats().clone()) - .unwrap_or_default(); let remote_epoch = connection.remote_epoch(); let peer_node_addr = *verified_identity.node_addr(); diff --git a/src/node/tests/mod.rs b/src/node/tests/mod.rs index 57d2500..dc75785 100644 --- a/src/node/tests/mod.rs +++ b/src/node/tests/mod.rs @@ -90,6 +90,23 @@ pub(super) fn make_peer_identity() -> PeerIdentity { /// session, indices, and transport info set, and is installed on the node /// through [`Node::seed_handshake_machine`] — the test-surface twin of /// `Node::add_connection`. +pub(super) fn seed_completed_connection( + node: &mut Node, + link_id: LinkId, + transport_id: TransportId, + current_time_ms: u64, +) -> PeerIdentity { + let our_index = node.index_allocator.allocate().unwrap(); + seed_completed_connection_with(node, link_id, current_time_ms, |seed| { + seed.with_our_index(our_index) + .with_their_index(SessionIndex::new(42)) + .with_transport_id(transport_id) + .with_source_addr(TransportAddr::from_string("127.0.0.1:5000")) + }) +} + +/// [`seed_completed_connection`] with the seed left to the caller, for tests +/// that need a leg deliberately missing one of the fields promotion requires. /// /// The Noise exchange runs on the already-seeded leg, where it used to run /// before the leg was handed over. That reordering is neutral, but not @@ -100,24 +117,21 @@ pub(super) fn make_peer_identity() -> PeerIdentity { /// been learned, and an outbound leg never runs that method. The remaining /// reads (`link_id`, `started_at`, `is_outbound`, `their_index`, /// `transport_id`) are genuinely untouched by the handshake. -pub(super) fn seed_completed_connection( +pub(super) fn seed_completed_connection_with( node: &mut Node, link_id: LinkId, - transport_id: TransportId, current_time_ms: u64, + shape: impl FnOnce(HandshakeSeed) -> HandshakeSeed, ) -> PeerIdentity { let peer_identity_full = Identity::generate(); // Must use from_pubkey_full to preserve parity for ECDH let peer_identity = PeerIdentity::from_pubkey_full(peer_identity_full.pubkey_full()); - let our_index = node.index_allocator.allocate().unwrap(); - node.seed_handshake_machine( - HandshakeSeed::outbound(link_id, peer_identity, current_time_ms) - .with_our_index(our_index) - .with_their_index(SessionIndex::new(42)) - .with_transport_id(transport_id) - .with_source_addr(TransportAddr::from_string("127.0.0.1:5000")), - ) + node.seed_handshake_machine(shape(HandshakeSeed::outbound( + link_id, + peer_identity, + current_time_ms, + ))) .unwrap(); // Run initiator side of handshake diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 3185ba8..138119e 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -418,6 +418,102 @@ fn test_peer_maps_coherence_detects_orphaned_machine() { ); } +/// Promotion detaches the pending connection before it validates anything, and +/// then validates the required fields in a fixed order. Both properties are +/// depended on: the caller of a rejected promotion disposes of a machine it +/// expects to be leg-less, and the reported error names the first field that is +/// missing, not an arbitrary one. +#[test] +fn test_promote_connection_error_order_and_leg_detach() { + fn expect_promotion_failure( + shape: impl FnOnce(HandshakeSeed) -> HandshakeSeed, + expected_reason: &str, + ) { + let mut node = make_node(); + let link_id = LinkId::new(1); + let identity = seed_completed_connection_with(&mut node, link_id, 1000, shape); + + let err = node + .promote_connection(link_id, identity, 2000) + .expect_err("promotion must reject an incomplete connection"); + match err { + NodeError::PromotionFailed { reason, .. } => assert_eq!( + reason, expected_reason, + "promotion named the wrong missing field" + ), + other => panic!("expected PromotionFailed, got {other:?}"), + } + assert!( + node.peer_machines + .get(&link_id) + .expect("the control machine survives a failed promotion") + .leg() + .is_none(), + "a failed promotion must leave the machine leg-less" + ); + assert_eq!(node.peer_count(), 0); + } + + // Each case leaves every later field missing as well, so a promotion that + // gathered all of them before validating would name the wrong one. + expect_promotion_failure(|seed| seed, "missing our_index"); + expect_promotion_failure( + |seed| seed.with_our_index(SessionIndex::new(7)), + "missing their_index", + ); + expect_promotion_failure( + |seed| { + seed.with_our_index(SessionIndex::new(7)) + .with_their_index(SessionIndex::new(42)) + }, + "missing transport_id", + ); + expect_promotion_failure( + |seed| { + seed.with_our_index(SessionIndex::new(7)) + .with_their_index(SessionIndex::new(42)) + .with_transport_id(TransportId::new(1)) + }, + "missing source_addr", + ); + + // An unknown link and a machine carrying no connection are both + // ConnectionNotFound, ahead of every field check. + let mut node = make_node(); + let identity = make_peer_identity(); + assert!(matches!( + node.promote_connection(LinkId::new(9), identity, 2000), + Err(NodeError::ConnectionNotFound(_)) + )); + + let link_id = LinkId::new(2); + node.peer_machines + .insert(link_id, PeerMachine::new_inbound(link_id, 1000)); + assert!(matches!( + node.promote_connection(link_id, identity, 2000), + Err(NodeError::ConnectionNotFound(_)) + )); + + // A connection that never ran the handshake fails on the session check, + // after the detach and ahead of every field check — the seed below + // supplies none of the four fields above. + let link_id = LinkId::new(3); + node.seed_handshake_machine(HandshakeSeed::outbound(link_id, identity, 1000)) + .unwrap(); + assert!(matches!( + node.promote_connection(link_id, identity, 2000), + Err(NodeError::HandshakeIncomplete(_)) + )); + assert!( + node.peer_machines + .get(&link_id) + .expect("the control machine survives a failed promotion") + .leg() + .is_none(), + "an incomplete handshake must still leave the machine leg-less" + ); +} + #[test] fn test_node_promote_connection() { let mut node = make_node(); From 11ec16777c6e35b104d09878faf79e0230d66c26 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 18 Jul 2026 22:06:46 +0000 Subject: [PATCH 3/7] node: yield the control machine when iterating pending connections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Node::connections()` yielded the pending connection itself, so its consumers reached the handshake-phase fields through that value. The pending connection is being folded into the control machine, and the fields will move off it, so yield the machine (keyed by its link) and let each consumer reach what it needs from there. Membership is unchanged: the iterator still selects machines that carry a pending connection, which is the predicate `connection_count` and the stale-connection sweep already use. Every consumer takes the same value from the same place, one hop further out, and no field changes carrier here. The method is now internal to the crate. It yields the control machine, which is not part of the published surface, and nothing outside the crate iterates pending connections — operator views of the handshake phase go through the query surface instead. --- src/control/queries.rs | 1 + src/node/handlers/handshake.rs | 14 +++++++++----- src/node/lifecycle/mod.rs | 17 +++++++++++------ src/node/mod.rs | 18 ++++++++++++++---- src/node/tests/unit.rs | 2 ++ 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/src/control/queries.rs b/src/control/queries.rs index 46227c2..53f276e 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -1303,6 +1303,7 @@ pub fn show_connections(node: &Node) -> Value { let now = now_ms(); let connections: Vec = node .connections() + .filter_map(|(_, machine)| machine.leg()) .map(|conn| { let mut conn_json = json!({ "link_id": conn.link_id().as_u64(), diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 4bc9ad7..3b78bf6 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -39,11 +39,14 @@ impl EstablishView for Node { rekey_in_progress: existing.map(|p| p.rekey_in_progress()).unwrap_or(false), existing_msg2: existing.and_then(|p| p.handshake_msg2().map(|m| m.to_vec())), at_max_peers: max_peers > 0 && self.peers.len() >= max_peers, - has_pending_outbound_to_peer: self.connections().any(|conn| { - conn.expected_identity() - .map(|id| id.node_addr() == peer_addr) - .unwrap_or(false) - }), + has_pending_outbound_to_peer: self + .connections() + .filter_map(|(_, machine)| machine.leg()) + .any(|conn| { + conn.expected_identity() + .map(|id| id.node_addr() == peer_addr) + .unwrap_or(false) + }), rekey_enabled: self.config().node.rekey.enabled, our_node_addr: *self.identity().node_addr(), } @@ -1552,6 +1555,7 @@ impl Node { // the 30s handshake timeout. let pending_to_same_peer: Vec = self .connections() + .filter_map(|(_, machine)| machine.leg()) .filter(|conn| { conn.expected_identity() .map(|id| *id.node_addr() == peer_node_addr) diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 2b06c8a..22fa219 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -372,11 +372,13 @@ impl Node { } fn is_connecting_to_peer(&self, peer_node_addr: &NodeAddr) -> bool { - self.connections().any(|conn| { - conn.expected_identity() - .map(|id| id.node_addr() == peer_node_addr) - .unwrap_or(false) - }) + self.connections() + .filter_map(|(_, machine)| machine.leg()) + .any(|conn| { + conn.expected_identity() + .map(|id| id.node_addr() == peer_node_addr) + .unwrap_or(false) + }) } fn is_connecting_to_peer_on_path( @@ -928,6 +930,7 @@ impl Node { let now_ms = Self::now_ms(); let stale: Vec = self .connections() + .filter_map(|(_, machine)| machine.leg()) .filter(|conn| { conn.expected_identity() .map(|id| id.node_addr() == &peer_addr) @@ -2720,10 +2723,11 @@ impl Node { let connected: HashSet = self.peers.keys().copied().collect(); let connecting: HashSet = self .connections() + .filter_map(|(_, machine)| machine.leg()) .filter_map(|conn| conn.expected_identity().map(|id| *id.node_addr())) .collect(); let mut in_flight_by_peer: HashMap = HashMap::new(); - for conn in self.connections() { + for conn in self.connections().filter_map(|(_, machine)| machine.leg()) { if let Some(id) = conn.expected_identity() { *in_flight_by_peer.entry(*id.node_addr()).or_default() += 1; } @@ -2793,6 +2797,7 @@ impl Node { let in_flight_for_peer = self .connections() + .filter_map(|(_, machine)| machine.leg()) .filter(|conn| { conn.expected_identity() .map(|identity| identity.node_addr() == peer_node_addr) diff --git a/src/node/mod.rs b/src/node/mod.rs index 9799b02..bb7d2dc 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1978,6 +1978,7 @@ impl Node { // --- connections (show_connections) --- let connection_rows: Vec = self .connections() + .filter_map(|(_, machine)| machine.leg()) .map(|conn| snap::ConnectionRow { link_id: conn.link_id().as_u64(), direction: format!("{}", conn.direction()), @@ -2557,11 +2558,20 @@ impl Node { connection } - /// Iterate over all connections. - pub fn connections(&self) -> impl Iterator { + /// Iterate over the control machines that carry a pending connection. + /// + /// Carrying a pending connection is what makes a machine handshake-phase, + /// so the filter below is the membership rule. It is the same predicate + /// that `connection_count` applies, and the one the stale-connection sweep + /// narrows further. + /// + /// Internal to the crate: this yields the control machine, which is not + /// part of the published surface. Callers outside the crate that need a + /// view of the pending handshakes go through the operator queries. + pub(crate) fn connections(&self) -> impl Iterator { self.peer_machines - .values() - .filter_map(|machine| machine.leg()) + .iter() + .filter(|(_, machine)| machine.leg().is_some()) } // === Peer Management (Active Phase) === diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 138119e..da4b34a 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -138,6 +138,7 @@ async fn test_try_peer_addresses_races_all_concrete_udp_candidates() { let mut addrs = node .connections() + .filter_map(|(_, machine)| machine.leg()) .filter_map(|conn| conn.source_addr().and_then(|addr| addr.as_str())) .collect::>(); addrs.sort(); @@ -1342,6 +1343,7 @@ async fn update_peers_races_new_alternative_without_dropping_active_peer() { assert_eq!(node.connection_count(), 1); assert_eq!( node.connections() + .filter_map(|(_, machine)| machine.leg()) .next() .and_then(|conn| conn.source_addr()), Some(&new_addr) From 347cbe60bd692fa3d54283948e96b7459874381d Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 18 Jul 2026 22:55:38 +0000 Subject: [PATCH 4/7] peer: move the Noise handshake operations onto the control machine The pending connection drove its own Noise handshake while the control machine held it, so the crypto and the state it produces lived on the carrier that is going away. Move the six operations onto the machine: starting and completing an initiation, processing an inbound initiation, taking the session, testing for one, and dropping the handle on failure. Bodies are unchanged apart from reaching the handles through the attached connection. Each operation now records its results on both carriers. The learned identity, the remote epoch, and the activity stamp are written to the machine's own bookkeeping at the same point and with the same value as they are written to the connection's. The connection's copies still have readers until those reads are repointed, so both have to be written; the machine's copy of the learned identity was previously never populated for an inbound connection, which is why an inbound pending row showed no expected peer. Driving the handshake from the machine means the machine has to exist before the crypto runs. On the inbound path it is built above the message-1 processing and still kept local, so a rejected message leaves no registry entry and allocates no index. On the outbound path it already existed from the dial, so it simply takes the connection before the index allocation, and both failure arms unwind it as they did. Completing message 2 no longer mirrors the activity stamp separately, since the completion itself now writes both carriers at the point the mirror was approximating. Three tests cover what the compiler cannot. A connection whose handshake failed holds neither Noise handle yet must stay visible to the stale-connection sweep, or every failed connection would leak and hold a peering-budget slot forever. A message 1 rejected by the crypto or by the ACL must leave no machine, no index, and the same rejection count as before. A dial whose message-1 preparation fails must unwind the machine registered at dial time; that test drives the index-allocation failure rather than a crypto failure, which is the arm this change actually widens, since the allocation now happens with the connection already attached. --- src/node/handlers/handshake.rs | 97 ++++--- src/node/lifecycle/mod.rs | 85 +++--- src/node/tests/acl.rs | 53 +++- src/node/tests/establish_chartests.rs | 14 +- src/node/tests/handshake.rs | 56 ++-- src/node/tests/mod.rs | 32 ++- src/node/tests/spanning_tree.rs | 3 +- src/node/tests/unit.rs | 140 +++++++++- src/peer/connection.rs | 303 ++------------------- src/peer/machine.rs | 375 +++++++++++++++++++++++++- 10 files changed, 747 insertions(+), 411 deletions(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 3b78bf6..c5ebd3c 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -266,16 +266,28 @@ impl Node { // === CRYPTO COST PAID HERE === let link_id = self.allocate_link_id(); - let mut conn = PeerConnection::inbound_with_transport( + let conn = PeerConnection::inbound_with_transport( link_id, packet.transport_id, packet.remote_addr.clone(), packet.timestamp_ms, ); + // The control machine drives the handshake, so it is built here, above + // the crypto, carrying the pending connection. It stays a local: it + // enters `peer_machines` only at the promote tails, so a rejected msg1 + // still leaves no registry trace and allocates no index. + let mut machine = PeerMachine::new_inbound(link_id, packet.timestamp_ms); + // The inbound connection carries the transport ID from msg1, but the + // machine's carrier is only written on the outbound dial. Seed it here + // so the promotion hand-off reads it from the surviving carrier, + // matching the connection's own inbound seed. + machine.set_conn_transport_id(packet.transport_id); + machine.set_leg(conn); + let our_keypair = self.identity().keypair(); let noise_msg1 = &packet.data[header.noise_msg1_offset..]; - let msg2_response = match conn.receive_handshake_init( + let msg2_response = match machine.receive_handshake_init( our_keypair, self.startup_epoch(), noise_msg1, @@ -295,7 +307,11 @@ impl Node { }; // Learn peer identity from msg1 - let peer_identity = match conn.expected_identity() { + let peer_identity = match machine + .leg() + .expect("pending connection attached above") + .expected_identity() + { Some(id) => *id, None => { self.msg1_rate_limiter.complete_handshake(); @@ -314,7 +330,10 @@ impl Node { // state; from here the decision reads only `wire` and the snapshot. let wire = WireOutcome { peer_identity, - remote_epoch: conn.remote_epoch(), + remote_epoch: machine + .leg() + .expect("pending connection attached above") + .remote_epoch(), their_index: header.sender_idx, msg2_payload: msg2_response, }; @@ -336,12 +355,6 @@ impl Node { // shared authorize → allocate → send-msg2 → promote tail; the other // variants complete the rate-limiter and return here. The local machine // enters `peer_machines` only at the promote tails. - let mut machine = PeerMachine::new_inbound(link_id, packet.timestamp_ms); - // The inbound leg carries the transport ID from msg1, but the machine's - // carrier is only written on the outbound dial. Seed it here so the - // promotion hand-off reads it from the surviving carrier, matching the - // leg's inbound seed. - machine.set_conn_transport_id(packet.transport_id); let (decision, actions) = machine.inbound_msg1(link_id, &wire, est, packet.timestamp_ms); match decision { InboundDecision::Reject { @@ -443,7 +456,7 @@ impl Node { } // Rekey: process as responder, store new session as pending. - let noise_session = conn.take_session(); + let noise_session = machine.take_session(); let our_new_index = match self.index_allocator.allocate() { Ok(idx) => idx, Err(e) => { @@ -623,7 +636,10 @@ impl Node { // connection, then build + store the framed msg2. The old index was // already freed by `remove_active_peer` above, BEFORE this fresh // allocation — matching the pre-refactor allocation sequence. - conn.set_our_index(our_index); + machine + .leg_mut() + .expect("pending connection attached above") + .set_our_index(our_index); let link = Link::connectionless( link_id, packet.transport_id, @@ -638,9 +654,8 @@ impl Node { // msg1 resend while the connection is still pending. machine.set_conn_handshake_msg2(wire_msg2.clone()); - // Register the machine, carrying the connection + // Register the machine, which already carries the connection // (Promote/Restart tail only). - machine.set_leg(conn); self.peer_machines.insert(link_id, machine); // Execute [SendHandshake, PromoteToActive]. Because the old peer was @@ -771,7 +786,10 @@ impl Node { // Shell registry surgery, in the pre-refactor order: // set indices on the shell connection, insert link / reverse map / // connection, then build + store the framed msg2. - conn.set_our_index(our_index); + machine + .leg_mut() + .expect("pending connection attached above") + .set_our_index(our_index); let link = Link::connectionless( link_id, packet.transport_id, @@ -786,10 +804,9 @@ impl Node { // msg1 resend while the connection is still pending. machine.set_conn_handshake_msg2(wire_msg2.clone()); - // Register the machine, carrying the connection (Promote tail - // only — discarded on every reject/resend/rekey arm per the - // insertion discipline). - machine.set_leg(conn); + // Register the machine, which already carries the connection + // (Promote tail only — discarded on every reject/resend/rekey + // arm per the insertion discipline). self.peer_machines.insert(link_id, machine); // Execute [SendHandshake, PromoteToActive]. The executor frames + @@ -1001,32 +1018,32 @@ impl Node { } let (peer_identity, our_index) = { - let conn = self.leg_mut(&link_id).unwrap(); + let machine = self.peer_machines.get_mut(&link_id).unwrap(); let noise_msg2 = &packet.data[header.noise_msg2_offset..]; - if let Err(e) = conn.complete_handshake(noise_msg2, packet.timestamp_ms) { + if let Err(e) = machine.complete_handshake(noise_msg2, packet.timestamp_ms) { warn!( link_id = %link_id, error = %e, "Handshake completion failed" ); - // Drop the leg's Noise handle (byte-identical point) and record - // the failure on the control machine as `send_failed` — the - // failure state's new home. The machine PHASE stays exactly - // where the old leg-carried failure left it (`SentMsg1`): the - // stale-connection sweep reclaims the leg unconditionally via - // the machine `is_failed()` at the next tick, before any - // projection or resend, so the phase in that window is - // byte-identical to the pre-collapse machine. - conn.mark_failed(); - if let Some(machine) = self.peer_machines.get_mut(&link_id) { - machine.mark_send_failed(); - } + // Drop the Noise handle (byte-identical point) and record the + // failure on the control machine as `send_failed` — the + // failure state's home. The machine PHASE stays exactly where + // the old failure left it (`SentMsg1`): the stale-connection + // sweep reclaims the connection unconditionally via the + // machine `is_failed()` at the next tick, before any + // projection or resend. + machine.mark_failed(); + machine.mark_send_failed(); self.stats_mut() .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); return; } + let conn = machine + .leg_mut() + .expect("pending connection present for msg2 completion"); conn.set_source_addr(packet.remote_addr.clone()); let peer_identity = match conn.expected_identity() { @@ -1042,13 +1059,6 @@ impl Node { (peer_identity, conn.our_index()) }; - // Mirror the leg's completion `touch` on the surviving carrier so the - // connection's last-activity advances at msg2 completion, matching the - // leg's clock. - if let Some(machine) = self.peer_machines.get_mut(&link_id) { - machine.touch_conn(packet.timestamp_ms); - } - if self .authorize_peer( &peer_identity, @@ -1173,7 +1183,7 @@ impl Node { // We're the smaller node. Swap to outbound session + indices. // The peer will keep their inbound session (complement of ours). let outbound_our_index = conn.our_index(); - let outbound_session = conn.take_session(); + let outbound_session = conn.noise_session.take(); let (outbound_session, outbound_our_index) = match ( outbound_session, @@ -1391,12 +1401,13 @@ impl Node { let link_stats = machine.conn_link_stats().clone(); // Verify handshake is complete and extract session - if !connection.has_session() { + if connection.noise_session.is_none() { return Err(NodeError::HandshakeIncomplete(link_id)); } let noise_session = connection - .take_session() + .noise_session + .take() .ok_or(NodeError::NoSession(link_id))?; let our_index = connection diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 22fa219..f8652d6 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -585,7 +585,20 @@ impl Node { // Create connection in handshake phase (outbound knows expected identity) let current_time_ms = Self::now_ms(); - let mut connection = PeerConnection::outbound(link_id, peer_identity, current_time_ms); + let connection = PeerConnection::outbound(link_id, peer_identity, current_time_ms); + + // The control machine drives the handshake, so it takes the connection + // before the crypto runs. The machine was born at dial and persisted in + // `initiate_connection`, so every live caller already has one; recover + // with a fresh one if a direct caller ever skips the dial. + debug_assert!( + self.peer_machines.contains_key(&link_id), + "outbound msg1 prepared for link {link_id} with no dial-time machine" + ); + self.peer_machines + .entry(link_id) + .or_insert_with(|| PeerMachine::new_outbound(link_id, peer_identity, current_time_ms)) + .set_leg(connection); // Allocate a session index for this handshake let our_index = match self.index_allocator.allocate() { @@ -602,23 +615,35 @@ impl Node { // Start the Noise handshake and get message 1 let our_keypair = self.identity().keypair(); - let noise_msg1 = - match connection.start_handshake(our_keypair, self.startup_epoch(), current_time_ms) { - Ok(msg) => msg, - Err(e) => { - // Clean up the index, link, and dial-time machine - let _ = self.index_allocator.free(our_index); - self.links.remove(&link_id); - self.addr_to_link - .remove(&(transport_id, remote_addr.clone())); - self.remove_peer_machine(link_id); - return Err(NodeError::HandshakeFailed(e.to_string())); - } - }; + let startup_epoch = self.startup_epoch(); + let noise_msg1 = match self + .peer_machines + .get_mut(&link_id) + .expect("dial-time machine carries the connection") + .start_handshake(our_keypair, startup_epoch, current_time_ms) + { + Ok(msg) => msg, + Err(e) => { + // Clean up the index, link, and dial-time machine + let _ = self.index_allocator.free(our_index); + self.links.remove(&link_id); + self.addr_to_link + .remove(&(transport_id, remote_addr.clone())); + self.remove_peer_machine(link_id); + return Err(NodeError::HandshakeFailed(e.to_string())); + } + }; // Set index and transport info on the connection - connection.set_our_index(our_index); - connection.set_source_addr(remote_addr.clone()); + { + let conn = self + .peer_machines + .get_mut(&link_id) + .and_then(|machine| machine.leg_mut()) + .expect("dial-time machine carries the connection"); + conn.set_our_index(our_index); + conn.set_source_addr(remote_addr.clone()); + } // Build wire format msg1: [0x01][sender_idx:4 LE][noise_msg1:82] let wire_msg1 = build_msg1(our_index, &noise_msg1); @@ -641,21 +666,13 @@ impl Node { self.pending_outbound .insert((transport_id, our_index.as_u32()), link_id); - // The dial-born machine (persisted in `initiate_connection`) carries - // the prepared connection from here. Every live caller dialed first, - // so the machine exists; recover with a fresh one if a direct caller - // ever skips the dial. - debug_assert!( - self.peer_machines.contains_key(&link_id), - "outbound msg1 prepared for link {link_id} with no dial-time machine" - ); let machine = self .peer_machines - .entry(link_id) - .or_insert_with(|| PeerMachine::new_outbound(link_id, peer_identity, current_time_ms)); + .get_mut(&link_id) + .expect("dial-time machine carries the connection"); // The dial-born machine carrier was stamped at dial; re-stamp it with the - // leg's msg1-prep clock so the surviving `started_at`/`last_activity` - // carry the leg's provenance. The two clocks differ when a connect + // msg1-prep clock so the surviving `started_at`/`last_activity` carry + // the preparation's provenance. The two clocks differ when a connect // round-trip separates dial from msg1 preparation. machine.set_conn_started_at(current_time_ms); machine.touch_conn(current_time_ms); @@ -663,14 +680,14 @@ impl Node { // projects it to the promotion hand-off); holds even if a direct caller // reached here without the dial-time `on_dial` write. machine.set_conn_transport_id(transport_id); - // Record our session index on the surviving carrier — the same index just - // written on the leg above — so the carrier is the single index home on - // the outbound path (the inbound path writes it at authorize). + // Record our session index on the surviving carrier — the same index + // just written on the connection above — so the carrier is the single + // index home on the outbound path (the inbound path writes it at + // authorize). machine.set_conn_our_index(our_index); - // Store the msg1 wire on the surviving carrier (the leg no longer holds - // the resend source); the retransmit driver reads it from here. + // Store the msg1 wire on the surviving carrier (the connection does not + // hold the resend source); the retransmit driver reads it from here. machine.set_conn_handshake_msg1(wire_msg1, first_resend_at_ms); - machine.set_leg(connection); Ok(()) } diff --git a/src/node/tests/acl.rs b/src/node/tests/acl.rs index 1ce5dbc..5a87260 100644 --- a/src/node/tests/acl.rs +++ b/src/node/tests/acl.rs @@ -56,7 +56,7 @@ async fn test_inbound_msg1_denied_by_acl() { node_b.reload_peer_acl().await; let peer_b_identity = PeerIdentity::from_pubkey_full(node_b.identity().pubkey_full()); - let mut conn_a = PeerConnection::outbound(LinkId::new(1), peer_b_identity, 1000); + let mut conn_a = outbound_leg(LinkId::new(1), peer_b_identity, 1000); let noise_msg1 = conn_a .start_handshake(node_a.identity().keypair(), node_a.startup_epoch(), 1000) .unwrap(); @@ -96,7 +96,8 @@ async fn test_outbound_msg2_denied_after_acl_reload() { let keypair_a = node_a.identity().keypair(); let epoch_a = node_a.startup_epoch(); let noise_msg1 = node_a - .get_connection_mut(&link_id_a) + .peer_machines + .get_mut(&link_id_a) .unwrap() .start_handshake(keypair_a, epoch_a, 1000) .unwrap(); @@ -116,7 +117,7 @@ async fn test_outbound_msg2_denied_after_acl_reload() { .pending_outbound .insert((transport_id, our_index_a.as_u32()), link_id_a); - let mut conn_b = PeerConnection::inbound(LinkId::new(2), 1000); + let mut conn_b = inbound_leg(LinkId::new(2), 1000); let responder_epoch = [0x11; 8]; let noise_msg2 = conn_b .receive_handshake_init( @@ -185,3 +186,49 @@ async fn test_outbound_connect_not_denied_by_allowlist_miss() { assert!(!matches!(result, Err(NodeError::AccessDenied(_)))); } + +/// The ACL-rejected arm of the same property the Noise-failure arm pins in +/// `unit.rs`: a msg1 that is admitted by the crypto but denied by the ACL +/// still leaves nothing behind. The control machine is built above the crypto +/// so it can drive the handshake, but it stays a local until a promote tail +/// inserts it, so a denial drops it. +#[tokio::test] +async fn test_acl_rejected_msg1_leaves_no_registry_trace() { + let (dir, mut node_b) = make_acl_node(); + let node_a = make_node(); + + std::fs::write(deny_path(&dir), format!("{}\n", node_a.npub())).unwrap(); + node_b.reload_peer_acl().await; + + let peer_b_identity = PeerIdentity::from_pubkey_full(node_b.identity().pubkey_full()); + let mut conn_a = outbound_leg(LinkId::new(1), peer_b_identity, 1000); + let noise_msg1 = conn_a + .start_handshake(node_a.identity().keypair(), node_a.startup_epoch(), 1000) + .unwrap(); + let wire_msg1 = build_msg1(SessionIndex::new(7), &noise_msg1); + let packet = ReceivedPacket::with_timestamp( + TransportId::new(1), + TransportAddr::from_string("127.0.0.1:5000"), + wire_msg1, + 1000, + ); + + node_b.handle_msg1(packet).await; + + assert!( + node_b.peer_machines.is_empty(), + "an ACL-denied msg1 must leave no control machine behind" + ); + assert_eq!(node_b.connection_count(), 0); + assert_eq!(node_b.peer_count(), 0); + assert_eq!(node_b.link_count(), 0); + assert!( + node_b.peers_by_index.is_empty(), + "an ACL-denied msg1 must allocate no session index" + ); + assert_eq!( + node_b.stats().handshake.bad_state, + 1, + "the denial is attributed to the handshake state-machine counter" + ); +} diff --git a/src/node/tests/establish_chartests.rs b/src/node/tests/establish_chartests.rs index 995e887..42fd01f 100644 --- a/src/node/tests/establish_chartests.rs +++ b/src/node/tests/establish_chartests.rs @@ -49,7 +49,7 @@ fn craft_msg1_wire( use crate::proto::fmp::wire::build_msg1; let peer_b_identity = PeerIdentity::from_pubkey_full(node.identity().pubkey_full()); let link_id = LinkId::new(0x0BAD_C0DE); - let mut conn = PeerConnection::outbound(link_id, peer_b_identity, ts); + let mut conn = outbound_leg(link_id, peer_b_identity, ts); let noise_msg1 = conn .start_handshake(sender.keypair(), epoch, ts) .expect("start_handshake produces noise msg1"); @@ -341,7 +341,8 @@ async fn chartest_msg1_inbound_promote_defers_pending_outbound_to_same_identity( let our_keypair = node.identity().keypair(); let startup_epoch = node.startup_epoch(); let _ = node - .get_connection_mut(&out_link) + .peer_machines + .get_mut(&out_link) .unwrap() .start_handshake(our_keypair, startup_epoch, 1000) .unwrap(); @@ -421,7 +422,8 @@ async fn chartest_msg1_at_cap_with_pending_outbound_bypasses_early_gate() { let our_keypair = node.identity().keypair(); let startup_epoch = node.startup_epoch(); let _ = node - .get_connection_mut(&out_link) + .peer_machines + .get_mut(&out_link) .unwrap() .start_handshake(our_keypair, startup_epoch, 1000) .unwrap(); @@ -524,7 +526,8 @@ async fn chartest_cross_connection_tiebreak_winner_and_loser() { let keypair_a = node_a.identity().keypair(); let epoch_a = node_a.startup_epoch(); let noise_msg1_a = node_a - .get_connection_mut(&link_a_out) + .peer_machines + .get_mut(&link_a_out) .unwrap() .start_handshake(keypair_a, epoch_a, 1000) .unwrap(); @@ -560,7 +563,8 @@ async fn chartest_cross_connection_tiebreak_winner_and_loser() { let keypair_b = node_b.identity().keypair(); let epoch_b = node_b.startup_epoch(); let noise_msg1_b = node_b - .get_connection_mut(&link_b_out) + .peer_machines + .get_mut(&link_b_out) .unwrap() .start_handshake(keypair_b, epoch_b, 1000) .unwrap(); diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index da5e087..dae87c6 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -70,7 +70,8 @@ async fn test_two_node_handshake_udp() { let our_keypair_a = node_a.identity().keypair(); let startup_epoch_a = node_a.startup_epoch(); let noise_msg1 = node_a - .get_connection_mut(&link_id_a) + .peer_machines + .get_mut(&link_id_a) .unwrap() .start_handshake(our_keypair_a, startup_epoch_a, 1000) .unwrap(); @@ -314,7 +315,8 @@ async fn test_run_rx_loop_handshake() { let our_keypair_a = node_a.identity().keypair(); let startup_epoch_a = node_a.startup_epoch(); let noise_msg1 = node_a - .get_connection_mut(&link_id_a) + .peer_machines + .get_mut(&link_id_a) .unwrap() .start_handshake(our_keypair_a, startup_epoch_a, 1000) .unwrap(); @@ -508,7 +510,8 @@ async fn test_cross_connection_both_initiate() { let our_keypair_a = node_a.identity().keypair(); let startup_epoch_a = node_a.startup_epoch(); let noise_msg1_a = node_a - .get_connection_mut(&link_id_a_out) + .peer_machines + .get_mut(&link_id_a_out) .unwrap() .start_handshake(our_keypair_a, startup_epoch_a, 1000) .unwrap(); @@ -544,7 +547,8 @@ async fn test_cross_connection_both_initiate() { let our_keypair_b = node_b.identity().keypair(); let startup_epoch_b = node_b.startup_epoch(); let noise_msg1_b = node_b - .get_connection_mut(&link_id_b_out) + .peer_machines + .get_mut(&link_id_b_out) .unwrap() .start_handshake(our_keypair_b, startup_epoch_b, 1000) .unwrap(); @@ -700,7 +704,8 @@ async fn test_stale_connection_cleanup() { let our_keypair = node.identity().keypair(); let startup_epoch = node.startup_epoch(); let _noise_msg1 = node - .get_connection_mut(&link_id) + .peer_machines + .get_mut(&link_id) .unwrap() .start_handshake(our_keypair, startup_epoch, past_time_ms) .unwrap(); @@ -783,7 +788,8 @@ async fn test_failed_connection_cleanup() { let our_keypair = node.identity().keypair(); let startup_epoch = node.startup_epoch(); let _noise_msg1 = node - .get_connection_mut(&link_id) + .peer_machines + .get_mut(&link_id) .unwrap() .start_handshake(our_keypair, startup_epoch, now_ms) .unwrap(); @@ -853,24 +859,26 @@ async fn test_msg1_stored_for_resend() { .map(|d| d.as_millis() as u64) .unwrap_or(0); let link_id = node.allocate_link_id(); - let mut conn = PeerConnection::outbound(link_id, peer_identity, now_ms); + let mut conn = outbound_leg(link_id, peer_identity, now_ms); let our_index = node.index_allocator.allocate().unwrap(); let our_keypair = node.identity().keypair(); let noise_msg1 = conn .start_handshake(our_keypair, node.startup_epoch(), now_ms) .unwrap(); - conn.set_our_index(our_index); - conn.set_transport_id(transport_id); - conn.set_source_addr(remote_addr.clone()); + conn.leg_mut().unwrap().set_our_index(our_index); + conn.leg_mut().unwrap().set_transport_id(transport_id); + conn.leg_mut().unwrap().set_source_addr(remote_addr.clone()); // Build wire msg1 and store it (as initiate_peer_connection does) let wire_msg1 = build_msg1(our_index, &noise_msg1); let resend_interval = node.config().node.rate_limit.handshake_resend_interval_ms; - conn.set_handshake_msg1(wire_msg1.clone(), now_ms + resend_interval); + conn.leg_mut() + .unwrap() + .set_handshake_msg1(wire_msg1.clone(), now_ms + resend_interval); // Verify stored msg1 matches what was built - assert_eq!(conn.handshake_msg1().unwrap(), &wire_msg1); + assert_eq!(conn.leg().unwrap().handshake_msg1().unwrap(), &wire_msg1); } /// Test that resend scheduling respects max_resends and backoff. @@ -884,20 +892,22 @@ async fn test_resend_scheduling() { let now_ms = 100_000u64; // Use a fixed time for predictable testing let link_id = node.allocate_link_id(); - let mut conn = PeerConnection::outbound(link_id, peer_identity, now_ms); + let mut conn = outbound_leg(link_id, peer_identity, now_ms); let our_index = node.index_allocator.allocate().unwrap(); let our_keypair = node.identity().keypair(); let noise_msg1 = conn .start_handshake(our_keypair, node.startup_epoch(), now_ms) .unwrap(); - conn.set_our_index(our_index); - conn.set_transport_id(transport_id); - conn.set_source_addr(remote_addr.clone()); + conn.leg_mut().unwrap().set_our_index(our_index); + conn.leg_mut().unwrap().set_transport_id(transport_id); + conn.leg_mut().unwrap().set_source_addr(remote_addr.clone()); // Store msg1 with first resend at now + 1000ms let wire_msg1 = crate::proto::fmp::wire::build_msg1(our_index, &noise_msg1); - conn.set_handshake_msg1(wire_msg1.clone(), now_ms + 1000); + conn.leg_mut() + .unwrap() + .set_handshake_msg1(wire_msg1.clone(), now_ms + 1000); let link = Link::connectionless( link_id, @@ -931,7 +941,7 @@ async fn test_resend_scheduling() { // The msg1 wire lives on the machine's carrier (the retransmit driver's // resend source), mirroring `prepare_outbound_msg1`. machine.set_conn_handshake_msg1(wire_msg1, now_ms + 1000); - machine.set_leg(conn); + machine.set_leg(conn.take_leg().unwrap()); node.peer_machines.insert(link_id, machine); node.peer_timers.entry(link_id).or_default().insert( crate::peer::machine::TimerKind::HandshakeRetransmit, @@ -970,15 +980,15 @@ async fn test_handshake_timeout_drive() { let dial_ms = 1000u64; let link_id = node.allocate_link_id(); - let mut conn = PeerConnection::outbound(link_id, peer_identity, dial_ms); + let mut conn = outbound_leg(link_id, peer_identity, dial_ms); let our_index = node.index_allocator.allocate().unwrap(); let our_keypair = node.identity().keypair(); let _ = conn .start_handshake(our_keypair, node.startup_epoch(), dial_ms) .unwrap(); - conn.set_our_index(our_index); - conn.set_transport_id(transport_id); - conn.set_source_addr(remote_addr.clone()); + conn.leg_mut().unwrap().set_our_index(our_index); + conn.leg_mut().unwrap().set_transport_id(transport_id); + conn.leg_mut().unwrap().set_source_addr(remote_addr.clone()); let link = Link::connectionless( link_id, @@ -1007,7 +1017,7 @@ async fn test_handshake_timeout_drive() { dial_ms, &mut node.index_allocator, ); - machine.set_leg(conn); + machine.set_leg(conn.take_leg().unwrap()); node.peer_machines.insert(link_id, machine); node.peer_timers.entry(link_id).or_default().insert( crate::peer::machine::TimerKind::HandshakeTimeout, diff --git a/src/node/tests/mod.rs b/src/node/tests/mod.rs index dc75785..a302e26 100644 --- a/src/node/tests/mod.rs +++ b/src/node/tests/mod.rs @@ -84,6 +84,30 @@ pub(super) fn make_peer_identity() -> PeerIdentity { PeerIdentity::from_pubkey(identity.pubkey()) } +/// A control machine carrying a fresh outbound connection, for tests that +/// drive one end of a handshake without a whole node behind it. The machine +/// owns the handshake operations, so it is what the crypto runs on. +pub(super) fn outbound_leg( + link_id: LinkId, + expected_identity: PeerIdentity, + current_time_ms: u64, +) -> PeerMachine { + let mut machine = PeerMachine::new_outbound(link_id, expected_identity, current_time_ms); + machine.set_leg(PeerConnection::outbound( + link_id, + expected_identity, + current_time_ms, + )); + machine +} + +/// The responder twin of [`outbound_leg`]. +pub(super) fn inbound_leg(link_id: LinkId, current_time_ms: u64) -> PeerMachine { + let mut machine = PeerMachine::new_inbound(link_id, current_time_ms); + machine.set_leg(PeerConnection::inbound(link_id, current_time_ms)); + machine +} + /// Seed a control machine whose leg carries a completed Noise IK handshake. /// /// Returns the peer identity. The leg is outbound, in Complete state, with @@ -138,13 +162,14 @@ pub(super) fn seed_completed_connection_with( let our_keypair = node.identity().keypair(); let startup_epoch = node.startup_epoch(); let msg1 = node - .get_connection_mut(&link_id) + .peer_machines + .get_mut(&link_id) .unwrap() .start_handshake(our_keypair, startup_epoch, current_time_ms) .unwrap(); // Run responder side to generate msg2 - let mut resp_conn = PeerConnection::inbound(LinkId::new(999), current_time_ms); + let mut resp_conn = inbound_leg(LinkId::new(999), current_time_ms); let peer_keypair = peer_identity_full.keypair(); let mut resp_epoch = [0u8; 8]; rand::Rng::fill_bytes(&mut rand::rng(), &mut resp_epoch); @@ -153,7 +178,8 @@ pub(super) fn seed_completed_connection_with( .unwrap(); // Complete initiator handshake - node.get_connection_mut(&link_id) + node.peer_machines + .get_mut(&link_id) .unwrap() .complete_handshake(&msg2, current_time_ms) .unwrap(); diff --git a/src/node/tests/spanning_tree.rs b/src/node/tests/spanning_tree.rs index cb6e8eb..84af809 100644 --- a/src/node/tests/spanning_tree.rs +++ b/src/node/tests/spanning_tree.rs @@ -131,7 +131,8 @@ pub(super) async fn initiate_handshake(nodes: &mut [TestNode], i: usize, j: usiz let startup_epoch = initiator.node.startup_epoch(); let noise_msg1 = initiator .node - .get_connection_mut(&link_id) + .peer_machines + .get_mut(&link_id) .unwrap() .start_handshake(our_keypair, startup_epoch, 1000) .unwrap(); diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index da4b34a..adccce9 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -808,7 +808,8 @@ fn test_promote_cleans_up_pending_outbound_to_same_peer() { let our_keypair = node.identity().keypair(); let startup_epoch = node.startup_epoch(); let _msg1 = node - .get_connection_mut(&pending_link_id) + .peer_machines + .get_mut(&pending_link_id) .unwrap() .start_handshake(our_keypair, startup_epoch, pending_time_ms) .unwrap(); @@ -850,13 +851,14 @@ fn test_promote_cleans_up_pending_outbound_to_same_peer() { let our_keypair = node.identity().keypair(); let startup_epoch = node.startup_epoch(); let msg1 = node - .get_connection_mut(&completing_link_id) + .peer_machines + .get_mut(&completing_link_id) .unwrap() .start_handshake(our_keypair, startup_epoch, completing_time_ms) .unwrap(); // B responds - let mut resp_conn = PeerConnection::inbound(LinkId::new(999), completing_time_ms); + let mut resp_conn = inbound_leg(LinkId::new(999), completing_time_ms); let peer_keypair = peer_b_full.keypair(); let mut resp_epoch = [0u8; 8]; rand::Rng::fill_bytes(&mut rand::rng(), &mut resp_epoch); @@ -864,7 +866,8 @@ fn test_promote_cleans_up_pending_outbound_to_same_peer() { .receive_handshake_init(peer_keypair, resp_epoch, &msg1, completing_time_ms) .unwrap(); - node.get_connection_mut(&completing_link_id) + node.peer_machines + .get_mut(&completing_link_id) .unwrap() .complete_handshake(&msg2, completing_time_ms) .unwrap(); @@ -2083,7 +2086,7 @@ async fn craft_and_send_msg1( let sender_node_addr = *sender_pubkey_id.node_addr(); let link_id = LinkId::new(0xDEAD_BEEF); - let mut conn = PeerConnection::outbound(link_id, peer_b_identity, timestamp_ms); + let mut conn = outbound_leg(link_id, peer_b_identity, timestamp_ms); let sender_keypair = sender_identity.keypair(); let mut startup_epoch = [0u8; 8]; @@ -2345,3 +2348,130 @@ async fn start_skips_system_tun_when_app_owned() { node.stop().await.unwrap(); } + +/// A connection whose handshake failed is retained with BOTH Noise handles +/// empty, and the stale-connection sweep depends on that: presence of the +/// pending connection — not presence of a handle — is what marks a machine as +/// handshake-phase. If presence were ever derived from the handles, every +/// failed connection would become invisible to the sweep and leak forever, +/// holding a peering-budget slot and a wrong `connection_count` permanently. +#[test] +fn test_failed_connection_is_retained_and_reaped() { + use crate::proto::fmp::LifecycleView; + + let mut node = make_node(); + let link_id = LinkId::new(1); + let peer_identity = make_peer_identity(); + + node.seed_handshake_machine(HandshakeSeed::outbound(link_id, peer_identity, 1000)) + .unwrap(); + let our_keypair = node.identity().keypair(); + let startup_epoch = node.startup_epoch(); + node.peer_machines + .get_mut(&link_id) + .unwrap() + .start_handshake(our_keypair, startup_epoch, 1000) + .unwrap(); + + // The send of that stored initiation fails. + let machine = node.peer_machines.get_mut(&link_id).unwrap(); + machine.mark_failed(); + machine.mark_send_failed(); + + // Both handles are now empty — the initiation handle was dropped and no + // session was ever reached — yet the connection is deliberately retained. + let leg = node.peer_machines.get(&link_id).unwrap().leg().unwrap(); + assert!( + leg.noise_handshake.is_none() && leg.noise_session.is_none(), + "a failed connection holds neither handle" + ); + + // (a) it still counts while it waits for the sweep + assert_eq!( + node.connection_count(), + 1, + "a failed connection stays counted until it is reaped" + ); + + // (b) the sweep yields it + let stale = node.stale_connections(2000, 30_000); + assert_eq!( + stale.len(), + 1, + "the sweep must see a failed connection despite its empty handles" + ); + assert_eq!(stale[0].link, link_id); + + // (c) reaping it clears the carrier + node.remove_peer_machine(link_id); + assert_eq!(node.connection_count(), 0); +} + +/// A msg1 that fails Noise processing must leave no trace in the registry. +/// The control machine is built above the crypto so it can drive the +/// handshake, but it stays a local until a promote tail inserts it — a +/// rejected msg1 drops it. +#[tokio::test] +async fn test_rejected_msg1_leaves_no_registry_trace() { + let mut node = make_node(); + + // Well-formed framing, garbage Noise payload: processing fails. + let wire_msg1 = crate::proto::fmp::wire::build_msg1( + SessionIndex::new(7), + &[0u8; crate::noise::HANDSHAKE_MSG1_SIZE], + ); + let packet = ReceivedPacket::with_timestamp( + TransportId::new(1), + TransportAddr::from_string("127.0.0.1:5000"), + wire_msg1, + 1000, + ); + + node.handle_msg1(packet).await; + + assert!( + node.peer_machines.is_empty(), + "a rejected msg1 must leave no control machine behind" + ); + assert_eq!(node.connection_count(), 0); + assert_eq!(node.peer_count(), 0); + assert_eq!(node.link_count(), 0); + assert!( + node.peers_by_index.is_empty(), + "a rejected msg1 must allocate no session index" + ); + assert_eq!( + node.stats().handshake.bad_state, + 1, + "the rejection is attributed to the handshake state-machine counter" + ); +} + +/// The outbound path registers its control machine at dial, before msg1 is +/// prepared, so a preparation failure has to unwind that registration rather +/// than drop a local. +#[tokio::test] +async fn test_failed_msg1_preparation_unwinds_the_dial_machine() { + let mut node = make_node(); + let link_id = LinkId::new(1); + let transport_id = TransportId::new(1); + let remote_addr = TransportAddr::from_string("127.0.0.1:5000"); + let peer_identity = make_peer_identity(); + + // Stand in for the dial: the machine exists before msg1 is prepared. + node.peer_machines.insert( + link_id, + PeerMachine::new_outbound(link_id, peer_identity, 1000), + ); + // Force the index allocation inside msg1 preparation to fail. + node.index_allocator = crate::utils::index::IndexAllocator::with_max_attempts(0); + + let result = node.prepare_outbound_msg1(link_id, transport_id, &remote_addr, peer_identity); + + assert!(matches!(result, Err(NodeError::IndexAllocationFailed(_)))); + assert!( + !node.peer_machines.contains_key(&link_id), + "a failed msg1 preparation must unwind the dial-time machine" + ); + assert_eq!(node.connection_count(), 0); +} diff --git a/src/peer/connection.rs b/src/peer/connection.rs index ea689bd..6eb00d0 100644 --- a/src/peer/connection.rs +++ b/src/peer/connection.rs @@ -2,17 +2,16 @@ //! //! Represents an in-progress connection before authentication completes. //! PeerConnection tracks the Noise IK handshake and transitions to -//! ActivePeer upon successful authentication. The handshake *phase* (initial / -//! sent_msg1 / complete / failed) is no longer tracked here — it lives on the -//! per-peer control machine; the leg's crypto methods gate on the presence of -//! their Noise handles (`noise_handshake` / `noise_session`) directly. +//! ActivePeer upon successful authentication. Neither the handshake *phase* +//! (initial / sent_msg1 / complete / failed) nor the handshake operations are +//! tracked here — both live on the per-peer control machine, which drives the +//! Noise handles held below. use crate::PeerIdentity; -use crate::noise::{self, NoiseError, NoiseSession}; +use crate::noise::{self, NoiseSession}; use crate::proto::fmp::ConnectionState; use crate::transport::{LinkDirection, LinkId, TransportAddr, TransportId}; use crate::utils::index::SessionIndex; -use secp256k1::Keypair; use std::fmt; /// A connection in the handshake phase, before authentication completes. @@ -23,17 +22,21 @@ use std::fmt; /// This is the shell holder for the FMP crypto/state split: the pure /// connection bookkeeping lives in [`ConnectionState`] (`proto::fmp::state`), /// and the two Noise crypto handles stay here beside it. Pure public methods -/// delegate to `self.state`; the XX transition methods drive the crypto and -/// write results back through `self.state`'s setters. +/// delegate to `self.state`; the control machine drives the handles and +/// records each result here and on its own carrier. pub struct PeerConnection { /// Pure, runtime-agnostic connection bookkeeping. state: ConnectionState, /// Noise handshake state (consumes on completion). - noise_handshake: Option, + /// + /// Driven by the control machine, which owns the handshake operations. + pub(crate) noise_handshake: Option, /// Completed Noise session (available after handshake complete). - noise_session: Option, + /// + /// Driven by the control machine, which owns the handshake operations. + pub(crate) noise_session: Option, } impl PeerConnection { @@ -202,146 +205,13 @@ impl PeerConnection { self.state.handshake_msg2() } - // === Noise Handshake Operations (shell: drives crypto, updates pure state) === + // === Crypto handle plumbing (the control machine drives the handshake) === - /// Start the handshake as initiator and generate message 1. - /// - /// For outbound connections only. Returns the handshake message to send. - /// The epoch is our startup epoch, encrypted into msg1 for restart detection. - pub fn start_handshake( - &mut self, - our_keypair: Keypair, - epoch: [u8; 8], - current_time_ms: u64, - ) -> Result, NoiseError> { - if self.state.direction() != LinkDirection::Outbound { - return Err(NoiseError::WrongState { - expected: "outbound connection".to_string(), - got: "inbound connection".to_string(), - }); - } - - let remote_static = self - .state - .expected_identity() - .expect("outbound must have expected identity") - .pubkey_full(); - - let mut hs = noise::HandshakeState::new_initiator(our_keypair, remote_static); - hs.set_local_epoch(epoch); - let msg1 = hs.write_message_1()?; - - self.noise_handshake = Some(hs); - self.state.touch(current_time_ms); - - Ok(msg1) - } - - /// Initialize responder and process incoming message 1. - /// - /// For inbound connections only. Returns the handshake message 2 to send. - /// The epoch is our startup epoch, encrypted into msg2 for restart detection. - pub fn receive_handshake_init( - &mut self, - our_keypair: Keypair, - epoch: [u8; 8], - message: &[u8], - current_time_ms: u64, - ) -> Result, NoiseError> { - if self.state.direction() != LinkDirection::Inbound { - return Err(NoiseError::WrongState { - expected: "inbound connection".to_string(), - got: "outbound connection".to_string(), - }); - } - - let mut hs = noise::HandshakeState::new_responder(our_keypair); - hs.set_local_epoch(epoch); - - // Process message 1 (this reveals the initiator's identity and epoch) - hs.read_message_1(message)?; - - // Extract the discovered identity from the crypto and record it as - // pure data on the state. - let remote_static = *hs - .remote_static() - .expect("remote static available after msg1"); - self.state - .set_expected_identity(PeerIdentity::from_pubkey_full(remote_static)); - - // Capture remote epoch from msg1 - self.state.set_remote_epoch(hs.remote_epoch()); - - // Generate message 2 - let msg2 = hs.write_message_2()?; - - // Handshake is complete for responder - let session = hs.into_session()?; - self.noise_session = Some(session); - self.state.touch(current_time_ms); - - Ok(msg2) - } - - /// Complete the handshake by processing message 2. - /// - /// For outbound connections only (initiator completing handshake). - pub fn complete_handshake( - &mut self, - message: &[u8], - current_time_ms: u64, - ) -> Result<(), NoiseError> { - // The leg is at `SentMsg1` iff its Noise handshake handle is present - // (set by `start_handshake`, taken here on completion). Gate on the - // handle directly now that the phase enum is gone — byte-equivalent to - // the old `!= SentMsg1` guard for every reachable transition. - if self.noise_handshake.is_none() { - return Err(NoiseError::WrongState { - expected: "sent_msg1 state".to_string(), - got: "no active handshake".to_string(), - }); - } - - let mut hs = self - .noise_handshake - .take() - .expect("noise handshake must exist in SentMsg1 state"); - - hs.read_message_2(message)?; - - // Capture remote epoch from msg2 - self.state.set_remote_epoch(hs.remote_epoch()); - - let session = hs.into_session()?; - self.noise_session = Some(session); - self.state.touch(current_time_ms); - - Ok(()) - } - - /// Take the completed Noise session. - /// - /// Returns the NoiseSession for use in ActivePeer. Can only be called - /// once after handshake completes. - pub fn take_session(&mut self) -> Option { - // The session exists iff the handshake reached `Complete`, so taking it - // unconditionally is byte-equivalent to the old `== Complete` gate. - self.noise_session.take() - } - - /// Check if we have a completed session ready to take. - pub fn has_session(&self) -> bool { - self.noise_session.is_some() - } - - // === State Transitions (for manual control if needed) === - - /// Drop the shell-owned crypto handshake handle. The failure *state* now - /// lives on the control machine (`PeerMachine`); this only releases the - /// leg's Noise handle at the identical point it was released before, so a - /// subsequent `complete_handshake` on this leg still reports `WrongState`. - pub fn mark_failed(&mut self) { - self.noise_handshake = None; + /// Mutable access to the pure bookkeeping, so the control machine's + /// handshake operations can record their results here as well as on the + /// surviving carrier. + pub(crate) fn state_mut(&mut self) -> &mut ConnectionState { + &mut self.state } // === Validation === @@ -373,108 +243,12 @@ impl fmt::Debug for PeerConnection { mod tests { use super::*; use crate::Identity; - use rand::Rng; fn make_peer_identity() -> PeerIdentity { let identity = Identity::generate(); PeerIdentity::from_pubkey(identity.pubkey()) } - fn make_keypair() -> Keypair { - let identity = Identity::generate(); - identity.keypair() - } - - fn make_epoch() -> [u8; 8] { - let mut epoch = [0u8; 8]; - rand::rng().fill_bytes(&mut epoch); - epoch - } - - #[test] - fn test_outbound_connection() { - let identity = make_peer_identity(); - let conn = PeerConnection::outbound(LinkId::new(1), identity, 1000); - - assert!(conn.is_outbound()); - assert!(!conn.is_inbound()); - assert!(!conn.has_session()); - assert!(conn.expected_identity().is_some()); - assert_eq!(conn.started_at(), 1000); - } - - #[test] - fn test_inbound_connection() { - let conn = PeerConnection::inbound(LinkId::new(2), 2000); - - assert!(conn.is_inbound()); - assert!(!conn.is_outbound()); - assert!(!conn.has_session()); - assert!(conn.expected_identity().is_none()); - assert_eq!(conn.started_at(), 2000); - } - - #[test] - fn test_full_handshake_flow() { - // Create identities - let initiator_identity = Identity::generate(); - let responder_identity = Identity::generate(); - - let initiator_keypair = initiator_identity.keypair(); - let responder_keypair = responder_identity.keypair(); - let initiator_epoch = make_epoch(); - let responder_epoch = make_epoch(); - - // Use from_pubkey_full to preserve parity for ECDH - let responder_peer_id = PeerIdentity::from_pubkey_full(responder_identity.pubkey_full()); - - // Create connections - let mut initiator_conn = PeerConnection::outbound(LinkId::new(1), responder_peer_id, 1000); - let mut responder_conn = PeerConnection::inbound(LinkId::new(2), 1000); - - // Initiator starts handshake - let msg1 = initiator_conn - .start_handshake(initiator_keypair, initiator_epoch, 1100) - .unwrap(); - // Post-msg1 the initiator holds an in-flight handshake, not yet a session. - assert!(!initiator_conn.has_session()); - - // Responder processes msg1 and sends msg2 - let msg2 = responder_conn - .receive_handshake_init(responder_keypair, responder_epoch, &msg1, 1200) - .unwrap(); - // The IK responder completes in one step: it now holds a session. - assert!(responder_conn.has_session()); - - // Responder learned initiator's identity - let discovered = responder_conn.expected_identity().unwrap(); - assert_eq!(discovered.pubkey(), initiator_identity.pubkey()); - - // Responder learned initiator's epoch - assert_eq!(responder_conn.remote_epoch(), Some(initiator_epoch)); - - // Initiator completes handshake - initiator_conn.complete_handshake(&msg2, 1300).unwrap(); - assert!(initiator_conn.has_session()); - - // Initiator learned responder's epoch - assert_eq!(initiator_conn.remote_epoch(), Some(responder_epoch)); - - // Both have sessions - assert!(initiator_conn.has_session()); - assert!(responder_conn.has_session()); - - // Take and verify sessions work - let mut init_session = initiator_conn.take_session().unwrap(); - let mut resp_session = responder_conn.take_session().unwrap(); - - // Encrypt/decrypt test - let plaintext = b"test message"; - let ciphertext = init_session.encrypt(plaintext).unwrap(); - let decrypted = resp_session.decrypt(&ciphertext).unwrap(); - assert_eq!(decrypted, plaintext); - } - #[test] fn test_connection_timing() { let identity = make_peer_identity(); @@ -485,43 +259,4 @@ mod tests { assert!(!conn.is_timed_out(1500, 1000)); assert!(conn.is_timed_out(2500, 1000)); } - - #[test] - fn test_connection_failure() { - // `mark_failed` releases the leg's Noise handshake handle. The failure - // *state* now lives on the control machine, but the leg-local effect is - // still observable: a completion attempt afterward reports `WrongState` - // (the handle-presence gate) and no session is produced. - let identity = make_peer_identity(); - let keypair = make_keypair(); - let mut conn = PeerConnection::outbound(LinkId::new(1), identity, 1000); - conn.start_handshake(keypair, make_epoch(), 1100).unwrap(); - - conn.mark_failed(); - - assert!(!conn.has_session()); - assert!(conn.complete_handshake(&[0u8; 96], 1200).is_err()); - } - - #[test] - fn test_wrong_direction_errors() { - let identity = make_peer_identity(); - let keypair = make_keypair(); - - // Outbound can't receive_handshake_init - let mut outbound = PeerConnection::outbound(LinkId::new(1), identity, 1000); - assert!( - outbound - .receive_handshake_init(keypair, make_epoch(), &[0u8; 106], 1100) - .is_err() - ); - - // Inbound can't start_handshake - let mut inbound = PeerConnection::inbound(LinkId::new(2), 1000); - assert!( - inbound - .start_handshake(keypair, make_epoch(), 1100) - .is_err() - ); - } } diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 91941ea..6c3157c 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -56,6 +56,7 @@ #![allow(dead_code)] +use crate::noise::{self, NoiseError, NoiseSession}; use crate::peer::PeerConnection; use crate::proto::fmp::{ ConnAction, ConnSnapshot, ConnectionState, EstablishSnapshot, Fmp, InboundDecision, @@ -63,9 +64,10 @@ use crate::proto::fmp::{ RekeyResendSnapshot, WireOutcome, }; use crate::proto::link::LinkMessageType; -use crate::transport::{LinkId, LinkStats, TransportAddr, TransportId}; +use crate::transport::{LinkDirection, LinkId, LinkStats, TransportAddr, TransportId}; use crate::utils::index::{IndexAllocator, SessionIndex}; use crate::{NodeAddr, PeerIdentity}; +use secp256k1::Keypair; // ============================================================================ // Timing placeholders @@ -406,6 +408,15 @@ pub(crate) enum PeerAction { // The machine (control tier) // ============================================================================ +/// The handshake operations are only reachable while a pending connection is +/// attached; every path that drives the crypto attaches it first. +fn no_pending_connection() -> NoiseError { + NoiseError::WrongState { + expected: "attached connection".to_string(), + got: "no connection".to_string(), + } +} + /// Per-peer control FSM. Holds control-tier lifecycle state only; the /// send-critical state is published as `PeerSendState` and mutated via the /// emitted [`PeerAction`]s. @@ -413,11 +424,12 @@ pub(crate) struct PeerMachine { state: PeerState, link: LinkId, identity: Option, - /// The pending handshake connection this machine owns while the leg is in - /// the handshake window. `None` before the connection is built (the dial + /// The pending handshake connection this machine owns while it is in the + /// handshake window. `None` before the connection is built (the dial /// window) and after promotion consumes it (the machine survives as the - /// active peer's control machine). Pure storage — the machine never reads - /// or drives it; the shell reaches it through the accessors below. + /// active peer's control machine). Its bookkeeping is storage the shell + /// reaches through the accessors below; its Noise handles are driven by + /// this machine's handshake operations. leg: Option, /// Pure handshake-phase bookkeeping (link/direction/indices/transport/ /// stored handshake bytes/epoch). Reused verbatim from the FMP state core. @@ -542,6 +554,181 @@ impl PeerMachine { self.leg = Some(leg); } + // === Noise handshake operations === + // + // Mechanism, not decision: these are called by the shell, are never + // reached from `step()`, and no event triggers them. Each drives the + // Noise crypto on the pending connection and records the results on both + // that connection and the surviving carrier, so a reader of either sees + // the same value at the same point. + + /// Start the handshake as initiator and generate message 1. + /// + /// For outbound connections only. Returns the handshake message to send. + /// The epoch is our startup epoch, encrypted into msg1 for restart detection. + pub(crate) fn start_handshake( + &mut self, + our_keypair: Keypair, + epoch: [u8; 8], + current_time_ms: u64, + ) -> Result, NoiseError> { + let msg1 = { + let leg = self.leg.as_mut().ok_or_else(no_pending_connection)?; + + if leg.direction() != LinkDirection::Outbound { + return Err(NoiseError::WrongState { + expected: "outbound connection".to_string(), + got: "inbound connection".to_string(), + }); + } + + let remote_static = leg + .expected_identity() + .expect("outbound must have expected identity") + .pubkey_full(); + + let mut hs = noise::HandshakeState::new_initiator(our_keypair, remote_static); + hs.set_local_epoch(epoch); + let msg1 = hs.write_message_1()?; + + leg.noise_handshake = Some(hs); + leg.state_mut().touch(current_time_ms); + + msg1 + }; + self.conn.touch(current_time_ms); + + Ok(msg1) + } + + /// Initialize responder and process incoming message 1. + /// + /// For inbound connections only. Returns the handshake message 2 to send. + /// The epoch is our startup epoch, encrypted into msg2 for restart detection. + pub(crate) fn receive_handshake_init( + &mut self, + our_keypair: Keypair, + epoch: [u8; 8], + message: &[u8], + current_time_ms: u64, + ) -> Result, NoiseError> { + let (msg2, learned_identity, remote_epoch) = { + let leg = self.leg.as_mut().ok_or_else(no_pending_connection)?; + + if leg.direction() != LinkDirection::Inbound { + return Err(NoiseError::WrongState { + expected: "inbound connection".to_string(), + got: "outbound connection".to_string(), + }); + } + + let mut hs = noise::HandshakeState::new_responder(our_keypair); + hs.set_local_epoch(epoch); + + // Process message 1 (this reveals the initiator's identity and epoch) + hs.read_message_1(message)?; + + // Extract the discovered identity from the crypto and record it as + // pure data on the state. + let remote_static = *hs + .remote_static() + .expect("remote static available after msg1"); + let learned_identity = PeerIdentity::from_pubkey_full(remote_static); + leg.state_mut().set_expected_identity(learned_identity); + + // Capture remote epoch from msg1 + let remote_epoch = hs.remote_epoch(); + leg.state_mut().set_remote_epoch(remote_epoch); + + // Generate message 2 + let msg2 = hs.write_message_2()?; + + // Handshake is complete for responder + let session = hs.into_session()?; + leg.noise_session = Some(session); + leg.state_mut().touch(current_time_ms); + + (msg2, learned_identity, remote_epoch) + }; + self.conn.set_expected_identity(learned_identity); + self.conn.set_remote_epoch(remote_epoch); + self.conn.touch(current_time_ms); + + Ok(msg2) + } + + /// Complete the handshake by processing message 2. + /// + /// For outbound connections only (initiator completing handshake). + pub(crate) fn complete_handshake( + &mut self, + message: &[u8], + current_time_ms: u64, + ) -> Result<(), NoiseError> { + let remote_epoch = { + let leg = self.leg.as_mut().ok_or_else(no_pending_connection)?; + + // The connection is at `SentMsg1` iff its Noise handshake handle is + // present (set by `start_handshake`, taken here on completion). + // Gating on the handle directly is byte-equivalent to the old + // `!= SentMsg1` guard for every reachable transition. + if leg.noise_handshake.is_none() { + return Err(NoiseError::WrongState { + expected: "sent_msg1 state".to_string(), + got: "no active handshake".to_string(), + }); + } + + let mut hs = leg + .noise_handshake + .take() + .expect("noise handshake must exist in SentMsg1 state"); + + hs.read_message_2(message)?; + + // Capture remote epoch from msg2 + let remote_epoch = hs.remote_epoch(); + leg.state_mut().set_remote_epoch(remote_epoch); + + let session = hs.into_session()?; + leg.noise_session = Some(session); + leg.state_mut().touch(current_time_ms); + + remote_epoch + }; + self.conn.set_remote_epoch(remote_epoch); + self.conn.touch(current_time_ms); + + Ok(()) + } + + /// Take the completed Noise session. + /// + /// Returns the NoiseSession for use in ActivePeer. Can only be called + /// once after the handshake completes. + pub(crate) fn take_session(&mut self) -> Option { + // The session exists iff the handshake reached `Complete`, so taking it + // unconditionally is byte-equivalent to the old `== Complete` gate. + self.leg.as_mut().and_then(|leg| leg.noise_session.take()) + } + + /// Check if we have a completed session ready to take. + pub(crate) fn has_session(&self) -> bool { + self.leg + .as_ref() + .is_some_and(|leg| leg.noise_session.is_some()) + } + + /// Drop the crypto handshake handle. The failure *state* lives on this + /// machine; this only releases the Noise handle at the identical point it + /// was released before, so a subsequent `complete_handshake` still reports + /// `WrongState`. + pub(crate) fn mark_failed(&mut self) { + if let Some(leg) = self.leg.as_mut() { + leg.noise_handshake = None; + } + } + /// The session index we allocated for this peer, read from the surviving /// carrier. Populated once the index is allocated on either establish path /// (inbound at `on_authorized`, outbound at msg1 preparation). `None` before @@ -861,11 +1048,9 @@ impl PeerMachine { /// (`is_handshaking_sent_msg1`) survives until the sweep, and no timer /// actions are emitted. fn on_handshake_send_failed(&mut self) -> Vec { - if let Some(leg) = self.leg.as_mut() { - // Drop the leg's Noise handshake handle at the identical point as - // before; the failure *state* is recorded on the machine. - leg.mark_failed(); - } + // Drop the Noise handshake handle at the identical point as before; + // the failure *state* is recorded on the machine. + self.mark_failed(); self.send_failed = true; Vec::new() } @@ -2970,6 +3155,176 @@ mod tests { assert_eq!(m.state(), PeerState::Active { addr }); assert_eq!(alloc.count(), 0); } + + // ---- Moved from `PeerConnection`'s own test module --------------------- + // These exercise the Noise handshake operations, which now live on the + // control machine. Constructed as a machine with a leg attached; the + // assertions are unchanged. + + fn make_peer_identity() -> PeerIdentity { + let identity = Identity::generate(); + PeerIdentity::from_pubkey(identity.pubkey()) + } + + fn make_keypair() -> Keypair { + let identity = Identity::generate(); + identity.keypair() + } + + fn make_epoch() -> [u8; 8] { + let mut epoch = [0u8; 8]; + rand::Rng::fill_bytes(&mut rand::rng(), &mut epoch); + epoch + } + + fn outbound_leg( + link_id: LinkId, + expected_identity: PeerIdentity, + current_time_ms: u64, + ) -> PeerMachine { + let mut machine = PeerMachine::new_outbound(link_id, expected_identity, current_time_ms); + machine.set_leg(PeerConnection::outbound( + link_id, + expected_identity, + current_time_ms, + )); + machine + } + + fn inbound_leg(link_id: LinkId, current_time_ms: u64) -> PeerMachine { + let mut machine = PeerMachine::new_inbound(link_id, current_time_ms); + machine.set_leg(PeerConnection::inbound(link_id, current_time_ms)); + machine + } + + #[test] + fn test_outbound_connection() { + let identity = make_peer_identity(); + let conn = outbound_leg(LinkId::new(1), identity, 1000); + + assert!(conn.leg().unwrap().is_outbound()); + assert!(!conn.leg().unwrap().is_inbound()); + assert!(!conn.has_session()); + assert!(conn.leg().unwrap().expected_identity().is_some()); + assert_eq!(conn.leg().unwrap().started_at(), 1000); + } + + #[test] + fn test_inbound_connection() { + let conn = inbound_leg(LinkId::new(2), 2000); + + assert!(conn.leg().unwrap().is_inbound()); + assert!(!conn.leg().unwrap().is_outbound()); + assert!(!conn.has_session()); + assert!(conn.leg().unwrap().expected_identity().is_none()); + assert_eq!(conn.leg().unwrap().started_at(), 2000); + } + + #[test] + fn test_full_handshake_flow() { + // Create identities + let initiator_identity = Identity::generate(); + let responder_identity = Identity::generate(); + + let initiator_keypair = initiator_identity.keypair(); + let responder_keypair = responder_identity.keypair(); + let initiator_epoch = make_epoch(); + let responder_epoch = make_epoch(); + + // Use from_pubkey_full to preserve parity for ECDH + let responder_peer_id = PeerIdentity::from_pubkey_full(responder_identity.pubkey_full()); + + // Create connections + let mut initiator_conn = outbound_leg(LinkId::new(1), responder_peer_id, 1000); + let mut responder_conn = inbound_leg(LinkId::new(2), 1000); + + // Initiator starts handshake + let msg1 = initiator_conn + .start_handshake(initiator_keypair, initiator_epoch, 1100) + .unwrap(); + // Post-msg1 the initiator holds an in-flight handshake, not yet a session. + assert!(!initiator_conn.has_session()); + + // Responder processes msg1 and sends msg2 + let msg2 = responder_conn + .receive_handshake_init(responder_keypair, responder_epoch, &msg1, 1200) + .unwrap(); + // The IK responder completes in one step: it now holds a session. + assert!(responder_conn.has_session()); + + // Responder learned initiator's identity + let discovered = responder_conn.leg().unwrap().expected_identity().unwrap(); + assert_eq!(discovered.pubkey(), initiator_identity.pubkey()); + + // Responder learned initiator's epoch + assert_eq!( + responder_conn.leg().unwrap().remote_epoch(), + Some(initiator_epoch) + ); + + // Initiator completes handshake + initiator_conn.complete_handshake(&msg2, 1300).unwrap(); + assert!(initiator_conn.has_session()); + + // Initiator learned responder's epoch + assert_eq!( + initiator_conn.leg().unwrap().remote_epoch(), + Some(responder_epoch) + ); + + // Both have sessions + assert!(initiator_conn.has_session()); + assert!(responder_conn.has_session()); + + // Take and verify sessions work + let mut init_session = initiator_conn.take_session().unwrap(); + let mut resp_session = responder_conn.take_session().unwrap(); + + // Encrypt/decrypt test + let plaintext = b"test message"; + let ciphertext = init_session.encrypt(plaintext).unwrap(); + let decrypted = resp_session.decrypt(&ciphertext).unwrap(); + assert_eq!(decrypted, plaintext); + } + + #[test] + fn test_connection_failure() { + // `mark_failed` releases the leg's Noise handshake handle. The failure + // *state* now lives on the control machine, but the leg-local effect is + // still observable: a completion attempt afterward reports `WrongState` + // (the handle-presence gate) and no session is produced. + let identity = make_peer_identity(); + let keypair = make_keypair(); + let mut conn = outbound_leg(LinkId::new(1), identity, 1000); + conn.start_handshake(keypair, make_epoch(), 1100).unwrap(); + + conn.mark_failed(); + + assert!(!conn.has_session()); + assert!(conn.complete_handshake(&[0u8; 96], 1200).is_err()); + } + + #[test] + fn test_wrong_direction_errors() { + let identity = make_peer_identity(); + let keypair = make_keypair(); + + // Outbound can't receive_handshake_init + let mut outbound = outbound_leg(LinkId::new(1), identity, 1000); + assert!( + outbound + .receive_handshake_init(keypair, make_epoch(), &[0u8; 106], 1100) + .is_err() + ); + + // Inbound can't start_handshake + let mut inbound = inbound_leg(LinkId::new(2), 1000); + assert!( + inbound + .start_handshake(keypair, make_epoch(), 1100) + .is_err() + ); + } } /// T-SANSIO: the action vocabulary must stay plain, comparable data. From e7537929ba5f63594086e23b4150606d0e751ec7 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 18 Jul 2026 23:17:21 +0000 Subject: [PATCH 5/7] node: move link, direction, and peer address onto the control machine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These three had no counterpart on the control machine, so readers still reached them through the pending connection. Add machine-side accessors and repoint every reader, then drop the connection's. The peer address needed its writes lifted, not just its reads repointed: the machine's copy was never written. It is now written at each of the points the connection's copy was — the inbound seed, the dial, message-2 completion, and the two paths that seed a machine from a pre-built connection — so promotion and the resend path read a value with the same provenance at the same time as before. Link and direction need no lift. Both machine constructors already seed them from the same arguments the connection is built with, so an outbound machine carries outbound state and an inbound one inbound, and the machine's link always equals the connection's. The handshake operations' direction guards read the machine's copy for the same reason. The stale-connection sweep's teardown log and its resend path both now take the transport and address from the machine, and each keeps its own check that a pending connection is still attached rather than relying on the caller to have established it. Add a test pinning link, direction, and address on the two shapes that seed a carrier independently — the dial and an accepted message 1. The cross-connection winner reads both values from a carrier one of those two already seeded. --- src/control/queries.rs | 18 ++++---- src/node/handlers/handshake.rs | 29 ++++++------ src/node/handlers/timeout.rs | 37 +++++++++------- src/node/lifecycle/mod.rs | 16 ++++--- src/node/mod.rs | 39 +++++++++------- src/node/tests/handshake.rs | 6 +-- src/node/tests/unit.rs | 81 ++++++++++++++++++++++++++++++++-- src/peer/connection.rs | 36 +++------------ src/peer/machine.rs | 47 +++++++++++++++++--- 9 files changed, 204 insertions(+), 105 deletions(-) diff --git a/src/control/queries.rs b/src/control/queries.rs index 53f276e..9c82847 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -1303,18 +1303,18 @@ pub fn show_connections(node: &Node) -> Value { let now = now_ms(); let connections: Vec = node .connections() - .filter_map(|(_, machine)| machine.leg()) - .map(|conn| { + .map(|(_, machine)| { + let link_id = machine.link_id(); let mut conn_json = json!({ - "link_id": conn.link_id().as_u64(), - "direction": format!("{}", conn.direction()), - "handshake_state": node.connection_handshake_state(conn.link_id()), - "started_at_ms": node.connection_started_at(conn.link_id()), - "idle_ms": now.saturating_sub(node.connection_last_activity(conn.link_id())), - "resend_count": node.connection_resend_count(conn.link_id()), + "link_id": link_id.as_u64(), + "direction": format!("{}", machine.conn_direction()), + "handshake_state": node.connection_handshake_state(link_id), + "started_at_ms": node.connection_started_at(link_id), + "idle_ms": now.saturating_sub(node.connection_last_activity(link_id)), + "resend_count": node.connection_resend_count(link_id), }); - if let Some(identity) = node.connection_expected_identity(conn.link_id()) { + if let Some(identity) = node.connection_expected_identity(link_id) { conn_json["expected_peer"] = json!(identity.npub()); } diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index c5ebd3c..3ea26f8 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -283,6 +283,9 @@ impl Node { // so the promotion hand-off reads it from the surviving carrier, // matching the connection's own inbound seed. machine.set_conn_transport_id(packet.transport_id); + // The inbound connection is constructed carrying the peer's address; + // seed the surviving carrier with it at the same point. + machine.set_conn_source_addr(packet.remote_addr.clone()); machine.set_leg(conn); let our_keypair = self.identity().keypair(); @@ -1041,10 +1044,10 @@ impl Node { return; } + machine.set_conn_source_addr(packet.remote_addr.clone()); let conn = machine .leg_mut() .expect("pending connection present for msg2 completion"); - conn.set_source_addr(packet.remote_addr.clone()); let peer_identity = match conn.expected_identity() { Some(id) => *id, @@ -1398,6 +1401,8 @@ impl Node { .ok_or(NodeError::ConnectionNotFound(link_id))?; let carrier_their_index = machine.conn_their_index(); let carrier_transport_id = machine.conn_transport_id(); + let carrier_source_addr = machine.conn_source_addr().cloned(); + let carrier_is_outbound = machine.conn_is_outbound(); let link_stats = machine.conn_link_stats().clone(); // Verify handshake is complete and extract session @@ -1424,17 +1429,14 @@ impl Node { link_id, reason: "missing transport_id".into(), })?; - let current_addr = connection - .source_addr() - .ok_or_else(|| NodeError::PromotionFailed { - link_id, - reason: "missing source_addr".into(), - })? - .clone(); + let current_addr = carrier_source_addr.ok_or_else(|| NodeError::PromotionFailed { + link_id, + reason: "missing source_addr".into(), + })?; let remote_epoch = connection.remote_epoch(); let peer_node_addr = *verified_identity.node_addr(); - let is_outbound = connection.is_outbound(); + let is_outbound = carrier_is_outbound; // Check for cross-connection if let Some(existing_peer) = self.peers.get(&peer_node_addr) { @@ -1566,13 +1568,14 @@ impl Node { // the 30s handshake timeout. let pending_to_same_peer: Vec = self .connections() - .filter_map(|(_, machine)| machine.leg()) - .filter(|conn| { - conn.expected_identity() + .filter(|(_, machine)| { + machine + .leg() + .and_then(|conn| conn.expected_identity()) .map(|id| *id.node_addr() == peer_node_addr) .unwrap_or(false) }) - .map(|conn| conn.link_id()) + .map(|(_, machine)| machine.link_id()) .collect(); for pending_link_id in &pending_to_same_peer { diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index 741c702..7599cc2 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -27,9 +27,9 @@ impl LifecycleView for Node { timers.contains_key(&TimerKind::HandshakeTimeout) })) }) - .map(|(link_id, _machine, conn)| ConnSnapshot { + .map(|(link_id, machine, conn)| ConnSnapshot { link: *link_id, - is_outbound: conn.is_outbound(), + is_outbound: machine.conn_is_outbound(), retry_addr: conn.expected_identity().map(|id| *id.node_addr()), resend_count: 0, msg1: Vec::new(), @@ -77,8 +77,12 @@ impl Node { .peer_machines .get(&link) .is_some_and(|machine| machine.is_failed()); - if let Some(conn) = self.leg(&link) { - let direction = conn.direction(); + if let Some(machine) = self + .peer_machines + .get(&link) + .filter(|machine| machine.leg().is_some()) + { + let direction = machine.conn_direction(); if is_failed { debug!( link_id = %link, @@ -197,7 +201,11 @@ impl Node { .is_some_and(|machine| machine.conn_is_timed_out(now_ms, timeout_ms)); let (reap, retry_peer) = match self.leg(&link) { Some(conn) if timed_out => { - let retry_peer = if conn.is_outbound() { + let retry_peer = if self + .peer_machines + .get(&link) + .is_some_and(|machine| machine.conn_is_outbound()) + { conn.expected_identity().map(|id| *id.node_addr()) } else { None @@ -314,17 +322,14 @@ impl Node { continue; }; - let (transport_id, remote_addr) = match self.leg(&link) { - Some(conn) => match ( - self.peer_machines - .get(&link) - .and_then(|machine| machine.conn_transport_id()), - conn.source_addr(), - ) { - (Some(tid), Some(addr)) => (tid, addr.clone()), - _ => continue, - }, - None => continue, + let (transport_id, remote_addr) = match self.peer_machines.get(&link) { + Some(machine) if machine.leg().is_some() => { + match (machine.conn_transport_id(), machine.conn_source_addr()) { + (Some(tid), Some(addr)) => (tid, addr.clone()), + _ => continue, + } + } + _ => continue, }; let sent = if let Some(transport) = self.transports.get(&transport_id) { diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index f8652d6..78c747c 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -393,7 +393,7 @@ impl Node { .map(|id| id.node_addr() == peer_node_addr) .unwrap_or(false) && machine.conn_transport_id() == Some(transport_id) - && conn.source_addr() == Some(remote_addr) + && machine.conn_source_addr() == Some(remote_addr) }) }) || self.peering.pending_connects.iter().any(|pending| { pending.peer_identity.node_addr() == peer_node_addr @@ -642,8 +642,11 @@ impl Node { .and_then(|machine| machine.leg_mut()) .expect("dial-time machine carries the connection"); conn.set_our_index(our_index); - conn.set_source_addr(remote_addr.clone()); } + self.peer_machines + .get_mut(&link_id) + .expect("dial-time machine carries the connection") + .set_conn_source_addr(remote_addr.clone()); // Build wire format msg1: [0x01][sender_idx:4 LE][noise_msg1:82] let wire_msg1 = build_msg1(our_index, &noise_msg1); @@ -947,13 +950,14 @@ impl Node { let now_ms = Self::now_ms(); let stale: Vec = self .connections() - .filter_map(|(_, machine)| machine.leg()) - .filter(|conn| { - conn.expected_identity() + .filter(|(_, machine)| { + machine + .leg() + .and_then(|conn| conn.expected_identity()) .map(|id| id.node_addr() == &peer_addr) .unwrap_or(false) }) - .map(|conn| conn.link_id()) + .map(|(_, machine)| machine.link_id()) .collect(); for link_id in stale { self.cleanup_stale_connection(link_id, now_ms); diff --git a/src/node/mod.rs b/src/node/mod.rs index bb7d2dc..3287337 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1978,16 +1978,17 @@ impl Node { // --- connections (show_connections) --- let connection_rows: Vec = self .connections() - .filter_map(|(_, machine)| machine.leg()) - .map(|conn| snap::ConnectionRow { - link_id: conn.link_id().as_u64(), - direction: format!("{}", conn.direction()), - handshake_state: self.connection_handshake_state(conn.link_id()).to_string(), - started_at_ms: self.connection_started_at(conn.link_id()), - last_activity_ms: self.connection_last_activity(conn.link_id()), - resend_count: self.connection_resend_count(conn.link_id()), + .map(|(_, machine)| snap::ConnectionRow { + link_id: machine.link_id().as_u64(), + direction: format!("{}", machine.conn_direction()), + handshake_state: self + .connection_handshake_state(machine.link_id()) + .to_string(), + started_at_ms: self.connection_started_at(machine.link_id()), + last_activity_ms: self.connection_last_activity(machine.link_id()), + resend_count: self.connection_resend_count(machine.link_id()), expected_peer: self - .connection_expected_identity(conn.link_id()) + .connection_expected_identity(machine.link_id()) .map(|id| id.npub()), }) .collect(); @@ -2426,7 +2427,7 @@ impl Node { /// than through the dial or inbound-msg1 paths, which build the machine /// themselves. pub fn add_connection(&mut self, connection: PeerConnection) -> Result<(), NodeError> { - let link_id = connection.link_id(); + let link_id = connection.state().link_id(); if self .peer_machines @@ -2444,8 +2445,8 @@ impl Node { let machine = self.peer_machines.entry(link_id).or_insert_with(|| { let now = connection.started_at(); - match connection.expected_identity() { - Some(identity) if connection.is_outbound() => { + match connection.state().expected_identity() { + Some(identity) if connection.state().is_outbound() => { PeerMachine::new_outbound(link_id, *identity, now) } _ => PeerMachine::new_inbound(link_id, now), @@ -2460,6 +2461,9 @@ impl Node { if let Some(tid) = connection.transport_id() { machine.set_conn_transport_id(tid); } + if let Some(addr) = connection.state().source_addr() { + machine.set_conn_source_addr(addr.clone()); + } machine.set_leg(connection); Ok(()) } @@ -2492,9 +2496,7 @@ impl Node { if let Some(id) = seed.transport_id { connection.set_transport_id(id); } - if let Some(addr) = seed.source_addr { - connection.set_source_addr(addr); - } + let seeded_source_addr = seed.source_addr.clone(); if let Some(index) = seed.our_index { connection.set_our_index(index); } @@ -2518,8 +2520,8 @@ impl Node { let machine = self.peer_machines.entry(link_id).or_insert_with(|| { let now = connection.started_at(); - match connection.expected_identity() { - Some(identity) if connection.is_outbound() => { + match connection.state().expected_identity() { + Some(identity) if connection.state().is_outbound() => { PeerMachine::new_outbound(link_id, *identity, now) } _ => PeerMachine::new_inbound(link_id, now), @@ -2531,6 +2533,9 @@ impl Node { if let Some(tid) = connection.transport_id() { machine.set_conn_transport_id(tid); } + if let Some(addr) = seeded_source_addr { + machine.set_conn_source_addr(addr); + } machine.set_leg(connection); Ok(()) } diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index dae87c6..7367b1e 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -868,7 +868,7 @@ async fn test_msg1_stored_for_resend() { .unwrap(); conn.leg_mut().unwrap().set_our_index(our_index); conn.leg_mut().unwrap().set_transport_id(transport_id); - conn.leg_mut().unwrap().set_source_addr(remote_addr.clone()); + conn.set_conn_source_addr(remote_addr.clone()); // Build wire msg1 and store it (as initiate_peer_connection does) let wire_msg1 = build_msg1(our_index, &noise_msg1); @@ -901,7 +901,7 @@ async fn test_resend_scheduling() { .unwrap(); conn.leg_mut().unwrap().set_our_index(our_index); conn.leg_mut().unwrap().set_transport_id(transport_id); - conn.leg_mut().unwrap().set_source_addr(remote_addr.clone()); + conn.set_conn_source_addr(remote_addr.clone()); // Store msg1 with first resend at now + 1000ms let wire_msg1 = crate::proto::fmp::wire::build_msg1(our_index, &noise_msg1); @@ -988,7 +988,7 @@ async fn test_handshake_timeout_drive() { .unwrap(); conn.leg_mut().unwrap().set_our_index(our_index); conn.leg_mut().unwrap().set_transport_id(transport_id); - conn.leg_mut().unwrap().set_source_addr(remote_addr.clone()); + conn.set_conn_source_addr(remote_addr.clone()); let link = Link::connectionless( link_id, diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index adccce9..404596d 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -138,8 +138,7 @@ async fn test_try_peer_addresses_races_all_concrete_udp_candidates() { let mut addrs = node .connections() - .filter_map(|(_, machine)| machine.leg()) - .filter_map(|conn| conn.source_addr().and_then(|addr| addr.as_str())) + .filter_map(|(_, machine)| machine.conn_source_addr().and_then(|addr| addr.as_str())) .collect::>(); addrs.sort(); assert_eq!(addrs, vec!["127.0.0.1:10", "127.0.0.1:9"]); @@ -1346,9 +1345,8 @@ async fn update_peers_races_new_alternative_without_dropping_active_peer() { assert_eq!(node.connection_count(), 1); assert_eq!( node.connections() - .filter_map(|(_, machine)| machine.leg()) .next() - .and_then(|conn| conn.source_addr()), + .and_then(|(_, machine)| machine.conn_source_addr()), Some(&new_addr) ); let active = node.get_peer(&peer_node_addr).unwrap(); @@ -2475,3 +2473,78 @@ async fn test_failed_msg1_preparation_unwinds_the_dial_machine() { ); assert_eq!(node.connection_count(), 0); } + +/// The link, direction, and peer address that promotion and the operator view +/// read now come from the control machine rather than the pending connection. +/// A machine's direction is seeded at construction and must match the side that +/// actually opened the link, and its address must be populated by the time +/// promotion needs it. +/// +/// This covers the two shapes that seed a carrier independently: the dial and +/// an accepted inbound message 1. The cross-connection winner derives both +/// values from a carrier one of those two already seeded, so it has nothing +/// separate to pin. +#[tokio::test] +async fn test_machine_carries_link_direction_and_address_on_dial_and_inbound() { + // Outbound: the dial builds the machine, msg1 preparation fills it in. + let mut node = make_node(); + let link_id = LinkId::new(1); + let transport_id = TransportId::new(1); + let remote_addr = TransportAddr::from_string("127.0.0.1:5000"); + let peer_identity = make_peer_identity(); + node.peer_machines.insert( + link_id, + PeerMachine::new_outbound(link_id, peer_identity, 1000), + ); + node.prepare_outbound_msg1(link_id, transport_id, &remote_addr, peer_identity) + .unwrap(); + + let machine = node.peer_machines.get(&link_id).unwrap(); + assert_eq!(machine.link_id(), link_id); + assert!(machine.conn_is_outbound(), "a dial is outbound"); + assert!(!machine.conn_is_inbound()); + assert_eq!(machine.conn_direction(), LinkDirection::Outbound); + assert_eq!( + machine.conn_source_addr(), + Some(&remote_addr), + "the dialled address must reach the surviving carrier" + ); + + // Inbound: msg1 builds the machine from the packet. + let mut responder = make_node(); + let initiator = make_node(); + let responder_identity = PeerIdentity::from_pubkey_full(responder.identity().pubkey_full()); + let mut initiator_leg = outbound_leg(LinkId::new(9), responder_identity, 1000); + let noise_msg1 = initiator_leg + .start_handshake( + initiator.identity().keypair(), + initiator.startup_epoch(), + 1000, + ) + .unwrap(); + let inbound_addr = TransportAddr::from_string("127.0.0.1:6000"); + let packet = ReceivedPacket::with_timestamp( + TransportId::new(1), + inbound_addr.clone(), + crate::proto::fmp::wire::build_msg1(SessionIndex::new(7), &noise_msg1), + 1000, + ); + responder.handle_msg1(packet).await; + + // The responder completes at msg1 and promotes, so the machine survives as + // the active peer's control machine — the carrier outlives the connection. + let (link, machine) = responder + .peer_machines + .iter() + .next() + .expect("msg1 leaves a control machine behind"); + assert!(machine.conn_is_inbound(), "an accepted msg1 is inbound"); + assert!(!machine.conn_is_outbound()); + assert_eq!(machine.conn_direction(), LinkDirection::Inbound); + assert_eq!( + machine.conn_source_addr(), + Some(&inbound_addr), + "the sender's address must reach the surviving carrier" + ); + assert_eq!(machine.link_id(), *link); +} diff --git a/src/peer/connection.rs b/src/peer/connection.rs index 6eb00d0..c77670a 100644 --- a/src/peer/connection.rs +++ b/src/peer/connection.rs @@ -10,7 +10,7 @@ use crate::PeerIdentity; use crate::noise::{self, NoiseSession}; use crate::proto::fmp::ConnectionState; -use crate::transport::{LinkDirection, LinkId, TransportAddr, TransportId}; +use crate::transport::{LinkId, TransportAddr, TransportId}; use crate::utils::index::SessionIndex; use std::fmt; @@ -91,31 +91,11 @@ impl PeerConnection { // === Accessors (delegated to the pure ConnectionState) === - /// Get the link ID. - pub fn link_id(&self) -> LinkId { - self.state.link_id() - } - - /// Get the connection direction. - pub fn direction(&self) -> LinkDirection { - self.state.direction() - } - /// Get the expected/learned peer identity, if known. pub fn expected_identity(&self) -> Option<&PeerIdentity> { self.state.expected_identity() } - /// Check if this is an outbound connection. - pub fn is_outbound(&self) -> bool { - self.state.is_outbound() - } - - /// Check if this is an inbound connection. - pub fn is_inbound(&self) -> bool { - self.state.is_inbound() - } - /// When the connection started. Retained only to seed a control machine's /// carrier from a pre-built leg (`Node::add_connection`); the operator-facing /// `started_at_ms`/`last_activity_ms` telemetry now reads the machine carrier, @@ -166,16 +146,6 @@ impl PeerConnection { self.state.set_transport_id(id); } - /// Get the source address (if known). - pub fn source_addr(&self) -> Option<&TransportAddr> { - self.state.source_addr() - } - - /// Set the source address. - pub fn set_source_addr(&mut self, addr: TransportAddr) { - self.state.set_source_addr(addr); - } - // === Epoch Accessors === /// Get the remote peer's startup epoch (available after handshake). @@ -210,6 +180,10 @@ impl PeerConnection { /// Mutable access to the pure bookkeeping, so the control machine's /// handshake operations can record their results here as well as on the /// surviving carrier. + pub(crate) fn state(&self) -> &ConnectionState { + &self.state + } + pub(crate) fn state_mut(&mut self) -> &mut ConnectionState { &mut self.state } diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 6c3157c..5add30f 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -573,9 +573,10 @@ impl PeerMachine { current_time_ms: u64, ) -> Result, NoiseError> { let msg1 = { + let direction = self.conn.direction(); let leg = self.leg.as_mut().ok_or_else(no_pending_connection)?; - if leg.direction() != LinkDirection::Outbound { + if direction != LinkDirection::Outbound { return Err(NoiseError::WrongState { expected: "outbound connection".to_string(), got: "inbound connection".to_string(), @@ -613,9 +614,10 @@ impl PeerMachine { current_time_ms: u64, ) -> Result, NoiseError> { let (msg2, learned_identity, remote_epoch) = { + let direction = self.conn.direction(); let leg = self.leg.as_mut().ok_or_else(no_pending_connection)?; - if leg.direction() != LinkDirection::Inbound { + if direction != LinkDirection::Inbound { return Err(NoiseError::WrongState { expected: "inbound connection".to_string(), got: "outbound connection".to_string(), @@ -805,6 +807,39 @@ impl PeerMachine { self.conn.set_handshake_msg2(msg2); } + /// The link this machine controls. + pub(crate) fn link_id(&self) -> LinkId { + self.link + } + + /// Which side opened this connection, read from the surviving carrier. + /// Seeded at construction: an outbound machine carries an outbound + /// connection state and an inbound machine an inbound one. + pub(crate) fn conn_direction(&self) -> LinkDirection { + self.conn.direction() + } + + /// Whether we opened this connection. + pub(crate) fn conn_is_outbound(&self) -> bool { + self.conn.is_outbound() + } + + /// Whether the peer opened this connection. + pub(crate) fn conn_is_inbound(&self) -> bool { + self.conn.is_inbound() + } + + /// The peer's address on this link, read from the surviving carrier. + /// Written at the same three points the connection's own copy was: the + /// inbound seed, the outbound dial, and message-2 completion. + pub(crate) fn conn_source_addr(&self) -> Option<&TransportAddr> { + self.conn.source_addr() + } + + pub(crate) fn set_conn_source_addr(&mut self, addr: TransportAddr) { + self.conn.set_source_addr(addr); + } + /// Peer session index of the surviving carrier — the source for the /// promotion hand-off now that the leg no longer projects it. pub(crate) fn conn_their_index(&self) -> Option { @@ -3202,8 +3237,8 @@ mod tests { let identity = make_peer_identity(); let conn = outbound_leg(LinkId::new(1), identity, 1000); - assert!(conn.leg().unwrap().is_outbound()); - assert!(!conn.leg().unwrap().is_inbound()); + assert!(conn.conn_is_outbound()); + assert!(!conn.conn_is_inbound()); assert!(!conn.has_session()); assert!(conn.leg().unwrap().expected_identity().is_some()); assert_eq!(conn.leg().unwrap().started_at(), 1000); @@ -3213,8 +3248,8 @@ mod tests { fn test_inbound_connection() { let conn = inbound_leg(LinkId::new(2), 2000); - assert!(conn.leg().unwrap().is_inbound()); - assert!(!conn.leg().unwrap().is_outbound()); + assert!(conn.conn_is_inbound()); + assert!(!conn.conn_is_outbound()); assert!(!conn.has_session()); assert!(conn.leg().unwrap().expected_identity().is_none()); assert_eq!(conn.leg().unwrap().started_at(), 2000); From e21e09d7e6e343de8d1f15f50efb9c8c8e10d671 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 19 Jul 2026 00:25:17 +0000 Subject: [PATCH 6/7] peer: source the handshake identity and index family from the control machine The pending connection and the per-peer control machine have carried duplicate copies of the handshake-phase fields since the machine gained its own connection state. Read them from the machine and drop the connection's projections. The peer identity is the sharp one. The connection learned it from msg1 and the machine's carrier did not, so the two views genuinely disagreed for inbound connections until the Noise operations moved onto the machine and began recording each result on both. That is now in place, so every reader can take the machine's copy unchanged: the establish snapshot, the promotion sweep for competing connections, the dial and path in-progress checks, the peering observations and per-peer in-flight budget, and the stale-connection sweep's retry address. Also repointed: our_index, their_index, transport_id, started_at, the stored handshake message bytes, and the idle-timeout check. Two duplicate index writes on the connection are dropped, both immediately preceded by the machine-side write of the same value. Reads that previously came off a connection detached just before its machine was disposed now capture the machine's value first. Test seeding follows the establish paths: the seed builders write our_index to the carrier, which promotion now reads. A new test pins the inbound identity learn on the carrier and the retry address a failed inbound connection reports to the sweep, so a silent regression to a blank identity cannot pass. ConnectionState::duration loses its last non-test caller with the connection accessor and is marked test-only. --- src/node/handlers/handshake.rs | 66 ++++++++++------------- src/node/handlers/timeout.rs | 31 ++++++----- src/node/lifecycle/mod.rs | 59 +++++++++------------ src/node/mod.rs | 37 +++++++------ src/node/tests/handshake.rs | 31 +++++------ src/node/tests/unit.rs | 62 ++++++++++++++++++++++ src/peer/connection.rs | 95 ++-------------------------------- src/peer/machine.rs | 22 ++++---- src/proto/fmp/state.rs | 1 + 9 files changed, 182 insertions(+), 222 deletions(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 3ea26f8..21f41e3 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -39,14 +39,12 @@ impl EstablishView for Node { rekey_in_progress: existing.map(|p| p.rekey_in_progress()).unwrap_or(false), existing_msg2: existing.and_then(|p| p.handshake_msg2().map(|m| m.to_vec())), at_max_peers: max_peers > 0 && self.peers.len() >= max_peers, - has_pending_outbound_to_peer: self - .connections() - .filter_map(|(_, machine)| machine.leg()) - .any(|conn| { - conn.expected_identity() - .map(|id| id.node_addr() == peer_addr) - .unwrap_or(false) - }), + has_pending_outbound_to_peer: self.connections().any(|(_, machine)| { + machine + .conn_expected_identity() + .map(|id| id.node_addr() == peer_addr) + .unwrap_or(false) + }), rekey_enabled: self.config().node.rekey.enabled, our_node_addr: *self.identity().node_addr(), } @@ -310,11 +308,8 @@ impl Node { }; // Learn peer identity from msg1 - let peer_identity = match machine - .leg() - .expect("pending connection attached above") - .expected_identity() - { + assert!(machine.leg().is_some(), "pending connection attached above"); + let peer_identity = match machine.conn_expected_identity() { Some(id) => *id, None => { self.msg1_rate_limiter.complete_handshake(); @@ -639,10 +634,6 @@ impl Node { // connection, then build + store the framed msg2. The old index was // already freed by `remove_active_peer` above, BEFORE this fresh // allocation — matching the pre-refactor allocation sequence. - machine - .leg_mut() - .expect("pending connection attached above") - .set_our_index(our_index); let link = Link::connectionless( link_id, packet.transport_id, @@ -789,10 +780,6 @@ impl Node { // Shell registry surgery, in the pre-refactor order: // set indices on the shell connection, insert link / reverse map / // connection, then build + store the framed msg2. - machine - .leg_mut() - .expect("pending connection attached above") - .set_our_index(our_index); let link = Link::connectionless( link_id, packet.transport_id, @@ -1045,11 +1032,12 @@ impl Node { } machine.set_conn_source_addr(packet.remote_addr.clone()); - let conn = machine - .leg_mut() - .expect("pending connection present for msg2 completion"); + assert!( + machine.leg().is_some(), + "pending connection present for msg2 completion" + ); - let peer_identity = match conn.expected_identity() { + let peer_identity = match machine.conn_expected_identity() { Some(id) => *id, None => { warn!(link_id = %link_id, "No identity after handshake"); @@ -1059,7 +1047,7 @@ impl Node { } }; - (peer_identity, conn.our_index()) + (peer_identity, machine.our_index()) }; if self @@ -1166,10 +1154,10 @@ impl Node { // right after the take — unconditionally, whether or not a // connection was carried — so none of this block's exits leave a // dangling machine. - let taken_conn = self - .peer_machines - .get_mut(&link_id) - .and_then(|machine| machine.take_leg()); + let (taken_conn, carrier_our_index) = match self.peer_machines.get_mut(&link_id) { + Some(machine) => (machine.take_leg(), machine.our_index()), + None => (None, None), + }; self.remove_peer_machine(link_id); let mut conn = match taken_conn { Some(c) => c, @@ -1185,7 +1173,7 @@ impl Node { if swap { // We're the smaller node. Swap to outbound session + indices. // The peer will keep their inbound session (complement of ours). - let outbound_our_index = conn.our_index(); + let outbound_our_index = carrier_our_index; let outbound_session = conn.noise_session.take(); let (outbound_session, outbound_our_index) = match ( @@ -1250,7 +1238,7 @@ impl Node { // their outbound session, that index is exactly what they'll use. // The msg2 sender_idx we see here is the peer's INBOUND our_index, // which becomes stale after the peer swaps. - let outbound_our_index = conn.our_index(); + let outbound_our_index = carrier_our_index; if let Some(peer) = self.peers.get(&peer_node_addr) { debug!( @@ -1399,6 +1387,7 @@ impl Node { let mut connection = machine .take_leg() .ok_or(NodeError::ConnectionNotFound(link_id))?; + let carrier_our_index = machine.our_index(); let carrier_their_index = machine.conn_their_index(); let carrier_transport_id = machine.conn_transport_id(); let carrier_source_addr = machine.conn_source_addr().cloned(); @@ -1415,12 +1404,10 @@ impl Node { .take() .ok_or(NodeError::NoSession(link_id))?; - let our_index = connection - .our_index() - .ok_or_else(|| NodeError::PromotionFailed { - link_id, - reason: "missing our_index".into(), - })?; + let our_index = carrier_our_index.ok_or_else(|| NodeError::PromotionFailed { + link_id, + reason: "missing our_index".into(), + })?; let their_index = carrier_their_index.ok_or_else(|| NodeError::PromotionFailed { link_id, reason: "missing their_index".into(), @@ -1570,8 +1557,7 @@ impl Node { .connections() .filter(|(_, machine)| { machine - .leg() - .and_then(|conn| conn.expected_identity()) + .conn_expected_identity() .map(|id| *id.node_addr() == peer_node_addr) .unwrap_or(false) }) diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index 7599cc2..ad7267c 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -19,18 +19,18 @@ impl LifecycleView for Node { // reap. self.peer_machines .iter() - .filter_map(|(link_id, machine)| machine.leg().map(|conn| (link_id, machine, conn))) - .filter(|(link_id, machine, _conn)| { + .filter(|(_, machine)| machine.leg().is_some()) + .filter(|(link_id, machine)| { machine.is_failed() || (machine.conn_is_timed_out(now_ms, timeout_ms) && !self.peer_timers.get(*link_id).is_some_and(|timers| { timers.contains_key(&TimerKind::HandshakeTimeout) })) }) - .map(|(link_id, machine, conn)| ConnSnapshot { + .map(|(link_id, machine)| ConnSnapshot { link: *link_id, is_outbound: machine.conn_is_outbound(), - retry_addr: conn.expected_identity().map(|id| *id.node_addr()), + retry_addr: machine.conn_expected_identity().map(|id| *id.node_addr()), resend_count: 0, msg1: Vec::new(), }) @@ -120,7 +120,7 @@ impl Node { // dangling machine. A no-op for promoted peers — `promote_connection` // already consumed their connection, so this reaper never runs for // them. - let conn = match self + let _detached_leg = match self .peer_machines .get_mut(&link_id) .and_then(|machine| machine.take_leg()) @@ -128,16 +128,16 @@ impl Node { Some(c) => c, None => return, }; - // Read the transport ID off the surviving carrier before disposing the - // machine (the leg no longer projects it). - let transport_id = self - .peer_machines - .get(&link_id) - .and_then(|machine| machine.conn_transport_id()); + // Read the transport ID and session index off the surviving carrier + // before disposing the machine (the leg no longer projects them). + let (transport_id, our_index) = match self.peer_machines.get(&link_id) { + Some(machine) => (machine.conn_transport_id(), machine.our_index()), + None => (None, None), + }; self.remove_peer_machine(link_id); // Free session index and pending_outbound if allocated - if let Some(idx) = conn.our_index() { + if let Some(idx) = our_index { if let Some(tid) = transport_id { self.pending_outbound.remove(&(tid, idx.as_u32())); } @@ -200,13 +200,16 @@ impl Node { .get(&link) .is_some_and(|machine| machine.conn_is_timed_out(now_ms, timeout_ms)); let (reap, retry_peer) = match self.leg(&link) { - Some(conn) if timed_out => { + Some(_) if timed_out => { let retry_peer = if self .peer_machines .get(&link) .is_some_and(|machine| machine.conn_is_outbound()) { - conn.expected_identity().map(|id| *id.node_addr()) + self.peer_machines + .get(&link) + .and_then(|machine| machine.conn_expected_identity()) + .map(|id| *id.node_addr()) } else { None }; diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 78c747c..6df800e 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -372,13 +372,12 @@ impl Node { } fn is_connecting_to_peer(&self, peer_node_addr: &NodeAddr) -> bool { - self.connections() - .filter_map(|(_, machine)| machine.leg()) - .any(|conn| { - conn.expected_identity() - .map(|id| id.node_addr() == peer_node_addr) - .unwrap_or(false) - }) + self.connections().any(|(_, machine)| { + machine + .conn_expected_identity() + .map(|id| id.node_addr() == peer_node_addr) + .unwrap_or(false) + }) } fn is_connecting_to_peer_on_path( @@ -388,13 +387,13 @@ impl Node { remote_addr: &TransportAddr, ) -> bool { self.peer_machines.values().any(|machine| { - machine.leg().is_some_and(|conn| { - conn.expected_identity() + machine.leg().is_some() + && machine + .conn_expected_identity() .map(|id| id.node_addr() == peer_node_addr) .unwrap_or(false) - && machine.conn_transport_id() == Some(transport_id) - && machine.conn_source_addr() == Some(remote_addr) - }) + && machine.conn_transport_id() == Some(transport_id) + && machine.conn_source_addr() == Some(remote_addr) }) || self.peering.pending_connects.iter().any(|pending| { pending.peer_identity.node_addr() == peer_node_addr && pending.transport_id == transport_id @@ -635,14 +634,6 @@ impl Node { }; // Set index and transport info on the connection - { - let conn = self - .peer_machines - .get_mut(&link_id) - .and_then(|machine| machine.leg_mut()) - .expect("dial-time machine carries the connection"); - conn.set_our_index(our_index); - } self.peer_machines .get_mut(&link_id) .expect("dial-time machine carries the connection") @@ -683,10 +674,8 @@ impl Node { // projects it to the promotion hand-off); holds even if a direct caller // reached here without the dial-time `on_dial` write. machine.set_conn_transport_id(transport_id); - // Record our session index on the surviving carrier — the same index - // just written on the connection above — so the carrier is the single - // index home on the outbound path (the inbound path writes it at - // authorize). + // Record our session index on the surviving carrier, the single index + // home on the outbound path (the inbound path writes it at authorize). machine.set_conn_our_index(our_index); // Store the msg1 wire on the surviving carrier (the connection does not // hold the resend source); the retransmit driver reads it from here. @@ -717,7 +706,11 @@ impl Node { Some(w) => w.to_vec(), None => return, }; - let our_index = self.leg(&link_id).and_then(|c| c.our_index()); + let our_index = self + .peer_machines + .get(&link_id) + .filter(|machine| machine.leg().is_some()) + .and_then(|machine| machine.our_index()); // Send the wire format handshake message if let Some(transport) = self.transports.get(&transport_id) { @@ -952,8 +945,7 @@ impl Node { .connections() .filter(|(_, machine)| { machine - .leg() - .and_then(|conn| conn.expected_identity()) + .conn_expected_identity() .map(|id| id.node_addr() == &peer_addr) .unwrap_or(false) }) @@ -2744,12 +2736,11 @@ impl Node { let connected: HashSet = self.peers.keys().copied().collect(); let connecting: HashSet = self .connections() - .filter_map(|(_, machine)| machine.leg()) - .filter_map(|conn| conn.expected_identity().map(|id| *id.node_addr())) + .filter_map(|(_, machine)| machine.conn_expected_identity().map(|id| *id.node_addr())) .collect(); let mut in_flight_by_peer: HashMap = HashMap::new(); - for conn in self.connections().filter_map(|(_, machine)| machine.leg()) { - if let Some(id) = conn.expected_identity() { + for (_, machine) in self.connections() { + if let Some(id) = machine.conn_expected_identity() { *in_flight_by_peer.entry(*id.node_addr()).or_default() += 1; } } @@ -2818,9 +2809,9 @@ impl Node { let in_flight_for_peer = self .connections() - .filter_map(|(_, machine)| machine.leg()) - .filter(|conn| { - conn.expected_identity() + .filter(|(_, machine)| { + machine + .conn_expected_identity() .map(|identity| identity.node_addr() == peer_node_addr) .unwrap_or(false) }) diff --git a/src/node/mod.rs b/src/node/mod.rs index 3287337..2d5fa71 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -2444,7 +2444,7 @@ impl Node { } let machine = self.peer_machines.entry(link_id).or_insert_with(|| { - let now = connection.started_at(); + let now = connection.state().started_at(); match connection.state().expected_identity() { Some(identity) if connection.state().is_outbound() => { PeerMachine::new_outbound(link_id, *identity, now) @@ -2455,10 +2455,13 @@ impl Node { // Seed the surviving carrier's peer index and transport from the // pre-built leg so the promotion hand-off reads them from the machine, // matching the establish paths that write them on the machine directly. - if let Some(their) = connection.their_index() { + if let Some(ours) = connection.state().our_index() { + machine.set_conn_our_index(ours); + } + if let Some(their) = connection.state().their_index() { machine.set_conn_their_index(their); } - if let Some(tid) = connection.transport_id() { + if let Some(tid) = connection.state().transport_id() { machine.set_conn_transport_id(tid); } if let Some(addr) = connection.state().source_addr() { @@ -2473,12 +2476,13 @@ impl Node { /// free-standing leg first. /// /// The carrier seeding below is a verbatim copy of `add_connection`'s: the - /// two conditional writes (`their_index`, `transport_id`) and `set_leg`, - /// built through the same `entry(..).or_insert_with(..)` so an existing - /// leg-less machine keeps its constructor-side fields. Nothing else is - /// written to the carrier — `our_index`, `source_addr`, post-construction - /// `started_at`, and the stored handshake bytes stay leg-only, exactly as - /// they do for a test that goes through `add_connection` today. + /// conditional writes (`our_index`, `their_index`, `transport_id`, + /// `source_addr`) and `set_leg`, built through the same + /// `entry(..).or_insert_with(..)` so an existing leg-less machine keeps its + /// constructor-side fields. The seeded carrier matches what the establish + /// paths write: every field a promotion reads is present. Post-construction + /// `started_at` and the stored handshake bytes are not seeded here, exactly + /// as they are not for a test that goes through `add_connection` today. /// /// The duplication is deliberate: keeping the carrier writes visible here /// is what lets each later step of the leg dissolution revise them at a @@ -2494,14 +2498,14 @@ impl Node { None => PeerConnection::inbound(link_id, seed.started_at_ms), }; if let Some(id) = seed.transport_id { - connection.set_transport_id(id); + connection.state_mut().set_transport_id(id); } let seeded_source_addr = seed.source_addr.clone(); if let Some(index) = seed.our_index { - connection.set_our_index(index); + connection.state_mut().set_our_index(index); } if let Some(index) = seed.their_index { - connection.set_their_index(index); + connection.state_mut().set_their_index(index); } if self @@ -2519,7 +2523,7 @@ impl Node { } let machine = self.peer_machines.entry(link_id).or_insert_with(|| { - let now = connection.started_at(); + let now = connection.state().started_at(); match connection.state().expected_identity() { Some(identity) if connection.state().is_outbound() => { PeerMachine::new_outbound(link_id, *identity, now) @@ -2527,10 +2531,13 @@ impl Node { _ => PeerMachine::new_inbound(link_id, now), } }); - if let Some(their) = connection.their_index() { + if let Some(ours) = connection.state().our_index() { + machine.set_conn_our_index(ours); + } + if let Some(their) = connection.state().their_index() { machine.set_conn_their_index(their); } - if let Some(tid) = connection.transport_id() { + if let Some(tid) = connection.state().transport_id() { machine.set_conn_transport_id(tid); } if let Some(addr) = seeded_source_addr { diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 7367b1e..4c535bd 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -866,19 +866,17 @@ async fn test_msg1_stored_for_resend() { let noise_msg1 = conn .start_handshake(our_keypair, node.startup_epoch(), now_ms) .unwrap(); - conn.leg_mut().unwrap().set_our_index(our_index); - conn.leg_mut().unwrap().set_transport_id(transport_id); + conn.set_conn_our_index(our_index); + conn.set_conn_transport_id(transport_id); conn.set_conn_source_addr(remote_addr.clone()); // Build wire msg1 and store it (as initiate_peer_connection does) let wire_msg1 = build_msg1(our_index, &noise_msg1); let resend_interval = node.config().node.rate_limit.handshake_resend_interval_ms; - conn.leg_mut() - .unwrap() - .set_handshake_msg1(wire_msg1.clone(), now_ms + resend_interval); + conn.set_conn_handshake_msg1(wire_msg1.clone(), now_ms + resend_interval); // Verify stored msg1 matches what was built - assert_eq!(conn.leg().unwrap().handshake_msg1().unwrap(), &wire_msg1); + assert_eq!(conn.conn_handshake_msg1().unwrap(), &wire_msg1); } /// Test that resend scheduling respects max_resends and backoff. @@ -899,15 +897,10 @@ async fn test_resend_scheduling() { let noise_msg1 = conn .start_handshake(our_keypair, node.startup_epoch(), now_ms) .unwrap(); - conn.leg_mut().unwrap().set_our_index(our_index); - conn.leg_mut().unwrap().set_transport_id(transport_id); conn.set_conn_source_addr(remote_addr.clone()); // Store msg1 with first resend at now + 1000ms let wire_msg1 = crate::proto::fmp::wire::build_msg1(our_index, &noise_msg1); - conn.leg_mut() - .unwrap() - .set_handshake_msg1(wire_msg1.clone(), now_ms + 1000); let link = Link::connectionless( link_id, @@ -941,6 +934,8 @@ async fn test_resend_scheduling() { // The msg1 wire lives on the machine's carrier (the retransmit driver's // resend source), mirroring `prepare_outbound_msg1`. machine.set_conn_handshake_msg1(wire_msg1, now_ms + 1000); + machine.set_conn_our_index(our_index); + machine.set_conn_transport_id(transport_id); machine.set_leg(conn.take_leg().unwrap()); node.peer_machines.insert(link_id, machine); node.peer_timers.entry(link_id).or_default().insert( @@ -986,8 +981,6 @@ async fn test_handshake_timeout_drive() { let _ = conn .start_handshake(our_keypair, node.startup_epoch(), dial_ms) .unwrap(); - conn.leg_mut().unwrap().set_our_index(our_index); - conn.leg_mut().unwrap().set_transport_id(transport_id); conn.set_conn_source_addr(remote_addr.clone()); let link = Link::connectionless( @@ -1017,6 +1010,8 @@ async fn test_handshake_timeout_drive() { dial_ms, &mut node.index_allocator, ); + machine.set_conn_our_index(our_index); + machine.set_conn_transport_id(transport_id); machine.set_leg(conn.take_leg().unwrap()); node.peer_machines.insert(link_id, machine); node.peer_timers.entry(link_id).or_default().insert( @@ -1045,17 +1040,17 @@ async fn test_handshake_timeout_drive() { ); } -/// Test that msg2 is stored on PeerConnection for responder resend. +/// Test that msg2 is stored on the control machine's carrier for responder resend. #[test] fn test_msg2_stored_on_connection() { - let mut conn = PeerConnection::inbound(LinkId::new(1), 1000); + let mut machine = crate::peer::machine::PeerMachine::new_inbound(LinkId::new(1), 1000); - assert!(conn.handshake_msg2().is_none()); + assert!(machine.conn_handshake_msg2().is_none()); let msg2_bytes = vec![0x01, 0x02, 0x03, 0x04]; - conn.set_handshake_msg2(msg2_bytes.clone()); + machine.set_conn_handshake_msg2(msg2_bytes.clone()); - assert_eq!(conn.handshake_msg2().unwrap(), &msg2_bytes); + assert_eq!(machine.conn_handshake_msg2().unwrap(), &msg2_bytes); } /// Test that duplicate msg2 is silently dropped when pending_outbound is already cleared. diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 404596d..280bbf4 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -2405,6 +2405,68 @@ fn test_failed_connection_is_retained_and_reaped() { assert_eq!(node.connection_count(), 0); } +/// The identity a responder discovers in msg1 must land on the surviving +/// carrier, not only on the pending leg. Everything that names an inbound +/// peer mid-handshake reads the carrier: the stale-connection sweep's +/// `retry_addr` decides whether a reaped leg is retried or torn down, and a +/// blank identity there silently changes that choreography. +#[test] +fn inbound_msg1_records_the_learned_identity_on_the_carrier() { + use crate::proto::fmp::LifecycleView; + + let mut node = make_node(); + let link_id = LinkId::new(77); + + // A genuine IK msg1 addressed to this node, from a known sender. + let sender = Identity::generate(); + let sender_identity = PeerIdentity::from_pubkey_full(sender.pubkey_full()); + let node_identity = PeerIdentity::from_pubkey_full(node.identity().pubkey_full()); + let initiator_link = LinkId::new(78); + let mut initiator = + crate::peer::machine::PeerMachine::new_outbound(initiator_link, node_identity, 1000); + initiator.set_leg(crate::peer::PeerConnection::outbound( + initiator_link, + node_identity, + 1000, + )); + let noise_msg1 = initiator + .start_handshake(sender.keypair(), [9u8; 8], 1000) + .unwrap(); + + // Drive the responder half over an inbound leg that stays pending. + node.seed_handshake_machine(HandshakeSeed::inbound(link_id, 1000)) + .unwrap(); + let our_keypair = node.identity().keypair(); + let startup_epoch = node.startup_epoch(); + let machine = node.peer_machines.get_mut(&link_id).unwrap(); + machine + .receive_handshake_init(our_keypair, startup_epoch, &noise_msg1, 1000) + .unwrap(); + + assert_eq!( + machine.conn_expected_identity(), + Some(&sender_identity), + "msg1 identity learn must be recorded on the surviving carrier" + ); + + // The send of the responder's msg2 fails: the leg is retained, empty, for + // the sweep to reclaim. + machine.mark_failed(); + machine.mark_send_failed(); + + let stale = node.stale_connections(2000, 30_000); + assert_eq!( + stale.len(), + 1, + "the failed inbound leg must reach the sweep" + ); + assert_eq!( + stale[0].retry_addr, + Some(*sender_identity.node_addr()), + "a failed inbound leg still names the peer it learned from msg1" + ); +} + /// A msg1 that fails Noise processing must leave no trace in the registry. /// The control machine is built above the crypto so it can drive the /// handshake, but it stays a local until a promote tail inserts it — a diff --git a/src/peer/connection.rs b/src/peer/connection.rs index c77670a..4a26caf 100644 --- a/src/peer/connection.rs +++ b/src/peer/connection.rs @@ -11,7 +11,6 @@ use crate::PeerIdentity; use crate::noise::{self, NoiseSession}; use crate::proto::fmp::ConnectionState; use crate::transport::{LinkId, TransportAddr, TransportId}; -use crate::utils::index::SessionIndex; use std::fmt; /// A connection in the handshake phase, before authentication completes. @@ -89,63 +88,6 @@ impl PeerConnection { } } - // === Accessors (delegated to the pure ConnectionState) === - - /// Get the expected/learned peer identity, if known. - pub fn expected_identity(&self) -> Option<&PeerIdentity> { - self.state.expected_identity() - } - - /// When the connection started. Retained only to seed a control machine's - /// carrier from a pre-built leg (`Node::add_connection`); the operator-facing - /// `started_at_ms`/`last_activity_ms` telemetry now reads the machine carrier, - /// not the leg. - pub fn started_at(&self) -> u64 { - self.state.started_at() - } - - /// Connection duration so far. - pub fn duration(&self, current_time_ms: u64) -> u64 { - self.state.duration(current_time_ms) - } - - /// Time since last activity. - pub fn idle_time(&self, current_time_ms: u64) -> u64 { - self.state.idle_time(current_time_ms) - } - - // === Index Accessors === - - /// Get our session index (if set). - pub fn our_index(&self) -> Option { - self.state.our_index() - } - - /// Set our session index. - pub fn set_our_index(&mut self, index: SessionIndex) { - self.state.set_our_index(index); - } - - /// Get their session index (if known). - pub fn their_index(&self) -> Option { - self.state.their_index() - } - - /// Set their session index. - pub fn set_their_index(&mut self, index: SessionIndex) { - self.state.set_their_index(index); - } - - /// Get the transport ID (if set). - pub fn transport_id(&self) -> Option { - self.state.transport_id() - } - - /// Set the transport ID. - pub fn set_transport_id(&mut self, id: TransportId) { - self.state.set_transport_id(id); - } - // === Epoch Accessors === /// Get the remote peer's startup epoch (available after handshake). @@ -153,28 +95,6 @@ impl PeerConnection { self.state.remote_epoch() } - // === Handshake Resend === - - /// Store the wire-format msg1 bytes for resend and schedule the first resend. - pub fn set_handshake_msg1(&mut self, msg1: Vec, first_resend_at_ms: u64) { - self.state.set_handshake_msg1(msg1, first_resend_at_ms); - } - - /// Store the wire-format msg2 bytes for resend on duplicate msg1. - pub fn set_handshake_msg2(&mut self, msg2: Vec) { - self.state.set_handshake_msg2(msg2); - } - - /// Get the stored msg1 bytes (if any). - pub fn handshake_msg1(&self) -> Option<&[u8]> { - self.state.handshake_msg1() - } - - /// Get the stored msg2 bytes (if any). - pub fn handshake_msg2(&self) -> Option<&[u8]> { - self.state.handshake_msg2() - } - // === Crypto handle plumbing (the control machine drives the handshake) === /// Mutable access to the pure bookkeeping, so the control machine's @@ -187,13 +107,6 @@ impl PeerConnection { pub(crate) fn state_mut(&mut self) -> &mut ConnectionState { &mut self.state } - - // === Validation === - - /// Check if the connection has timed out. - pub fn is_timed_out(&self, current_time_ms: u64, timeout_ms: u64) -> bool { - self.state.is_timed_out(current_time_ms, timeout_ms) - } } impl fmt::Debug for PeerConnection { @@ -228,9 +141,9 @@ mod tests { let identity = make_peer_identity(); let conn = PeerConnection::outbound(LinkId::new(1), identity, 1000); - assert_eq!(conn.duration(1500), 500); - assert_eq!(conn.idle_time(1500), 500); - assert!(!conn.is_timed_out(1500, 1000)); - assert!(conn.is_timed_out(2500, 1000)); + assert_eq!(conn.state().duration(1500), 500); + assert_eq!(conn.state().idle_time(1500), 500); + assert!(!conn.state().is_timed_out(1500, 1000)); + assert!(conn.state().is_timed_out(2500, 1000)); } } diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 5add30f..f3adcc4 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -574,6 +574,7 @@ impl PeerMachine { ) -> Result, NoiseError> { let msg1 = { let direction = self.conn.direction(); + let expected_identity = self.conn.expected_identity().copied(); let leg = self.leg.as_mut().ok_or_else(no_pending_connection)?; if direction != LinkDirection::Outbound { @@ -583,8 +584,7 @@ impl PeerMachine { }); } - let remote_static = leg - .expected_identity() + let remote_static = expected_identity .expect("outbound must have expected identity") .pubkey_full(); @@ -776,9 +776,11 @@ impl PeerMachine { /// Expected peer identity of the surviving carrier — the home for the /// operator-visible `expected_peer` now that the leg no longer projects it. - /// Outbound carries the dial identity from construction; inbound stays `None` - /// in this view (the identity learned mid-handshake never rests here, as the - /// leg is consumed by promotion within the same message-handling step). + /// Outbound carries the dial identity from construction; inbound records + /// the identity discovered in msg1, written here by `receive_handshake_init` + /// at the same point it reaches the pending connection. Everything that + /// names a peer mid-handshake reads this, including the stale-connection + /// sweep's retry address. pub(crate) fn conn_expected_identity(&self) -> Option<&PeerIdentity> { self.conn.expected_identity() } @@ -3240,8 +3242,8 @@ mod tests { assert!(conn.conn_is_outbound()); assert!(!conn.conn_is_inbound()); assert!(!conn.has_session()); - assert!(conn.leg().unwrap().expected_identity().is_some()); - assert_eq!(conn.leg().unwrap().started_at(), 1000); + assert!(conn.conn_expected_identity().is_some()); + assert_eq!(conn.conn_started_at(), 1000); } #[test] @@ -3251,8 +3253,8 @@ mod tests { assert!(conn.conn_is_inbound()); assert!(!conn.conn_is_outbound()); assert!(!conn.has_session()); - assert!(conn.leg().unwrap().expected_identity().is_none()); - assert_eq!(conn.leg().unwrap().started_at(), 2000); + assert!(conn.conn_expected_identity().is_none()); + assert_eq!(conn.conn_started_at(), 2000); } #[test] @@ -3288,7 +3290,7 @@ mod tests { assert!(responder_conn.has_session()); // Responder learned initiator's identity - let discovered = responder_conn.leg().unwrap().expected_identity().unwrap(); + let discovered = responder_conn.conn_expected_identity().unwrap(); assert_eq!(discovered.pubkey(), initiator_identity.pubkey()); // Responder learned initiator's epoch diff --git a/src/proto/fmp/state.rs b/src/proto/fmp/state.rs index ab71351..a961516 100644 --- a/src/proto/fmp/state.rs +++ b/src/proto/fmp/state.rs @@ -210,6 +210,7 @@ impl ConnectionState { } /// Connection duration so far. + #[cfg(test)] pub fn duration(&self, current_time_ms: u64) -> u64 { current_time_ms.saturating_sub(self.started_at) } From 7fe1d75637430967638e7863ace69160c80d9459 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sun, 19 Jul 2026 01:18:05 +0000 Subject: [PATCH 7/7] peer: delete the pending-connection type, leaving the control machine whole The per-peer control machine has absorbed every field the pending connection carried. What remained was a struct holding two Noise handles beside a duplicate copy of bookkeeping nobody read. Replace it with a small carrier for the two handles and delete the type. Presence of that carrier, not the state of the handles inside it, is what marks a machine as mid-handshake. The distinction is essential rather than stylistic: a failed handshake drops its initiation handle and is deliberately retained so the stale sweep can reclaim it, and a completed one has its session taken before disposal. Deriving presence from the handles would make both invisible to the sweep, the connection count, and the peering budget at once, leaking the slot permanently. A test drives an empty carrier past every presence predicate and then detaches it, so a future edit cannot quietly couple the two. The remote startup epoch now comes from the surviving carrier, which the handshake operations already wrote at the same two points with the same value. The paired writes onto the pending connection's own bookkeeping had no readers left and are gone. The handshake-phase surface leaves the public API: it was public by accident rather than design, and the machine behind it is crate internal. Callers outside the crate that need a view of pending handshakes go through the operator queries, which are unchanged. ConnectionState::inbound_with_transport loses its last non-test caller with the inbound seed and is marked test-only. --- src/lib.rs | 2 +- src/node/handlers/handshake.rs | 40 ++---- src/node/handlers/timeout.rs | 12 +- src/node/lifecycle/mod.rs | 17 +-- src/node/mod.rs | 183 ++++++-------------------- src/node/tests/establish_chartests.rs | 4 +- src/node/tests/handshake.rs | 2 +- src/node/tests/mod.rs | 12 +- src/node/tests/unit.rs | 136 +++++++++++++++++-- src/peer/active.rs | 2 +- src/peer/connection.rs | 149 --------------------- src/peer/machine.rs | 120 ++++++++++------- src/peer/mod.rs | 5 +- src/proto/fmp/core.rs | 8 +- src/proto/fmp/mod.rs | 5 +- src/proto/fmp/state.rs | 18 +-- src/transport/tcp/mod.rs | 2 +- 17 files changed, 290 insertions(+), 427 deletions(-) delete mode 100644 src/peer/connection.rs diff --git a/src/lib.rs b/src/lib.rs index f2c94ec..fd1097e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -92,7 +92,7 @@ pub use cache::{CacheEntry, CacheError, CacheStats, CoordCache}; pub use proto::fmp::{PromotionResult, cross_connection_winner}; // Re-export peer types -pub use peer::{ActivePeer, ConnectivityState, PeerConnection, PeerError}; +pub use peer::{ActivePeer, ConnectivityState, PeerError}; // Re-export node types pub use node::{Node, NodeError, NodeState, UpdatePeersOutcome}; diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 21f41e3..ec2280e 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -6,11 +6,11 @@ use crate::node::acl::PeerAclContext; use crate::node::dataplane::PeerActionCtx; use crate::node::reject::{HandshakeReject, RejectReason}; use crate::node::{Node, NodeError}; +use crate::peer::ActivePeer; use crate::peer::machine::{ - CrossConnOutcome, FailReason, HandshakePhase, PeerAction, PeerEvent, PeerMachine, PeerState, - TimerKind, + CrossConnOutcome, FailReason, HandshakeCrypto, HandshakePhase, PeerAction, PeerEvent, + PeerMachine, PeerState, TimerKind, }; -use crate::peer::{ActivePeer, PeerConnection}; use crate::proto::fmp::wire::{Msg1Header, Msg2Header, build_msg2}; use crate::proto::fmp::{ EstablishSnapshot, EstablishView, InboundDecision, InboundReject, OutboundSnapshot, @@ -264,27 +264,17 @@ impl Node { // === CRYPTO COST PAID HERE === let link_id = self.allocate_link_id(); - let conn = PeerConnection::inbound_with_transport( - link_id, - packet.transport_id, - packet.remote_addr.clone(), - packet.timestamp_ms, - ); // The control machine drives the handshake, so it is built here, above - // the crypto, carrying the pending connection. It stays a local: it - // enters `peer_machines` only at the promote tails, so a rejected msg1 - // still leaves no registry trace and allocates no index. + // the crypto. It stays a local: it enters `peer_machines` only at the + // promote tails, so a rejected msg1 still leaves no registry trace and + // allocates no index. let mut machine = PeerMachine::new_inbound(link_id, packet.timestamp_ms); - // The inbound connection carries the transport ID from msg1, but the - // machine's carrier is only written on the outbound dial. Seed it here - // so the promotion hand-off reads it from the surviving carrier, - // matching the connection's own inbound seed. + // Seed the carrier with the transport and address msg1 arrived on, so + // the promotion hand-off reads them from it. machine.set_conn_transport_id(packet.transport_id); - // The inbound connection is constructed carrying the peer's address; - // seed the surviving carrier with it at the same point. machine.set_conn_source_addr(packet.remote_addr.clone()); - machine.set_leg(conn); + machine.set_leg(HandshakeCrypto::new()); let our_keypair = self.identity().keypair(); let noise_msg1 = &packet.data[header.noise_msg1_offset..]; @@ -328,10 +318,7 @@ impl Node { // state; from here the decision reads only `wire` and the snapshot. let wire = WireOutcome { peer_identity, - remote_epoch: machine - .leg() - .expect("pending connection attached above") - .remote_epoch(), + remote_epoch: machine.conn_remote_epoch(), their_index: header.sender_idx, msg2_payload: msg2_response, }; @@ -900,8 +887,8 @@ impl Node { }; // Check if this is a rekey msg2: the handshake state is on the - // ActivePeer (not a PeerConnection), so the link's machine — if one - // survives at all — carries no pending connection. A bare machine + // ActivePeer, not in a handshake carrier, so the link's machine — if + // one survives at all — carries no pending handshake. A bare machine // lookup would NOT discriminate here: an established peer's machine // stays keyed by this link, so the pending connection's presence is // what marks a fresh establish. Look for a peer with matching @@ -1392,6 +1379,7 @@ impl Node { let carrier_transport_id = machine.conn_transport_id(); let carrier_source_addr = machine.conn_source_addr().cloned(); let carrier_is_outbound = machine.conn_is_outbound(); + let carrier_remote_epoch = machine.conn_remote_epoch(); let link_stats = machine.conn_link_stats().clone(); // Verify handshake is complete and extract session @@ -1420,7 +1408,7 @@ impl Node { link_id, reason: "missing source_addr".into(), })?; - let remote_epoch = connection.remote_epoch(); + let remote_epoch = carrier_remote_epoch; let peer_node_addr = *verified_identity.node_addr(); let is_outbound = carrier_is_outbound; diff --git a/src/node/handlers/timeout.rs b/src/node/handlers/timeout.rs index ad7267c..e950d11 100644 --- a/src/node/handlers/timeout.rs +++ b/src/node/handlers/timeout.rs @@ -193,14 +193,14 @@ impl Node { .collect(); for link in timer_links { // The idle-timeout threshold reads the survivor carrier's - // last-activity (the leg no longer projects it); the leg still - // supplies direction/identity for the retry decision below. + // last-activity; presence of a pending handshake is what decides + // between reaping and dropping an orphan timer. let timed_out = self .peer_machines .get(&link) .is_some_and(|machine| machine.conn_is_timed_out(now_ms, timeout_ms)); - let (reap, retry_peer) = match self.leg(&link) { - Some(_) if timed_out => { + let (reap, retry_peer) = match self.has_pending_leg(&link) { + true if timed_out => { let retry_peer = if self .peer_machines .get(&link) @@ -216,8 +216,8 @@ impl Node { (true, retry_peer) } // Not yet idle-timed-out: leave the timer for a later tick. - Some(_) => (false, None), - None => { + true => (false, None), + false => { // Orphan timer (connection already reaped elsewhere) — drop it. if let Some(timers) = self.peer_timers.get_mut(&link) { timers.remove(&TimerKind::HandshakeTimeout); diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 6df800e..f7ae0aa 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -15,8 +15,7 @@ use crate::node::acl::PeerAclContext; use crate::node::dataplane::PeerActionCtx; use crate::nostr::{BootstrapEvent, NostrRendezvous}; use crate::nostr::{BootstrapHandoffResult, EstablishedTraversal}; -use crate::peer::PeerConnection; -use crate::peer::machine::{PeerEvent, PeerMachine}; +use crate::peer::machine::{HandshakeCrypto, PeerEvent, PeerMachine}; use crate::proto::fmp::wire::build_msg1; use crate::proto::fmp::{Disconnect, DisconnectReason}; use crate::transport::{Link, LinkDirection, LinkId, TransportAddr, TransportId, packet_channel}; @@ -380,7 +379,7 @@ impl Node { }) } - fn is_connecting_to_peer_on_path( + pub(in crate::node) fn is_connecting_to_peer_on_path( &self, peer_node_addr: &NodeAddr, transport_id: TransportId, @@ -582,14 +581,12 @@ impl Node { ) -> Result<(), NodeError> { let peer_node_addr = *peer_identity.node_addr(); - // Create connection in handshake phase (outbound knows expected identity) let current_time_ms = Self::now_ms(); - let connection = PeerConnection::outbound(link_id, peer_identity, current_time_ms); - // The control machine drives the handshake, so it takes the connection - // before the crypto runs. The machine was born at dial and persisted in - // `initiate_connection`, so every live caller already has one; recover - // with a fresh one if a direct caller ever skips the dial. + // The control machine drives the handshake, so it takes the crypto + // carrier before the crypto runs. The machine was born at dial and + // persisted in `initiate_connection`, so every live caller already has + // one; recover with a fresh one if a direct caller ever skips the dial. debug_assert!( self.peer_machines.contains_key(&link_id), "outbound msg1 prepared for link {link_id} with no dial-time machine" @@ -597,7 +594,7 @@ impl Node { self.peer_machines .entry(link_id) .or_insert_with(|| PeerMachine::new_outbound(link_id, peer_identity, current_time_ms)) - .set_leg(connection); + .set_leg(HandshakeCrypto::new()); // Allocate a session index for this handshake let our_index = match self.index_allocator.allocate() { diff --git a/src/node/mod.rs b/src/node/mod.rs index 2d5fa71..c308633 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -37,8 +37,8 @@ use self::reloadable::Reloadable; pub(crate) const REKEY_JITTER_SECS: i64 = 15; use crate::cache::CoordCache; use crate::node::session::SessionEntry; +use crate::peer::ActivePeer; use crate::peer::machine::{PeerMachine, TimerKind}; -use crate::peer::{ActivePeer, PeerConnection}; use crate::proto::bloom::{BloomFilter, BloomState}; use crate::proto::fmp::Fmp; use crate::proto::fmp::wire::{ @@ -351,9 +351,9 @@ pub struct Node { // === Per-Peer Control Machines === /// Per-peer lifecycle control FSMs, keyed by the stable `LinkId` that spans - /// the handshake→active lifetime. Each machine owns its pending handshake - /// connection (the `PeerConnection` leg) while the handshake is in - /// progress — the single LinkId-keyed per-peer map on `Node`; `peers` + /// the handshake→active lifetime. Each machine owns its handshake crypto + /// carrier while the handshake is in progress — the single LinkId-keyed + /// per-peer map on `Node`; `peers` /// stays byte-unchanged (hot path pristine). /// Machines are inserted at dial and inbound msg1, and stepped in production /// by the handshake handlers, the rekey-cadence and liveness-reap routers, @@ -2405,114 +2405,29 @@ impl Node { // === Connection Management (Handshake Phase) === - /// The pending connection for `link_id`, read through the control machine - /// that carries it. - fn leg(&self, link_id: &LinkId) -> Option<&PeerConnection> { + /// Whether `link_id` has a pending handshake, read through the control + /// machine that carries it. + fn has_pending_leg(&self, link_id: &LinkId) -> bool { self.peer_machines .get(link_id) - .and_then(|machine| machine.leg()) - } - - /// Mutable access to the pending connection for `link_id`. - fn leg_mut(&mut self, link_id: &LinkId) -> Option<&mut PeerConnection> { - self.peer_machines - .get_mut(link_id) - .and_then(|machine| machine.leg_mut()) - } - - /// Add a pending connection. - /// - /// Seeds a control machine for the leg when none exists yet and embeds the - /// connection on it, for callers that insert a connection directly rather - /// than through the dial or inbound-msg1 paths, which build the machine - /// themselves. - pub fn add_connection(&mut self, connection: PeerConnection) -> Result<(), NodeError> { - let link_id = connection.state().link_id(); - - if self - .peer_machines - .get(&link_id) .is_some_and(|machine| machine.leg().is_some()) - { - return Err(NodeError::ConnectionAlreadyExists(link_id)); - } - - if self.max_connections() > 0 && self.connection_count() >= self.max_connections() { - return Err(NodeError::MaxConnectionsExceeded { - max: self.max_connections(), - }); - } - - let machine = self.peer_machines.entry(link_id).or_insert_with(|| { - let now = connection.state().started_at(); - match connection.state().expected_identity() { - Some(identity) if connection.state().is_outbound() => { - PeerMachine::new_outbound(link_id, *identity, now) - } - _ => PeerMachine::new_inbound(link_id, now), - } - }); - // Seed the surviving carrier's peer index and transport from the - // pre-built leg so the promotion hand-off reads them from the machine, - // matching the establish paths that write them on the machine directly. - if let Some(ours) = connection.state().our_index() { - machine.set_conn_our_index(ours); - } - if let Some(their) = connection.state().their_index() { - machine.set_conn_their_index(their); - } - if let Some(tid) = connection.state().transport_id() { - machine.set_conn_transport_id(tid); - } - if let Some(addr) = connection.state().source_addr() { - machine.set_conn_source_addr(addr.clone()); - } - machine.set_leg(connection); - Ok(()) } - /// Test-support: seed a control machine for `seed.link_id` the way - /// [`Node::add_connection`] does, without the caller having to build a - /// free-standing leg first. + /// Test-support: seed a control machine for `seed.link_id` directly, + /// without the caller having to stage a pending handshake by hand. /// - /// The carrier seeding below is a verbatim copy of `add_connection`'s: the - /// conditional writes (`our_index`, `their_index`, `transport_id`, - /// `source_addr`) and `set_leg`, built through the same - /// `entry(..).or_insert_with(..)` so an existing leg-less machine keeps its - /// constructor-side fields. The seeded carrier matches what the establish - /// paths write: every field a promotion reads is present. Post-construction - /// `started_at` and the stored handshake bytes are not seeded here, exactly - /// as they are not for a test that goes through `add_connection` today. - /// - /// The duplication is deliberate: keeping the carrier writes visible here - /// is what lets each later step of the leg dissolution revise them at a - /// single reviewable site. The two bodies must stay in sync until - /// `add_connection` itself is removed; any drift surfaces as a test - /// failure while both paths still exist. + /// The machine is chosen the way the establish paths choose it — outbound + /// when the seed names a peer, inbound otherwise — and its carrier is + /// seeded with every field a promotion reads. Built through + /// `entry(..).or_insert_with(..)`, so an existing handshake-less machine + /// keeps its constructor-side fields rather than being rebuilt. + /// Post-construction `started_at` and the stored handshake bytes are not + /// seeded; the establish paths write those at their own points. #[cfg(test)] pub(crate) fn seed_handshake_machine(&mut self, seed: HandshakeSeed) -> Result<(), NodeError> { let link_id = seed.link_id; - let mut connection = match seed.expected_identity { - Some(identity) => PeerConnection::outbound(link_id, identity, seed.started_at_ms), - None => PeerConnection::inbound(link_id, seed.started_at_ms), - }; - if let Some(id) = seed.transport_id { - connection.state_mut().set_transport_id(id); - } - let seeded_source_addr = seed.source_addr.clone(); - if let Some(index) = seed.our_index { - connection.state_mut().set_our_index(index); - } - if let Some(index) = seed.their_index { - connection.state_mut().set_their_index(index); - } - - if self - .peer_machines - .get(&link_id) - .is_some_and(|machine| machine.leg().is_some()) - { + if self.has_pending_leg(&link_id) { return Err(NodeError::ConnectionAlreadyExists(link_id)); } @@ -2522,54 +2437,31 @@ impl Node { }); } - let machine = self.peer_machines.entry(link_id).or_insert_with(|| { - let now = connection.state().started_at(); - match connection.state().expected_identity() { - Some(identity) if connection.state().is_outbound() => { - PeerMachine::new_outbound(link_id, *identity, now) - } - _ => PeerMachine::new_inbound(link_id, now), - } - }); - if let Some(ours) = connection.state().our_index() { - machine.set_conn_our_index(ours); + let started_at_ms = seed.started_at_ms; + let expected_identity = seed.expected_identity; + let machine = + self.peer_machines + .entry(link_id) + .or_insert_with(|| match expected_identity { + Some(identity) => PeerMachine::new_outbound(link_id, identity, started_at_ms), + None => PeerMachine::new_inbound(link_id, started_at_ms), + }); + if let Some(index) = seed.our_index { + machine.set_conn_our_index(index); } - if let Some(their) = connection.state().their_index() { - machine.set_conn_their_index(their); + if let Some(index) = seed.their_index { + machine.set_conn_their_index(index); } - if let Some(tid) = connection.state().transport_id() { - machine.set_conn_transport_id(tid); + if let Some(id) = seed.transport_id { + machine.set_conn_transport_id(id); } - if let Some(addr) = seeded_source_addr { + if let Some(addr) = seed.source_addr { machine.set_conn_source_addr(addr); } - machine.set_leg(connection); + machine.set_leg(crate::peer::machine::HandshakeCrypto::new()); Ok(()) } - /// Get a connection by LinkId. - pub fn get_connection(&self, link_id: &LinkId) -> Option<&PeerConnection> { - self.leg(link_id) - } - - /// Get a mutable connection by LinkId. - pub fn get_connection_mut(&mut self, link_id: &LinkId) -> Option<&mut PeerConnection> { - self.leg_mut(link_id) - } - - /// Remove a connection, disposing its control machine alongside - /// (the disposal complement of `add_connection`'s machine seeding). The - /// connection is taken off the machine BEFORE the machine is dropped, so - /// the caller still receives it. - pub fn remove_connection(&mut self, link_id: &LinkId) -> Option { - let connection = self - .peer_machines - .get_mut(link_id) - .and_then(|machine| machine.take_leg()); - self.remove_peer_machine(*link_id); - connection - } - /// Iterate over the control machines that carry a pending connection. /// /// Carrying a pending connection is what makes a machine handshake-phase, @@ -3279,10 +3171,9 @@ impl fmt::Debug for Node { /// Test-support seed spec for [`Node::seed_handshake_machine`]. /// -/// Mirrors the leg constructors plus the setters tests apply to a connection -/// *before* handing it to `add_connection`. Only fields that exist on the leg -/// at add time belong here; crypto is run afterwards through -/// `get_connection_mut`, which `add_connection` never reads. +/// Carries the carrier fields a seeded machine needs before any crypto runs. +/// Only fields the establish paths write at seed time belong here; the Noise +/// handshake is driven afterwards through the machine's own crypto methods. #[cfg(test)] #[derive(Debug, Clone)] pub(crate) struct HandshakeSeed { diff --git a/src/node/tests/establish_chartests.rs b/src/node/tests/establish_chartests.rs index 42fd01f..1004eba 100644 --- a/src/node/tests/establish_chartests.rs +++ b/src/node/tests/establish_chartests.rs @@ -243,7 +243,7 @@ async fn chartest_msg1_duplicate_pending_resends_stored_msg2() { ); assert_eq!(node.peer_count(), 0, "duplicate msg1 promotes nothing"); assert!( - node.get_connection(&link_id).is_some(), + node.has_pending_leg(&link_id), "pending connection is left intact" ); assert_eq!( @@ -376,7 +376,7 @@ async fn chartest_msg1_inbound_promote_defers_pending_outbound_to_same_identity( assert!(peer.has_session()); assert_eq!(node.peer_count(), 1); assert!( - node.get_connection(&out_link).is_some(), + node.has_pending_leg(&out_link), "pending outbound to the same identity must be preserved (deferred cleanup)" ); assert!( diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 4c535bd..67f9395 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -814,7 +814,7 @@ async fn test_failed_connection_cleanup() { let machine = node .peer_machines .get_mut(&link_id) - .expect("machine seeded by add_connection"); + .expect("machine seeded by the handshake seeder"); let alloc = &mut node.index_allocator; let actions = machine.step( crate::peer::machine::PeerEvent::HandshakeSendFailed, diff --git a/src/node/tests/mod.rs b/src/node/tests/mod.rs index a302e26..9d80748 100644 --- a/src/node/tests/mod.rs +++ b/src/node/tests/mod.rs @@ -1,5 +1,6 @@ use super::*; use crate::PeerIdentity; +use crate::peer::machine::HandshakeCrypto; use crate::transport::{LinkDirection, ReceivedPacket, TransportAddr, packet_channel}; use crate::utils::index::SessionIndex; use std::time::Duration; @@ -93,18 +94,14 @@ pub(super) fn outbound_leg( current_time_ms: u64, ) -> PeerMachine { let mut machine = PeerMachine::new_outbound(link_id, expected_identity, current_time_ms); - machine.set_leg(PeerConnection::outbound( - link_id, - expected_identity, - current_time_ms, - )); + machine.set_leg(HandshakeCrypto::new()); machine } /// The responder twin of [`outbound_leg`]. pub(super) fn inbound_leg(link_id: LinkId, current_time_ms: u64) -> PeerMachine { let mut machine = PeerMachine::new_inbound(link_id, current_time_ms); - machine.set_leg(PeerConnection::inbound(link_id, current_time_ms)); + machine.set_leg(HandshakeCrypto::new()); machine } @@ -112,8 +109,7 @@ pub(super) fn inbound_leg(link_id: LinkId, current_time_ms: u64) -> PeerMachine /// /// Returns the peer identity. The leg is outbound, in Complete state, with /// session, indices, and transport info set, and is installed on the node -/// through [`Node::seed_handshake_machine`] — the test-surface twin of -/// `Node::add_connection`. +/// through [`Node::seed_handshake_machine`]. pub(super) fn seed_completed_connection( node: &mut Node, link_id: LinkId, diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 280bbf4..63a1ea6 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -342,9 +342,9 @@ fn test_node_connection_management() { assert_eq!(node.connection_count(), 1); - assert!(node.get_connection(&link_id).is_some()); + assert!(node.has_pending_leg(&link_id)); - node.remove_connection(&link_id); + node.remove_peer_machine(link_id); assert_eq!(node.connection_count(), 0); } @@ -364,7 +364,7 @@ fn test_node_connection_duplicate() { #[cfg(debug_assertions)] #[test] -fn test_peer_maps_coherent_after_add_connection() { +fn test_peer_maps_coherent_after_seeding_a_handshake() { let mut node = make_node(); let identity = make_peer_identity(); @@ -374,7 +374,7 @@ fn test_peer_maps_coherent_after_add_connection() { assert!( node.peer_machines.contains_key(&link_id), - "add_connection seeds a control machine for its leg" + "seeding a handshake creates its control machine" ); node.debug_assert_peer_maps_coherent(); } @@ -2067,7 +2067,7 @@ fn nostr_rendezvous_outbound_admission_atomic_roundtrip() { /// to `addr_b`. Returns the sender's NodeAddr so the test can assert on /// identity-keyed maps. /// -/// Uses the same outbound-PeerConnection->Noise IK pattern as the +/// Uses the same outbound-machine->Noise IK pattern as the /// integration handshake tests, but inlined and unit-scoped. async fn craft_and_send_msg1( node_b: &Node, @@ -2405,6 +2405,126 @@ fn test_failed_connection_is_retained_and_reaped() { assert_eq!(node.connection_count(), 0); } +/// Handshake-phase membership is decided by whether a crypto carrier is +/// ATTACHED, never by whether either Noise handle inside it is populated. +/// +/// The distinction is the whole reason the carrier is a struct rather than a +/// pair of bare handle fields. A carrier legitimately sits attached and empty: +/// `mark_failed` drops the initiation handle and deliberately keeps the +/// carrier so the sweep can reclaim it, `take_session` empties the other, and +/// every handshake begins with both handles unset. If presence were derived +/// from the handles, every failed handshake would vanish from the sweep, +/// the count, and the peering budget at once — a permanent leak that no +/// existing test would notice. +/// +/// This drives the empty-carrier shape past every presence predicate on +/// `Node` and asserts each one reports "present", then detaches and asserts +/// each reports "absent". +#[test] +fn handshake_presence_tracks_the_carrier_not_the_noise_handles() { + use crate::proto::fmp::LifecycleView; + + let mut node = make_node(); + let link_id = LinkId::new(31); + let transport_id = TransportId::new(9); + let peer_identity = make_peer_identity(); + let peer_addr = TransportAddr::from_string("10.0.0.9:9999"); + + node.seed_handshake_machine( + HandshakeSeed::outbound(link_id, peer_identity, 1000) + .with_transport_id(transport_id) + .with_source_addr(peer_addr.clone()), + ) + .unwrap(); + + // A freshly seeded carrier holds neither handle — the construction window. + let leg = node.peer_machines.get(&link_id).unwrap().leg().unwrap(); + assert!( + leg.noise_handshake.is_none() && leg.noise_session.is_none(), + "the seeded carrier must start with both handles empty" + ); + + // Every presence predicate must see it, handles or not. + let assert_present = |node: &Node, when: &str| { + assert_eq!(node.connection_count(), 1, "connection_count: {when}"); + assert_eq!(node.connections().count(), 1, "connections(): {when}"); + assert!(node.has_pending_leg(&link_id), "has_pending_leg: {when}"); + assert_eq!( + node.stale_connections(1_000_000, 30_000).len(), + 1, + "stale_connections: {when}" + ); + assert!( + node.is_connecting_to_peer_on_path(peer_identity.node_addr(), transport_id, &peer_addr), + "is_connecting_to_peer_on_path: {when}" + ); + // The last two predicates sit inline in functions with no callable + // seam, so these MIRROR them rather than exercising them: the shape is + // pinned here, but a mutation at the production site would not fail + // this test. Both sites read `machine.leg().is_some()` verbatim. + assert!( + node.peer_machines.values().any(|machine| { + machine.leg().is_some() && machine.conn_transport_id() == Some(transport_id) + }), + "transport-in-use: {when}" + ); + // The complement of the rekey-msg2 discriminator: a machine carrying + // a pending handshake marks a fresh establish, so `handle_msg2` must + // NOT take its rekey-completion branch. + assert!( + node.peer_machines + .get(&link_id) + .is_some_and(|machine| machine.leg().is_some()), + "rekey-msg2 discriminator: {when}" + ); + // Fires the live-carrier coherence assertion; a machine that had gone + // invisible would panic here rather than fail an assert_eq above. + node.debug_assert_peer_maps_coherent(); + }; + + assert_present(&node, "freshly seeded, both handles empty"); + + // Drive to the failed shape: the initiation handle is dropped and the + // carrier is deliberately retained for the sweep. + let our_keypair = node.identity().keypair(); + let startup_epoch = node.startup_epoch(); + let machine = node.peer_machines.get_mut(&link_id).unwrap(); + machine + .start_handshake(our_keypair, startup_epoch, 1000) + .unwrap(); + assert!( + machine.leg().unwrap().noise_handshake.is_some(), + "start_handshake arms the initiation handle" + ); + machine.mark_failed(); + machine.mark_send_failed(); + + let leg = node.peer_machines.get(&link_id).unwrap().leg().unwrap(); + assert!( + leg.noise_handshake.is_none() && leg.noise_session.is_none(), + "a failed handshake holds neither handle" + ); + assert_present(&node, "failed, both handles empty"); + + // Detaching the carrier — and only that — ends handshake-phase membership. + node.peer_machines.get_mut(&link_id).unwrap().take_leg(); + assert_eq!(node.connection_count(), 0, "connection_count after detach"); + assert_eq!(node.connections().count(), 0, "connections() after detach"); + assert!( + !node.has_pending_leg(&link_id), + "has_pending_leg after detach" + ); + assert_eq!( + node.stale_connections(1_000_000, 30_000).len(), + 0, + "stale_connections after detach" + ); + assert!( + !node.is_connecting_to_peer_on_path(peer_identity.node_addr(), transport_id, &peer_addr), + "is_connecting_to_peer_on_path after detach" + ); +} + /// The identity a responder discovers in msg1 must land on the surviving /// carrier, not only on the pending leg. Everything that names an inbound /// peer mid-handshake reads the carrier: the stale-connection sweep's @@ -2424,11 +2544,7 @@ fn inbound_msg1_records_the_learned_identity_on_the_carrier() { let initiator_link = LinkId::new(78); let mut initiator = crate::peer::machine::PeerMachine::new_outbound(initiator_link, node_identity, 1000); - initiator.set_leg(crate::peer::PeerConnection::outbound( - initiator_link, - node_identity, - 1000, - )); + initiator.set_leg(crate::peer::machine::HandshakeCrypto::new()); let noise_msg1 = initiator .start_handshake(sender.keypair(), [9u8; 8], 1000) .unwrap(); diff --git a/src/peer/active.rs b/src/peer/active.rs index 9690430..3b5ace3 100644 --- a/src/peer/active.rs +++ b/src/peer/active.rs @@ -315,7 +315,7 @@ impl ActivePeer { /// Create from verified identity with existing link stats. /// - /// Used when promoting from PeerConnection, preserving handshake stats. + /// Used when promoting a completed handshake, preserving its link stats. /// For peers with Noise sessions, use `with_session` instead. pub fn with_stats( identity: PeerIdentity, diff --git a/src/peer/connection.rs b/src/peer/connection.rs deleted file mode 100644 index 4a26caf..0000000 --- a/src/peer/connection.rs +++ /dev/null @@ -1,149 +0,0 @@ -//! Peer Connection (Handshake Phase) -//! -//! Represents an in-progress connection before authentication completes. -//! PeerConnection tracks the Noise IK handshake and transitions to -//! ActivePeer upon successful authentication. Neither the handshake *phase* -//! (initial / sent_msg1 / complete / failed) nor the handshake operations are -//! tracked here — both live on the per-peer control machine, which drives the -//! Noise handles held below. - -use crate::PeerIdentity; -use crate::noise::{self, NoiseSession}; -use crate::proto::fmp::ConnectionState; -use crate::transport::{LinkId, TransportAddr, TransportId}; -use std::fmt; - -/// A connection in the handshake phase, before authentication completes. -/// -/// For outbound connections, we know the expected peer identity from config. -/// For inbound connections, we learn the identity during the Noise handshake. -/// -/// This is the shell holder for the FMP crypto/state split: the pure -/// connection bookkeeping lives in [`ConnectionState`] (`proto::fmp::state`), -/// and the two Noise crypto handles stay here beside it. Pure public methods -/// delegate to `self.state`; the control machine drives the handles and -/// records each result here and on its own carrier. -pub struct PeerConnection { - /// Pure, runtime-agnostic connection bookkeeping. - state: ConnectionState, - - /// Noise handshake state (consumes on completion). - /// - /// Driven by the control machine, which owns the handshake operations. - pub(crate) noise_handshake: Option, - - /// Completed Noise session (available after handshake complete). - /// - /// Driven by the control machine, which owns the handshake operations. - pub(crate) noise_session: Option, -} - -impl PeerConnection { - /// Create a new outbound connection (we are initiating). - /// - /// For outbound, we know who we're trying to reach from configuration. - /// The Noise handshake will be initialized when `start_handshake` is called. - pub fn outbound( - link_id: LinkId, - expected_identity: PeerIdentity, - current_time_ms: u64, - ) -> Self { - Self { - state: ConnectionState::outbound(link_id, expected_identity, current_time_ms), - noise_handshake: None, - noise_session: None, - } - } - - /// Create a new inbound connection (they are initiating). - /// - /// For inbound, we don't know who they are until we decrypt their - /// identity from Noise message 1. - pub fn inbound(link_id: LinkId, current_time_ms: u64) -> Self { - Self { - state: ConnectionState::inbound(link_id, current_time_ms), - noise_handshake: None, - noise_session: None, - } - } - - /// Create a new inbound connection with transport information. - /// - /// Used when processing msg1 where we know the transport and source address. - pub fn inbound_with_transport( - link_id: LinkId, - transport_id: TransportId, - source_addr: TransportAddr, - current_time_ms: u64, - ) -> Self { - Self { - state: ConnectionState::inbound_with_transport( - link_id, - transport_id, - source_addr, - current_time_ms, - ), - noise_handshake: None, - noise_session: None, - } - } - - // === Epoch Accessors === - - /// Get the remote peer's startup epoch (available after handshake). - pub fn remote_epoch(&self) -> Option<[u8; 8]> { - self.state.remote_epoch() - } - - // === Crypto handle plumbing (the control machine drives the handshake) === - - /// Mutable access to the pure bookkeeping, so the control machine's - /// handshake operations can record their results here as well as on the - /// surviving carrier. - pub(crate) fn state(&self) -> &ConnectionState { - &self.state - } - - pub(crate) fn state_mut(&mut self) -> &mut ConnectionState { - &mut self.state - } -} - -impl fmt::Debug for PeerConnection { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("PeerConnection") - .field("link_id", &self.state.link_id()) - .field("direction", &self.state.direction()) - .field("expected_identity", &self.state.expected_identity()) - .field("has_noise_handshake", &self.noise_handshake.is_some()) - .field("has_noise_session", &self.noise_session.is_some()) - .field("our_index", &self.state.our_index()) - .field("their_index", &self.state.their_index()) - .field("transport_id", &self.state.transport_id()) - .field("started_at", &self.state.started_at()) - .field("last_activity", &self.state.last_activity()) - .finish() - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::Identity; - - fn make_peer_identity() -> PeerIdentity { - let identity = Identity::generate(); - PeerIdentity::from_pubkey(identity.pubkey()) - } - - #[test] - fn test_connection_timing() { - let identity = make_peer_identity(); - let conn = PeerConnection::outbound(LinkId::new(1), identity, 1000); - - assert_eq!(conn.state().duration(1500), 500); - assert_eq!(conn.state().idle_time(1500), 500); - assert!(!conn.state().is_timed_out(1500, 1000)); - assert!(conn.state().is_timed_out(2500, 1000)); - } -} diff --git a/src/peer/machine.rs b/src/peer/machine.rs index f3adcc4..115aeef 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -57,7 +57,6 @@ #![allow(dead_code)] use crate::noise::{self, NoiseError, NoiseSession}; -use crate::peer::PeerConnection; use crate::proto::fmp::{ ConnAction, ConnSnapshot, ConnectionState, EstablishSnapshot, Fmp, InboundDecision, OutboundDecision, OutboundSnapshot, PeerSnapshot, PromotionResult, RekeyCfg, @@ -417,6 +416,46 @@ fn no_pending_connection() -> NoiseError { } } +/// The handshake-phase Noise crypto, owned by the control machine. +/// +/// PRESENCE OF THIS STRUCT (`PeerMachine::leg().is_some()`) IS THE +/// HANDSHAKE-PHASE CARRIER SIGNAL — it is what `Node::connections()`, +/// `connection_count()`, the stale-connection sweep, the transport-in-use +/// check, and the peering budget all key on. It is attached and detached at +/// exactly the points the pending connection was, and its presence is NOT a +/// function of whether either handle is populated. +/// +/// A present-but-empty value is legal and load-bearing: `mark_failed` drops +/// the initiation handle while deliberately retaining the carrier so the +/// sweep can reclaim it, and `take_session` empties the other. Deriving +/// presence from handle presence would make every failed handshake invisible +/// to the sweep — a permanent leak. See the presence tests in this module. +pub(crate) struct HandshakeCrypto { + /// Noise handshake state (consumed on completion). + pub(crate) noise_handshake: Option, + /// Completed Noise session (available once the handshake completes). + pub(crate) noise_session: Option, +} + +impl HandshakeCrypto { + /// A fresh carrier holding neither handle, as every handshake begins. + pub(crate) fn new() -> Self { + Self { + noise_handshake: None, + noise_session: None, + } + } +} + +impl std::fmt::Debug for HandshakeCrypto { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("HandshakeCrypto") + .field("has_noise_handshake", &self.noise_handshake.is_some()) + .field("has_noise_session", &self.noise_session.is_some()) + .finish() + } +} + /// Per-peer control FSM. Holds control-tier lifecycle state only; the /// send-critical state is published as `PeerSendState` and mutated via the /// emitted [`PeerAction`]s. @@ -424,13 +463,13 @@ pub(crate) struct PeerMachine { state: PeerState, link: LinkId, identity: Option, - /// The pending handshake connection this machine owns while it is in the - /// handshake window. `None` before the connection is built (the dial - /// window) and after promotion consumes it (the machine survives as the - /// active peer's control machine). Its bookkeeping is storage the shell - /// reaches through the accessors below; its Noise handles are driven by - /// this machine's handshake operations. - leg: Option, + /// The handshake-phase Noise crypto this machine owns while it is in the + /// handshake window. `None` before the handshake begins (the dial window) + /// and after promotion consumes it (the machine survives as the active + /// peer's control machine). Its presence — not the state of the handles + /// inside it — is what marks this machine as carrying a pending + /// handshake; see [`HandshakeCrypto`]. + leg: Option, /// Pure handshake-phase bookkeeping (link/direction/indices/transport/ /// stored handshake bytes/epoch). Reused verbatim from the FMP state core. conn: ConnectionState, @@ -532,25 +571,21 @@ impl PeerMachine { self.state } - /// The pending handshake connection, if this leg is still in the - /// handshake window. - pub(crate) fn leg(&self) -> Option<&PeerConnection> { + /// The handshake crypto carrier, if this machine is still in the + /// handshake window. Presence answers "is there a pending handshake + /// here", independently of whether either handle is populated. + pub(crate) fn leg(&self) -> Option<&HandshakeCrypto> { self.leg.as_ref() } - /// Mutable access to the pending handshake connection. - pub(crate) fn leg_mut(&mut self) -> Option<&mut PeerConnection> { - self.leg.as_mut() - } - - /// Take the pending handshake connection off the machine (promotion and + /// Take the handshake crypto carrier off the machine (promotion and /// teardown consume it by value). - pub(crate) fn take_leg(&mut self) -> Option { + pub(crate) fn take_leg(&mut self) -> Option { self.leg.take() } - /// Embed a pending handshake connection on the machine. - pub(crate) fn set_leg(&mut self, leg: PeerConnection) { + /// Attach a handshake crypto carrier to the machine. + pub(crate) fn set_leg(&mut self, leg: HandshakeCrypto) { self.leg = Some(leg); } @@ -593,7 +628,6 @@ impl PeerMachine { let msg1 = hs.write_message_1()?; leg.noise_handshake = Some(hs); - leg.state_mut().touch(current_time_ms); msg1 }; @@ -636,11 +670,9 @@ impl PeerMachine { .remote_static() .expect("remote static available after msg1"); let learned_identity = PeerIdentity::from_pubkey_full(remote_static); - leg.state_mut().set_expected_identity(learned_identity); // Capture remote epoch from msg1 let remote_epoch = hs.remote_epoch(); - leg.state_mut().set_remote_epoch(remote_epoch); // Generate message 2 let msg2 = hs.write_message_2()?; @@ -648,7 +680,6 @@ impl PeerMachine { // Handshake is complete for responder let session = hs.into_session()?; leg.noise_session = Some(session); - leg.state_mut().touch(current_time_ms); (msg2, learned_identity, remote_epoch) }; @@ -690,11 +721,9 @@ impl PeerMachine { // Capture remote epoch from msg2 let remote_epoch = hs.remote_epoch(); - leg.state_mut().set_remote_epoch(remote_epoch); let session = hs.into_session()?; leg.noise_session = Some(session); - leg.state_mut().touch(current_time_ms); remote_epoch }; @@ -785,6 +814,14 @@ impl PeerMachine { self.conn.expected_identity() } + /// Remote startup epoch of the surviving carrier, recorded by the + /// handshake operations at the message that reveals it (msg1 inbound, + /// msg2 outbound). Promotion reads it to seed the active peer and to + /// detect a peer restart across a reconnect. + pub(crate) fn conn_remote_epoch(&self) -> Option<[u8; 8]> { + self.conn.remote_epoch() + } + /// Stored wire-format msg1 of the surviving carrier — the resend source for /// the outbound handshake retransmit, now that the leg no longer carries it. pub(crate) fn conn_handshake_msg1(&self) -> Option<&[u8]> { @@ -860,17 +897,14 @@ impl PeerMachine { self.conn.link_stats() } - /// Record the peer session index on the surviving carrier. Seeds the - /// carrier from a pre-built leg (`Node::add_connection`) so the promotion - /// hand-off matches the establish paths that write it on the machine. + /// Record the peer session index on the surviving carrier, so the + /// promotion hand-off reads it from the machine. pub(crate) fn set_conn_their_index(&mut self, index: SessionIndex) { self.conn.set_their_index(index); } - /// Record the transport ID on the surviving carrier. Populated on the - /// inbound establish path (the leg seeds it at msg1, but the machine's - /// carrier is only written on the outbound dial) and when seeding the - /// carrier from a pre-built leg (`Node::add_connection`). + /// Record the transport ID on the surviving carrier. Written on the + /// inbound establish path at msg1 and on the outbound dial. pub(crate) fn set_conn_transport_id(&mut self, id: TransportId) { self.conn.set_transport_id(id); } @@ -2871,7 +2905,7 @@ mod tests { 100, &mut alloc, ); - m.set_leg(PeerConnection::outbound(LinkId::new(1), peer, 100)); + m.set_leg(HandshakeCrypto::new()); assert!(m.is_handshaking_sent_msg1()); assert!(!m.is_failed()); assert_eq!(m.displayed_handshake_state(), "sent_msg1"); @@ -3220,17 +3254,13 @@ mod tests { current_time_ms: u64, ) -> PeerMachine { let mut machine = PeerMachine::new_outbound(link_id, expected_identity, current_time_ms); - machine.set_leg(PeerConnection::outbound( - link_id, - expected_identity, - current_time_ms, - )); + machine.set_leg(HandshakeCrypto::new()); machine } fn inbound_leg(link_id: LinkId, current_time_ms: u64) -> PeerMachine { let mut machine = PeerMachine::new_inbound(link_id, current_time_ms); - machine.set_leg(PeerConnection::inbound(link_id, current_time_ms)); + machine.set_leg(HandshakeCrypto::new()); machine } @@ -3294,20 +3324,14 @@ mod tests { assert_eq!(discovered.pubkey(), initiator_identity.pubkey()); // Responder learned initiator's epoch - assert_eq!( - responder_conn.leg().unwrap().remote_epoch(), - Some(initiator_epoch) - ); + assert_eq!(responder_conn.conn_remote_epoch(), Some(initiator_epoch)); // Initiator completes handshake initiator_conn.complete_handshake(&msg2, 1300).unwrap(); assert!(initiator_conn.has_session()); // Initiator learned responder's epoch - assert_eq!( - initiator_conn.leg().unwrap().remote_epoch(), - Some(responder_epoch) - ); + assert_eq!(initiator_conn.conn_remote_epoch(), Some(responder_epoch)); // Both have sessions assert!(initiator_conn.has_session()); diff --git a/src/peer/mod.rs b/src/peer/mod.rs index 690dfd0..1443fe3 100644 --- a/src/peer/mod.rs +++ b/src/peer/mod.rs @@ -1,17 +1,16 @@ //! Peer Management //! //! Two-phase peer lifecycle: -//! 1. **PeerConnection** - Handshake phase, before identity is verified +//! 1. **PeerMachine** with a handshake carrier attached - handshake phase, +//! before identity is verified //! 2. **ActivePeer** - Authenticated phase, after successful Noise handshake mod active; #[cfg(any(target_os = "linux", target_os = "macos"))] pub(crate) mod connected_udp; -mod connection; pub(crate) mod machine; pub use active::{ActivePeer, ConnectivityState}; -pub use connection::PeerConnection; use crate::NodeAddr; use crate::transport::LinkId; diff --git a/src/proto/fmp/core.rs b/src/proto/fmp/core.rs index 4d46ce2..d134974 100644 --- a/src/proto/fmp/core.rs +++ b/src/proto/fmp/core.rs @@ -188,9 +188,9 @@ pub(crate) struct RekeyCfg { /// The result of the shell-side Noise wire step (Phase B) for one inbound /// handshake msg1, handed to the establish decision core. /// -/// The Noise step (`receive_handshake_init`) runs shell-side on the -/// `PeerConnection`: it reads **no** `Node` registry state — the load-bearing -/// invariant of this decomposition — and yields the learned peer identity, the +/// The Noise step (`receive_handshake_init`) runs on the control machine: it +/// reads **no** `Node` registry state — the essential invariant of this +/// decomposition — and yields the learned peer identity, the /// remote startup epoch, the sender's session index, and the opaque msg2 noise /// payload to frame and send. The core never parses or builds Noise bytes; the /// payload is an opaque blob. @@ -404,7 +404,7 @@ pub(crate) enum InboundReject { /// cross-connection and the tie-break decides whether we swap our session to the /// (winning) outbound one or keep our existing inbound session. The rekey-msg2 /// completion path is handled by a separate shell driver (it mutates -/// `ActivePeer`, not a `PeerConnection`) and never reaches this decision. +/// `ActivePeer`, not a pending handshake) and never reaches this decision. #[derive(Debug, PartialEq, Eq)] pub(crate) enum OutboundDecision { /// No existing peer for this identity: promote the completed outbound diff --git a/src/proto/fmp/mod.rs b/src/proto/fmp/mod.rs index 96d2e8a..8a97041 100644 --- a/src/proto/fmp/mod.rs +++ b/src/proto/fmp/mod.rs @@ -17,8 +17,9 @@ //! the [`cross_connection_winner`] tie-break helper. //! - `limits.rs` — the pure connection-retry backoff math. //! - `state.rs` — [`ConnectionState`], the pure handshake-phase connection -//! bookkeeping (owned by the shell `PeerConnection` beside its Noise crypto -//! handles), plus [`Fmp`], the (stateless) lifecycle anchor owned by `Node`. +//! bookkeeping (owned by the per-peer control machine beside its Noise +//! crypto carrier), plus [`Fmp`], the (stateless) lifecycle anchor owned by +//! `Node`. //! The handshake phase itself lives on the per-peer control machine. //! - `wire.rs` — the FMP link-framing codec: handshake message types, //! disconnect reasons, and the orderly disconnect message. Also carries the diff --git a/src/proto/fmp/state.rs b/src/proto/fmp/state.rs index a961516..5903d04 100644 --- a/src/proto/fmp/state.rs +++ b/src/proto/fmp/state.rs @@ -9,11 +9,11 @@ //! //! [`ConnectionState`] owns every **pure** field of the handshake-phase //! connection. The Noise crypto handles (`noise::HandshakeState`, -//! `NoiseSession`) stay shell-owned in -//! [`PeerConnection`](crate::peer::PeerConnection), which holds a -//! `ConnectionState` alongside them and drives the two halves side by side. The -//! shell's XX transition methods drive the Noise objects, then write learned -//! results back through the pure setters here (`set_expected_identity`, +//! `NoiseSession`) stay shell-owned in the per-peer control machine's +//! handshake carrier, which the machine drives alongside its own +//! `ConnectionState`. The machine's transition methods drive the Noise +//! objects, then write learned results back through the pure setters here +//! (`set_expected_identity`, //! `set_remote_epoch`, `touch`). //! //! This state is `no_std`+`alloc`-clean with respect to transport: the @@ -35,10 +35,9 @@ use crate::utils::index::SessionIndex; /// Pure, runtime-agnostic bookkeeping for a connection in the handshake phase. /// /// Owns every non-crypto field of the handshake-phase connection. The Noise -/// crypto handles live beside it in the shell -/// [`PeerConnection`](crate::peer::PeerConnection); this struct is written only -/// as plain data — the shell extracts learned identity/epoch out of the crypto -/// objects and sets them here through the setters. +/// crypto handles live beside it on the per-peer control machine; this struct +/// is written only as plain data — the machine extracts learned identity and +/// epoch out of the crypto objects and sets them here through the setters. #[derive(Debug)] pub struct ConnectionState { // === Link Reference === @@ -148,6 +147,7 @@ impl ConnectionState { /// Create the pure state for a new inbound connection with transport info. /// /// Used when processing msg1 where we know the transport and source address. + #[cfg(test)] pub fn inbound_with_transport( link_id: LinkId, transport_id: TransportId, diff --git a/src/transport/tcp/mod.rs b/src/transport/tcp/mod.rs index 23e95bd..5d000fb 100644 --- a/src/transport/tcp/mod.rs +++ b/src/transport/tcp/mod.rs @@ -124,7 +124,7 @@ impl TcpTransport { /// node-wide `node.limits.max_connections` > built-in default. This is a /// per-transport *raw-accept* ceiling; the true node-wide peer budget is /// still enforced downstream by the handshake-phase `max_connections` - /// admission check (`Node::add_connection`), so deriving this ceiling + /// admission check, so deriving this ceiling /// from `max_connections` does not let multiple transports exceed the /// node-wide total — it only stops the transport from rejecting inbound /// below the configured node budget.