Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add retries to DNS lookup for enode hostnames #8358

Merged

Conversation

sergerad
Copy link
Contributor

@sergerad sergerad commented May 23, 2024

Relates to #8188

  • Move DNS resolution function to DNSNodeRecord::
  • Add RetryStrategy struct for optional retries of node hostname DNS resolution
  • Add RetryStrategy field to NodeConfig

@sergerad sergerad force-pushed the serge/enode-retry branch 5 times, most recently from 66ee0ee to 2777af1 Compare May 23, 2024 09:26
@sergerad
Copy link
Contributor Author

@Rjected the retry impl is in place here with some tests.

Next I am thinking of adding the relevant retry params to the pub struct NodeConfig and feeding it through into pub async fn load_toml_config(.

Please let me know if that approach does / doesn't make sense.

Also do you know why I am getting those lint failures? I can't recreate them locally with the same commands. Fixing them crate by crate is slow because of this.

@Rjected
Copy link
Member

Rjected commented May 23, 2024

Also do you know why I am getting those lint failures? I can't recreate them locally with the same commands. Fixing them crate by crate is slow because of this.

We're fixing some nightly clippy lints this morning, once that's merged it would probably be good to rebase

@Rjected
Copy link
Member

Rjected commented May 23, 2024

changes are merged, I'll rebase my branch on main

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

Rjected commented May 23, 2024

changes are merged, I'll rebase my branch on main

just rebased, so feel free to rebase on top of my updated branch

@sergerad sergerad force-pushed the serge/enode-retry branch 3 times, most recently from 3b68660 to 65c8678 Compare May 24, 2024 03:20
@sergerad sergerad marked this pull request as ready for review May 24, 2024 04:17
@sergerad
Copy link
Contributor Author

sergerad commented May 24, 2024

@Rjected the code for retries and CLI args is in place. I moved the dns resolve fn into DNSNodeRecord scope also.

The --dns-retry-strategy interval=500,attempts=10 flag is an awkward approach. LMK what is preferred. E.G. perhaps --retry-count 10 and --retry-ms 500 instead

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great start!

E.G. perhaps --retry-count 10 and --retry-ms 500 instead

yeah I think this would be better, see comment

crates/net/types/src/dns_node_record.rs Outdated Show resolved Hide resolved
@sergerad sergerad requested a review from Rjected May 24, 2024 22:33
@sergerad
Copy link
Contributor Author

Flags updated, parsing logic removed 👍

@emhane emhane added A-devp2p Related to the Ethereum P2P protocol A-discv4 Related to discv4 discovery A-dnsdisc Related to DNS discovery A-discv5 Related to discv5 discovery A-cli Related to the reth CLI C-enhancement New feature or request and removed A-dnsdisc Related to DNS discovery labels May 25, 2024
@sergerad
Copy link
Contributor Author

sergerad commented May 26, 2024

@Rjected are we planning on adding DNSNodeRecord to the toml config file contents also? E.G. the PeersConfig struct uses NodeRecord:

pub struct Config {
// ...
    /// Configuration for the discovery service.
    pub peers: PeersConfig,
// ...
}
// ...
pub struct PeersConfig {
// ...
    pub trusted_nodes: HashSet<NodeRecord>,
// ...
    pub basic_nodes: HashSet<NodeRecord>,
// ...
}

EDIT: We might prefer to consolidate the DNSNodeRecord type into NodeRecord instead. I believe its possible if we update these getter fns (alongside a few other changes) to resolve the hostname to an IP if it hasn't been done yet:

impl NodeRecord {
// ...
    /// The TCP socket address of this node
    #[must_use]
    pub fn tcp_addr(&self) -> SocketAddr {
        SocketAddr::new(self.address, self.tcp_port)
    }

    /// The UDP socket address of this node
    #[must_use]
    pub fn udp_addr(&self) -> SocketAddr {
        SocketAddr::new(self.address, self.udp_port)
    }
}

@Rjected
Copy link
Member

Rjected commented May 31, 2024

if we update these getter fns (alongside a few other changes) to resolve the hostname to an IP if it hasn't been done yet

While having less node record types is desirable, if we do this we need to make sure that we do not accept domain node records from discovery - these domain records should really only be used for trusted peers.

are we planning on adding DNSNodeRecord to the toml config file

That would make sense, although I can probably just do that in #8188

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@Rjected Rjected merged commit 077f686 into paradigmxyz:dan/dns-resolution-on-enodes May 31, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Related to the reth CLI A-devp2p Related to the Ethereum P2P protocol A-discv4 Related to discv4 discovery A-discv5 Related to discv5 discovery C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants