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.

Sign up for GitHub

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

refactor: clean-up discv5 configuration #9143

Merged
merged 3 commits into from
Jun 27, 2024
Merged

refactor: clean-up discv5 configuration #9143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8bfd6ec
refactor: clean-up discv5 configuration
klkvr Jun 27, 2024
76016c6
fix: apply instance earlier
klkvr Jun 27, 2024
04a071d
fix
klkvr Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

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

7 changes: 1 addition & 6 deletions bin/reth/src/commands/debug_cmd/execution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use reth_stages::{
};
use reth_static_file::StaticFileProducer;
use reth_tasks::TaskExecutor;
use std::{net::SocketAddr, path::PathBuf, sync::Arc};
use std::{path::PathBuf, sync::Arc};
use tokio::sync::watch;
use tracing::*;

Expand Down Expand Up @@ -130,11 +130,6 @@ impl Command {
.network
.network_config(config, provider_factory.chain_spec(), secret_key, default_peers_path)
.with_task_executor(Box::new(task_executor))
.listener_addr(SocketAddr::new(self.network.addr, self.network.port))
.discovery_addr(SocketAddr::new(
self.network.discovery.addr,
self.network.discovery.port,
))
.build(provider_factory)
.start_network()
.await?;
Expand Down
7 changes: 1 addition & 6 deletions bin/reth/src/commands/debug_cmd/in_memory_merkle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use reth_revm::database::StateProviderDatabase;
use reth_stages::StageId;
use reth_tasks::TaskExecutor;
use reth_trie::{updates::TrieKey, StateRoot};
use std::{net::SocketAddr, path::PathBuf, sync::Arc};
use std::{path::PathBuf, sync::Arc};
use tracing::*;

/// `reth debug in-memory-merkle` command
Expand Down Expand Up @@ -64,11 +64,6 @@ impl Command {
.network
.network_config(config, provider_factory.chain_spec(), secret_key, default_peers_path)
.with_task_executor(Box::new(task_executor))
.listener_addr(SocketAddr::new(self.network.addr, self.network.port))
.discovery_addr(SocketAddr::new(
self.network.discovery.addr,
self.network.discovery.port,
))
.build(provider_factory)
.start_network()
.await?;
Expand Down
7 changes: 1 addition & 6 deletions bin/reth/src/commands/debug_cmd/merkle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use reth_stages::{
ExecInput, Stage, StageCheckpoint,
};
use reth_tasks::TaskExecutor;
use std::{net::SocketAddr, path::PathBuf, sync::Arc};
use std::{path::PathBuf, sync::Arc};
use tracing::*;

/// `reth debug merkle` command
Expand Down Expand Up @@ -69,11 +69,6 @@ impl Command {
.network
.network_config(config, provider_factory.chain_spec(), secret_key, default_peers_path)
.with_task_executor(Box::new(task_executor))
.listener_addr(SocketAddr::new(self.network.addr, self.network.port))
.discovery_addr(SocketAddr::new(
self.network.discovery.addr,
self.network.discovery.port,
))
.build(provider_factory)
.start_network()
.await?;
Expand Down
7 changes: 1 addition & 6 deletions bin/reth/src/commands/debug_cmd/replay_engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use reth_stages::Pipeline;
use reth_static_file::StaticFileProducer;
use reth_tasks::TaskExecutor;
use reth_transaction_pool::noop::NoopTransactionPool;
use std::{net::SocketAddr, path::PathBuf, sync::Arc, time::Duration};
use std::{path::PathBuf, sync::Arc, time::Duration};
use tokio::sync::oneshot;
use tracing::*;

Expand Down Expand Up @@ -65,11 +65,6 @@ impl Command {
.network
.network_config(config, provider_factory.chain_spec(), secret_key, default_peers_path)
.with_task_executor(Box::new(task_executor))
.listener_addr(SocketAddr::new(self.network.addr, self.network.port))
.discovery_addr(SocketAddr::new(
self.network.discovery.addr,
self.network.discovery.port,
))
.build(provider_factory)
.start_network()
.await?;
Expand Down
51 changes: 3 additions & 48 deletions bin/reth/src/commands/p2p/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ use crate::{
args::{
get_secret_key,
utils::{chain_help, chain_value_parser, hash_or_num_value_parser, SUPPORTED_CHAINS},
DatabaseArgs, DiscoveryArgs, NetworkArgs,
DatabaseArgs, NetworkArgs,
},
utils::get_single_header,
};
use backon::{ConstantBuilder, Retryable};
use clap::{Parser, Subcommand};
use discv5::ListenConfig;
use reth_chainspec::ChainSpec;
use reth_config::Config;
use reth_db::create_db;
Expand All @@ -19,11 +18,7 @@ use reth_network_p2p::bodies::client::BodiesClient;
use reth_node_core::args::DatadirArgs;
use reth_primitives::BlockHashOrNumber;
use reth_provider::{providers::StaticFileProvider, ProviderFactory};
use std::{
net::{IpAddr, SocketAddrV4, SocketAddrV6},
path::PathBuf,
sync::Arc,
};
use std::{path::PathBuf, sync::Arc};

/// `reth p2p` command
#[derive(Debug, Parser)]
Expand Down Expand Up @@ -113,47 +108,7 @@ impl Command {
.disable_discv4_discovery_if(self.chain.chain.is_optimism())
.boot_nodes(boot_nodes.clone())
.apply(|builder| {
self.network
.discovery
.apply_to_builder(builder, rlpx_socket)
.map_discv5_config_builder(|builder| {
let DiscoveryArgs {
discv5_addr,
discv5_addr_ipv6,
discv5_port,
discv5_port_ipv6,
discv5_lookup_interval,
discv5_bootstrap_lookup_interval,
discv5_bootstrap_lookup_countdown,
..
} = self.network.discovery;

// Use rlpx address if none given
let discv5_addr_ipv4 = discv5_addr.or(match self.network.addr {
IpAddr::V4(ip) => Some(ip),
IpAddr::V6(_) => None,
});
let discv5_addr_ipv6 = discv5_addr_ipv6.or(match self.network.addr {
IpAddr::V4(_) => None,
IpAddr::V6(ip) => Some(ip),
});

builder
.discv5_config(
discv5::ConfigBuilder::new(ListenConfig::from_two_sockets(
discv5_addr_ipv4
.map(|addr| SocketAddrV4::new(addr, discv5_port)),
discv5_addr_ipv6.map(|addr| {
SocketAddrV6::new(addr, discv5_port_ipv6, 0, 0)
}),
))
.build(),
)
.add_unsigned_boot_nodes(boot_nodes.into_iter())
.lookup_interval(discv5_lookup_interval)
.bootstrap_lookup_interval(discv5_bootstrap_lookup_interval)
.bootstrap_lookup_countdown(discv5_bootstrap_lookup_countdown)
})
self.network.discovery.apply_to_builder(builder, rlpx_socket, boot_nodes)
})
.build(Arc::new(ProviderFactory::new(
noop_db,
Expand Down
41 changes: 10 additions & 31 deletions crates/net/network/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,36 +411,6 @@ impl NetworkConfigBuilder {
}
}

/// Calls a closure on [`reth_discv5::ConfigBuilder`], if discv5 discovery is enabled and the
/// builder has been set.
/// ```
/// use reth_chainspec::MAINNET;
/// use reth_network::NetworkConfigBuilder;
/// use reth_provider::test_utils::NoopProvider;
/// use secp256k1::{rand::thread_rng, SecretKey};
///
/// let sk = SecretKey::new(&mut thread_rng());
/// let fork_id = MAINNET.latest_fork_id();
/// let network_config = NetworkConfigBuilder::new(sk)
/// .map_discv5_config_builder(|builder| builder.fork(b"eth", fork_id))
/// .build(NoopProvider::default());
/// ```
pub fn map_discv5_config_builder(
mut self,
f: impl FnOnce(reth_discv5::ConfigBuilder) -> reth_discv5::ConfigBuilder,
) -> Self {
if let Some(mut builder) = self.discovery_v5_builder {
if let Some(network_stack_id) = NetworkStackId::id(&self.chain_spec) {
let fork_id = self.chain_spec.latest_fork_id();
builder = builder.fork(network_stack_id, fork_id);
}

self.discovery_v5_builder = Some(f(builder));
}

self
}

/// Adds a new additional protocol to the `RLPx` sub-protocol list.
pub fn add_rlpx_sub_protocol(mut self, protocol: impl IntoRlpxSubProtocol) -> Self {
self.extra_protocols.push(protocol);
Expand Down Expand Up @@ -480,7 +450,7 @@ impl NetworkConfigBuilder {
secret_key,
mut dns_discovery_config,
discovery_v4_builder,
discovery_v5_builder,
mut discovery_v5_builder,
boot_nodes,
discovery_addr,
listener_addr,
Expand All @@ -497,6 +467,15 @@ impl NetworkConfigBuilder {
transactions_manager_config,
} = self;

discovery_v5_builder = discovery_v5_builder.map(|mut builder| {
if let Some(network_stack_id) = NetworkStackId::id(&chain_spec) {
let fork_id = chain_spec.latest_fork_id();
builder = builder.fork(network_stack_id, fork_id)
}

builder
});

let listener_addr = listener_addr.unwrap_or(DEFAULT_DISCOVERY_ADDRESS);

let mut hello_message =
Expand Down
101 changes: 80 additions & 21 deletions crates/node-core/src/args/network.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use crate::version::P2P_CLIENT_VERSION;
use clap::Args;
use reth_chainspec::{net::mainnet_nodes, ChainSpec};
use reth_config::Config;
use reth_discv4::{DEFAULT_DISCOVERY_ADDR, DEFAULT_DISCOVERY_PORT};
use reth_discv4::{NodeRecord, DEFAULT_DISCOVERY_ADDR, DEFAULT_DISCOVERY_PORT};
use reth_discv5::{
DEFAULT_COUNT_BOOTSTRAP_LOOKUPS, DEFAULT_DISCOVERY_V5_PORT,
discv5::ListenConfig, DEFAULT_COUNT_BOOTSTRAP_LOOKUPS, DEFAULT_DISCOVERY_V5_PORT,
DEFAULT_SECONDS_BOOTSTRAP_LOOKUP_INTERVAL, DEFAULT_SECONDS_LOOKUP_INTERVAL,
};
use reth_net_nat::NatResolver;
Expand All @@ -21,7 +21,7 @@ use reth_network::{
use reth_network_peers::TrustedPeer;
use secp256k1::SecretKey;
use std::{
net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr},
net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6},
ops::Not,
path::PathBuf,
sync::Arc,
Expand Down Expand Up @@ -173,23 +173,17 @@ impl NetworkArgs {
// apply discovery settings
.apply(|builder| {
let rlpx_socket = (self.addr, self.port).into();
self.discovery.apply_to_builder(builder, rlpx_socket)
})
// modify discv5 settings if enabled in previous step
.map_discv5_config_builder(|builder| {
let DiscoveryArgs {
discv5_lookup_interval,
discv5_bootstrap_lookup_interval,
discv5_bootstrap_lookup_countdown,
..
} = self.discovery;

builder
.add_unsigned_boot_nodes(chain_bootnodes)
.lookup_interval(discv5_lookup_interval)
.bootstrap_lookup_interval(discv5_bootstrap_lookup_interval)
.bootstrap_lookup_countdown(discv5_bootstrap_lookup_countdown)
self.discovery.apply_to_builder(builder, rlpx_socket, chain_bootnodes)
})
.listener_addr(SocketAddr::new(
self.addr, // set discovery port based on instance number
self.port,
))
.discovery_addr(SocketAddr::new(
self.discovery.addr,
// set discovery port based on instance number
self.discovery.port,
))
}

/// If `no_persist_peers` is false then this returns the path to the persistent peers file path.
Expand All @@ -211,6 +205,17 @@ impl NetworkArgs {
self.discovery = self.discovery.with_unused_discovery_port();
self
}

/// Change networking port numbers based on the instance number.
/// Ports are updated to `previous_value + instance - 1`
///
/// # Panics
/// Warning: if `instance` is zero in debug mode, this will panic.
pub fn adjust_instance_ports(&mut self, instance: u16) {
debug_assert_ne!(instance, 0, "instance must be non-zero");
self.port += instance - 1;
self.discovery.adjust_instance_ports(instance);
}
}

impl Default for NetworkArgs {
Expand Down Expand Up @@ -309,6 +314,7 @@ impl DiscoveryArgs {
&self,
mut network_config_builder: NetworkConfigBuilder,
rlpx_tcp_socket: SocketAddr,
boot_nodes: impl IntoIterator<Item = NodeRecord>,
) -> NetworkConfigBuilder {
if self.disable_discovery || self.disable_dns_discovery {
network_config_builder = network_config_builder.disable_dns_discovery();
Expand All @@ -319,19 +325,72 @@ impl DiscoveryArgs {
}

if !self.disable_discovery && self.enable_discv5_discovery {
network_config_builder =
network_config_builder.discovery_v5(reth_discv5::Config::builder(rlpx_tcp_socket));
network_config_builder = network_config_builder
.discovery_v5(self.discovery_v5_builder(rlpx_tcp_socket, boot_nodes));
}

network_config_builder
}

/// Creates a [`reth_discv5::ConfigBuilder`] filling it with the values from this struct.
pub fn discovery_v5_builder(
&self,
rlpx_tcp_socket: SocketAddr,
boot_nodes: impl IntoIterator<Item = NodeRecord>,
) -> reth_discv5::ConfigBuilder {
let Self {
discv5_addr,
discv5_addr_ipv6,
discv5_port,
discv5_port_ipv6,
discv5_lookup_interval,
discv5_bootstrap_lookup_interval,
discv5_bootstrap_lookup_countdown,
..
} = self;

// Use rlpx address if none given
let discv5_addr_ipv4 = discv5_addr.or(match rlpx_tcp_socket {
SocketAddr::V4(addr) => Some(*addr.ip()),
SocketAddr::V6(_) => None,
});
let discv5_addr_ipv6 = discv5_addr_ipv6.or(match rlpx_tcp_socket {
SocketAddr::V4(_) => None,
SocketAddr::V6(addr) => Some(*addr.ip()),
});

reth_discv5::Config::builder(rlpx_tcp_socket)
.discv5_config(
reth_discv5::discv5::ConfigBuilder::new(ListenConfig::from_two_sockets(
discv5_addr_ipv4.map(|addr| SocketAddrV4::new(addr, *discv5_port)),
discv5_addr_ipv6.map(|addr| SocketAddrV6::new(addr, *discv5_port_ipv6, 0, 0)),
))
.build(),
)
.add_unsigned_boot_nodes(boot_nodes)
.lookup_interval(*discv5_lookup_interval)
.bootstrap_lookup_interval(*discv5_bootstrap_lookup_interval)
.bootstrap_lookup_countdown(*discv5_bootstrap_lookup_countdown)
}
Comment on lines +335 to +374
Copy link
Member

Choose a reason for hiding this comment

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

nice, I wanted smthg like this from the start but de-prioritised it, happy it's implemented now


/// Set the discovery port to zero, to allow the OS to assign a random unused port when
/// discovery binds to the socket.
pub const fn with_unused_discovery_port(mut self) -> Self {
self.port = 0;
self
}

/// Change networking port numbers based on the instance number.
/// Ports are updated to `previous_value + instance - 1`
///
/// # Panics
/// Warning: if `instance` is zero in debug mode, this will panic.
pub fn adjust_instance_ports(&mut self, instance: u16) {
debug_assert_ne!(instance, 0, "instance must be non-zero");
self.port += instance - 1;
self.discv5_port += instance - 1;
self.discv5_port_ipv6 += instance - 1;
}
}

impl Default for DiscoveryArgs {
Expand Down
1 change: 1 addition & 0 deletions crates/node-core/src/node_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ impl NodeConfig {
/// [`RpcServerArgs::adjust_instance_ports`] method.
pub fn adjust_instance_ports(&mut self) {
self.rpc.adjust_instance_ports(self.instance);
self.network.adjust_instance_ports(self.instance);
}

/// Sets networking and RPC ports to zero, causing the OS to choose random unused ports when
Expand Down
Loading
Loading