fix(node/p2p): resolve dns addresses for peer bootnodes by theochap · Pull Request #3173 · op-rs/kona

theochap · 2025-12-09T17:59:28Z

Description

This PR fixes our peer resolution process for bootnodes in kona. We were not properly resolving bootnodes that were only providing a DNS address

op-will

👍

codecov · 2025-12-09T19:02:46Z

Codecov Report

❌ Patch coverage is 83.33333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.9%. Comparing base (3b125d4) to head (897da55).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...tes/providers/providers-alloy/src/beacon_client.rs	72.7%	3 Missing ⚠️
crates/node/peers/src/record.rs	90.9%	2 Missing ⚠️
bin/node/src/flags/engine/providers.rs	0.0%	1 Missing ⚠️
crates/node/service/src/service/builder.rs	80.0%	1 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copilot

Pull request overview

This PR adds DNS resolution support for peer bootnodes and introduces a slot duration override feature for the beacon client. The main change allows bootnode addresses specified with DNS names (e.g., localhost) to be properly resolved to IP addresses during parsing, fixing an issue where only IP addresses were previously supported. Additionally, the PR refactors the beacon client API by renaming methods to be more specific about their purpose and adds an optional slot duration override for testing scenarios where the beacon spec endpoint is unavailable.

Key Changes:

DNS resolution for bootnode peer records using domain names instead of only IP addresses
Beacon client method renames (config_spec → slot_interval, beacon_genesis → genesis_time) for clarity
Optional L1 slot duration override to bypass beacon spec endpoint calls during initialization

Reviewed changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`crates/node/peers/src/record.rs`	Added DNS-to-IP resolution for domain-based bootnode addresses using `to_socket_addrs()`, with test coverage for localhost resolution
`crates/providers/providers-alloy/src/beacon_client.rs`	Renamed trait methods for clarity, added `l1_slot_duration` field and `with_l1_slot_duration_override()` builder method to support fixed slot duration
`crates/providers/providers-alloy/src/blobs.rs`	Updated method calls to use renamed beacon client methods
`crates/node/service/src/service/builder.rs`	Integrated slot duration override into service builder, added `slot_duration_override` field to `L1ConfigBuilder`
`bin/node/src/flags/engine/providers.rs`	Added `l1_slot_duration_override` CLI argument with documentation for testing use cases
`bin/node/src/commands/node.rs`	Wired up CLI argument to service builder configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-09T19:41:01Z


 impl NodeRecord {
-    /// Converts the `address` into an [`Ipv4Addr`] if the `address` is a mapped
+    /// Converts the `address` into an [`core::net::Ipv4Addr`] if the `address` is a mapped


[nitpick] The documentation comment refers to core::net::Ipv4Addr but this appears unnecessarily verbose. Since Ipv4Addr is a commonly known type and was previously referenced simply as Ipv4Addr in the original comment, the fully qualified path core::net::Ipv4Addr adds unnecessary complexity without providing additional clarity.

Consider reverting to the simpler form: Converts the address into an Ipv4Addr if...

Suggested change

/// Converts the `address` into an [`core::net::Ipv4Addr`] if the `address` is a mapped

/// Converts the `address` into an [`Ipv4Addr`] if the `address` is a mapped

Copilot · 2025-12-09T19:41:02Z

+    /// Returns the slot interval in seconds.
+    async fn slot_interval(&self) -> Result<APIConfigResponse, Self::Error>;


The method name slot_interval() and its documentation "Returns the slot interval in seconds" suggest it returns a u64 value, but it actually returns an APIConfigResponse wrapper. This creates a naming inconsistency.

Consider one of the following:

Keep the name more general like fetch_slot_interval() or get_slot_config() to indicate it returns a config response object

Update the documentation to clarify: "Fetches the slot configuration containing the slot interval in seconds"

The same applies to genesis_time() which returns APIGenesisResponse rather than a raw u64.

Copilot · 2025-12-09T19:41:02Z


-    /// Returns the beacon genesis.
-    async fn beacon_genesis(&self) -> Result<APIGenesisResponse, Self::Error>;
+    /// Returns the beacon genesis time.


Similar to slot_interval(), the method name genesis_time() and its documentation "Returns the beacon genesis time" suggest it returns a u64 value, but it actually returns an APIGenesisResponse wrapper.

Consider renaming to fetch_genesis_time() or get_genesis_config(), or update the documentation to: "Fetches the beacon genesis configuration containing the genesis time".

Suggested change

/// Returns the beacon genesis time.

/// Fetches the beacon genesis configuration containing the genesis time.

Copilot · 2025-12-09T19:41:02Z

+    #[test]
+    fn test_url_parse_domain() {
+        let url = "enode://6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@localhost:30303?discport=30301";
+        let node: NodeRecord = url.parse().unwrap();
+        assert_eq!(node, NodeRecord {
+            address: IpAddr::V4([127,0,0,1].into()),
+            tcp_port: 30303,
+            udp_port: 30301,
+            id: "6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0".parse().unwrap(),
+        })
+    }


The new DNS resolution logic lacks test coverage for failure scenarios. Consider adding tests for:

A domain that fails to resolve (e.g., enode://[email protected]:30303)

A domain that resolves but returns no addresses

Network errors during DNS resolution

These tests would ensure proper error handling and help document expected behavior when DNS resolution fails.

Copilot · 2025-12-09T19:41:02Z

+            Some(Host::Domain(dns)) => format!("{dns}:{port}")
+                .to_socket_addrs()
+                .map_err(|e| NodeRecordParseError::InvalidUrl(e.to_string()))?
+                .next()
+                .map(|addr| addr.ip())
+                .ok_or_else(|| {
+                    NodeRecordParseError::InvalidUrl(format!("no IP found for host: {url:?}"))
+                })?,


Using to_socket_addrs() performs blocking DNS resolution which can cause delays during bootnode initialization. While this code is currently called in lazy_static! blocks during initialization, blocking I/O in these contexts can still delay startup.

Consider one of the following alternatives:

Use an async DNS resolver (e.g., tokio::net::lookup_host() or trust_dns_resolver) and make this parsing async

Add a note in the documentation that DNS resolution happens synchronously and may block during initialization

Cache DNS resolutions or pre-resolve common bootnode hostnames

Note: The current implementation in lazy_static blocks means DNS resolution happens at first access, which could cause unexpected delays.

einar-oplabs

lgtm

) ## Description This PR fixes our peer resolution process for bootnodes in kona. We were not properly resolving bootnodes that were only providing a DNS address

theochap added 2 commits December 9, 2025 12:53

feat(l1-config/slot-time): override slot time for l1

e30da45

chore(providers/beacon): rename beacon provider methods

34d9e3d

theochap requested review from BioMark3r, einar-oplabs, emhane and op-will as code owners December 9, 2025 17:59

theochap added 2 commits December 9, 2025 13:00

fix(node/p2p): fix dns resolution

b305c53

fix(node/p2p): resolve dns addresses for bootnodes

203e328

theochap force-pushed the theo/fix-p2p branch from c5e2be1 to 203e328 Compare December 9, 2025 18:00

op-will approved these changes Dec 9, 2025

View reviewed changes

Comment thread crates/node/peers/src/record.rs Outdated

Base automatically changed from theo/specify-l1-slot-time to main December 9, 2025 19:08

Copilot AI review requested due to automatic review settings December 9, 2025 19:36

Copilot started reviewing on behalf of theochap December 9, 2025 19:37 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

chore(node/p2p): add test for dns parsing + nits

897da55

theochap force-pushed the theo/fix-p2p branch from b2dbed9 to 897da55 Compare December 9, 2025 20:01

einar-oplabs approved these changes Dec 9, 2025

View reviewed changes

theochap merged commit 6480997 into main Dec 9, 2025
46 of 47 checks passed

theochap deleted the theo/fix-p2p branch December 9, 2025 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(node/p2p): resolve dns addresses for peer bootnodes#3173

fix(node/p2p): resolve dns addresses for peer bootnodes#3173
theochap merged 5 commits intomainfrom
theo/fix-p2p

theochap commented Dec 9, 2025 •

edited

Loading

Uh oh!

op-will left a comment

Uh oh!

Uh oh!

codecov bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

einar-oplabs left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	/// Converts the `address` into an [`core::net::Ipv4Addr`] if the `address` is a mapped
	/// Converts the `address` into an [`Ipv4Addr`] if the `address` is a mapped

		/// Returns the slot interval in seconds.
		async fn slot_interval(&self) -> Result<APIConfigResponse, Self::Error>;

	/// Returns the beacon genesis time.
	/// Fetches the beacon genesis configuration containing the genesis time.

Conversation

theochap commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

op-will left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

einar-oplabs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

theochap commented Dec 9, 2025 •

edited

Loading

codecov bot commented Dec 9, 2025 •

edited

Loading