feat(networking): update ports in ENR #457

petarjuki7 · 2025-07-28T00:43:22Z

Issue Addressed

Addresses issue #255

Proposed Changes

Much of the logic insipred by Lighthouse.
Listening on discv5's SocketUpdated event and updating the local ENR socket.
Listening on libp2p's NewListenAddr and updating the ENR accordingly.

Additional Info

Needed to finish #444

dknopik

Thanks for the PR!

anchor/network/src/discovery.rs

anchor/network/src/network.rs

anchor/network/src/discovery.rs

mergify · 2025-07-28T10:50:47Z

Some required checks have failed. Could you please take a look @petarjuki7? 🙏

petarjuki7 · 2025-07-28T11:28:07Z

anchor/network/src/discovery.rs

    ) {
    }

+    #[allow(clippy::single_match)]


I'd rather keep it a single match, so it shows there are more variants we are ignoring for now.
I can change it to a if let Some(event)... if needed, no problem.

diegomrsantos · 2025-07-28T12:36:30Z

anchor/network/src/discovery.rs

        }
    }

+    /// Updates the local ENR TCP port.


/// Internal helper that updates a single ENR port field (IPv4 or IPv6) if it differs. /// /// - `desired_port` is the port we want to have in the ENR. /// - `is_ipv6` selects the v6 accessor/key when `true`, otherwise v4. /// - `key_v4` / `key_v6` are the ENR key names to write (e.g. `"tcp"` / `"tcp6"`). /// - `current_v4` / `current_v6` are accessors that read the current port /// from the external ENR (`tcp4()`, `tcp6()`, `quic4()`, `quic6()`, etc.). /// /// Returns: /// - `Ok(true)` — field updated and ENR persisted to disk. /// - `Ok(false)` — no change required (already set to `desired_port`). /// - `Err(_)` — failed to write into the ENR. fn update_enr_port<Fv4, Fv6>( &mut self, desired_port: u16, is_ipv6: bool, key_v4: &'static str, key_v6: &'static str, current_v4: Fv4, current_v6: Fv6, ) -> Result<bool, String> where Fv4: Fn(&Enr<CombinedKey>) -> Option<u16>, Fv6: Fn(&Enr<CombinedKey>) -> Option<u16>, { // Check if the value is already set. { let external_enr = self.discv5.external_enr().read(); let already_set = if is_ipv6 { current_v6(&external_enr) == Some(desired_port) } else { current_v4(&external_enr) == Some(desired_port) }; if already_set { return Ok(false); } } // Update the appropriate ENR key. let enr_key = if is_ipv6 { key_v6 } else { key_v4 }; self.discv5 .enr_insert(enr_key, &desired_port) .map_err(|e| format!("{e:?}"))?; // Persist modified ENR to disk. save_enr_to_disk(Path::new(&self.enr_dir), &self.discv5.local_enr()); Ok(true) } /// Update the ENR **TCP** port (IPv4 or IPv6). /// /// This only updates the port field in the ENR and **does not** modify the address. /// Discovery is expected to update the external address automatically. /// If you need to change the external address, use `update_enr_udp_socket`. /// /// Returns `Ok(true)` if the ENR was changed and persisted, `Ok(false)` if the /// existing value already matches `port`. pub fn update_enr_tcp_port(&mut self, port: u16, is_ipv6: bool) -> Result<bool, String> { self.update_enr_port(port, is_ipv6, "tcp", "tcp6", |e| e.tcp4(), |e| e.tcp6()) } /// Update the ENR **QUIC** port (IPv4 or IPv6). /// /// This only updates the port field in the ENR and **does not** modify the address. /// Discovery is expected to update the external address automatically. /// If you need to change the external address, use `update_enr_udp_socket`. /// /// Returns `Ok(true)` if the ENR was changed and persisted, `Ok(false)` if the /// existing value already matches `port`. pub fn update_enr_quic_port(&mut self, port: u16, is_ipv6: bool) -> Result<bool, String> { self.update_enr_port(port, is_ipv6, "quic", "quic6", |e| e.quic4(), |e| e.quic6()) } }

wdyt about keeping the if is_ipv6 in update_enr_tcp_port and update_enr_quic_port? Then we only have the parameters desired_port, key and current to the main function, which seems easier to understand

Then we would keep some duplication. IMO, the current code isn't that hard to understand, but I don't have a strong opinion.

I combined the two aproaches, the closures were elegant but maybe a bit hard to reason about at first glance IMO, can change it either way

diegomrsantos · 2025-07-28T13:00:00Z

anchor/network/src/discovery.rs

+                            if (self.update_ports.tcp4 && socket_addr.is_ipv4())
+                                || (self.update_ports.tcp6 && socket_addr.is_ipv6())
+                            {
+                                self.discv5.update_local_enr_socket(socket_addr, true);


Why do we have to call update_local_enr_socket if SocketUpdated is described as /// Our local ENR IP address has been updated.

If I'm reading the discv5 code correctly, the SocketUpdated event is emitted when only the UDP port is updated, with this we update our TCP port.

https://github.com/sigp/discv5/blob/ac91ad48e398665ef251746adcc68c5ed490854c/src/service.rs#L917-L933

I see, thanks. The event and comment in discv5 should be changed to make it clear.

True, I agree

I believe that after this event, the IP address and the UDP port have been updated. self.discv5.update_local_enr_socket(socket_addr, true) updates the IP address and the TCP port, but it doesn't update the QUIC one. Would it be sufficient to update only the TCP and QUIC ports instead?

Afaik the discv5 doesn't "speak" QUIC, that's why we dont update it. We update it when we get the NewListenAddr event from libp2p because we can directly check if the QUIC port changed. We can maybe assume and update it here also, but this SocketUpdated event doesn't tell us about QUIC

discv5 doesn't know anything about the TCP port either

True... This is the way it was done in Lighthouse so I took it as inspiration, should I also do update_enr_quic_port() here?

Thinking more about it, we can't set the discv5 UDP and QUIC port to the same value. I believe LH sets the TCP port here as a way to improve connectivity for nodes behind a NAT. But does nothing for QUIC. @AgeManning could you please clarify?

anchor/network/src/discovery.rs

diegomrsantos · 2025-07-29T17:55:39Z

@petarjuki7

What do you think about using only one function to update the port?

    /// Try to update an ENR port based on port type and configuration.
    ///
    /// This method centralizes all port update logic in one place:
    /// 1. Checks if updates are allowed for this port type
    /// 2. Gets current port value from ENR
    /// 3. Updates the port if needed
    /// 4. Persists changes to disk
    ///
    /// Parameters:
    /// - `is_tcp`: Whether this is a TCP port (true) or QUIC port (false)
    /// - `is_ipv6`: Whether this is an IPv6 port (true) or IPv4 port (false)
    /// - `port`: The new port value to set
    ///
    /// Returns:
    /// - `Ok(true)`: Port was updated and persisted to disk
    /// - `Ok(false)`: No update was needed (config disallows it or port already matches)
    /// - `Err(String)`: Update failed with the given error message
    pub fn try_update_port(&mut self, is_tcp: bool, is_ipv6: bool, new_port: u16) -> Result<bool, String> {
        
        let (read_fn, key): (fn(&_) -> Option<u16>, &str) = match (is_tcp, is_ipv6) {
            (true, false) if self.update_ports.tcp4 => (Enr::tcp4, "tcp"),
            (true, true) if self.update_ports.tcp6 => (Enr::tcp6, "tcp6"),
            (false, false) if self.update_ports.quic4 => (Enr::quic4, "quic4"),
            (false, true) if self.update_ports.quic6 => (Enr::quic6, "quic6"),
            _ => return Ok(false)
        };
        let port_opt = read_fn(&self.discv5.external_enr().read());

        if port_opt == Some(new_port) {
            return Ok(false);
        }
        
        self.discv5
            .enr_insert(key, &new_port)
            .map_err(|e| format!("{e:?}"))?;

        save_enr_to_disk(Path::new(&self.enr_dir), &self.discv5.local_enr());
        
        Ok(true)
    }

diegomrsantos · 2025-08-04T11:33:34Z

Hi @petarjuki7 , are you still interested in completing this, or can I go ahead and continue?

petarjuki7 · 2025-08-04T12:23:22Z

Hi @petarjuki7 , are you still interested in completing this, or can I go ahead and continue?

Hi, if it's fine you can continue. Thanks @diegomrsantos

Copilot

Pull Request Overview

Updates the networking layer to dynamically update ENR (Ethereum Node Record) ports based on actual listening addresses from both libp2p and discv5 events, addressing issue #255.

Listen to libp2p's NewListenAddr events to update ENR with TCP and QUIC ports
Listen to discv5's SocketUpdated events to update ENR with UDP discovery ports
Implement centralized port update logic with proper validation and disk persistence

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
anchor/network/src/network.rs	Adds handling of `NewListenAddr` events and implements port update logic for TCP/QUIC protocols
anchor/network/src/discovery.rs	Implements `try_update_port` method and handles discv5 `SocketUpdated` events for UDP port updates

anchor/network/src/discovery.rs

# Conflicts: # anchor/network/src/discovery.rs # anchor/network/src/network.rs

dknopik

I found a bug while improving the code and decide to just push it instead of describing it:

anchor/network/src/discovery.rs

dknopik · 2025-08-06T10:49:52Z

anchor/network/src/discovery.rs

+        if let EventStream::Awaiting(future) = self
+            && let Poll::Ready(Ok(receiver)) = future.as_mut().poll(cx)
+        {
+            *self = EventStream::Present(receiver);
+        }


I added this as we else never actually have a present EventStream

Could you elaborate more?

When we construct Discovery, we put an EventStream::Awaiting into event_stream. We need to convert it into an EventStream::Present somewhere.

diegomrsantos

Thanks @petarjuki7 for starting this.

Addresses issue sigp#255 Much of the logic insipred by Lighthouse. Listening on discv5's `SocketUpdated` event and updating the local ENR socket. Listening on libp2p's `NewListenAddr` and updating the ENR accordingly.

feat(networking): update ports in ENR

119170d

dknopik requested changes Jul 28, 2025

View reviewed changes

dknopik added the waiting-on-author label Jul 28, 2025

dknopik requested a review from diegomrsantos July 28, 2025 09:40

small refactoring

1dfea62

petarjuki7 requested a review from dknopik July 28, 2025 10:29

dknopik added ready-for-review This PR is ready to be reviewed network v0.3.0 Third and final testnet-only release and removed waiting-on-author labels Jul 28, 2025

mergify bot added waiting-on-author and removed ready-for-review This PR is ready to be reviewed labels Jul 28, 2025

petarjuki7 added 2 commits July 28, 2025 13:20

clippy fix

03705bd

remove redundant code

fd31679

petarjuki7 commented Jul 28, 2025

View reviewed changes

diegomrsantos reviewed Jul 28, 2025

View reviewed changes

anchor/network/src/discovery.rs Outdated Show resolved Hide resolved

refactor port update

b9d5e77

simplify code

bb1ec16

diegomrsantos requested a review from Copilot August 5, 2025 12:54

Copilot AI reviewed Aug 5, 2025

View reviewed changes

anchor/network/src/discovery.rs Outdated Show resolved Hide resolved

anchor/network/src/discovery.rs Outdated Show resolved Hide resolved

anchor/network/src/discovery.rs Outdated Show resolved Hide resolved

diegomrsantos added 4 commits August 5, 2025 15:12

save only once to disk

b2517b2

save only once to disk

ee3cf87

Merge remote-tracking branch 'upstream/unstable' into update_ports_enr

6e2a3f0

# Conflicts: # anchor/network/src/discovery.rs # anchor/network/src/network.rs

fix borrow issue

99d2b9e

dknopik added 2 commits August 6, 2025 12:46

fix event stream polling and improve code readability

93c085c

Merge branch 'unstable' into fork/petarjuki7/update_ports_enr

e51d4d8

dknopik reviewed Aug 6, 2025

View reviewed changes

dknopik self-requested a review August 6, 2025 12:02

diegomrsantos approved these changes Aug 6, 2025

View reviewed changes

dknopik approved these changes Aug 6, 2025

View reviewed changes

diegomrsantos added ready-for-merge and removed waiting-on-author labels Aug 6, 2025

mergify bot merged commit 0f60a0d into sigp:unstable Aug 6, 2025
14 checks passed

feat(networking): update ports in ENR #457

feat(networking): update ports in ENR #457

Uh oh!

Conversation

petarjuki7 commented Jul 28, 2025

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

dknopik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Jul 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

diegomrsantos commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegomrsantos commented Aug 4, 2025

Uh oh!

petarjuki7 commented Aug 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dknopik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegomrsantos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

diegomrsantos commented Jul 29, 2025 •

edited

Loading