feat: resolve domains in enode strings #8188

Rjected · 2024-05-09T20:05:01Z

Introduces a type that should properly deserialize bootnode enode strings with domains, since the current NodeRecord type does not do this.

fixes #8187

emhane

nice to have the new type DNSNodeRecord live in reth-net-common

emhane · 2024-05-10T17:58:21Z

crates/net/discv5/src/config.rs

+    pub fn from_unsigned(node_record: DNSNodeRecord) -> Result<Self, secp256k1::Error> {
+        let DNSNodeRecord { host, udp_port, id, .. } = node_record;


can we move this change before discv5? it doesn't really make much sense to break the public API here, which I know for example flashbots is using already. better to read the boot nodes from file into DNSNodeRecord, and then resolve them to NodeRecord once before adding to either discv4, discv5 or rlpx.

emhane · 2024-05-10T18:05:54Z

crates/net/types/src/dns_node_record.rs

+        f.write_char(':')?;
+        self.tcp_port.fmt(f)?;
+        if self.tcp_port != self.udp_port {
+            f.write_str("?discport=")?;


we should open an issue to spec this behaviour with discport

sergerad · 2024-05-17T22:51:55Z

crates/node/builder/src/launch/common.rs

+
+            // resolve trusted peers if they use a domain instead of dns
+            for peer in &config.network.trusted_peers {
+                let resolved = resolve_dns_node_record(peer.clone())


Are we sure we want to resolve DNS at this point?

When deploying geth on k8s, we have found its behaviour to eagerly resolve DNS of hostnames in enode strings to be a blocker for trusted peer setups (e.g. trying to resolve hostname for a clusterip of a statefulset that is being deployed in parallel, preventing peers from starting up further).

to make it work for this use case, what would be the best way to resolve?

The hostname for a statefulset pod on k8s should stabilize within a few seconds of the pod being up and running. The issue with geth was that the pods would terminate almost instantly on start up due to the hostname resolution failure.

To give us the best shot of not having the same issue, we could consider some or all of these options:

Build in a configurable number of retries (and maybe backoff strategy) for resolving peer hostnames;

Make the type for accessing peer records handle hostname resolution just in time (e.g. peer.ip_addr() would perform resolution if it had not been done before).

Option 1 would allow operators to ensure start up does not fail on the basis of hostnames that are in the process of being available on cluster.

Option 2 is not guaranteed to solve the issue, but might mitigate the number of retries required.

Let me know if you think we should address this in a separate issue and PR. And if you want me to help finish off this PR at all.

Build in a configurable number of retries (and maybe backoff strategy) for resolving peer hostnames;

yeah, to me this seems like the most likely to work, and option 2 seems more difficult anyways. It would be great if you could help implement it! Feel free to submit a PR to this branch!

sergerad · 2024-05-18T03:45:26Z

@Rjected I looked into this unit test failure

thread 'dns_node_record::tests::test_url_parse' panicked at crates/net/types/src/dns_node_record.rs:130:9:
assertion `left == right` failed
  left: DNSNodeRecord { host: Domain("10.3.58.6"), tcp_port: 30303, udp_port: 30301, id: 0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0 }
 right: DNSNodeRecord { host: Ipv4(10.3.58.6), tcp_port: 30303, udp_port: 30301, id: 0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0 }

The enode prefix in the URL leads the parser to use parse_opaque() on the host string which always returns a Host::Domain whereas we expect a Host::Ipv4 here.

From what I can see our only option is to post-process the url.host after that parsing has occurred. Would be better if we could control the url parsing logic but haven't seen a way to do that in our favour here.

mattsse

not super obvious to me why we need this.

this basically replaces the Noderecord usage, why?

what's the progress here?

mattsse · 2024-05-22T08:49:35Z

crates/net/common/src/dns_node_record_resolve.rs

+/// cannot be resolved, an error is returned.
+///
+/// If the host is already an IP address, the [NodeRecord] is returned immediately.
+pub async fn resolve_dns_node_record(node_record: DNSNodeRecord) -> Result<NodeRecord, Error> {


why is this a standalone function and not a fn of dnsrecord?

sergerad · 2024-05-22T09:03:03Z

not super obvious to me why we need this.

this basically replaces the Noderecord usage, why?

what's the progress here?

I have only just started contributing to reth so please take what I say with a grain of salt.

My understanding is that the current impl of NodeRecord does not support domain names, only IPv4 and IPv6 addrs:

pub struct NodeRecord {
    /// The Address of a node.
    pub address: IpAddr

This PR is adding support for domain names. @mattsse are you suggesting we should be adding this support with the existing NodeRecord struct instead of DNSNodeRecord? Given that domain name support will be a requirement for many L2 networks (like the ones I work with).

Apologies if I have misinterpreted anything 🙏

mattsse · 2024-05-22T09:47:06Z

gotcha,

@Rjected please document all of this because having like 4 different node variants is super confusing

Rjected added A-devp2p Related to the Ethereum P2P protocol A-cli Related to the reth CLI A-discv4 Related to discv4 discovery A-discv5 Related to discv5 discovery labels May 9, 2024

emhane requested changes May 10, 2024

View reviewed changes

sergerad reviewed May 17, 2024

View reviewed changes

sergerad mentioned this pull request May 22, 2024

Fix enode URL parse failure due to enode prefix #8339

Merged

mattsse reviewed May 22, 2024

View reviewed changes

sergerad mentioned this pull request May 23, 2024

Add retries to DNS lookup for enode hostnames #8358

Open

mattsse mentioned this pull request May 23, 2024

rename reth-network-types to reth-network-peer-types or reth-peer-types #8367

Open

Rjected and others added 2 commits May 23, 2024 12:11

feat: resolve domains in enode strings

f17b065

Fix enode URL parse failure due to enode prefix (#8339)

cdfdaff

Rjected force-pushed the dan/dns-resolution-on-enodes branch from 3510f0f to cdfdaff Compare May 23, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: resolve domains in enode strings #8188

feat: resolve domains in enode strings #8188

Rjected commented May 9, 2024 •

edited

emhane left a comment

emhane May 10, 2024

emhane May 10, 2024

sergerad May 17, 2024 •

edited

Rjected May 20, 2024

sergerad May 20, 2024

Rjected May 21, 2024

sergerad commented May 18, 2024

mattsse left a comment

mattsse May 22, 2024

sergerad commented May 22, 2024

mattsse commented May 22, 2024 •

edited

		pub fn from_unsigned(node_record: DNSNodeRecord) -> Result<Self, secp256k1::Error> {
		let DNSNodeRecord { host, udp_port, id, .. } = node_record;

feat: resolve domains in enode strings #8188

Are you sure you want to change the base?

feat: resolve domains in enode strings #8188

Conversation

Rjected commented May 9, 2024 • edited

emhane left a comment

Choose a reason for hiding this comment

emhane May 10, 2024

Choose a reason for hiding this comment

emhane May 10, 2024

Choose a reason for hiding this comment

sergerad May 17, 2024 • edited

Choose a reason for hiding this comment

Rjected May 20, 2024

Choose a reason for hiding this comment

sergerad May 20, 2024

Choose a reason for hiding this comment

Rjected May 21, 2024

Choose a reason for hiding this comment

sergerad commented May 18, 2024

mattsse left a comment

Choose a reason for hiding this comment

mattsse May 22, 2024

Choose a reason for hiding this comment

sergerad commented May 22, 2024

mattsse commented May 22, 2024 • edited

Rjected commented May 9, 2024 •

edited

sergerad May 17, 2024 •

edited

mattsse commented May 22, 2024 •

edited