Skip to content

fix(l1): ignore unknown protocols in capability exchange #3543

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.

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

Merged
merged 9 commits into from
Jul 8, 2025

Conversation

MegaRedHand
Copy link
Collaborator

@MegaRedHand MegaRedHand commented Jul 7, 2025

Motivation

Failing due to a peer having extra capabilities can make us lose exceptional peers. Hence, we want to ignore any extra capabilities they have.

Description

This PR changes Capability.protocol to be an 8-byte array instead of a string, allowing us to store any string we receive.

Copy link

github-actions bot commented Jul 7, 2025

Lines of code report

Total lines added: 44
Total lines removed: 9
Total lines changed: 53

Detailed view
+---------------------------------------------+-------+------+
| File                                        | Lines | Diff |
+---------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/p2p.rs    | 312   | +44  |
+---------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/admin/peers.rs | 103   | -9   |
+---------------------------------------------+-------+------+

@MegaRedHand MegaRedHand changed the title fix: ignore unknown protocols in capability exchange fix(core): ignore unknown protocols in capability exchange Jul 7, 2025
@github-actions github-actions bot added L1 Ethereum client L2 Rollup client labels Jul 7, 2025
@MegaRedHand MegaRedHand marked this pull request as ready for review July 7, 2025 21:32
@Copilot Copilot AI review requested due to automatic review settings July 7, 2025 21:32
@MegaRedHand MegaRedHand requested a review from a team as a code owner July 7, 2025 21:32
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors how protocol identifiers are stored and matched, switching from strings to fixed-size byte arrays to allow unknown protocols to be ignored rather than causing failures.

  • Changed Capability.protocol from &'static str to [u8; 8] with padding
  • Updated all matching sites to call the new protocol() method
  • Added helper pad_right, updated RLP (de)serialization, and new decode tests

Reviewed Changes

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

File Description
crates/networking/rpc/admin/peers.rs Switched to calling cap.protocol() and refactored test setup to use Capability::eth() and snap()
crates/networking/p2p/rlpx/p2p.rs Introduced pad_right, changed protocol field to [u8; 8], updated RLP (de)serialization, and added tests
crates/networking/p2p/rlpx/connection/server.rs Updated server match clauses to use the new protocol() accessor
Comments suppressed due to low confidence (1)

crates/networking/p2p/rlpx/p2p.rs:346

  • You might add a test for decoding an unknown protocol (e.g., a custom >8-byte name) to verify that the code correctly stores arbitrary bytes and handles invalid lengths.
    fn test_decode_capability() {

@MegaRedHand MegaRedHand moved this to In Review in ethrex_l1 Jul 7, 2025
@MegaRedHand MegaRedHand moved this to In Review in ethrex_l2 Jul 7, 2025
@mpaulucci mpaulucci changed the title fix(core): ignore unknown protocols in capability exchange fix(l1): ignore unknown protocols in capability exchange Jul 8, 2025
@mpaulucci mpaulucci removed this from ethrex_l2 Jul 8, 2025
@jrchatruc jrchatruc added this pull request to the merge queue Jul 8, 2025
Merged via the queue into main with commit d1ceb86 Jul 8, 2025
28 checks passed
@jrchatruc jrchatruc deleted the p2p/ignore-unknown-capabilities branch July 8, 2025 17:27
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Jul 8, 2025
d-roak pushed a commit to 1sixtech/ethrex that referenced this pull request Jul 17, 2025
…#3543)

**Motivation**

Failing due to a peer having extra capabilities can make us lose
exceptional peers. Hence, we want to ignore any extra capabilities they
have.

**Description**

This PR changes `Capability.protocol` to be an 8-byte array instead of a
string, allowing us to store any string we receive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L1 Ethereum client L2 Rollup client
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants