Skip to content

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

Open
refcell wants to merge 1 commit intomainfrom
rf/fix/bootnode-discv5-enr-port
Open

chore(execution): Refactor & Test Discovery Services#2341
refcell wants to merge 1 commit 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.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
base Ignored Ignored Preview Apr 25, 2026 3:54pm

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 25, 2026

Review Summary

The refactoring cleanly extracts discv4_config() and discv5_config() into testable methods, and the discv5_discovery_port_matches_v5_addr parametrized test covers the key invariant that motivated the PR (discv5 discovery port matching --v5-addr).

Previous review findings (tautological test, Config::builder(self.v4_addr) using the wrong address) appear to have been addressed in the latest revision.

Minor observations (not blocking):

  • Duplicated log (bootnode.rs:80,87): info!(enr = %discv5.local_enr(), "Started discv5") appears in both arms of the NAT-resolution match. Moving it after the match block would remove the duplication.

  • assert_ne! tests the fixture, not the code (bootnode.rs:183): assert_ne!(discv5_port, c.v4_addr.port()) only holds because the test cases use different ports for v4/v5. It's asserting a property of the test data rather than the code under test. If a case like ("0.0.0.0:9200", "0.0.0.0:9200") were added, it would fail even though the discv5 config correctly uses v5_addr. The assert_eq! on line 182 already validates the important invariant.

No correctness, safety, or concurrency issues found.

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