diff --git a/node/bft/events/src/disconnect.rs b/node/bft/events/src/disconnect.rs index 720e394d62..e5e5012acf 100644 --- a/node/bft/events/src/disconnect.rs +++ b/node/bft/events/src/disconnect.rs @@ -15,20 +15,34 @@ use super::*; -use std::io; +use tracing::warn; /// The reason behind the node disconnecting from a peer. #[derive(Copy, Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] #[repr(u8)] pub enum DisconnectReason { /// The peer's challenge response is invalid. - InvalidChallengeResponse, + InvalidChallengeResponse = 0, /// No reason given. - NoReasonGiven, + NoReasonGiven = 1, /// The peer is not following the protocol. - ProtocolViolation, + ProtocolViolation = 2, /// The peer's client is outdated, judging by its version. - OutdatedClientVersion, + OutdatedClientVersion = 3, + /// The disconnect reason is not known. This is used for when the peers sends a disconnect reason that is not known to us. + UnknownReason = u8::MAX, +} + +impl std::fmt::Display for DisconnectReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::InvalidChallengeResponse => write!(f, "invalid challenge response"), + Self::NoReasonGiven => write!(f, "no reason given"), + Self::ProtocolViolation => write!(f, "protocol violation"), + Self::OutdatedClientVersion => write!(f, "outdated client version"), + Self::UnknownReason => write!(f, "unknown"), + } + } } #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -52,19 +66,30 @@ impl EventTrait for Disconnect { impl ToBytes for Disconnect { fn write_le(&self, mut writer: W) -> IoResult<()> { - (self.reason as u8).write_le(&mut writer)?; - Ok(()) + if self.reason == DisconnectReason::UnknownReason { + return Err(io_error("Cannot serialize unknown disconnect reason")); + } + + (self.reason as u8).write_le(&mut writer) } } impl FromBytes for Disconnect { fn read_le(mut reader: R) -> IoResult { - let reason = match u8::read_le(&mut reader) { - Ok(0) => DisconnectReason::InvalidChallengeResponse, - Ok(1) => DisconnectReason::NoReasonGiven, - Ok(2) => DisconnectReason::ProtocolViolation, - Ok(3) => DisconnectReason::OutdatedClientVersion, - _ => return Err(io::Error::other("Invalid 'Disconnect' event")), + let index = match u8::read_le(&mut reader) { + Ok(index) => index, + Err(err) => return Err(io_error(format!("Failed to deserialize disconnect reason: {err}"))), + }; + + let reason = match index { + 0 => DisconnectReason::InvalidChallengeResponse, + 1 => DisconnectReason::NoReasonGiven, + 2 => DisconnectReason::ProtocolViolation, + 3 => DisconnectReason::OutdatedClientVersion, + val => { + warn!("received unknown disconnect reason (id={val})"); + DisconnectReason::UnknownReason + } }; Ok(Self { reason }) @@ -99,10 +124,10 @@ mod tests { } #[test] - #[should_panic(expected = "Invalid 'Disconnect' event")] - fn deserializing_invalid_data_panics() { + fn deserialize_unknown_reason() { let mut buf = BytesMut::default().writer(); - "not a DisconnectReason-value".as_bytes().write_le(&mut buf).unwrap(); - let _disconnect = Disconnect::read_le(buf.into_inner().reader()).unwrap(); + 51u8.to_le_bytes().write_le(&mut buf).unwrap(); + let disconnect = Disconnect::read_le(buf.into_inner().reader()).unwrap(); + assert_eq!(disconnect.reason, DisconnectReason::UnknownReason); } } diff --git a/node/bft/src/gateway.rs b/node/bft/src/gateway.rs index 091219ed01..2225dea1c9 100644 --- a/node/bft/src/gateway.rs +++ b/node/bft/src/gateway.rs @@ -20,7 +20,7 @@ use crate::{ MAX_BATCH_DELAY_IN_MS, MEMORY_POOL_PORT, Worker, - events::{EventCodec, PrimaryPing}, + events::{Disconnect as DisconnectEvent, DisconnectReason, EventCodec, PrimaryPing}, helpers::{Cache, PrimarySender, Storage, SyncSender, WorkerSender, assign_to_worker}, spawn_blocking, }; @@ -34,7 +34,6 @@ use snarkos_node_bft_events::{ ChallengeRequest, ChallengeResponse, DataBlocks, - DisconnectReason, Event, EventTrait, TransmissionRequest, @@ -684,7 +683,7 @@ impl Gateway { } Event::Disconnect(message) => { // The peer informs us that they had disconnected. Disconnect from them too. - debug!("Peer '{peer_ip}' decided to disconnect due to '{:?}'", message.reason); + debug!("Peer '{peer_ip}' decided to disconnect due to '{}'", message.reason); self.disconnect(peer_ip); Ok(false) } @@ -1356,8 +1355,8 @@ macro_rules! expect_event { data } // Received a disconnect event, abort. - Some(Event::Disconnect(reason)) => { - return Err(error(format!("{CONTEXT} '{}' disconnected: {reason:?}", $peer_addr))); + Some(Event::Disconnect(DisconnectEvent { reason })) => { + return Err(error(format!("{CONTEXT} '{}' disconnected: {reason}", $peer_addr))); } // Received an unexpected event, abort. Some(ty) => { @@ -1429,7 +1428,7 @@ impl Gateway { .await { send_event(&mut framed, peer_addr, reason.into()).await?; - return Err(error(format!("Dropped '{peer_addr}' for reason: {reason:?}"))); + return Err(error(format!("Dropped '{peer_addr}' for reason: {reason}"))); } // Verify the challenge request. If a disconnect reason was returned, send the disconnect message and abort. if let Some(reason) = self.verify_challenge_request(peer_addr, &peer_request) { @@ -1438,7 +1437,7 @@ impl Gateway { // The Aleo address is already connected; no reason to return an error. return Ok(None); } else { - return Err(error(format!("Dropped '{peer_addr}' for reason: {reason:?}"))); + return Err(error(format!("Dropped '{peer_addr}' for reason: {reason}"))); } } @@ -1500,7 +1499,7 @@ impl Gateway { // The Aleo address is already connected; no reason to return an error. return Ok(None); } else { - return Err(error(format!("Dropped '{peer_addr}' for reason: {reason:?}"))); + return Err(io_error(format!("Dropped '{peer_addr}' for reason: {reason}"))); } } @@ -1536,7 +1535,7 @@ impl Gateway { .await { send_event(&mut framed, peer_addr, reason.into()).await?; - return Err(error(format!("Dropped '{peer_addr}' for reason: {reason:?}"))); + return Err(io_error(format!("Dropped '{peer_addr}' for reason: {reason}"))); } Ok(Some(peer_request)) diff --git a/node/router/messages/src/disconnect.rs b/node/router/messages/src/disconnect.rs index c76046eb19..b33db328f6 100644 --- a/node/router/messages/src/disconnect.rs +++ b/node/router/messages/src/disconnect.rs @@ -90,12 +90,4 @@ mod tests { assert_eq!(reason, &disconnect.reason); } } - - #[test] - #[should_panic] - fn disconnect_invalid_data_panics() { - let mut buf = BytesMut::default().writer(); - "not a DisconnectReason-value".as_bytes().write_le(&mut buf).unwrap(); - let _disconnect = Disconnect::read_le(buf.into_inner().reader()).unwrap(); - } } diff --git a/node/router/messages/src/helpers/disconnect.rs b/node/router/messages/src/helpers/disconnect.rs index e642911cd7..599bb15b0e 100644 --- a/node/router/messages/src/helpers/disconnect.rs +++ b/node/router/messages/src/helpers/disconnect.rs @@ -13,7 +13,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use snarkvm::prelude::{FromBytes, ToBytes, error}; +use snarkvm::prelude::{FromBytes, ToBytes, io_error}; use std::io; @@ -50,6 +50,8 @@ pub enum DisconnectReason { YouNeedToSyncFirst, /// The peer's listening port is closed. YourPortIsClosed(u16), + /// The disconnect reason is not known. This is used for when the peers sends a disconnect reason that is not known to us. + UnknownReason, } impl ToBytes for DisconnectReason { @@ -73,32 +75,66 @@ impl ToBytes for DisconnectReason { 14u8.write_le(&mut writer)?; port.write_le(writer) } + Self::UnknownReason => Err(io_error("Cannot serialize unknown disconnect reason")), } } } impl FromBytes for DisconnectReason { fn read_le(mut reader: R) -> io::Result { - match u8::read_le(&mut reader)? { - 0 => Ok(Self::ExceededForkRange), - 1 => Ok(Self::InvalidChallengeResponse), - 2 => Ok(Self::InvalidForkDepth), - 3 => Ok(Self::INeedToSyncFirst), - 4 => Ok(Self::NoReasonGiven), - 5 => Ok(Self::ProtocolViolation), - 6 => Ok(Self::OutdatedClientVersion), - 7 => Ok(Self::PeerHasDisconnected), - 8 => Ok(Self::PeerRefresh), - 9 => Ok(Self::ShuttingDown), - 10 => Ok(Self::SyncComplete), - 11 => Ok(Self::TooManyFailures), - 12 => Ok(Self::TooManyPeers), - 13 => Ok(Self::YouNeedToSyncFirst), + let index = match u8::read_le(&mut reader) { + Ok(index) => index, + Err(err) => return Err(io_error(format!("Failed to deserialize disconnect reason: {err}"))), + }; + + let reason = match index { + 0 => Self::ExceededForkRange, + 1 => Self::InvalidChallengeResponse, + 2 => Self::InvalidForkDepth, + 3 => Self::INeedToSyncFirst, + 4 => Self::NoReasonGiven, + 5 => Self::ProtocolViolation, + 6 => Self::OutdatedClientVersion, + 7 => Self::PeerHasDisconnected, + 8 => Self::PeerRefresh, + 9 => Self::ShuttingDown, + 10 => Self::SyncComplete, + 11 => Self::TooManyFailures, + 12 => Self::TooManyPeers, + 13 => Self::YouNeedToSyncFirst, 14 => { let port = u16::read_le(reader)?; - Ok(Self::YourPortIsClosed(port)) + Self::YourPortIsClosed(port) + } + val => { + warn!("Received unknown disconnect reason (id={val})"); + Self::UnknownReason } - _ => Err(error("Invalid disconnect reason")), + }; + + Ok(reason) + } +} + +impl std::fmt::Display for DisconnectReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::ExceededForkRange => write!(f, "exceeded fork range"), + Self::InvalidChallengeResponse => write!(f, "invalid challenge response"), + Self::InvalidForkDepth => write!(f, "invalid fork depth"), + Self::INeedToSyncFirst => write!(f, "I need to sync first"), + Self::NoReasonGiven => write!(f, "no reason given"), + Self::ProtocolViolation => write!(f, "protocol violation"), + Self::OutdatedClientVersion => write!(f, "outdated client version"), + Self::PeerHasDisconnected => write!(f, "peer has disconnected"), + Self::PeerRefresh => write!(f, "periodic peer refresh"), + Self::ShuttingDown => write!(f, "shutting down"), + Self::SyncComplete => write!(f, "block sync complete"), + Self::TooManyFailures => write!(f, "too many failures"), + Self::TooManyPeers => write!(f, "too many peers"), + Self::YouNeedToSyncFirst => write!(f, "you need to sync first"), + Self::YourPortIsClosed(port) => write!(f, "your port is closed ({port})"), + Self::UnknownReason => write!(f, "unknown reason"), } } } diff --git a/node/router/src/handshake.rs b/node/router/src/handshake.rs index ba4c4876f5..bfe5e6398c 100644 --- a/node/router/src/handshake.rs +++ b/node/router/src/handshake.rs @@ -24,7 +24,7 @@ use snarkos_node_network::log_repo_sha_comparison; use snarkos_node_tcp::{ConnectionSide, P2P, Tcp}; use snarkvm::{ ledger::narwhal::Data, - prelude::{Address, Field, Network, block::Header, error}, + prelude::{Address, Field, Network, block::Header, error, io_error}, }; use anyhow::{Result, bail}; @@ -45,7 +45,9 @@ impl P2P for Router { /// A macro unwrapping the expected handshake message or returning an error for unexpected messages. #[macro_export] macro_rules! expect_message { - ($msg_ty:path, $framed:expr, $peer_addr:expr) => { + ($msg_ty:path, $framed:expr, $peer_addr:expr) => {{ + use snarkvm::utilities::io_error; + match $framed.try_next().await? { // Received the expected message, proceed. Some($msg_ty(data)) => { @@ -53,27 +55,27 @@ macro_rules! expect_message { data } // Received a disconnect message, abort. - Some(Message::Disconnect(reason)) => { - return Err(error(format!("'{}' disconnected: {reason:?}", $peer_addr))) + Some(Message::Disconnect($crate::messages::Disconnect { reason })) => { + return Err(io_error(format!("'{}' disconnected: {reason}", $peer_addr))); } // Received an unexpected message, abort. Some(ty) => { - return Err(error(format!( + return Err(io_error(format!( "'{}' did not follow the handshake protocol: received {:?} instead of {}", $peer_addr, ty.name(), stringify!($msg_ty), - ))) + ))); } // Received nothing. None => { - return Err(error(format!( + return Err(io_error(format!( "the peer disconnected before sending {:?}, likely due to peer saturation or shutdown", stringify!($msg_ty), - ))) + ))); } } - }; + }}; } /// Send the given message to the peer. @@ -217,12 +219,12 @@ impl Router { .await { send(&mut framed, peer_addr, reason.into()).await?; - return Err(error(format!("Dropped '{peer_addr}' for reason: {reason:?}"))); + return Err(io_error(format!("Dropped '{peer_addr}' for reason: {reason}"))); } // Verify the challenge request. If a disconnect reason was returned, send the disconnect message and abort. if let Some(reason) = self.verify_challenge_request(peer_addr, &peer_request) { send(&mut framed, peer_addr, reason.into()).await?; - return Err(error(format!("Dropped '{peer_addr}' for reason: {reason:?}"))); + return Err(io_error(format!("Dropped '{peer_addr}' for reason: {reason}"))); } /* Step 3: Send the challenge response. */ @@ -280,7 +282,7 @@ impl Router { // Verify the challenge request. If a disconnect reason was returned, send the disconnect message and abort. if let Some(reason) = self.verify_challenge_request(peer_addr, &peer_request) { send(&mut framed, peer_addr, reason.into()).await?; - return Err(error(format!("Dropped '{peer_addr}' for reason: {reason:?}"))); + return Err(io_error(format!("Dropped '{peer_addr}' for reason: {reason}"))); } /* Step 2: Send the challenge response followed by own challenge request. */ @@ -327,7 +329,7 @@ impl Router { .await { send(&mut framed, peer_addr, reason.into()).await?; - return Err(error(format!("Dropped '{peer_addr}' for reason: {reason:?}"))); + return Err(io_error(format!("Dropped '{peer_addr}' for reason: {reason}"))); } Ok(Some(peer_request)) diff --git a/node/router/src/inbound.rs b/node/router/src/inbound.rs index d3d9e51ebd..22cbd53226 100644 --- a/node/router/src/inbound.rs +++ b/node/router/src/inbound.rs @@ -161,7 +161,7 @@ pub trait Inbound: Reading + Outbound { } Message::Disconnect(message) => { // The peer informs us that they had disconnected. Disconnect from them too. - debug!("Peer '{peer_ip}' decided to disconnect due to '{:?}'", message.reason); + debug!("Peer '{peer_ip}' decided to disconnect due to '{}'", message.reason); self.router().disconnect(peer_ip); Ok(false) } diff --git a/node/src/client/router.rs b/node/src/client/router.rs index 7607248e2c..398f31eb98 100644 --- a/node/src/client/router.rs +++ b/node/src/client/router.rs @@ -268,7 +268,7 @@ impl> Inbound for Client { /// Saves the latest epoch hash and latest block header in the node. fn puzzle_response(&self, peer_ip: SocketAddr, _epoch_hash: N::BlockHash, _header: Header) -> bool { - debug!("Disconnecting '{peer_ip}' for the following reason - {:?}", DisconnectReason::ProtocolViolation); + debug!("Disconnecting '{peer_ip}' for the following reason - {}", DisconnectReason::ProtocolViolation); false } diff --git a/node/src/prover/router.rs b/node/src/prover/router.rs index 4c13198785..fe15e1d3e9 100644 --- a/node/src/prover/router.rs +++ b/node/src/prover/router.rs @@ -162,7 +162,7 @@ impl> Inbound for Prover { /// Handles a `BlockRequest` message. fn block_request(&self, peer_ip: SocketAddr, _message: BlockRequest) -> bool { - debug!("Disconnecting '{peer_ip}' for the following reason - {:?}", DisconnectReason::ProtocolViolation); + debug!("Disconnecting '{peer_ip}' for the following reason - {}", DisconnectReason::ProtocolViolation); false } @@ -173,7 +173,7 @@ impl> Inbound for Prover { _blocks: Vec>, _latest_consensus_version: Option, ) -> bool { - debug!("Disconnecting '{peer_ip}' for the following reason - {:?}", DisconnectReason::ProtocolViolation); + debug!("Disconnecting '{peer_ip}' for the following reason - {}", DisconnectReason::ProtocolViolation); false } @@ -201,7 +201,7 @@ impl> Inbound for Prover { /// Disconnects on receipt of a `PuzzleRequest` message. fn puzzle_request(&self, peer_ip: SocketAddr) -> bool { - debug!("Disconnecting '{peer_ip}' for the following reason - {:?}", DisconnectReason::ProtocolViolation); + debug!("Disconnecting '{peer_ip}' for the following reason - {}", DisconnectReason::ProtocolViolation); false } diff --git a/node/src/validator/router.rs b/node/src/validator/router.rs index a8ad2026ba..eb4f1ee3ba 100644 --- a/node/src/validator/router.rs +++ b/node/src/validator/router.rs @@ -247,7 +247,7 @@ impl> Inbound for Validator { /// Disconnects on receipt of a `PuzzleResponse` message. fn puzzle_response(&self, peer_ip: SocketAddr, _epoch_hash: N::BlockHash, _header: Header) -> bool { - debug!("Disconnecting '{peer_ip}' for the following reason - {:?}", DisconnectReason::ProtocolViolation); + debug!("Disconnecting '{peer_ip}' for the following reason - {}", DisconnectReason::ProtocolViolation); false } diff --git a/node/tests/common/test_peer.rs b/node/tests/common/test_peer.rs index 24985f9187..e7700ef32e 100644 --- a/node/tests/common/test_peer.rs +++ b/node/tests/common/test_peer.rs @@ -21,7 +21,7 @@ use snarkos_node_router::{ }; use snarkvm::{ ledger::narwhal::Data, - prelude::{Address, Field, FromBytes, MainnetV0 as CurrentNetwork, Network, TestRng, block::Block, error}, + prelude::{Address, Field, FromBytes, MainnetV0 as CurrentNetwork, Network, TestRng, block::Block}, }; use std::{