fix(l1): update existing contact ENR on NODES response

crates/networking/p2p/discv5/peer_table.rs

@@ -843,28 +843,53 @@ impl PeerTableServer {
 
     async fn new_contact_records(&mut self, node_records: Vec<NodeRecord>, local_node_id: H256) {
         for node_record in node_records {
+            if !node_record.verify_signature() {
+                continue;
+            }
             if let Ok(node) = Node::from_enr(&node_record) {
                 let node_id = node.node_id();
-                if let Entry::Vacant(vacant_entry) = self.contacts.entry(node_id)
-                    && !self.discarded_contacts.contains(&node_id)
-                    && node_id != local_node_id
-                {
-                    let mut contact = Contact::from(node);
-                    let is_fork_id_valid =
-                        if let Some(remote_fork_id) = node_record.decode_pairs().eth {
-                            backend::is_fork_id_valid(&self.store, &remote_fork_id)
-                                .await
-                                .ok()
-                                .or(Some(false))
-                        } else {
-                            Some(false)
+                if self.discarded_contacts.contains(&node_id) || node_id == local_node_id {
+                    continue;
+                }
+                match self.contacts.entry(node_id) {
+                    Entry::Vacant(vacant_entry) => {
+                        let is_fork_id_valid =
+                            if let Some(remote_fork_id) = node_record.decode_pairs().eth {
+                                backend::is_fork_id_valid(&self.store, &remote_fork_id)
+                                    .await
+                                    .ok()
+                                    .or(Some(false))
+                            } else {
+                                Some(false)
+                            };
+                        let mut contact = Contact::from(node);
+                        contact.is_fork_id_valid = is_fork_id_valid;
+                        contact.record = Some(node_record);
+                        vacant_entry.insert(contact);
+                        METRICS.record_new_discovery().await;
+                    }
+                    Entry::Occupied(mut occupied_entry) => {
+                        let should_update = match occupied_entry.get().record.as_ref() {
+                            None => true,
+                            Some(r) => node_record.seq > r.seq,
                         };
-                    contact.is_fork_id_valid = is_fork_id_valid;
-                    contact.record = Some(node_record);
-                    vacant_entry.insert(contact);
-                    METRICS.record_new_discovery().await;
+                        if should_update {
+                            let is_fork_id_valid =
+                                if let Some(remote_fork_id) = node_record.decode_pairs().eth {
+                                    backend::is_fork_id_valid(&self.store, &remote_fork_id)
+                                        .await
+                                        .ok()
+                                        .or(Some(false))
+                                } else {
+                                    Some(false)
+                                };
+                            let contact = occupied_entry.get_mut();
+                            contact.node = node;
+                            contact.record = Some(node_record);
+                            contact.is_fork_id_valid = is_fork_id_valid;
+                        }
+                    }
                 }
-                // TODO Handle the case the contact is already present
             }
         }
     }

crates/networking/p2p/discv5/server.rs

@@ -418,7 +418,7 @@ impl DiscoveryServer {
             }
         }
         Message::FindNode(FindNodeMessage {
-            req_id: Bytes::from(rng.r#gen::<u64>().to_be_bytes().to_vec()),
+            req_id: generate_req_id(),
             distances,
         })
     }
@@ -476,7 +476,25 @@ impl DiscoveryServer {
             .record_pong_received(&sender_id, pong_message.req_id)
             .await?;
 
-        // TODO: If sender's enr_seq > our cached version, request updated ENR
+        // If sender's enr_seq is higher than our cached version, request updated ENR.
+        if let Some(contact) = self.peer_table.get_contact(sender_id).await? {
+            // If we have no cached record, default to 0 so any PONG with enr_seq > 0
+            // triggers a FINDNODE to fetch the ENR we're missing.
+            let cached_seq = contact.record.as_ref().map_or(0, |r| r.seq);
+            if pong_message.enr_seq > cached_seq {
+                trace!(
+                    from = %sender_id,
+                    cached_seq,
+                    pong_seq = pong_message.enr_seq,
+                    "ENR seq mismatch, requesting updated ENR (FINDNODE distance 0)"
+                );
+                let find_node = Message::FindNode(FindNodeMessage {
+                    req_id: generate_req_id(),
+                    distances: vec![0],
+                });
+                self.send_ordinary(find_node, &contact.node).await?;
+            }
+        }
 
         Ok(())
     }
@@ -534,8 +552,7 @@ impl DiscoveryServer {
     }
 
     async fn send_ping(&mut self, node: &Node) -> Result<(), DiscoveryServerError> {
-        let mut rng = OsRng;
-        let req_id = Bytes::from(rng.r#gen::<u64>().to_be_bytes().to_vec());
+        let req_id = generate_req_id();
 
         let ping = Message::Ping(PingMessage {
             req_id: req_id.clone(),
@@ -879,13 +896,19 @@ pub fn lookup_interval_function(progress: f64, lower_limit: f64, upper_limit: f6
     )
 }
 
+fn generate_req_id() -> Bytes {
+    let mut rng = OsRng;
+    Bytes::from(rng.r#gen::<u64>().to_be_bytes().to_vec())
+}
+
 #[cfg(test)]
 mod tests {
     use crate::{
-        discv5::server::DiscoveryServer,
+        discv5::{messages::PongMessage, server::DiscoveryServer, session::Session},
         peer_table::PeerTable,
         types::{Node, NodeRecord},
     };
+    use bytes::Bytes;
     use ethrex_common::H256;
     use ethrex_storage::{EngineType, Store};
     use rand::{SeedableRng, rngs::StdRng};
@@ -981,4 +1004,114 @@ mod tests {
         // Both IPs should now be in the rate limit map
         assert_eq!(server.whoareyou_rate_limit.len(), 2);
     }
+
+    #[tokio::test]
+    async fn test_enr_update_request_on_pong() {
+        // Create local node
+        let local_node = Node::from_enode_url(
+            "enode://d860a01f9722d78051619d1e2351aba3f43f943f6f00718d1b9baa4101932a1f5011f16bb2b1bb35db20d6fe28fa0bf09636d26a87d31de9ec6203eeedb1f666@18.138.108.67:30303",
+        ).expect("Bad enode url");
+        let signer = SecretKey::new(&mut rand::rngs::OsRng);
+        let local_node_record = NodeRecord::from_node(&local_node, 1, &signer).unwrap();
+
+        // Create remote node - use a template node for IP/ports, but the record will use remote_signer's key
+        let remote_signer = SecretKey::new(&mut rand::rngs::OsRng);
+        let remote_node_template = Node::from_enode_url(
+            "enode://a448f24c6d18e575453db127a3d8eeeea3e3426f0db43bd52067d85cc5a1e87ad09f44b2bbaa66bb3a8c47cff8082ca4cde4b03f5ba52c1e92b3d2b9125d6da5@127.0.0.1:30304",
+        ).expect("Bad enode url");
+
+        // Create NodeRecord for the remote node with seq = 5
+        // Note: from_node uses remote_signer's public key, so we derive node_id from the record
+        let remote_record =
+            NodeRecord::from_node(&remote_node_template, 5, &remote_signer).unwrap();
+        let remote_node = Node::from_enr(&remote_record).expect("Should create node from record");
+        let remote_node_id = remote_node.node_id();
+
+        let mut peer_table = PeerTable::spawn(
+            10,
+            Store::new("", EngineType::InMemory).expect("Failed to create store"),
+        );
+
+        // Add the remote node as a contact with its ENR record
+        peer_table
+            .new_contact_records(vec![remote_record], local_node.node_id())
+            .await
+            .unwrap();
+
+        // Set up a session for the remote node (required for send_ordinary)
+        let session = Session {
+            outbound_key: [0u8; 16],
+            inbound_key: [0u8; 16],
+        };
+        peer_table
+            .set_session_info(remote_node_id, session)
+            .await
+            .unwrap();
+
+        let mut server = DiscoveryServer {
+            local_node,
+            local_node_record,
+            signer,
+            udp_socket: Arc::new(UdpSocket::bind("127.0.0.1:0").await.unwrap()),
+            peer_table,
+            initial_lookup_interval: 1000.0,
+            counter: 0,
+            pending_by_nonce: Default::default(),
+            pending_challenges: Default::default(),
+            whoareyou_rate_limit: Default::default(),
+        };
+
+        // Verify the contact was added
+        let contact = server.peer_table.get_contact(remote_node_id).await.unwrap();
+        assert!(
+            contact.is_some(),
+            "Contact should have been added to peer_table"
+        );
+        let contact = contact.unwrap();
+        assert_eq!(
+            contact.record.as_ref().map(|r| r.seq),
+            Some(5),
+            "Contact should have ENR with seq=5"
+        );
+
+        // Test 1: PONG with same enr_seq should NOT trigger FINDNODE
+        let pong_same_seq = PongMessage {
+            req_id: Bytes::from(vec![1, 2, 3]),
+            enr_seq: 5, // Same as cached
+            recipient_addr: "127.0.0.1:30303".parse().unwrap(),
+        };
+        let initial_pending_count = server.pending_by_nonce.len();
+        server
+            .handle_pong(pong_same_seq, remote_node_id)
+            .await
+            .expect("handle_pong failed for matching enr_seq");
+        // No new message should be pending (no FINDNODE sent)
+        assert_eq!(server.pending_by_nonce.len(), initial_pending_count);
+
+        // Test 2: PONG with higher enr_seq should trigger FINDNODE
+        let pong_higher_seq = PongMessage {
+            req_id: Bytes::from(vec![4, 5, 6]),
+            enr_seq: 10, // Higher than cached (5)
+            recipient_addr: "127.0.0.1:30303".parse().unwrap(),
+        };
+        server
+            .handle_pong(pong_higher_seq, remote_node_id)
+            .await
+            .expect("handle_pong failed for higher enr_seq");
+        // A new message should be pending (FINDNODE sent)
+        assert_eq!(server.pending_by_nonce.len(), initial_pending_count + 1);
+
+        // Test 3: PONG with lower enr_seq should NOT trigger FINDNODE
+        let pong_lower_seq = PongMessage {
+            req_id: Bytes::from(vec![7, 8, 9]),
+            enr_seq: 3, // Lower than cached (5)
+            recipient_addr: "127.0.0.1:30303".parse().unwrap(),
+        };
+        server
+            .handle_pong(pong_lower_seq, remote_node_id)
+            .await
+            .expect("handle_pong failed for lower enr_seq");
+        // No new message should be pending
+        assert_eq!(server.pending_by_nonce.len(), initial_pending_count + 1);
+    }
 }