From b17a53615a574c3d8dc075669c24f6b8c0eb0ebc Mon Sep 17 00:00:00 2001 From: Benno Zeeman Date: Tue, 16 Jul 2024 14:48:13 +0200 Subject: [PATCH] feat!: limit error surface In spirit of streamlining our public APIs, it's important to make distinctions between errors. Our errors are currently grouped into big error types, while many functions only return a handful of errors, or even just one. This is inconvenient to handle. BREAKING CHANGE: added/changed an pub enum variant --- sn_client/src/api.rs | 19 +++++++++++++------ sn_client/src/error.rs | 3 --- sn_networking/src/driver.rs | 33 +++++++++++++++++++++++---------- sn_networking/src/error.rs | 7 +++---- 4 files changed, 39 insertions(+), 23 deletions(-) diff --git a/sn_client/src/api.rs b/sn_client/src/api.rs index f7aaf74d04..05ec3e67ff 100644 --- a/sn_client/src/api.rs +++ b/sn_client/src/api.rs @@ -57,6 +57,13 @@ pub const CONNECTION_TIMEOUT: Duration = Duration::from_secs(30); /// The timeout duration for the client to receive any response from the network. const INACTIVITY_TIMEOUT: Duration = Duration::from_secs(30); +// Init during compilation, instead of runtime error that should never happen +// Option::expect will be stabilised as const in the future (https://github.com/rust-lang/rust/issues/67441) +const QUORUM_2: NonZeroUsize = match NonZeroUsize::new(2) { + Some(v) => v, + None => panic!("2 is not be zero"), +}; + impl Client { /// A quick client with a random secret key and some peers. pub async fn quick_start(peers: Option>) -> Result { @@ -114,7 +121,9 @@ impl Client { #[cfg(feature = "open-metrics")] network_builder.metrics_registry(Some(Registry::default())); - let (network, mut network_event_receiver, swarm_driver) = network_builder.build_client()?; + let (network, mut network_event_receiver, swarm_driver) = network_builder + .build_client() + .map_err(NetworkError::MdnsBuildBehaviourError)?; info!("Client constructed network and swarm_driver"); // If the events broadcaster is not provided by the caller, then we create a new one. @@ -450,7 +459,7 @@ impl Client { ) -> Result { let key = NetworkAddress::from_register_address(address).to_record_key(); let get_quorum = if is_verifying { - Quorum::N(NonZeroUsize::new(2).ok_or(Error::NonZeroUsizeWasInitialisedAsZero)?) + Quorum::N(QUORUM_2) } else { Quorum::One }; @@ -651,9 +660,7 @@ impl Client { let verification = if verify_store { let verification_cfg = GetRecordCfg { - get_quorum: Quorum::N( - NonZeroUsize::new(2).ok_or(Error::NonZeroUsizeWasInitialisedAsZero)?, - ), + get_quorum: Quorum::N(QUORUM_2), retry_strategy, target_record: None, // Not used since we use ChunkProof expected_holders: Default::default(), @@ -768,7 +775,7 @@ impl Client { address.clone(), random_nonce, expected_proof, - Quorum::N(NonZeroUsize::new(2).ok_or(Error::NonZeroUsizeWasInitialisedAsZero)?), + Quorum::N(QUORUM_2), None, ) .await diff --git a/sn_client/src/error.rs b/sn_client/src/error.rs index adbe0ef884..50ce6525e1 100644 --- a/sn_client/src/error.rs +++ b/sn_client/src/error.rs @@ -90,9 +90,6 @@ pub enum Error { #[error("Total price exceed possible token amount")] TotalPriceTooHigh, - #[error("Logic error: NonZeroUsize was initialised as zero")] - NonZeroUsizeWasInitialisedAsZero, - #[error("Could not connect to the network in {0:?}")] ConnectionTimeout(Duration), diff --git a/sn_networking/src/driver.rs b/sn_networking/src/driver.rs index 50476fcf06..86dd6a373c 100644 --- a/sn_networking/src/driver.rs +++ b/sn_networking/src/driver.rs @@ -117,6 +117,13 @@ const NETWORKING_CHANNEL_SIZE: usize = 10_000; /// Time before a Kad query times out if no response is received const KAD_QUERY_TIMEOUT_S: Duration = Duration::from_secs(10); +// Init during compilation, instead of runtime error that should never happen +// Option::expect will be stabilised as const in the future (https://github.com/rust-lang/rust/issues/67441) +const REPLICATION_FACTOR: NonZeroUsize = match NonZeroUsize::new(CLOSE_GROUP_SIZE) { + Some(v) => v, + None => panic!("CLOSE_GROUP_SIZE should not be zero"), +}; + /// The various settings to apply to when fetching a record from network #[derive(Clone)] pub struct GetRecordCfg { @@ -305,10 +312,7 @@ impl NetworkBuilder { // 1mb packet size .set_max_packet_size(MAX_PACKET_SIZE) // How many nodes _should_ store data. - .set_replication_factor( - NonZeroUsize::new(CLOSE_GROUP_SIZE) - .ok_or_else(|| NetworkError::InvalidCloseGroupSize)?, - ) + .set_replication_factor(REPLICATION_FACTOR) .set_query_timeout(KAD_QUERY_TIMEOUT_S) // Require iterative queries to use disjoint paths for increased resiliency in the presence of potentially adversarial nodes. .disjoint_query_paths(true) @@ -377,7 +381,12 @@ impl NetworkBuilder { } /// Same as `build_node` API but creates the network components in client mode - pub fn build_client(self) -> Result<(Network, mpsc::Receiver, SwarmDriver)> { + pub fn build_client( + self, + ) -> std::result::Result< + (Network, mpsc::Receiver, SwarmDriver), + MdnsBuildBehaviourError, + > { // Create a Kademlia behaviour for client mode, i.e. set req/resp protocol // to outbound-only mode and don't listen on any address let mut kad_cfg = kad::Config::default(); // default query timeout is 60 secs @@ -389,10 +398,7 @@ impl NetworkBuilder { // Require iterative queries to use disjoint paths for increased resiliency in the presence of potentially adversarial nodes. .disjoint_query_paths(true) // How many nodes _should_ store data. - .set_replication_factor( - NonZeroUsize::new(CLOSE_GROUP_SIZE) - .ok_or_else(|| NetworkError::InvalidCloseGroupSize)?, - ); + .set_replication_factor(REPLICATION_FACTOR); let (network, net_event_recv, driver) = self.build( kad_cfg, @@ -416,7 +422,10 @@ impl NetworkBuilder { req_res_protocol: ProtocolSupport, identify_version: String, #[cfg(feature = "upnp")] upnp: bool, - ) -> Result<(Network, mpsc::Receiver, SwarmDriver)> { + ) -> std::result::Result< + (Network, mpsc::Receiver, SwarmDriver), + MdnsBuildBehaviourError, + > { let peer_id = PeerId::from(self.keypair.public()); // vdash metric (if modified please notify at https://github.com/happybeing/vdash/issues): #[cfg(not(target_arch = "wasm32"))] @@ -881,3 +890,7 @@ impl SwarmDriver { Ok(()) } } + +#[derive(Debug, thiserror::Error)] +#[error("building the mDNS behaviour failed: {0}")] +pub struct MdnsBuildBehaviourError(#[from] std::io::Error); diff --git a/sn_networking/src/error.rs b/sn_networking/src/error.rs index 103a79a9a3..5b413547ce 100644 --- a/sn_networking/src/error.rs +++ b/sn_networking/src/error.rs @@ -24,6 +24,8 @@ use thiserror::Error; use tokio::sync::oneshot; use xor_name::XorName; +use crate::driver::MdnsBuildBehaviourError; + pub(super) type Result = std::result::Result; /// GetRecord Query errors @@ -155,9 +157,6 @@ pub enum NetworkError { #[error("Could not get enough peers ({required}) to satisfy the request, found {found}")] NotEnoughPeers { found: usize, required: usize }, - #[error("Close group size must be a non-zero usize")] - InvalidCloseGroupSize, - #[error("Node Listen Address was not provided during construction")] ListenAddressNotProvided, @@ -185,7 +184,7 @@ pub enum NetworkError { OutgoingResponseDropped(Response), #[error("Error setting up behaviour: {0}")] - BahviourErr(String), + MdnsBuildBehaviourError(#[from] MdnsBuildBehaviourError), } #[cfg(test)]