Skip to content

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

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

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

Conversation

@refcell
Copy link
Copy Markdown
Contributor

@refcell refcell commented Apr 22, 2026 •
edited
Loading

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 •
edited
Loading

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

Project Deployment Actions Updated (UTC)
base Ready Ready Preview, Comment Apr 26, 2026 4:30pm

Request Review

github-actions[bot]
github-actions Bot reviewed Apr 22, 2026
View reviewed changes
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 •
edited
Loading

🟡 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
github-actions[bot]
github-actions Bot reviewed Apr 22, 2026
View reviewed changes
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
@refcell
fix(p2p): fix discv5 ENR port and add rstest coverage
f316305 
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

github-actions Bot commented Apr 26, 2026

Review Summary

Clean refactor. The config construction is correctly extracted into discv4_config() and discv5_config(), and the discv5_config() method now properly sets ListenConfig with the v5 address/port — fixing the original bug where the discv5 ENR could be stamped with the wrong port.

The discv5_discovery_port_matches_v5_addr test is the most valuable addition: it directly asserts the core invariant that discovery_socket().port() matches v5_addr.port() across multiple address combinations.

No correctness, safety, or concurrency issues found.

Minor nit (not blocking): The info!(enr = %discv5.local_enr(), "Started discv5") line is duplicated in both arms of the NAT resolution match (lines 80, 87). It could be hoisted after the match block to reduce duplication, but this is a style preference — the current form is readable and correct.

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

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@danyalprout danyalprout Awaiting requested review from danyalprout

@haardikk21 haardikk21 Awaiting requested review from haardikk21

@0x00101010 0x00101010 Awaiting requested review from 0x00101010

At least 1 approving review is required to merge this pull request.

Assignees

@refcell refcell

Labels

bug Flag: Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@refcell @cb-heimdall