From 1eabe085756979cba723e386292878b28c71d40d Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 25 Jul 2026 01:50:40 +0000 Subject: [PATCH] Correct what the address-to-link map claims to be for The doc said the map "enables dispatching incoming packets to the right connection before authentication completes." It does not: find_link_by_addr has no callers outside its own tests on any branch, and encrypted frames dispatch by session index. Its live readers are the msg1 admission fast path and the duplicate-inbound-handshake check. The comment mattered because the map has a trap that invites exactly the misuse it was advertising. An outbound dial registers the literal configured address string, which may be a hostname, while an inbound packet carries the resolved form; TransportAddr compares byte-wise, so the two never match and a lookup keyed on an inbound address returns "no such link" for every hostname-configured peer rather than failing. The entry is also single-valued per key, so an inbound handshake overwrites an outbound dial's entry for the same address. Both current readers survive this because each compares a key written in the same form it reads. That is a property of those two call sites and not of the map, so the comment now says so, and says not to key a peer-identity question on it. Documentation only; no behaviour changes. --- src/node/mod.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/node/mod.rs b/src/node/mod.rs index a5cac40..c3b2a45 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -318,8 +318,23 @@ struct PendingConnect { /// 1. **Connection phase** (`connections`): Handshake in progress, indexed by LinkId /// 2. **Active phase** (`peers`): Authenticated, indexed by NodeAddr /// -/// The `addr_to_link` map enables dispatching incoming packets to the right -/// connection before authentication completes. +/// The `addr_to_link` map is a reverse lookup from `(transport, address)` to +/// link. It is **not** a packet-dispatch path, despite what this comment used +/// to say: `find_link_by_addr` has no callers outside its own tests, and +/// encrypted frames are dispatched by session index. Its live readers are the +/// `should_admit_msg1` fast path and the duplicate-inbound-handshake check in +/// `handle_msg1`. +/// +/// **Do not key a peer-identity question on it.** The address form is not +/// canonical: an outbound dial registers the literal configured string, which +/// may be a hostname (`"node-b:2121"`), while an inbound packet carries the +/// resolved form (`"10.128.2.4:2121"`). `TransportAddr` compares byte-wise, so +/// those never match, and a lookup keyed on an inbound address silently returns +/// "no such link" for every hostname-configured peer rather than failing. The +/// entry is also single-valued per key, so an inbound handshake overwrites an +/// outbound dial's entry for the same address. The readers above tolerate this +/// because each compares a key written in the same form it reads; a new reader +/// that does not will be quietly wrong. // Discovery lookup constants moved to config: node.discovery.attempt_timeouts_secs, node.discovery.ttl pub struct Node { // === Immutable Context ===