chore(l1): use NodeRecordPairs to hold enr entries #5546
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the NodeRecord structure to use a structured NodeRecordPairs type instead of a raw Vec<(Bytes, Bytes)> for storing ENR (Ethereum Node Record) entries. This change simplifies ENR manipulation by eliminating the need for manual sorting and encoding/decoding of key-value pairs.
Key Changes
- Moved
NodeRecordPairsstruct beforeNodeRecordand made thepairsfield inNodeRecordprivate with typed access - Implemented
encode_pairs()and refactoreddecode_pairs()as static methods onNodeRecordPairsto handle conversion between structured and raw formats - Added helper methods
get_fork_id()andupdate()toNodeRecordfor cleaner fork ID management and record updates
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| crates/networking/p2p/types.rs | Restructured NodeRecord to use NodeRecordPairs internally; added encode/decode methods; added snap field support; refactored from_node and set_fork_id methods |
| crates/networking/p2p/discv4/server.rs | Updated to use new get_fork_id() method instead of decode_pairs() |
| crates/networking/p2p/discv4/messages.rs | Updated test constructors to use new NodeRecord::new() and decode_pairs() static method |
| crates/common/types/fork_id.rs | Added Serialize and Deserialize derives to ForkId |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/networking/p2p/types.rs
Outdated
| pub fn get_fork_id(&self) -> Option<ForkId> { | ||
| self.pairs.eth.clone() |
There was a problem hiding this comment.
Unnecessary clone of fork_id. Since ForkId is being moved into self.pairs.eth, you can directly move it without cloning: self.pairs.eth = Some(fork_id); (which you already do correctly). However, in get_fork_id, consider returning a reference instead of cloning the entire ForkId if possible.
| pub fn get_fork_id(&self) -> Option<ForkId> { | |
| self.pairs.eth.clone() | |
| pub fn get_fork_id(&self) -> Option<&ForkId> { | |
| self.pairs.eth.as_ref() |
crates/networking/p2p/types.rs
Outdated
| if bytes.len() < 33 { | ||
| continue; | ||
| return Err(RLPDecodeError::Custom(format!( | ||
| "Invalid signature length {}", |
There was a problem hiding this comment.
The error message refers to "signature" but this validation is for the secp256k1 public key field, not a signature. Update the error message to be more accurate, e.g., "Invalid secp256k1 public key length: expected at least 33 bytes, got {}".
| "Invalid signature length {}", | |
| "Invalid secp256k1 public key length: expected at least 33 bytes, got {}", |
crates/networking/p2p/types.rs
Outdated
| impl NodeRecord { | ||
| pub fn decode_pairs(&self) -> NodeRecordPairs { | ||
| impl NodeRecordPairs { | ||
| pub fn decode_pairs(pairs: &Vec<(Bytes, Bytes)>) -> Result<NodeRecordPairs, RLPDecodeError> { |
There was a problem hiding this comment.
The method signature is confusing - this is a static method that takes a Vec reference but is called decode_pairs. Consider renaming to from_pairs or try_from_pairs to better reflect that this is a constructor-like method that creates a new NodeRecordPairs instance from raw key-value pairs.
| "id" => decoded_pairs.id = Some(String::decode(&value)?), | ||
| "ip" => decoded_pairs.ip = Some(Ipv4Addr::decode(&value)?), | ||
| "ip6" => decoded_pairs.ip6 = Some(Ipv6Addr::decode(&value)?), | ||
| "tcp" => decoded_pairs.tcp_port = Some(u16::decode(&value)?), | ||
| "udp" => decoded_pairs.udp_port = Some(u16::decode(&value)?), | ||
| "secp256k1" => { | ||
| let Ok(bytes) = Bytes::decode(&value) else { | ||
| continue; | ||
| }; | ||
| let bytes = Bytes::decode(&value)?; | ||
| if bytes.len() < 33 { | ||
| continue; | ||
| return Err(RLPDecodeError::Custom(format!( | ||
| "Invalid signature length {}", | ||
| bytes.len() | ||
| ))); | ||
| } | ||
| decoded_pairs.secp256k1 = Some(H264::from_slice(&bytes)) | ||
| } | ||
| "snap" => decoded_pairs.snap = Some(Vec::<u32>::decode(&value)?), |
There was a problem hiding this comment.
The error handling has changed from silently skipping decode failures to returning errors. For known keys like "id", "ip", "tcp", etc., if decoding fails with ?, the entire decode operation now fails instead of continuing. This is a breaking behavior change that could reject previously accepted ENR records with malformed values. Verify this stricter validation is intentional, or consider maintaining backward compatibility by continuing to skip malformed known fields.
| pub signature: H512, | ||
| pub seq: u64, | ||
| /// The remainder of the record consists of arbitrary key/value pairs | ||
| pairs: NodeRecordPairs, |
There was a problem hiding this comment.
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.
crates/networking/p2p/types.rs
Outdated
| pub fn pairs(&self) -> NodeRecordPairs { | ||
| self.pairs.clone() |
There was a problem hiding this comment.
The pairs() method returns a clone of the entire NodeRecordPairs structure, which could be inefficient if called frequently. Consider whether a reference would be more appropriate here, or document why cloning is necessary.
| pub fn pairs(&self) -> NodeRecordPairs { | |
| self.pairs.clone() | |
| pub fn pairs(&self) -> &NodeRecordPairs { | |
| &self.pairs |
| node_record: NodeRecord, | ||
| ) -> Result<(), DiscoveryServerError> { | ||
| let pairs = node_record.decode_pairs(); | ||
| let local_fork_id = node_record.get_fork_id(); |
There was a problem hiding this comment.
The variable name local_fork_id is misleading - this is actually the fork ID from the received node record, not the local fork ID. Consider renaming to node_fork_id or received_fork_id for clarity.
crates/networking/p2p/types.rs
Outdated
| pub struct NodeRecord { | ||
| pub signature: H512, | ||
| pub seq: u64, | ||
| /// The remainder of the record consists of arbitrary key/value pairs |
There was a problem hiding this comment.
The comment says "The remainder of the record consists of arbitrary key/value pairs" but the field is now of type NodeRecordPairs, which is a specific structured type, not arbitrary pairs. Update the comment to reflect the actual implementation, such as "The remainder of the record consists of key/value pairs represented as NodeRecordPairs".
| /// The remainder of the record consists of arbitrary key/value pairs | |
| /// The remainder of the record consists of key/value pairs represented as NodeRecordPairs |
1 participant
Motivation
Instead of storing vec of arbitary blob use NodeRecordPairs which allow updating ENRs less tedious.
Description
Closes #5068