Skip to content

feat(l1): store validated ENR from handshake in peer table #6109

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
mikhailofff wants to merge 1 commit into
base: main
Choose a base branch
from
+17 −1
Open

feat(l1): store validated ENR from handshake in peer table #6109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 12 additions & 1 deletion crates/networking/p2p/discv5/peer_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,7 +864,18 @@ impl PeerTableServer {
vacant_entry.insert(contact);
METRICS.record_new_discovery().await;
}
// TODO Handle the case the contact is already present
// The contact is already present
Copy link
Contributor

@ElFantasma ElFantasma Feb 12, 2026

Choose a reason for hiding this comment

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

This was already implemented more thoroughly on a previous PR
This PR will conflict once that merges. Consider rebasing on top of it and focusing only on the handle_handshake insertion.

else if let Some(existing_contact) = self.contacts.get_mut(&node_id) {
if let Some(existing_record) = &existing_contact.record {
if node_record.seq > existing_record.seq {
existing_contact.record = Some(node_record);
METRICS.record_new_discovery().await;
}
} else {
existing_contact.record = Some(node_record);
METRICS.record_new_discovery().await;
}
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions crates/networking/p2p/discv5/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,11 @@ impl DiscoveryServer {
.secp256k1
.and_then(|pk| PublicKey::from_slice(pk.as_bytes()).ok());

// Add the peer to the peer table
self.peer_table
.new_contact_records(vec![record.clone()], self.local_node.node_id())
.await?;
Comment on lines +306 to +309
Copy link

@greptile-apps greptile-apps bot Feb 3, 2026

Choose a reason for hiding this comment

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

Peer added to table before verifying node_id matches src_id (line 315). If verification fails at line 317, the invalid peer remains in the table.

Move this call after the node_id verification (after line 319).

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/networking/p2p/discv5/server.rs
Line: 306:309

Comment:
Peer added to table before verifying node_id matches src_id (line 315). If verification fails at line 317, the invalid peer remains in the table.

Move this call after the node_id verification (after line 319).

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Contributor

@ElFantasma ElFantasma Feb 12, 2026

Choose a reason for hiding this comment

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

Peer added before full handshake authentication. This call runs before the derived_node_id != src_id check (line 316) and before the id-signature verification (line 337). A malicious node claiming src_id=X but sending a valid ENR for node Y would get Y inserted into the peer table — the ENR signature is valid (it's Y's real ENR), so new_contact_records accepts it. The function returns early at line 317, but the peer table entry persists.

Move this call after the id-signature verification at line 346 — there's no reason to store anything until the handshake is fully authenticated.


// Verify that the ENR's public key matches the claimed src_id
if let Some(pk) = &pubkey {
let uncompressed = pk.serialize_uncompressed();
Expand Down
Loading