Skip to content

Commit

Permalink
Replace tokio-retry with backon and remove RetryStrategy (#8613)
Browse files Browse the repository at this point in the history
  • Loading branch information
sergerad authored and Rjected committed Jun 5, 2024
1 parent c47916f commit 83323eb
Show file tree
Hide file tree
Showing 16 changed files with 35 additions and 151 deletions.
14 changes: 1 addition & 13 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ dyn-clone = "1.0.17"
sha2 = { version = "0.10", default-features = false }
paste = "1.0"
url = "2.3"
backon = "0.4"

# metrics
metrics = "0.22.0"
Expand All @@ -393,7 +394,6 @@ quote = "1.0"
tokio-stream = "0.1.11"
tokio = { version = "1.21", default-features = false }
tokio-util = { version = "0.7.4", features = ["codec"] }
tokio-retry = { version = "0.3.0" }

# async
async-stream = "0.3"
Expand Down
2 changes: 1 addition & 1 deletion bin/reth/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ aquamarine.workspace = true
eyre.workspace = true
clap = { workspace = true, features = ["derive", "env"] }
tempfile.workspace = true
backon = "0.4"
backon.workspace = true
similar-asserts.workspace = true
itertools.workspace = true
rayon.workspace = true
Expand Down
2 changes: 1 addition & 1 deletion bin/reth/src/commands/p2p/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ impl Command {
let mut config: Config = confy::load_path(&config_path).unwrap_or_default();

for peer in &self.network.trusted_peers {
config.peers.trusted_nodes.insert(peer.resolve(None).await?);
config.peers.trusted_nodes.insert(peer.resolve().await?);
}

if config.peers.trusted_nodes.is_empty() && self.network.trusted_only {
Expand Down
2 changes: 1 addition & 1 deletion bin/reth/src/commands/stage/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ impl Command {
config.peers.trusted_nodes_only = self.network.trusted_only;
if !self.network.trusted_peers.is_empty() {
for peer in &self.network.trusted_peers {
let peer = peer.resolve(None).await?;
let peer = peer.resolve().await?;
config.peers.trusted_nodes.insert(peer);
}
}
Expand Down
7 changes: 1 addition & 6 deletions book/cli/reth/node.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,7 @@ Networking:
Will fall back to a network-specific default if not specified.
--retry-millis <RETRY_MILLIS>
Amount of milliseconds to wait before retrying DNS resolution requests peering
[default: 1000]
--retry-attempts <RETRY_ATTEMPTS>
--dns-retries <DNS_RETRIES>
Amount of DNS resolution requests retries to perform when peering
[default: 0]
Expand Down
7 changes: 1 addition & 6 deletions book/cli/reth/p2p.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,7 @@ Networking:
Will fall back to a network-specific default if not specified.
--retry-millis <RETRY_MILLIS>
Amount of milliseconds to wait before retrying DNS resolution requests peering
[default: 1000]
--retry-attempts <RETRY_ATTEMPTS>
--dns-retries <DNS_RETRIES>
Amount of DNS resolution requests retries to perform when peering
[default: 0]
Expand Down
7 changes: 1 addition & 6 deletions book/cli/reth/stage/run.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,7 @@ Networking:

Will fall back to a network-specific default if not specified.

--retry-millis <RETRY_MILLIS>
Amount of milliseconds to wait before retrying DNS resolution requests peering

[default: 1000]

--retry-attempts <RETRY_ATTEMPTS>
--dns-retries <DNS_RETRIES>
Amount of DNS resolution requests retries to perform when peering

[default: 0]
Expand Down
7 changes: 1 addition & 6 deletions book/cli/reth/stage/unwind.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,7 @@ Networking:
Will fall back to a network-specific default if not specified.
--retry-millis <RETRY_MILLIS>
Amount of milliseconds to wait before retrying DNS resolution requests peering
[default: 1000]
--retry-attempts <RETRY_ATTEMPTS>
--dns-retries <DNS_RETRIES>
Amount of DNS resolution requests retries to perform when peering
[default: 0]
Expand Down
2 changes: 1 addition & 1 deletion crates/net/network/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ where
// resolve boot nodes
let mut resolved_boot_nodes = vec![];
for record in &boot_nodes {
let resolved = record.resolve(None).await?;
let resolved = record.resolve().await?;
resolved_boot_nodes.push(resolved);
}

Expand Down
1 change: 0 additions & 1 deletion crates/net/types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ serde_with.workspace = true
thiserror.workspace = true
url.workspace = true
tokio = { workspace = true, features = ["full"] }
tokio-retry = { workspace = true }

[dev-dependencies]
alloy-primitives = { workspace = true, features = ["rand"] }
Expand Down
2 changes: 1 addition & 1 deletion crates/net/types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ pub mod node_record;
pub use node_record::{NodeRecord, NodeRecordParseError};

pub mod trusted_peer;
pub use trusted_peer::{RetryStrategy, TrustedPeer};
pub use trusted_peer::TrustedPeer;

/// This tag should be set to indicate to libsecp256k1 that the following bytes denote an
/// uncompressed pubkey.
Expand Down
74 changes: 10 additions & 64 deletions crates/net/types/src/trusted_peer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ use std::{
use crate::{NodeRecord, PeerId};
use secp256k1::{SecretKey, SECP256K1};
use serde_with::{DeserializeFromStr, SerializeDisplay};
use std::time::Duration;
use tokio_retry::{strategy::FixedInterval, Retry};
use url::Host;

/// Represents the node record of a trusted peer. The only difference between this and a
Expand All @@ -35,22 +33,6 @@ pub struct TrustedPeer {
pub id: PeerId,
}

/// Retry strategy for DNS lookups.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct RetryStrategy {
/// The amount of time between attempts.
pub interval: Duration,
/// The number of attempts to make before failing.
pub attempts: usize,
}

impl RetryStrategy {
/// Create a new retry strategy.
pub const fn new(interval: Duration, attempts: usize) -> Self {
Self { interval, attempts }
}
}

impl TrustedPeer {
/// Derive the [`NodeRecord`] from the secret key and addr
pub fn from_secret_key(host: Host, port: u16, sk: &SecretKey) -> Self {
Expand All @@ -65,14 +47,7 @@ impl TrustedPeer {
}

/// Resolves the host in a [`TrustedPeer`] to an IP address, returning a [`NodeRecord`].
///
/// This takes as input a [`RetryStrategy`] which specifies how to handle DNS lookup failures.
///
/// Using a retry strategy of `None` will not retry the lookup if a lookup is made and it fails.
pub async fn resolve(
&self,
retry_strategy: Option<RetryStrategy>,
) -> Result<NodeRecord, Error> {
pub async fn resolve(&self) -> Result<NodeRecord, Error> {
let domain = match self.host.to_owned() {
Host::Ipv4(ip) => {
let id = self.id;
Expand All @@ -91,33 +66,18 @@ impl TrustedPeer {
Host::Domain(domain) => domain,
};

// Execute the lookup
let lookup = || async { Self::lookup_host(&domain).await };

// Use provided retry strategy or use strategy which does not retry
let ip = match retry_strategy {
Some(strategy) => {
let retry = FixedInterval::new(strategy.interval).take(strategy.attempts);
Retry::spawn(retry, lookup).await?
}
None => lookup().await?,
};

// Resolve the domain to an IP address
let mut ips = tokio::net::lookup_host(format!("{domain}:0")).await?;
let ip = ips
.next()
.ok_or_else(|| Error::new(std::io::ErrorKind::AddrNotAvailable, "No IP found"))?;
Ok(NodeRecord {
address: ip,
address: ip.ip(),
id: self.id,
tcp_port: self.tcp_port,
udp_port: self.udp_port,
})
}

async fn lookup_host(domain: &str) -> Result<std::net::IpAddr, Error> {
let mut ips = tokio::net::lookup_host(format!("{domain}:0")).await?;
let ip = ips
.next()
.ok_or_else(|| Error::new(std::io::ErrorKind::AddrNotAvailable, "No IP found"))?;
Ok(ip.ip())
}
}

impl fmt::Display for TrustedPeer {
Expand Down Expand Up @@ -317,30 +277,16 @@ mod tests {
#[tokio::test]
async fn test_resolve_dns_node_record() {
// Set up tests
let tests = vec![
(
"localhost",
Some(RetryStrategy { interval: Duration::from_millis(100), attempts: 0 }),
),
(
"localhost",
Some(RetryStrategy { interval: Duration::from_millis(100), attempts: 3 }),
),
("localhost", None),
(
"localhost",
Some(RetryStrategy { interval: Duration::from_millis(100), attempts: 0 }),
),
];
let tests = vec![("localhost")];

// Run tests
for (domain, retry_strategy) in tests {
for domain in tests {
// Construct record
let rec =
TrustedPeer::new(url::Host::Domain(domain.to_owned()), 30300, PeerId::random());

// Resolve domain and validate
let rec = rec.resolve(retry_strategy).await.unwrap();
let rec = rec.resolve().await.unwrap();
match rec.address {
std::net::IpAddr::V4(addr) => {
assert_eq!(addr, std::net::Ipv4Addr::new(127, 0, 0, 1))
Expand Down
40 changes: 7 additions & 33 deletions crates/node-core/src/args/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,9 @@ pub struct NetworkArgs {
#[arg(long, value_delimiter = ',')]
pub bootnodes: Option<Vec<TrustedPeer>>,

/// Amount of milliseconds to wait before retrying DNS resolution requests peering.
#[arg(long, default_value_t = 1000)]
pub retry_millis: u64,

/// Amount of DNS resolution requests retries to perform when peering.
#[arg(long, default_value_t = 0)]
pub retry_attempts: usize,
pub dns_retries: usize,

/// The path to the known peers file. Connected peers are dumped to this file on nodes
/// shutdown, and read on startup. Cannot be used with `--no-persist-peers`.
Expand Down Expand Up @@ -229,8 +225,7 @@ impl Default for NetworkArgs {
trusted_peers: vec![],
trusted_only: false,
bootnodes: None,
retry_attempts: 0,
retry_millis: 1000,
dns_retries: 0,
peers_file: None,
identity: P2P_CLIENT_VERSION.to_string(),
p2p_secret_key: None,
Expand Down Expand Up @@ -368,7 +363,6 @@ impl Default for DiscoveryArgs {
mod tests {
use super::*;
use clap::Parser;
use reth_network_types::RetryStrategy;
/// A helper type to parse Args more easily
#[derive(Parser)]
struct CommandParser<T: Args> {
Expand Down Expand Up @@ -426,37 +420,17 @@ mod tests {

#[test]
fn parse_retry_strategy_args() {
let tests = vec![
(
"1",
"0",
Some(RetryStrategy { interval: core::time::Duration::from_millis(1), attempts: 0 }),
),
(
"1000",
"10",
Some(RetryStrategy {
interval: core::time::Duration::from_millis(1000),
attempts: 10,
}),
),
];
let tests = vec![0, 10];

for (interval, attempts, expected) in tests {
for retries in tests {
let args = CommandParser::<NetworkArgs>::parse_from([
"reth",
"--retry-millis",
interval,
"--retry-attempts",
attempts,
"--dns-retries",
retries.to_string().as_str(),
])
.args;

let found = Some(RetryStrategy::new(
core::time::Duration::from_millis(args.retry_millis),
args.retry_attempts,
));
assert_eq!(found, expected);
assert_eq!(args.dns_retries, retries);
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/node/builder/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ reth-rpc-layer.workspace = true
reth-node-api.workspace = true
reth-node-core.workspace = true
reth-network.workspace = true
reth-network-types.workspace = true
reth-primitives.workspace = true
reth-payload-builder.workspace = true
reth-transaction-pool.workspace = true
Expand Down Expand Up @@ -59,6 +58,7 @@ eyre.workspace = true
fdlimit.workspace = true
confy.workspace = true
rayon.workspace = true
backon.workspace = true

[dev-dependencies]
tempfile.workspace = true
Loading

0 comments on commit 83323eb

Please sign in to comment.