feat(l1): detect and update stale enr on ping pong #5507

lakshya-sky · 2025-12-03T17:18:55Z

Motivation

Description

ElFantasma · 2025-12-04T17:12:14Z

crates/networking/p2p/discv4/server.rs

        Ok(())
    }

+    async fn send_enr_request(&mut self, node: &Node) -> Result<(), DiscoveryServerError> {


Let's factorize common code (message encoding, hash extraction, udp_socket.send, maybe set_disposable) with send_ping/send_ping_internal to an utility function to minimize code repetiton.

Added send_else_dispose to remove code duplication.

ElFantasma · 2025-12-04T17:24:50Z

crates/networking/p2p/discv4/server.rs

+            if let (Some(received), Some(stored)) = (received_enr_seq, stored_enr_seq)
+                && received > stored
+            {
+                self.send_enr_request(&node).await?;
+            }


Suggested change

if let (Some(received), Some(stored)) = (received_enr_seq, stored_enr_seq)

&& received > stored

{

self.send_enr_request(&node).await?;

}

if received_enr_seq.is_some_and(|r| stored_enr_seq.is_some_and(|s| r > s)) {

self.send_enr_request(&node).await?;

}

Not sure which one I prefer 🤔

Maybe transpose:

Suggested change

if let (Some(received), Some(stored)) = (received_enr_seq, stored_enr_seq)

&& received > stored

{

self.send_enr_request(&node).await?;

}

if [received_enr_seq, stored_enr_seq].transpose().is_some_and(|[received, stored]| received > stored) {

self.send_enr_request(&node).await?;

}

Or:

Suggested change

if let (Some(received), Some(stored)) = (received_enr_seq, stored_enr_seq)

&& received > stored

{

self.send_enr_request(&node).await?;

}

if let Some([received, stored]) = [received_enr_seq, stored_enr_seq].transpose() && received > stored {

self.send_enr_request(&node).await?;

}

transpose is an unstable feature.

MegaRedHand

LGTM

Oppen · 2025-12-05T15:43:47Z

crates/networking/p2p/discv4/server.rs

+                .is_err()
+            {
+                self.peer_table
+                    .set_disposable(&node.node_id())
+                    .await?;
+                METRICS.record_new_discarded_node().await;
+            }


Either put this inside the inspect_err or convert everything to an if let Err(e)...
Having two ways to check the same thing in the same place makes the code more confusing than it needs to be.

changed to if let Err(e).. for better error handling.

crates/networking/p2p/discv4/server.rs

Copilot

Pull request overview

This PR implements automatic detection and updating of stale Ethereum Node Records (ENR) during the ping/pong protocol exchange in the Discovery v4 implementation. When nodes exchange ping/pong messages that include ENR sequence numbers, the code now compares them against stored values and automatically requests updated ENRs when a peer advertises a newer version.

Key Changes:

Adds ENR sequence number comparison logic in both ping and pong message handlers to detect when peers have newer ENRs
Refactors message sending code by extracting reusable helper methods (send_enr_request and send_else_dispose)
Adds a new get_contact method to the peer table for retrieving contacts by node_id

Reviewed changes

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

File	Description
`crates/networking/p2p/discv4/server.rs`	Implements ENR staleness detection in ping/pong handlers, refactors message sending into helper methods, and updates the lookup methods for cleaner error handling
`crates/networking/p2p/discv4/peer_table.rs`	Adds `get_contact` method and corresponding message handling to support retrieving contacts by node_id

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-11T15:35:21Z

crates/networking/p2p/discv4/server.rs

            self.send_ping(&node).await?;
+        } else {
+            // If the contact has stale ENR then request the updated one.
+            let node_id = node_id(&sender_public_key);


The node_id is being computed redundantly here. The node parameter already has the same node_id since it was created from sender_public_key (see line 159-164). You can use node.node_id() directly instead of recomputing node_id(&sender_public_key) to avoid the extra keccak hash computation.

Suggested change

let node_id = node_id(&sender_public_key);

let node_id = node.node_id();

Copilot · 2025-12-11T15:35:21Z

crates/networking/p2p/discv4/server.rs

+        } else {
+            // If the contact has stale ENR then request the updated one.
+            let node_id = node_id(&sender_public_key);
+            let stored_enr_seq = self
+                .peer_table
+                .get_contact(node_id)
+                .await?
+                .and_then(|c| c.record)
+                .map(|r| r.seq);
+
+            let received_enr_seq = ping_message.enr_seq;
+
+            if let (Some(received), Some(stored)) = (received_enr_seq, stored_enr_seq)
+                && received > stored
+            {
+                self.send_enr_request(&node).await?;
+            }


The new ENR staleness detection logic in handle_ping lacks test coverage. Consider adding tests to verify that ENR requests are sent when a ping is received with a higher sequence number than the stored ENR.

Copilot · 2025-12-11T15:35:22Z

crates/networking/p2p/discv4/server.rs

+        // If the contact has stale ENR then request the updated one.
+        let stored_enr_seq = contact.record.map(|r| r.seq);
+        let received_enr_seq = message.enr_seq;
+        if let (Some(received), Some(stored)) = (received_enr_seq, stored_enr_seq)
+            && received > stored
+        {
+            self.send_enr_request(&contact.node).await?;
+        }


The new ENR staleness detection logic in handle_pong lacks test coverage. Consider adding tests to verify that ENR requests are sent when a pong is received with a higher sequence number than the stored ENR.

Copilot · 2025-12-11T15:35:22Z

crates/networking/p2p/discv4/server.rs

+    async fn send_else_dispose(
+        &mut self,
+        message: Message,
+        node: &Node,
+    ) -> Result<H256, DiscoveryServerError> {
+        let mut buf = BytesMut::new();
+        message.encode_with_header(&mut buf, &self.signer);
+        let message_hash: [u8; 32] = buf[..32]
+            .try_into()
+            .expect("first 32 bytes are the message hash");
+        if let Err(e) = self.udp_socket.send_to(&buf, node.udp_addr()).await {
+            error!(sending = ?message, addr = ?node.udp_addr(), to = ?node.node_id(), err=?e, "Error sending message");
+            self.peer_table.set_disposable(&node.node_id()).await?;
+            METRICS.record_new_discarded_node().await;
+        }
+        Ok(H256::from(message_hash))
+    }


The new send_else_dispose helper method lacks test coverage. Consider adding tests to verify its behavior when sending succeeds and when it fails, particularly ensuring that the node is marked as disposable and metrics are recorded on failure.

ElFantasma

Nice!

lakshya-sky added 3 commits December 3, 2025 12:17

make a method for sending enr requests

20a9ad1

add method to get contact by node id

ee151a0

fetch updated enr on ping/pong

cd8296b

lakshya-sky requested a review from a team as a code owner December 3, 2025 17:18

Merge branch 'main' into detect-and-update-stale-enr-on-ping-pong

fabee25

ElFantasma reviewed Dec 4, 2025

View reviewed changes

MegaRedHand approved these changes Dec 4, 2025

View reviewed changes

Oppen reviewed Dec 5, 2025

View reviewed changes

crates/networking/p2p/discv4/server.rs Outdated Show resolved Hide resolved

lakshya-sky and others added 5 commits December 5, 2025 14:32

Merge branch 'main' into detect-and-update-stale-enr-on-ping-pong

d3cab37

avoid code duplication and error handling confusion

3df7ae3

Merge branch 'main' into detect-and-update-stale-enr-on-ping-pong

bed7abc

Merge branch 'main' into detect-and-update-stale-enr-on-ping-pong

aa4130b

Merge branch 'main' into detect-and-update-stale-enr-on-ping-pong

14a776c

Copilot AI review requested due to automatic review settings December 11, 2025 15:30

Copilot started reviewing on behalf of lakshya-sky December 11, 2025 15:31 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

Merge branch 'main' into detect-and-update-stale-enr-on-ping-pong

756e6f2

ElFantasma approved these changes Dec 11, 2025

View reviewed changes

ElFantasma added this pull request to the merge queue Dec 11, 2025

Merged via the queue into lambdaclass:main with commit 78a5d85 Dec 11, 2025
39 checks passed

	let node_id = node_id(&sender_public_key);
	let node_id = node.node_id();

feat(l1): detect and update stale enr on ping pong #5507

feat(l1): detect and update stale enr on ping pong #5507

Uh oh!

Conversation

lakshya-sky commented Dec 3, 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!

Oppen Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MegaRedHand left a comment

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

ElFantasma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Oppen Dec 5, 2025 •

edited

Loading