Skip to content

chore(l1): use NodeRecordPairs to hold enr entries #5546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lakshya-sky wants to merge 8 commits into
base: main
Choose a base branch
from
Open

chore(l1): use NodeRecordPairs to hold enr entries #5546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6eaf6a6
use NodeRecordPairs to hold enr entries
lakshya-sky Dec 5, 2025
cb5e288
don't expose pairs, allow updating via methods
lakshya-sky Dec 6, 2025
6a1645e
error on decode if enr is not correct instead of ignoring
lakshya-sky Dec 6, 2025
27f6944
Merge branch 'main' into simplify-enr
lakshya-sky Dec 7, 2025
2776024
Merge branch 'main' into simplify-enr
lakshya-sky Dec 11, 2025
76acbbc
fix lints suggested by copilot
lakshya-sky Dec 11, 2025
20108cf
suggestions implemented
lakshya-sky Dec 12, 2025
f179d37
Merge branch 'main' into simplify-enr
lakshya-sky Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion crates/common/types/fork_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ use ethrex_rlp::{
};

use ethereum_types::H32;
use serde::{Deserialize, Serialize};
use tracing::debug;

use super::{BlockHash, BlockHeader, BlockNumber, ChainConfig};

// See https://github.com/ethereum/go-ethereum/blob/530adfc8e3ef9c8b6356facecdec10b30fb81d7d/core/forkid/forkid.go#L51
const TIMESTAMP_THRESHOLD: u64 = 1438269973;

#[derive(Clone, Debug, PartialEq, Default)]
#[derive(Clone, Debug, PartialEq, Default, Eq, Serialize, Deserialize)]
pub struct ForkId {
pub fork_hash: H32,
pub fork_next: BlockNumber,
Expand Down
14 changes: 8 additions & 6 deletions crates/networking/p2p/discv4/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,8 @@ impl RLPEncode for ENRResponseMessage {

#[cfg(test)]
mod tests {
use crate::types::NodeRecordPairs;

use super::*;
use bytes::Bytes;
use ethrex_common::{H256, H264};
Expand Down Expand Up @@ -759,11 +761,11 @@ mod tests {
(String::from("tcp").into(), tcp_rlp.clone().into()),
(String::from("udp").into(), udp_rlp.clone().into()),
];
let node_record = NodeRecord {
let node_record = NodeRecord::new(
signature,
seq,
pairs,
};
NodeRecordPairs::try_from_raw_pairs(&pairs).unwrap(),
);
let msg = Message::ENRResponse(ENRResponseMessage {
request_hash,
node_record,
Expand Down Expand Up @@ -885,11 +887,11 @@ mod tests {
(String::from("tcp").into(), tcp_rlp.clone().into()),
(String::from("udp").into(), udp_rlp.clone().into()),
];
let node_record = NodeRecord {
let node_record = NodeRecord::new(
signature,
seq,
pairs,
};
NodeRecordPairs::try_from_raw_pairs(&pairs).unwrap(),
);
let expected = Message::ENRResponse(ENRResponseMessage {
request_hash,
node_record,
Expand Down
4 changes: 2 additions & 2 deletions crates/networking/p2p/discv4/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,9 +539,9 @@ impl DiscoveryServer {
sender_public_key: H512,
node_record: NodeRecord,
) -> Result<(), DiscoveryServerError> {
let pairs = node_record.decode_pairs();
let node_fork_id = node_record.get_fork_id().cloned();

let Some(remote_fork_id) = pairs.eth else {
let Some(remote_fork_id) = node_fork_id else {
self.peer_table
.set_is_fork_id_valid(&node_id, false)
.await?;
Expand Down
218 changes: 116 additions & 102 deletions crates/networking/p2p/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ impl Node {
pub fn from_enr_url(enr: &str) -> Result<Self, NodeError> {
let base64_decoded = ethrex_common::base64::decode(&enr.as_bytes()[4..]);
let record = NodeRecord::decode(&base64_decoded).map_err(NodeError::from)?;
let pairs = record.decode_pairs();
let pairs = record.pairs;
let public_key = pairs.secp256k1.ok_or(NodeError::MissingField(
"public key not found in record".into(),
))?;
Expand Down Expand Up @@ -259,17 +259,7 @@ impl Display for Node {
}

/// Reference: [ENR records](https://github.com/ethereum/devp2p/blob/master/enr.md)
#[derive(Debug, PartialEq, Clone, Eq, Default, Serialize, Deserialize)]
pub struct NodeRecord {
pub signature: H512,
pub seq: u64,
// holds optional values in (key, value) format
// value represents the rlp encoded bytes
// The key/value pairs must be sorted by key and must be unique
pub pairs: Vec<(Bytes, Bytes)>,
}

#[derive(Debug, Default, PartialEq)]
#[derive(Debug, Default, PartialEq, Eq, Clone, Serialize, Deserialize)]
pub struct NodeRecordPairs {
/// The ID of the identity scheme: https://github.com/ethereum/devp2p/blob/master/enr.md#v4-identity-scheme
/// This is always "v4".
Expand All @@ -284,52 +274,105 @@ pub struct NodeRecordPairs {
pub secp256k1: Option<H264>,
// https://github.com/ethereum/devp2p/blob/master/enr-entries/eth.md
pub eth: Option<ForkId>,
// Snap entry is being used by some tests such as `test_encode_enr_response`.
pub snap: Option<Vec<u32>>,
// TODO implement ipv6 specific ports
}

impl NodeRecord {
pub fn decode_pairs(&self) -> NodeRecordPairs {
impl NodeRecordPairs {
pub fn try_from_raw_pairs(pairs: &[(Bytes, Bytes)]) -> Result<NodeRecordPairs, RLPDecodeError> {
let mut decoded_pairs = NodeRecordPairs::default();
for (key, value) in &self.pairs {
Copy link
Contributor

@Oppen Oppen Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check order and uniqueness as well?

let Ok(key) = String::from_utf8(key.to_vec()) else {
continue;
};
let value = value.to_vec();
match key.as_str() {
"id" => decoded_pairs.id = String::decode(&value).ok(),
"ip" => decoded_pairs.ip = Ipv4Addr::decode(&value).ok(),
"ip6" => decoded_pairs.ip6 = Ipv6Addr::decode(&value).ok(),
"tcp" => decoded_pairs.tcp_port = u16::decode(&value).ok(),
"udp" => decoded_pairs.udp_port = u16::decode(&value).ok(),
"secp256k1" => {
let Ok(bytes) = Bytes::decode(&value) else {
continue;
};
for (key, value) in pairs {
match key.as_ref() {
b"id" => decoded_pairs.id = Some(String::decode(value)?),
b"ip" => decoded_pairs.ip = Some(Ipv4Addr::decode(value)?),
b"ip6" => decoded_pairs.ip6 = Some(Ipv6Addr::decode(value)?),
b"tcp" => decoded_pairs.tcp_port = Some(u16::decode(value)?),
b"udp" => decoded_pairs.udp_port = Some(u16::decode(value)?),
b"secp256k1" => {
let bytes = Bytes::decode(value)?;
if bytes.len() < 33 {
continue;
return Err(RLPDecodeError::Custom(format!(
"Invalid secp256k1 public key length: expected at least 33 bytes, got {}",
bytes.len()
)));
}
decoded_pairs.secp256k1 = Some(H264::from_slice(&bytes))
}
Comment on lines +292 to 301
Copy link
Contributor

@Oppen Oppen Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, this can be simplified as follows:

Suggested change
b"secp256k1" => {
let bytes = Bytes::decode(value)?;
if bytes.len() < 33 {
continue;
return Err(RLPDecodeError::Custom(format!(
"Invalid secp256k1 public key length: expected at least 33 bytes, got {}",
bytes.len()
)));
}
decoded_pairs.secp256k1 = Some(H264::from_slice(&bytes))
}
b"secp256k1" => decoded_pairs.secp256k1 = Some(H264([u8; 33]::decode(value)?)),

"eth" => {
b"snap" => decoded_pairs.snap = Some(Vec::<u32>::decode(value)?),
b"eth" => {
// https://github.com/ethereum/devp2p/blob/master/enr-entries/eth.md
// entry-value = [[ forkHash, forkNext ], ...]
let Ok(decoder) = Decoder::new(&value) else {
continue;
};
let decoder = Decoder::new(value)?;
// Here we decode fork-id = [ forkHash, forkNext ]
// TODO(#3494): here we decode as optional to ignore any errors,
// but we should return an error if we can't decode it
let (fork_id, decoder) = decoder.decode_optional_field();
let (fork_id, decoder) = decoder.decode_field("forkId")?;

// As per the spec, we should ignore any additional list elements in entry-value
decoder.finish_unchecked();
decoded_pairs.eth = fork_id;
decoded_pairs.eth = Some(fork_id);
}
// Key is some random bytes sequence which we don't care
_ => {}
}
}

decoded_pairs
Ok(decoded_pairs)
}

/// Encodes to a list of (key, value) where keys are ascii bytes and values are rlp encoded bytes.
pub fn encode_pairs(&self) -> Vec<(Bytes, Bytes)> {
// The key/value pairs must be sorted by key and must be unique
let mut pairs = vec![];
if let Some(eth) = self.eth.clone() {
// Without the Vec wrapper, RLP encoding fork_id directly would produce:
// [forkHash, forkNext]
// But the spec requires nested lists:
// [[forkHash, forkNext]]
let eth = vec![eth];
pairs.push(("eth".into(), eth.encode_to_vec().into()));
}
if let Some(id) = self.id.as_ref() {
pairs.push(("id".into(), id.encode_to_vec().into()));
}
if let Some(ip) = self.ip {
pairs.push(("ip".into(), ip.encode_to_vec().into()));
}
if let Some(ip6) = self.ip6 {
pairs.push(("ip6".into(), ip6.encode_to_vec().into()));
}
if let Some(secp256k1) = self.secp256k1 {
pairs.push(("secp256k1".into(), secp256k1.encode_to_vec().into()));
}
if let Some(snap) = self.snap.as_ref() {
pairs.push(("snap".into(), snap.encode_to_vec().into()));
}

if let Some(tcp) = self.tcp_port {
pairs.push(("tcp".into(), tcp.encode_to_vec().into()));
}
if let Some(udp) = self.udp_port {
pairs.push(("udp".into(), udp.encode_to_vec().into()));
}
pairs
}
}

/// Reference: [ENR records](https://github.com/ethereum/devp2p/blob/master/enr.md#record-structure)
#[derive(Debug, PartialEq, Clone, Eq, Default, Serialize, Deserialize)]
pub struct NodeRecord {
pub signature: H512,
pub seq: u64,
/// The remainder of the record consists of key/value pairs represented as NodeRecordPairs
pairs: NodeRecordPairs,
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pairs field is now private but there's no getter method provided. However, a pairs() method is added later that returns a clone. Consider if this should be a direct reference getter instead, or if making the field public would be more appropriate for better API design, especially since the old structure had this field public.

Copilot uses AI. Check for mistakes.
}

impl NodeRecord {
pub fn new(signature: H512, seq: u64, pairs: NodeRecordPairs) -> Self {
Self {
signature,
seq,
pairs,
}
}

pub fn enr_url(&self) -> Result<String, NodeError> {
Expand All @@ -343,48 +386,41 @@ impl NodeRecord {
}

pub fn from_node(node: &Node, seq: u64, signer: &SecretKey) -> Result<Self, NodeError> {
let mut pairs = NodeRecordPairs {
id: Some("v4".to_string()),
secp256k1: Some(H264::from_slice(
&PublicKey::from_secret_key(secp256k1::SECP256K1, signer).serialize(),
)),
tcp_port: Some(node.tcp_port),
udp_port: Some(node.udp_port),
..Default::default()
};
match node.ip.to_canonical() {
IpAddr::V4(ip) => pairs.ip = Some(ip),
IpAddr::V6(ip) => pairs.ip6 = Some(ip),
}

let mut record = NodeRecord {
seq,
pairs,
..Default::default()
};
record
.pairs
.push(("id".into(), "v4".encode_to_vec().into()));
record
.pairs
.push(("ip".into(), node.ip.encode_to_vec().into()));
record.pairs.push((
"secp256k1".into(),
PublicKey::from_secret_key(secp256k1::SECP256K1, signer)
.serialize()
.encode_to_vec()
.into(),
));
record
.pairs
.push(("tcp".into(), node.tcp_port.encode_to_vec().into()));
record
.pairs
.push(("udp".into(), node.udp_port.encode_to_vec().into()));

record.signature = record.sign_record(signer)?;

Ok(record)
}

pub fn set_fork_id(&mut self, fork_id: ForkId, signer: &SecretKey) -> Result<(), NodeError> {
// Without the Vec wrapper, RLP encoding fork_id directly would produce:
// [forkHash, forkNext]
// But the spec requires nested lists:
// [[forkHash, forkNext]]
let eth = vec![fork_id];
self.pairs.push(("eth".into(), eth.encode_to_vec().into()));

//Pairs need to be sorted by their key.
//The keys are Bytes which implements Ord, so they can be compared directly. The sorting
//will be lexicographic (alphabetical for string keys like "eth", "id", "ip", etc.).
self.pairs.sort_by(|a, b| a.0.cmp(&b.0));
self.pairs.eth = Some(fork_id);
self.update(signer)
}

pub fn get_fork_id(&self) -> Option<&ForkId> {
self.pairs.eth.as_ref()
}

fn update(&mut self, signer: &SecretKey) -> Result<(), NodeError> {
self.seq += 1;
self.signature = self.sign_record(signer)?;
Ok(())
}
Expand All @@ -404,42 +440,19 @@ impl NodeRecord {
let mut rlp = vec![];
structs::Encoder::new(&mut rlp)
.encode_field(&self.seq)
.encode_key_value_list::<Bytes>(&self.pairs)
.encode_key_value_list::<Bytes>(&self.pairs.encode_pairs())
.finish();
keccak_hash(&rlp)
}

pub fn pairs(&self) -> &NodeRecordPairs {
&self.pairs
}
}

impl From<NodeRecordPairs> for Vec<(Bytes, Bytes)> {
fn from(value: NodeRecordPairs) -> Self {
let mut pairs = vec![];
if let Some(eth) = value.eth {
// Without the Vec wrapper, RLP encoding fork_id directly would produce:
// [forkHash, forkNext]
// But the spec requires nested lists:
// [[forkHash, forkNext]]
let eth = vec![eth];
pairs.push(("eth".into(), eth.encode_to_vec().into()));
}
if let Some(id) = value.id {
pairs.push(("id".into(), id.encode_to_vec().into()));
}
if let Some(ip) = value.ip {
pairs.push(("ip".into(), ip.encode_to_vec().into()));
}
if let Some(ip6) = value.ip6 {
pairs.push(("ip6".into(), ip6.encode_to_vec().into()));
}
if let Some(secp256k1) = value.secp256k1 {
pairs.push(("secp256k1".into(), secp256k1.encode_to_vec().into()));
}
if let Some(tcp) = value.tcp_port {
pairs.push(("tcp".into(), tcp.encode_to_vec().into()));
}
if let Some(udp) = value.udp_port {
pairs.push(("udp".into(), udp.encode_to_vec().into()));
}
pairs
value.encode_pairs()
}
}

Expand All @@ -456,6 +469,7 @@ impl RLPDecode for NodeRecord {
// all fields in pairs are optional except for id
let id_pair = pairs.iter().find(|(k, _v)| k.eq("id".as_bytes()));
if id_pair.is_some() {
let pairs = NodeRecordPairs::try_from_raw_pairs(&pairs)?;
let node_record = NodeRecord {
signature,
seq,
Expand Down Expand Up @@ -494,7 +508,7 @@ impl RLPEncode for NodeRecord {
structs::Encoder::new(buf)
.encode_field(&self.signature)
.encode_field(&self.seq)
.encode_key_value_list::<Bytes>(&self.pairs)
.encode_key_value_list::<Bytes>(&self.pairs.encode_pairs())
.finish();
}
}
Expand Down Expand Up @@ -601,7 +615,7 @@ mod tests {
);
let mut record = NodeRecord::from_node(&node, 1, &signer).unwrap();
// Drop fork ID since the test doesn't use it
record.pairs.retain(|(k, _)| k != "eth");
record.pairs.eth = None;
record.sign_record(&signer).unwrap();

let expected_enr_string = "enr:-Iu4QIQVZPoFHwH3TCVkFKpW3hm28yj5HteKEO0QTVsavAGgD9ISdBmAgsIyUzdD9Yrqc84EhT067h1VA1E1HSLKcMgBgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQJtSDUljLLg3EYuRCp8QJvH8G2F9rmUAQtPKlZjq_O7loN0Y3CCdl-DdWRwgnZf";
Expand Down Expand Up @@ -641,7 +655,7 @@ mod tests {
let enr_url = record.enr_url().unwrap();
let base64_decoded = ethrex_common::base64::decode(&enr_url.as_bytes()[4..]);
let parsed_record = NodeRecord::decode(&base64_decoded).unwrap();
let pairs = parsed_record.decode_pairs();
let pairs = parsed_record.pairs;

assert_eq!(pairs.eth, Some(fork_id));
}
Expand Down