From 1b770a347704a3c97fbd3be47e067e1d9a66553a Mon Sep 17 00:00:00 2001 From: Age Manning Date: Tue, 26 Mar 2024 16:05:36 +1100 Subject: [PATCH 1/3] Improve ENR updates --- .../lighthouse_network/src/discovery/mod.rs | 60 +++++++++++++++---- .../lighthouse_network/src/service/mod.rs | 14 +++-- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index 347c2352f18..5c4ea8a2e4e 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -398,16 +398,33 @@ impl Discovery { /// automatically update the external address. /// /// If the external address needs to be modified, use `update_enr_udp_socket. - pub fn update_enr_tcp_port(&mut self, port: u16) -> Result<(), String> { + /// + /// This returns Ok(true) if the ENR was updated, otherwise Ok(false) if nothing was done. + pub fn update_enr_tcp_port(&mut self, port: u16, v6: bool) -> Result { + + let enr_field = if v6 { + if self.discv5.external_enr().read().tcp6() == Some(port) { + // The field is already set to the same value, nothing to do + return Ok(false); + } + "tcp6" + } else { + if self.discv5.external_enr().read().tcp4() == Some(port) { + // The field is already set to the same value, nothing to do + return Ok(false); + } + "tcp" + }; + self.discv5 - .enr_insert("tcp", &port) + .enr_insert(enr_field, &port) .map_err(|e| format!("{:?}", e))?; // replace the global version *self.network_globals.local_enr.write() = self.discv5.local_enr(); // persist modified enr to disk enr::save_enr_to_disk(Path::new(&self.enr_dir), &self.local_enr(), &self.log); - Ok(()) + Ok(true) } // TODO: Group these functions here once the ENR is shared across discv5 and lighthouse and @@ -415,16 +432,36 @@ impl Discovery { // This currently doesn't support ipv6. All of these functions should be removed and // addressed properly in the following issue. // https://github.com/sigp/lighthouse/issues/4706 - pub fn update_enr_quic_port(&mut self, port: u16) -> Result<(), String> { + pub fn update_enr_quic_port(&mut self, port: u16, v6: bool) -> Result { + + let enr_field = if v6 { + if self.discv5.external_enr().read().quic6() == Some(port) { + // The field is already set to the same value, nothing to do + return Ok(false); + } + "quic6" + } else { + if self.discv5.external_enr().read().quic4() == Some(port) { + // The field is already set to the same value, nothing to do + return Ok(false); + } + "quic" + }; + let current_field = self.discv5.external_enr().read().quic4(); + if current_field == Some(port) { + // The current field is already set, no need to update. + return Ok(false); + } + self.discv5 - .enr_insert("quic", &port) + .enr_insert(enr_field, &port) .map_err(|e| format!("{:?}", e))?; // replace the global version *self.network_globals.local_enr.write() = self.discv5.local_enr(); // persist modified enr to disk enr::save_enr_to_disk(Path::new(&self.enr_dir), &self.local_enr(), &self.log); - Ok(()) + Ok(true) } /// Updates the local ENR UDP socket. @@ -1057,7 +1094,7 @@ impl NetworkBehaviour for Discovery { return; } - self.update_enr_tcp_port(port) + self.update_enr_tcp_port(port, false) } (Some(Protocol::Udp(port)), Some(Protocol::QuicV1)) => { if !self.update_ports.quic4 { @@ -1065,7 +1102,7 @@ impl NetworkBehaviour for Discovery { return; } - self.update_enr_quic_port(port) + self.update_enr_quic_port(port, false) } _ => { debug!(self.log, "Encountered unacceptable multiaddr for listening (unsupported transport)"; "addr" => ?addr); @@ -1079,7 +1116,7 @@ impl NetworkBehaviour for Discovery { return; } - self.update_enr_tcp_port(port) + self.update_enr_tcp_port(port, true) } (Some(Protocol::Udp(port)), Some(Protocol::QuicV1)) => { if !self.update_ports.quic6 { @@ -1087,7 +1124,7 @@ impl NetworkBehaviour for Discovery { return; } - self.update_enr_quic_port(port) + self.update_enr_quic_port(port, true) } _ => { debug!(self.log, "Encountered unacceptable multiaddr for listening (unsupported transport)"; "addr" => ?addr); @@ -1103,9 +1140,10 @@ impl NetworkBehaviour for Discovery { let local_enr: Enr = self.discv5.local_enr(); match attempt_enr_update { - Ok(_) => { + Ok(true) => { info!(self.log, "Updated local ENR"; "enr" => local_enr.to_base64(), "seq" => local_enr.seq(), "id"=> %local_enr.node_id(), "ip4" => ?local_enr.ip4(), "udp4"=> ?local_enr.udp4(), "tcp4" => ?local_enr.tcp4(), "tcp6" => ?local_enr.tcp6(), "udp6" => ?local_enr.udp6()) } + Ok(false) => {} // Nothing to do, ENR already configured Err(e) => warn!(self.log, "Failed to update ENR"; "error" => ?e), } } diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 899f91d115c..3f9753b11f0 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -1651,12 +1651,18 @@ impl Network { libp2p::upnp::Event::NewExternalAddr(addr) => { info!(self.log, "UPnP route established"; "addr" => %addr); let mut iter = addr.iter(); - // Skip Ip address. - iter.next(); + let is_ip6 = { + let addr = iter.next(); + if let Some(MProtocol::Ip6(_)) = addr { + true + } else { + false + } + }; match iter.next() { Some(multiaddr::Protocol::Udp(udp_port)) => match iter.next() { Some(multiaddr::Protocol::QuicV1) => { - if let Err(e) = self.discovery_mut().update_enr_quic_port(udp_port) { + if let Err(e) = self.discovery_mut().update_enr_quic_port(udp_port, is_ip6) { warn!(self.log, "Failed to update ENR"; "error" => e); } } @@ -1665,7 +1671,7 @@ impl Network { } }, Some(multiaddr::Protocol::Tcp(tcp_port)) => { - if let Err(e) = self.discovery_mut().update_enr_tcp_port(tcp_port) { + if let Err(e) = self.discovery_mut().update_enr_tcp_port(tcp_port, is_ip6) { warn!(self.log, "Failed to update ENR"; "error" => e); } } From 364bb6e5df6f7dacd590bb84fadb4aa1b37e2f91 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 28 Mar 2024 12:19:53 +1100 Subject: [PATCH 2/3] forever fmt --- beacon_node/lighthouse_network/src/discovery/mod.rs | 6 ++---- beacon_node/lighthouse_network/src/service/mod.rs | 6 ++++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index 5c4ea8a2e4e..5b37fef0bbc 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -401,14 +401,13 @@ impl Discovery { /// /// This returns Ok(true) if the ENR was updated, otherwise Ok(false) if nothing was done. pub fn update_enr_tcp_port(&mut self, port: u16, v6: bool) -> Result { - let enr_field = if v6 { if self.discv5.external_enr().read().tcp6() == Some(port) { // The field is already set to the same value, nothing to do return Ok(false); } "tcp6" - } else { + } else { if self.discv5.external_enr().read().tcp4() == Some(port) { // The field is already set to the same value, nothing to do return Ok(false); @@ -433,14 +432,13 @@ impl Discovery { // addressed properly in the following issue. // https://github.com/sigp/lighthouse/issues/4706 pub fn update_enr_quic_port(&mut self, port: u16, v6: bool) -> Result { - let enr_field = if v6 { if self.discv5.external_enr().read().quic6() == Some(port) { // The field is already set to the same value, nothing to do return Ok(false); } "quic6" - } else { + } else { if self.discv5.external_enr().read().quic4() == Some(port) { // The field is already set to the same value, nothing to do return Ok(false); diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 3f9753b11f0..2734f301955 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -1653,7 +1653,7 @@ impl Network { let mut iter = addr.iter(); let is_ip6 = { let addr = iter.next(); - if let Some(MProtocol::Ip6(_)) = addr { + if let Some(MProtocol::Ip6(_)) = addr { true } else { false @@ -1662,7 +1662,9 @@ impl Network { match iter.next() { Some(multiaddr::Protocol::Udp(udp_port)) => match iter.next() { Some(multiaddr::Protocol::QuicV1) => { - if let Err(e) = self.discovery_mut().update_enr_quic_port(udp_port, is_ip6) { + if let Err(e) = + self.discovery_mut().update_enr_quic_port(udp_port, is_ip6) + { warn!(self.log, "Failed to update ENR"; "error" => e); } } From 2dee89d520303cd0bae6f3dfab98c754a9c4a73d Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 28 Mar 2024 16:59:14 +1100 Subject: [PATCH 3/3] Appease my old friend clippy --- beacon_node/lighthouse_network/src/service/mod.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 2734f301955..6c86d943019 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -1653,11 +1653,7 @@ impl Network { let mut iter = addr.iter(); let is_ip6 = { let addr = iter.next(); - if let Some(MProtocol::Ip6(_)) = addr { - true - } else { - false - } + matches!(addr, Some(MProtocol::Ip6(_))) }; match iter.next() { Some(multiaddr::Protocol::Udp(udp_port)) => match iter.next() {