Skip to content

Conversation

@edg-l
Copy link
Contributor

@edg-l edg-l commented Dec 3, 2025

Motivation

Now that we are #5446 from peers, we can filter valid contacts to initiate RLPx connections on lookup.

Closes #5506

@github-actions github-actions bot added the L1 Ethereum client label Dec 3, 2025
@github-actions
Copy link

github-actions bot commented Dec 3, 2025

Lines of code report

Total lines added: 87
Total lines removed: 0
Total lines changed: 87

Detailed view
+---------------------------------------------------+-------+------+
| File                                              | Lines | Diff |
+---------------------------------------------------+-------+------+
| ethrex/crates/common/types/fork_id.rs             | 674   | +6   |
+---------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv4/peer_table.rs | 1038  | +28  |
+---------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv4/server.rs     | 620   | +53  |
+---------------------------------------------------+-------+------+

@edg-l edg-l force-pushed the fork_filtering branch 3 times, most recently from f3c00b6 to d1bfd70 Compare December 3, 2025 13:11
@edg-l edg-l marked this pull request as ready for review December 3, 2025 14:19
@edg-l edg-l requested a review from a team as a code owner December 3, 2025 14:19
@ethrex-project-sync ethrex-project-sync bot moved this to In Review in ethrex_l1 Dec 3, 2025
@edg-l edg-l marked this pull request as draft December 3, 2025 15:26
@ethrex-project-sync ethrex-project-sync bot moved this from In Review to In Progress in ethrex_l1 Dec 3, 2025
@edg-l edg-l changed the title feat(l1): fork id filtering feat(l1): enr fork id filtering Dec 4, 2025
@edg-l edg-l marked this pull request as ready for review December 4, 2025 10:41
@ethrex-project-sync ethrex-project-sync bot moved this from In Progress to In Review in ethrex_l1 Dec 4, 2025
Copy link
Contributor

@ElFantasma ElFantasma left a comment

Choose a reason for hiding this comment

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

LGTM

I think there is room for further code refactoring, but there are some pending PRs that may conflict with this one. Let's merge all and take another look at this code afterwards.

@Oppen
Copy link
Contributor

Oppen commented Dec 4, 2025

LGTM

I think there is room for further code refactoring, but there are some pending PRs that may conflict with this one. Let's merge all and take another look at this code afterwards.

On those lines, I would suggest:

  1. Tracking validated peers separately from unvalidated ones, so we can validate once and forget about it afterwards;
  2. Keeping track of invalid peers separately, in a more lightweight way.

Ideally, I think there would be a clear flow like this:

New peer -> validate +-> peer table
                     |
                     +-> bad peers list

Copy link
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

LGTM. I think snap-sync is faster now 🚀 🚀 🚀

Comment on lines +546 to +551
let local_fork_id = ForkId::new(
chain_config,
genesis_header.clone(),
latest_block_header.timestamp,
latest_block_number,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably cache this: #3660 (in another PR)

@ElFantasma
Copy link
Contributor

LGTM
I think there is room for further code refactoring, but there are some pending PRs that may conflict with this one. Let's merge all and take another look at this code afterwards.

On those lines, I would suggest:

1. Tracking validated peers separately from unvalidated ones, so we can validate once and forget about it afterwards;

2. Keeping track of invalid peers separately, in a more lightweight way.

Ideally, I think there would be a clear flow like this:

New peer -> validate +-> peer table
                     |
                     +-> bad peers list

Yeah, I definitely like this approach. Probably several lists with different categories and different update intervals: non-validated-contacts, ping-requested-contacts, enr-requested-contacts, once-connected-contacts, etc... But I think this should be done after #4245, or #4491

@MegaRedHand MegaRedHand added this pull request to the merge queue Dec 5, 2025
Merged via the queue into main with commit 4a3f3cf Dec 5, 2025
44 checks passed
@MegaRedHand MegaRedHand deleted the fork_filtering branch December 5, 2025 15:40
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Dec 5, 2025
rodrigo-o added a commit that referenced this pull request Dec 8, 2025
rodrigo-o added a commit that referenced this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Check fork_id before attempting to initiate an RLPx connection

5 participants