From 6def31bcf6e241a416ee1738028921ca098404b7 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 29 Apr 2026 19:24:40 +0000 Subject: [PATCH] Admit rekey msg1 from established peers regardless of accept_connections The accept_connections gate at the top of handle_msg1 was applied unconditionally, so rekey msg1 from a peer with whom an established link already existed was dropped on the same path as fresh handshakes from strangers. Combined with the dual-init tie-breaker, this deadlocked at ~25 minutes when both sides' rekey timers fired near-simultaneously: the smaller-NodeAddr side wins as initiator and expects the larger side to consume its rekey msg1, but if the larger side has accept_connections=false the gate dropped it. Both sides retried at 1 Hz indefinitely; the affected peer fell out of MMP-active rotation. Extract the gate decision into Node::should_admit_msg1, which admits unconditionally when addr_to_link already has an entry for the (transport_id, remote_addr) pair (rekey/restart on an established session) and otherwise consults the transport's accept_connections(). Fresh msg1 from strangers is still rejected before any Noise crypto. Three unit tests pin the truth table: no transport (admit), accept_off no-link (reject, behavior unchanged), accept_off with-link (admit, the carve-out). The fix generalizes for free to BLE, which has the same Node-level gate. TCP and Tor were never subject to this deadlock because their accept condition is runtime state (bind_addr.is_some() / onion_address.is_some()), not a config flag. --- src/node/handlers/handshake.rs | 33 ++++++++++++++--- src/node/tests/handshake.rs | 66 ++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 76dc85c..5fe5221 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -10,6 +10,30 @@ use std::time::Duration; use tracing::{debug, info, warn}; impl Node { + /// Returns true if an inbound msg1 should be admitted past the + /// `accept_connections` gate. + /// + /// Rekey/restart msg1 on an existing link is always admitted (the gate + /// is meant to filter fresh handshakes from strangers, not maintenance + /// traffic on established sessions). Otherwise the transport's + /// `accept_connections` config decides; absence of a registered + /// transport admits (no gate to apply). + pub(in crate::node) fn should_admit_msg1( + &self, + transport_id: crate::transport::TransportId, + remote_addr: &crate::transport::TransportAddr, + ) -> bool { + if self + .addr_to_link + .contains_key(&(transport_id, remote_addr.clone())) + { + return true; + } + self.transports + .get(&transport_id) + .is_none_or(|t| t.accept_connections()) + } + /// Handle handshake message 1 (phase 0x1). /// /// This creates a new inbound connection. Rate limiting is applied @@ -25,10 +49,11 @@ impl Node { return; } - // Check if this transport accepts inbound connections - if let Some(transport) = self.transports.get(&packet.transport_id) - && !transport.accept_connections() - { + // accept_connections gate. Rekey/restart msg1 on an existing link + // is always admitted; the gate only filters truly-fresh connections + // from strangers. Without this carve-out, the dual-init tie-breaker + // deadlocks when the larger-NodeAddr side has accept_connections=false. + if !self.should_admit_msg1(packet.transport_id, &packet.remote_addr) { self.msg1_rate_limiter.complete_handshake(); return; } diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index c42949e..ab08930 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -951,3 +951,69 @@ async fn test_duplicate_msg2_dropped() { assert_eq!(node.connection_count(), 0); assert_eq!(node.peer_count(), 0); } + +/// `should_admit_msg1` admits when no transport is registered for the id. +/// (No gate to apply — the caller's other checks decide the outcome.) +#[test] +fn test_should_admit_msg1_no_transport() { + let node = make_node(); + let addr = TransportAddr::from_string("10.0.0.2:2121"); + assert!(node.should_admit_msg1(TransportId::new(1), &addr)); +} + +/// `should_admit_msg1` rejects a fresh msg1 (no addr_to_link entry) when +/// the transport has accept_connections=false. Behavior unchanged from +/// before the carve-out. +#[tokio::test] +async fn test_should_admit_msg1_rejects_fresh_when_accept_off() { + use crate::config::TcpConfig; + use crate::transport::tcp::TcpTransport; + + let mut node = make_node(); + let transport_id = TransportId::new(1); + + // bind_addr=None → accept_connections() == false + let cfg = TcpConfig { + bind_addr: None, + ..Default::default() + }; + let (tx, _rx) = packet_channel(64); + let tcp = TcpTransport::new(transport_id, None, cfg, tx); + node.transports + .insert(transport_id, TransportHandle::Tcp(tcp)); + + let addr = TransportAddr::from_string("10.0.0.2:2121"); + assert!(!node.should_admit_msg1(transport_id, &addr)); +} + +/// ISSUE-2026-0004 regression test: `should_admit_msg1` admits rekey/restart +/// msg1 from a peer with an existing link even when the transport has +/// accept_connections=false. Without this, the dual-init tie-breaker +/// deadlocks (the larger-NodeAddr side drops the winner's rekey msg1). +#[tokio::test] +async fn test_should_admit_msg1_admits_rekey_when_accept_off() { + use crate::config::TcpConfig; + use crate::transport::tcp::TcpTransport; + + let mut node = make_node(); + let transport_id = TransportId::new(1); + + let cfg = TcpConfig { + bind_addr: None, + ..Default::default() + }; + let (tx, _rx) = packet_channel(64); + let tcp = TcpTransport::new(transport_id, None, cfg, tx); + node.transports + .insert(transport_id, TransportHandle::Tcp(tcp)); + + let addr = TransportAddr::from_string("10.0.0.2:2121"); + + // Pre-populate addr_to_link as if a session were established for this + // peer on this transport (rekey msg1 will arrive against this entry). + let link_id = node.allocate_link_id(); + node.addr_to_link + .insert((transport_id, addr.clone()), link_id); + + assert!(node.should_admit_msg1(transport_id, &addr)); +}