From 15350d1b78754ad9e5592eaa515b259af076fca5 Mon Sep 17 00:00:00 2001 From: ackintosh Date: Sun, 26 Apr 2026 15:10:13 +0900 Subject: [PATCH 1/5] test: add failing tests for ipv4/ipv6_contactable metrics not being updated Reproduces a bug in Config::build() where the inverted condition `if self.config.enr_update` always sets auto_nat_listen_duration to None (since enr_update defaults to true), preventing the auto-NAT timer from ever starting and leaving ipv4_contactable/ipv6_contactable permanently false. --- src/service/test.rs | 120 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/src/service/test.rs b/src/service/test.rs index 0a36de32..e9823275 100644 --- a/src/service/test.rs +++ b/src/service/test.rs @@ -7,6 +7,7 @@ use crate::{ handler::Handler, kbucket, kbucket::{BucketInsertResult, KBucketsTable, NodeStatus}, + metrics::METRICS, node_info::NodeContact, query_pool::{QueryId, QueryPool}, rpc::RequestId, @@ -14,6 +15,7 @@ use crate::{ socket::ListenConfig, ConfigBuilder, Enr, }; +use std::sync::atomic::Ordering; use enr::CombinedKey; use parking_lot::RwLock; use rand; @@ -510,3 +512,121 @@ async fn test_ipv6_update_amongst_ipv4_dominated_network() { // Should be 10 ipv6 pings assert_eq!(v6_pings, 10) } + +// Tests that METRICS.ipv4_contactable is set to true once enough incoming connections are received +// during an active auto-NAT listen window. +#[tokio::test] +async fn test_connectivity_metrics_updated_on_incoming_connections() { + init(); + + // Ensure a clean state for the global METRICS. + METRICS.ipv4_contactable.store(false, Ordering::Relaxed); + + let enr_key = CombinedKey::generate_secp256k1(); + let ip = Ipv4Addr::LOCALHOST; + let enr = Enr::builder() + .ip4(ip) + .udp4(DEFAULT_UDP_PORT) + .build(&enr_key) + .unwrap(); + + let mut service = build_service( + Arc::new(RwLock::new(enr)), + Arc::new(RwLock::new(enr_key)), + false, + ) + .await; + + let ipv4_socket: SocketAddr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 9000); + + // Simulate an ENR socket update to start the auto-NAT timer. Without this, + // received_incoming_connection() returns early because the wait timer is None. + service.connectivity_state.enr_socket_update(&ipv4_socket); + + // Inject incoming connections. Two are required to reach + // NUMBER_OF_INCOMING_CONNECTIONS_REQUIRED_TO_BE_VALID. + let peer_key = CombinedKey::generate_secp256k1(); + let peer_enr = Enr::builder() + .ip4(ip) + .udp4(DEFAULT_UDP_PORT) + .build(&peer_key) + .unwrap(); + + service.inject_session_established( + peer_enr.clone(), + &ipv4_socket, + ConnectionDirection::Incoming, + ); + service.inject_session_established( + peer_enr.clone(), + &ipv4_socket, + ConnectionDirection::Incoming, + ); + + // ipv4_contactable should now be true. + assert!( + METRICS.ipv4_contactable.load(Ordering::Relaxed), + "ipv4_contactable should be true after receiving enough incoming connections" + ); + + // Clean up to avoid affecting other tests. + METRICS.ipv4_contactable.store(false, Ordering::Relaxed); +} + +// Tests that METRICS.ipv6_contactable is set to true once enough incoming connections are received +// during an active auto-NAT listen window. +#[tokio::test] +async fn test_connectivity_metrics_updated_on_incoming_connections_ipv6() { + init(); + + // Ensure a clean state for the global METRICS. + METRICS.ipv6_contactable.store(false, Ordering::Relaxed); + + let enr_key = CombinedKey::generate_secp256k1(); + let enr = Enr::builder() + .ip4(Ipv4Addr::LOCALHOST) + .udp4(DEFAULT_UDP_PORT) + .build(&enr_key) + .unwrap(); + + let mut service = build_service( + Arc::new(RwLock::new(enr)), + Arc::new(RwLock::new(enr_key)), + false, + ) + .await; + + let ipv6_socket: SocketAddr = SocketAddr::new(Ipv6Addr::LOCALHOST.into(), 9000); + + // Simulate an ENR socket update to start the auto-NAT timer for IPv6. + service.connectivity_state.enr_socket_update(&ipv6_socket); + + // Inject incoming connections. Two are required to reach + // NUMBER_OF_INCOMING_CONNECTIONS_REQUIRED_TO_BE_VALID. + let peer_key = CombinedKey::generate_secp256k1(); + let peer_enr = Enr::builder() + .ip6(Ipv6Addr::LOCALHOST) + .udp6(DEFAULT_UDP_PORT) + .build(&peer_key) + .unwrap(); + + service.inject_session_established( + peer_enr.clone(), + &ipv6_socket, + ConnectionDirection::Incoming, + ); + service.inject_session_established( + peer_enr.clone(), + &ipv6_socket, + ConnectionDirection::Incoming, + ); + + // ipv6_contactable should now be true. + assert!( + METRICS.ipv6_contactable.load(Ordering::Relaxed), + "ipv6_contactable should be true after receiving enough incoming connections" + ); + + // Clean up to avoid affecting other tests. + METRICS.ipv6_contactable.store(false, Ordering::Relaxed); +} From e4efae592d9504e94b83ce1b7595e6fb68291d84 Mon Sep 17 00:00:00 2001 From: ackintosh Date: Sun, 26 Apr 2026 15:50:23 +0900 Subject: [PATCH 2/5] test: extract build_service_with helper and add build_service_ipv6 --- src/service/test.rs | 43 +++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/service/test.rs b/src/service/test.rs index e9823275..f45fe688 100644 --- a/src/service/test.rs +++ b/src/service/test.rs @@ -54,15 +54,13 @@ fn init() { .try_init(); } -async fn build_service( +async fn build_service_with( local_enr: Arc>, enr_key: Arc>, filters: bool, + listen_config: ListenConfig, + ip_mode: IpMode, ) -> Service { - let listen_config = ListenConfig::Ipv4 { - ip: local_enr.read().ip4().unwrap(), - port: local_enr.read().udp4().unwrap(), - }; let config = ConfigBuilder::new(listen_config) .executor(Box::::default()) .build(); @@ -111,11 +109,35 @@ async fn build_service( event_stream: None, exit, config, - ip_mode: Default::default(), + ip_mode, connectivity_state, } } +async fn build_service( + local_enr: Arc>, + enr_key: Arc>, + filters: bool, +) -> Service { + let listen_config = ListenConfig::Ipv4 { + ip: local_enr.read().ip4().unwrap(), + port: local_enr.read().udp4().unwrap(), + }; + build_service_with(local_enr, enr_key, filters, listen_config, IpMode::Ip4).await +} + +async fn build_service_ipv6( + local_enr: Arc>, + enr_key: Arc>, + filters: bool, +) -> Service { + let listen_config = ListenConfig::Ipv6 { + ip: local_enr.read().ip6().unwrap(), + port: local_enr.read().udp6().unwrap(), + }; + build_service_with(local_enr, enr_key, filters, listen_config, IpMode::Ip6).await +} + fn build_non_handler_service( local_enr: Arc>, enr_key: Arc>, @@ -583,13 +605,14 @@ async fn test_connectivity_metrics_updated_on_incoming_connections_ipv6() { METRICS.ipv6_contactable.store(false, Ordering::Relaxed); let enr_key = CombinedKey::generate_secp256k1(); + let ip6 = Ipv6Addr::LOCALHOST; let enr = Enr::builder() - .ip4(Ipv4Addr::LOCALHOST) - .udp4(DEFAULT_UDP_PORT) + .ip6(ip6) + .udp6(DEFAULT_UDP_PORT) .build(&enr_key) .unwrap(); - let mut service = build_service( + let mut service = build_service_ipv6( Arc::new(RwLock::new(enr)), Arc::new(RwLock::new(enr_key)), false, @@ -605,7 +628,7 @@ async fn test_connectivity_metrics_updated_on_incoming_connections_ipv6() { // NUMBER_OF_INCOMING_CONNECTIONS_REQUIRED_TO_BE_VALID. let peer_key = CombinedKey::generate_secp256k1(); let peer_enr = Enr::builder() - .ip6(Ipv6Addr::LOCALHOST) + .ip6(ip6) .udp6(DEFAULT_UDP_PORT) .build(&peer_key) .unwrap(); From cb3a6f6c32b84d92e45c8bc4ee9f58554697c583 Mon Sep 17 00:00:00 2001 From: ackintosh Date: Mon, 27 Apr 2026 06:39:57 +0900 Subject: [PATCH 3/5] cargo fmt --- src/service/test.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/service/test.rs b/src/service/test.rs index f45fe688..f87551b8 100644 --- a/src/service/test.rs +++ b/src/service/test.rs @@ -15,14 +15,13 @@ use crate::{ socket::ListenConfig, ConfigBuilder, Enr, }; -use std::sync::atomic::Ordering; use enr::CombinedKey; use parking_lot::RwLock; use rand; use std::{ collections::HashMap, net::{Ipv4Addr, Ipv6Addr}, - sync::Arc, + sync::{atomic::Ordering, Arc}, time::Duration, }; use tokio::sync::{ From 1f70cc957487f350dcc864a1e8b4a2bc01950843 Mon Sep 17 00:00:00 2001 From: ackintosh Date: Mon, 27 Apr 2026 07:13:46 +0900 Subject: [PATCH 4/5] cargo clippy --- src/query_pool/peers/closest.rs | 4 ++-- src/query_pool/peers/predicate.rs | 4 ++-- src/service.rs | 14 +++++++++----- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/query_pool/peers/closest.rs b/src/query_pool/peers/closest.rs index 334d8a39..58373f2d 100644 --- a/src/query_pool/peers/closest.rs +++ b/src/query_pool/peers/closest.rs @@ -324,8 +324,8 @@ where /// Consumes the query, returning the target and the closest peers. pub fn into_result(self) -> Vec { self.closest_peers - .into_iter() - .filter_map(|(_, peer)| { + .into_values() + .filter_map(|peer| { if let QueryPeerState::Succeeded = peer.state { Some(peer.key.into_preimage()) } else { diff --git a/src/query_pool/peers/predicate.rs b/src/query_pool/peers/predicate.rs index 3b444201..c1f2aa12 100644 --- a/src/query_pool/peers/predicate.rs +++ b/src/query_pool/peers/predicate.rs @@ -314,8 +314,8 @@ where /// Consumes the query, returning the peers who match the predicate. pub fn into_result(self) -> Vec { self.closest_peers - .into_iter() - .filter_map(|(_, peer)| { + .into_values() + .filter_map(|peer| { if let QueryPeerState::Succeeded = peer.state { if peer.predicate_match { Some(peer.key.into_preimage()) diff --git a/src/service.rs b/src/service.rs index 10b26d31..61b60334 100644 --- a/src/service.rs +++ b/src/service.rs @@ -616,12 +616,13 @@ impl Service { // check if we need to update the known ENR let mut to_request_enr = None; match self.kbuckets.write().entry(&node_address.node_id.into()) { - kbucket::Entry::Present(ref mut entry, _) => { - if entry.value().seq() < enr_seq { - let enr = entry.value().clone(); - to_request_enr = Some(enr); - } + kbucket::Entry::Present(ref entry, _) if entry.value().seq() < enr_seq => { + let enr = entry.value().clone(); + to_request_enr = Some(enr); } + // PendingEntry::value() requires &mut self, so the condition cannot be placed + // in the pattern guard (E0596). The inner `if` is intentional. + #[allow(clippy::collapsible_match)] kbucket::Entry::Pending(ref mut entry, _) => { if entry.value().seq() < enr_seq { let enr = entry.value().clone(); @@ -1297,6 +1298,9 @@ impl Service { kbucket::Entry::Present(entry, _) if entry.value().seq() < enr.seq() => { entry.remove() } + // PendingEntry::value() requires &mut self, so the condition cannot be placed + // in the pattern guard (E0596). The inner `if` is intentional. + #[allow(clippy::collapsible_match)] kbucket::Entry::Pending(mut entry, _) => { if entry.value().seq() < enr.seq() { entry.remove() From b9d2c002dc5bf7778adea5d51bbca91f0c784636 Mon Sep 17 00:00:00 2001 From: ackintosh Date: Mon, 27 Apr 2026 07:30:29 +0900 Subject: [PATCH 5/5] Fix inverted condition --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index 493ece23..90c07ca5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -352,7 +352,7 @@ impl ConfigBuilder { // If enr-update is set to false, then it is non-intuitive for discv5 to revoke ENR details // when determining NAT status. So we will not do this. - if self.config.enr_update { + if !self.config.enr_update { self.config.auto_nat_listen_duration = None; }