From 9da18d47704bb3eb20fab7c503e5246feb4a40c5 Mon Sep 17 00:00:00 2001 From: Roland Sherwin Date: Thu, 6 Jun 2024 14:04:01 +0530 Subject: [PATCH 1/2] refactor(peers): make peer acquisition function into a method --- sn_auditor/src/main.rs | 5 +- sn_cli/src/bin/main.rs | 3 +- sn_faucet/src/main.rs | 4 +- sn_node/src/bin/safenode/main.rs | 4 +- sn_node_manager/src/cmd/auditor.rs | 6 +- sn_node_manager/src/cmd/faucet.rs | 6 +- sn_node_manager/src/cmd/local.rs | 6 +- sn_node_manager/src/cmd/node.rs | 8 +-- sn_peers_acquisition/src/lib.rs | 110 +++++++++++++++-------------- 9 files changed, 76 insertions(+), 76 deletions(-) diff --git a/sn_auditor/src/main.rs b/sn_auditor/src/main.rs index 4398a17c14..02932a17a4 100644 --- a/sn_auditor/src/main.rs +++ b/sn_auditor/src/main.rs @@ -18,7 +18,6 @@ use color_eyre::eyre::{eyre, Result}; use dag_db::SpendDagDb; use sn_client::Client; use sn_logging::{Level, LogBuilder, LogFormat, LogOutputDest}; -use sn_peers_acquisition::get_peers_from_args; use sn_peers_acquisition::PeersArgs; use std::collections::BTreeSet; use std::path::PathBuf; @@ -138,8 +137,8 @@ fn logging_init( Ok(log_builder) } -async fn connect_to_network(peers: PeersArgs) -> Result { - let bootstrap_peers = get_peers_from_args(peers).await?; +async fn connect_to_network(peers_args: PeersArgs) -> Result { + let bootstrap_peers = peers_args.get_peers().await?; println!( "Connecting to the network with {} bootstrap peers", bootstrap_peers.len(), diff --git a/sn_cli/src/bin/main.rs b/sn_cli/src/bin/main.rs index 347e4d2465..f42beec76a 100644 --- a/sn_cli/src/bin/main.rs +++ b/sn_cli/src/bin/main.rs @@ -30,7 +30,6 @@ use sn_client::transfers::bls_secret_from_hex; use sn_client::{Client, ClientEvent, ClientEventsBroadcaster, ClientEventsReceiver}; #[cfg(feature = "metrics")] use sn_logging::{metrics::init_metrics, Level, LogBuilder, LogFormat}; -use sn_peers_acquisition::get_peers_from_args; use std::{io, path::PathBuf, time::Duration}; use tokio::{sync::broadcast::error::RecvError, task::JoinHandle}; @@ -101,7 +100,7 @@ async fn main() -> Result<()> { println!("Instantiating a SAFE client..."); let secret_key = get_client_secret_key(&client_data_dir_path)?; - let bootstrap_peers = get_peers_from_args(opt.peers).await?; + let bootstrap_peers = opt.peers.get_peers().await?; println!( "Connecting to the network with {} peers", diff --git a/sn_faucet/src/main.rs b/sn_faucet/src/main.rs index 31b6f884f1..a1552a4831 100644 --- a/sn_faucet/src/main.rs +++ b/sn_faucet/src/main.rs @@ -21,7 +21,7 @@ use sn_client::{ Client, ClientEvent, ClientEventsBroadcaster, ClientEventsReceiver, }; use sn_logging::{Level, LogBuilder, LogOutputDest}; -use sn_peers_acquisition::{get_peers_from_args, PeersArgs}; +use sn_peers_acquisition::PeersArgs; use sn_transfers::{get_faucet_data_dir, HotWallet, MainPubkey, NanoTokens, Transfer}; use std::{path::PathBuf, time::Duration}; use tokio::{sync::broadcast::error::RecvError, task::JoinHandle}; @@ -31,7 +31,7 @@ use tracing::{debug, error, info}; async fn main() -> Result<()> { let opt = Opt::parse(); - let bootstrap_peers = get_peers_from_args(opt.peers).await?; + let bootstrap_peers = opt.peers.get_peers().await?; let bootstrap_peers = if bootstrap_peers.is_empty() { // empty vec is returned if `local-discovery` flag is provided None diff --git a/sn_node/src/bin/safenode/main.rs b/sn_node/src/bin/safenode/main.rs index 54e427458c..e3bd77dd20 100644 --- a/sn_node/src/bin/safenode/main.rs +++ b/sn_node/src/bin/safenode/main.rs @@ -18,7 +18,7 @@ use libp2p::{identity::Keypair, PeerId}; use sn_logging::metrics::init_metrics; use sn_logging::{Level, LogFormat, LogOutputDest, ReloadHandle}; use sn_node::{Marker, NodeBuilder, NodeEvent, NodeEventsReceiver}; -use sn_peers_acquisition::{get_peers_from_args, PeersArgs}; +use sn_peers_acquisition::PeersArgs; use sn_protocol::{node::get_safenode_root_dir, node_rpc::NodeCtrl}; use std::{ env, @@ -190,7 +190,7 @@ fn main() -> Result<()> { init_logging(&opt, keypair.public().to_peer_id())?; let rt = Runtime::new()?; - let bootstrap_peers = rt.block_on(get_peers_from_args(opt.peers))?; + let bootstrap_peers = rt.block_on(opt.peers.get_peers())?; let msg = format!( "Running {} v{}", env!("CARGO_BIN_NAME"), diff --git a/sn_node_manager/src/cmd/auditor.rs b/sn_node_manager/src/cmd/auditor.rs index 359d8cd6d9..7c2f6bec32 100644 --- a/sn_node_manager/src/cmd/auditor.rs +++ b/sn_node_manager/src/cmd/auditor.rs @@ -16,7 +16,7 @@ use crate::{ use color_eyre::{eyre::eyre, Result}; use colored::Colorize; use semver::Version; -use sn_peers_acquisition::{get_peers_from_args, PeersArgs}; +use sn_peers_acquisition::PeersArgs; use sn_releases::{ReleaseType, SafeReleaseRepoActions}; use sn_service_management::{ auditor::AuditorService, @@ -30,7 +30,7 @@ pub async fn add( beta_encryption_key: Option, env_variables: Option>, log_dir_path: Option, - peers: PeersArgs, + peers_args: PeersArgs, src_path: Option, url: Option, version: Option, @@ -79,7 +79,7 @@ pub async fn add( auditor_src_bin_path, auditor_install_bin_path: PathBuf::from("/usr/local/bin/auditor"), beta_encryption_key, - bootstrap_peers: get_peers_from_args(peers).await?, + bootstrap_peers: peers_args.get_peers().await?, env_variables, service_log_dir_path, user: service_user.to_string(), diff --git a/sn_node_manager/src/cmd/faucet.rs b/sn_node_manager/src/cmd/faucet.rs index add34c5d2f..35267a7382 100644 --- a/sn_node_manager/src/cmd/faucet.rs +++ b/sn_node_manager/src/cmd/faucet.rs @@ -16,7 +16,7 @@ use crate::{ use color_eyre::{eyre::eyre, Result}; use colored::Colorize; use semver::Version; -use sn_peers_acquisition::{get_peers_from_args, PeersArgs}; +use sn_peers_acquisition::PeersArgs; use sn_releases::{ReleaseType, SafeReleaseRepoActions}; use sn_service_management::{ control::{ServiceControl, ServiceController}, @@ -28,7 +28,7 @@ use std::path::PathBuf; pub async fn add( env_variables: Option>, log_dir_path: Option, - peers: PeersArgs, + peers_args: PeersArgs, src_path: Option, url: Option, version: Option, @@ -74,7 +74,7 @@ pub async fn add( info!("Adding faucet service"); add_faucet( AddFaucetServiceOptions { - bootstrap_peers: get_peers_from_args(peers).await?, + bootstrap_peers: peers_args.get_peers().await?, env_variables, faucet_src_bin_path, faucet_install_bin_path: PathBuf::from("/usr/local/bin/faucet"), diff --git a/sn_node_manager/src/cmd/local.rs b/sn_node_manager/src/cmd/local.rs index 3b9cb435c8..ccef08613c 100644 --- a/sn_node_manager/src/cmd/local.rs +++ b/sn_node_manager/src/cmd/local.rs @@ -15,7 +15,7 @@ use crate::{ }; use color_eyre::{eyre::eyre, Help, Report, Result}; use sn_logging::LogFormat; -use sn_peers_acquisition::{get_peers_from_args, PeersArgs}; +use sn_peers_acquisition::PeersArgs; use sn_releases::{ReleaseType, SafeReleaseRepoActions}; use sn_service_management::{ control::ServiceController, get_local_node_registry_path, NodeRegistry, @@ -33,7 +33,7 @@ pub async fn join( log_format: Option, owner: Option, owner_prefix: Option, - peers: PeersArgs, + peers_args: PeersArgs, skip_validation: bool, verbosity: VerbosityLevel, ) -> Result<(), Report> { @@ -67,7 +67,7 @@ pub async fn join( // If no peers are obtained we will attempt to join the existing local network, if one // is running. - let peers = match get_peers_from_args(peers).await { + let peers = match peers_args.get_peers().await { Ok(peers) => Some(peers), Err(err) => match err { sn_peers_acquisition::error::Error::PeersNotObtained => { diff --git a/sn_node_manager/src/cmd/node.rs b/sn_node_manager/src/cmd/node.rs index c90c7edaed..394472528a 100644 --- a/sn_node_manager/src/cmd/node.rs +++ b/sn_node_manager/src/cmd/node.rs @@ -23,7 +23,7 @@ use colored::Colorize; use libp2p_identity::PeerId; use semver::Version; use sn_logging::LogFormat; -use sn_peers_acquisition::{get_peers_from_args, PeersArgs}; +use sn_peers_acquisition::PeersArgs; use sn_releases::{ReleaseType, SafeReleaseRepoActions}; use sn_service_management::{ control::{ServiceControl, ServiceController}, @@ -49,7 +49,7 @@ pub async fn add( metrics_port: Option, node_port: Option, owner: Option, - peers: PeersArgs, + peers_args: PeersArgs, rpc_address: Option, rpc_port: Option, src_path: Option, @@ -113,8 +113,8 @@ pub async fn add( // // It's simpler to not use the `network-contacts` feature in the node manager, because it can // return a huge peer list, and that's been problematic for service definition files. - let is_first = peers.first; - let bootstrap_peers = match get_peers_from_args(peers).await { + let is_first = peers_args.first; + let bootstrap_peers = match peers_args.get_peers().await { Ok(p) => { info!("Obtained peers of length {}", p.len()); p diff --git a/sn_peers_acquisition/src/lib.rs b/sn_peers_acquisition/src/lib.rs index 40509effbe..25f9624fd0 100644 --- a/sn_peers_acquisition/src/lib.rs +++ b/sn_peers_acquisition/src/lib.rs @@ -67,67 +67,69 @@ pub struct PeersArgs { pub network_contacts_url: Option, } -/// Gets the peers based on the arguments provided. -/// -/// If the `--first` flag is used, no peers will be provided. -/// -/// Otherwise, peers are obtained in the following order of precedence: -/// * The `--peer` argument. -/// * The `SAFE_PEERS` environment variable. -/// * Using the `local-discovery` feature, which will return an empty peer list. -/// * Using the `network-contacts` feature, which will download the peer list from a file on S3. -/// -/// Note: the current behaviour is that `--peer` and `SAFE_PEERS` will be combined. Some tests -/// currently rely on this. We will change it soon. -pub async fn get_peers_from_args(args: PeersArgs) -> Result> { - if args.first { - return Ok(vec![]); - } +impl PeersArgs { + /// Gets the peers based on the arguments provided. + /// + /// If the `--first` flag is used, no peers will be provided. + /// + /// Otherwise, peers are obtained in the following order of precedence: + /// * The `--peer` argument. + /// * The `SAFE_PEERS` environment variable. + /// * Using the `local-discovery` feature, which will return an empty peer list. + /// * Using the `network-contacts` feature, which will download the peer list from a file on S3. + /// + /// Note: the current behaviour is that `--peer` and `SAFE_PEERS` will be combined. Some tests + /// currently rely on this. We will change it soon. + pub async fn get_peers(self) -> Result> { + if self.first { + return Ok(vec![]); + } - let mut peers = if !args.peers.is_empty() { - info!("Using peers supplied with the --peer argument(s) or SAFE_PEERS"); - args.peers - } else if cfg!(feature = "local-discovery") { - info!("No peers given"); - info!( + let mut peers = if !self.peers.is_empty() { + info!("Using peers supplied with the --peer argument(s) or SAFE_PEERS"); + self.peers + } else if cfg!(feature = "local-discovery") { + info!("No peers given"); + info!( "The `local-discovery` feature is enabled, so peers will be discovered through mDNS." ); - return Ok(vec![]); - } else if cfg!(feature = "network-contacts") { - get_network_contacts(&args).await? - } else { - vec![] - }; - - if peers.is_empty() { - error!("Peers not obtained through any available options"); - return Err(Error::PeersNotObtained); - }; - - // Randomly sort peers before we return them to avoid overly hitting any one peer - let mut rng = thread_rng(); - peers.shuffle(&mut rng); - - Ok(peers) -} + return Ok(vec![]); + } else if cfg!(feature = "network-contacts") { + self.get_network_contacts().await? + } else { + vec![] + }; + + if peers.is_empty() { + error!("Peers not obtained through any available options"); + return Err(Error::PeersNotObtained); + }; + + // Randomly sort peers before we return them to avoid overly hitting any one peer + let mut rng = thread_rng(); + peers.shuffle(&mut rng); + + Ok(peers) + } -// should not be reachable, but needed for the compiler to be happy. -#[allow(clippy::unused_async)] -#[cfg(not(feature = "network-contacts"))] -async fn get_network_contacts(_args: &PeersArgs) -> Result> { - Ok(vec![]) -} + // should not be reachable, but needed for the compiler to be happy. + #[allow(clippy::unused_async)] + #[cfg(not(feature = "network-contacts"))] + async fn get_network_contacts(&self) -> Result> { + Ok(vec![]) + } -#[cfg(feature = "network-contacts")] -async fn get_network_contacts(args: &PeersArgs) -> Result> { - let url = args - .network_contacts_url - .clone() - .unwrap_or(Url::parse(NETWORK_CONTACTS_URL.as_str())?); + #[cfg(feature = "network-contacts")] + async fn get_network_contacts(&self) -> Result> { + let url = self + .network_contacts_url + .clone() + .unwrap_or(Url::parse(NETWORK_CONTACTS_URL.as_str())?); - info!("Trying to fetch the bootstrap peers from {url}"); + info!("Trying to fetch the bootstrap peers from {url}"); - get_bootstrap_peers_from_url(url).await + get_bootstrap_peers_from_url(url).await + } } /// Parse strings like `1.2.3.4:1234` and `/ip4/1.2.3.4/tcp/1234` into a multiaddr. From cd6e2a7fdeab350ea82e6007f1cc00a3fc5b2ace Mon Sep 17 00:00:00 2001 From: Roland Sherwin Date: Thu, 6 Jun 2024 14:19:56 +0530 Subject: [PATCH 2/2] feat(manager): don't fetch peers from network-contact file --- sn_node_manager/src/cmd/node.rs | 23 ++++++++++++----------- sn_peers_acquisition/src/lib.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/sn_node_manager/src/cmd/node.rs b/sn_node_manager/src/cmd/node.rs index 394472528a..05f3fa518c 100644 --- a/sn_node_manager/src/cmd/node.rs +++ b/sn_node_manager/src/cmd/node.rs @@ -106,18 +106,19 @@ pub async fn add( // Handle the `PeersNotObtained` error to make the `--peer` argument optional for the node // manager. // - // Since we don't use the `network-contacts` feature in the node manager, the `PeersNotObtained` - // error should occur when no `--peer` arguments were used. If the `safenode` binary we're using - // has `network-contacts` enabled (which is the case for released binaries), it's fine if the - // service definition doesn't call `safenode` with a `--peer` argument. - // - // It's simpler to not use the `network-contacts` feature in the node manager, because it can - // return a huge peer list, and that's been problematic for service definition files. + // Since any application making use of the node manager can enable the `network-contacts` feature on + // sn_peers_acquisition, we might end up getting having a huge peer list, and that's problematic for + // service definition files. + // Thus make use of get_peers_exclude_network_contacts() instead of get_peers() to make sure we only + // parse the --peers and SAFE_PEERS env var. + + // If the `safenode` binary we're using has `network-contacts` enabled (which is the case for released binaries), + // it's fine if the service definition doesn't call `safenode` with a `--peer` argument. let is_first = peers_args.first; - let bootstrap_peers = match peers_args.get_peers().await { - Ok(p) => { - info!("Obtained peers of length {}", p.len()); - p + let bootstrap_peers = match peers_args.get_peers_exclude_network_contacts().await { + Ok(peers) => { + info!("Obtained peers of length {}", peers.len()); + peers } Err(err) => match err { sn_peers_acquisition::error::Error::PeersNotObtained => { diff --git a/sn_peers_acquisition/src/lib.rs b/sn_peers_acquisition/src/lib.rs index 25f9624fd0..1d358b9993 100644 --- a/sn_peers_acquisition/src/lib.rs +++ b/sn_peers_acquisition/src/lib.rs @@ -81,7 +81,30 @@ impl PeersArgs { /// Note: the current behaviour is that `--peer` and `SAFE_PEERS` will be combined. Some tests /// currently rely on this. We will change it soon. pub async fn get_peers(self) -> Result> { + self.get_peers_inner(false).await + } + + /// Gets the peers based on the arguments provided. + /// + /// If the `--first` flag is used, no peers will be provided. + /// + /// Otherwise, peers are obtained in the following order of precedence: + /// * The `--peer` argument. + /// * The `SAFE_PEERS` environment variable. + /// * Using the `local-discovery` feature, which will return an empty peer list. + /// + /// This will not fetch the peers from network-contacts even if the `network-contacts` feature is enabled. Use + /// get_peers() instead. + /// + /// Note: the current behaviour is that `--peer` and `SAFE_PEERS` will be combined. Some tests + /// currently rely on this. We will change it soon. + pub async fn get_peers_exclude_network_contacts(self) -> Result> { + self.get_peers_inner(true).await + } + + async fn get_peers_inner(self, skip_network_contacts: bool) -> Result> { if self.first { + info!("First node in a new network"); return Ok(vec![]); } @@ -94,6 +117,9 @@ impl PeersArgs { "The `local-discovery` feature is enabled, so peers will be discovered through mDNS." ); return Ok(vec![]); + } else if skip_network_contacts { + info!("Skipping network contacts"); + return Ok(vec![]); } else if cfg!(feature = "network-contacts") { self.get_network_contacts().await? } else {