Skip to content

chore(execution): Refactor & Test Discovery Services#2341

Open
refcell wants to merge 2 commits intomainfrom
rf/fix/bootnode-discv5-enr-port
Open

chore(execution): Refactor & Test Discovery Services#2341
refcell wants to merge 2 commits intomainfrom
rf/fix/bootnode-discv5-enr-port

Conversation

@refcell
Copy link
Copy Markdown
Contributor

@refcell refcell commented Apr 22, 2026

Summary

#2339 fixed this isue where the discv5 ENR was being stamped with the discv4 port (v4_addr.port()) instead of the discv5 listen port (v5_addr.port()), making the node unreachable via discv5.

This PR refactors the config construction and adds a bunch of unit testing to ensure ports and addresses across the discovery services are consistent and reachable.

@refcell refcell added the bug Flag: Something isn't working label Apr 22, 2026
@refcell refcell self-assigned this Apr 22, 2026
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
base Ignored Ignored Preview Apr 26, 2026 2:44pm

Request Review

Comment thread crates/execution/cli/src/commands/p2p/bootnode.rs Outdated
Base automatically changed from hh/separate-bootnode-addrs to main April 22, 2026 19:30
@cb-heimdall
Copy link
Copy Markdown
Collaborator

cb-heimdall commented Apr 22, 2026

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@refcell refcell force-pushed the rf/fix/bootnode-discv5-enr-port branch from 3dab16f to 1ebf037 Compare April 22, 2026 19:41
@refcell refcell changed the title fix(p2p): Fix Discv5 ENR Port via ListenConfig chore( Apr 22, 2026
@refcell refcell changed the title chore( chore(execution): Refactor & Test Discovery SVCs Apr 22, 2026
@refcell refcell changed the title chore(execution): Refactor & Test Discovery SVCs chore(execution): Refactor & Test Discovery Services Apr 22, 2026
@refcell refcell force-pushed the rf/fix/bootnode-discv5-enr-port branch 2 times, most recently from 78edbc9 to 43af5d0 Compare April 22, 2026 20:33
Comment thread crates/execution/cli/src/commands/p2p/bootnode.rs Outdated
@refcell refcell marked this pull request as ready for review April 22, 2026 21:11
@refcell refcell enabled auto-merge April 22, 2026 21:12
@refcell refcell force-pushed the rf/fix/bootnode-discv5-enr-port branch from a80eaac to ba70349 Compare April 22, 2026 21:12
Set the discv5 UDP listen port explicitly via ListenConfig instead of
relying on the DEFAULT_DISCOVERY_V5_PORT constant, so the port in the
ENR always matches --v5-addr. Extracts discv4_config() and discv5_config()
methods to make the port wiring testable, and adds rstest parametrized
tests that assert discv5_config().discovery_socket().port() == v5_addr.port().
@refcell refcell force-pushed the rf/fix/bootnode-discv5-enr-port branch from ba70349 to f316305 Compare April 25, 2026 15:53
@refcell refcell requested a review from 0x00101010 April 25, 2026 15:54
@github-actions
Copy link
Copy Markdown
Contributor

Review Summary

Clean refactor. The two issues raised in prior review rounds have been addressed:

  1. Config::builder(self.v5_addr) — now correctly passes the discv5 address instead of the discv4 address.
  2. Tautological test removed — the test that was testing SocketAddr::new(ip, port).port() == port is gone.

The extracted discv4_config() / discv5_config() methods are a good improvement for testability, and the discv5_discovery_port_matches_v5_addr rstest covers the key invariant that motivated the original fix.

No new issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Flag: Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants