From 374aa045174630b9cfbd7ff8d7801640f8fd94ea Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Fri, 28 Jun 2024 15:56:39 -0400 Subject: [PATCH 01/22] squash inbound path inital work. local cluster works. --- network/framework/src/peer/fuzzing.rs | 21 ++--- network/framework/src/peer/mod.rs | 74 +++++++++++----- network/framework/src/peer_manager/builder.rs | 11 +-- network/framework/src/peer_manager/mod.rs | 27 +++--- .../framework/src/protocols/network/mod.rs | 86 ++++++++++++++++--- network/framework/src/protocols/rpc/mod.rs | 27 +++--- .../src/protocols/wire/messaging/v1/mod.rs | 32 +++++++ 7 files changed, 196 insertions(+), 82 deletions(-) diff --git a/network/framework/src/peer/fuzzing.rs b/network/framework/src/peer/fuzzing.rs index 3e213d220ca8d..0e70e1d7083f9 100644 --- a/network/framework/src/peer/fuzzing.rs +++ b/network/framework/src/peer/fuzzing.rs @@ -2,16 +2,12 @@ // Parts of the project are originally copyright © Meta Platforms, Inc. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - constants, - peer::Peer, - protocols::wire::{ - handshake::v1::{MessagingProtocolVersion, ProtocolIdSet}, - messaging::v1::{MultiplexMessage, MultiplexMessageSink}, - }, - testutils::fake_socket::ReadOnlyTestSocketVec, - transport::{Connection, ConnectionId, ConnectionMetadata}, -}; +use std::collections::HashMap; +use std::sync::Arc; +use crate::{constants, peer::Peer, protocols::wire::{ + handshake::v1::{MessagingProtocolVersion, ProtocolIdSet}, + messaging::v1::{MultiplexMessage, MultiplexMessageSink}, +}, testutils::fake_socket::ReadOnlyTestSocketVec, transport::{Connection, ConnectionId, ConnectionMetadata}}; use aptos_channels::{aptos_channel, message_queues::QueueStyle}; use aptos_config::{config::PeerRole, network_id::NetworkContext}; use aptos_memsocket::MemorySocket; @@ -92,7 +88,7 @@ pub fn fuzz(data: &[u8]) { let channel_size = 8; let (peer_reqs_tx, peer_reqs_rx) = aptos_channel::new(QueueStyle::FIFO, channel_size, None); - let (peer_notifs_tx, peer_notifs_rx) = aptos_channel::new(QueueStyle::FIFO, channel_size, None); + let upstream_handlers = Arc::new(HashMap::new()); // Spin up a new `Peer` actor let peer = Peer::new( @@ -102,7 +98,7 @@ pub fn fuzz(data: &[u8]) { connection, connection_notifs_tx, peer_reqs_rx, - peer_notifs_tx, + upstream_handlers, Duration::from_millis(constants::INBOUND_RPC_TIMEOUT_MS), constants::MAX_CONCURRENT_INBOUND_RPCS, constants::MAX_CONCURRENT_OUTBOUND_RPCS, @@ -120,7 +116,6 @@ pub fn fuzz(data: &[u8]) { // ACK the "remote" d/c and drop our handle to the Peer actor. Then wait // for all network notifs to drain out and finish. drop(peer_reqs_tx); - peer_notifs_rx.collect::>().await; }); } diff --git a/network/framework/src/peer/mod.rs b/network/framework/src/peer/mod.rs index a4d0a517cfbf6..415d996465523 100644 --- a/network/framework/src/peer/mod.rs +++ b/network/framework/src/peer/mod.rs @@ -17,8 +17,9 @@ use crate::{ counters::{ - self, network_application_inbound_traffic, network_application_outbound_traffic, - FAILED_LABEL, RECEIVED_LABEL, SENT_LABEL, + self, network_application_outbound_traffic, + FAILED_LABEL, SENT_LABEL, + // network_application_inbound_traffic, RECEIVED_LABEL, // TODO: bring back }, logging::NetworkSchema, peer_manager::{PeerManagerError, TransportNotification}, @@ -35,12 +36,11 @@ use crate::{ ProtocolId, }; use aptos_channels::{aptos_channel, message_queues::QueueStyle}; -use aptos_config::network_id::NetworkContext; +use aptos_config::network_id::{NetworkContext, PeerNetworkId}; use aptos_logger::prelude::*; use aptos_short_hex_str::AsShortHexStr; use aptos_time_service::{TimeService, TimeServiceTrait}; use aptos_types::PeerId; -use bytes::Bytes; use futures::{ self, channel::oneshot, @@ -51,10 +51,13 @@ use futures::{ use futures_util::stream::select; use serde::Serialize; use std::{fmt, panic, time::Duration}; +use std::collections::HashMap; +use std::sync::Arc; use tokio::{runtime::Handle, time::timeout}; use tokio_util::compat::{ FuturesAsyncReadCompatExt, TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt, }; +use crate::protocols::network::ReceivedMessage; #[cfg(test)] mod test; @@ -124,8 +127,8 @@ pub struct Peer { connection_notifs_tx: aptos_channels::Sender>, /// Channel to receive requests from PeerManager to send messages and rpcs. peer_reqs_rx: aptos_channel::Receiver, - /// Channel to notifty PeerManager of new inbound messages and rpcs. - peer_notifs_tx: aptos_channel::Sender, + /// Where to send inbound messages and rpcs. + upstream_handlers: Arc>>, /// Inbound rpc request queue for handling requests from remote peer. inbound_rpcs: InboundRpcs, /// Outbound rpc request queue for sending requests to remote peer and handling responses. @@ -152,7 +155,7 @@ where connection: Connection, connection_notifs_tx: aptos_channels::Sender>, peer_reqs_rx: aptos_channel::Receiver, - peer_notifs_tx: aptos_channel::Sender, + upstream_handlers: Arc>>, inbound_rpc_timeout: Duration, max_concurrent_inbound_rpcs: u32, max_concurrent_outbound_rpcs: u32, @@ -173,7 +176,7 @@ where connection: Some(socket), connection_notifs_tx, peer_reqs_rx, - peer_notifs_tx, + upstream_handlers, inbound_rpcs: InboundRpcs::new( network_context, time_service.clone(), @@ -447,8 +450,23 @@ where &mut self, message: NetworkMessage, ) -> Result<(), PeerManagerError> { - match message { - NetworkMessage::DirectSendMsg(message) => self.handle_inbound_direct_send(message), + match &message { + NetworkMessage::DirectSendMsg(direct) => { + // self.handle_inbound_direct_send(message) + match self.upstream_handlers.get(&direct.protocol_id) { + None => { + // TODO: count error + } + Some(handler) => { + let key = (self.connection_metadata.remote_peer_id, direct.protocol_id); + let sender = self.connection_metadata.remote_peer_id; + let network_id = self.network_context.network_id(); + let sender = PeerNetworkId::new(network_id, sender); + // TODO: ensure that this is non-blocking, channel must drop if full + handler.push(key, ReceivedMessage::new(message, sender))? + } + } + }, NetworkMessage::Error(error_msg) => { warn!( NetworkSchema::new(&self.network_context) @@ -461,21 +479,29 @@ where ); }, NetworkMessage::RpcRequest(request) => { - if let Err(err) = self - .inbound_rpcs - .handle_inbound_request(&mut self.peer_notifs_tx, request) - { - warn!( - NetworkSchema::new(&self.network_context) - .connection_metadata(&self.connection_metadata), - error = %err, - "{} Error handling inbound rpc request: {}", - self.network_context, - err - ); + match self.upstream_handlers.get(&request.protocol_id) { + None => { + // TODO: count error + } + Some(handler) => { + let sender = self.connection_metadata.remote_peer_id; + let network_id = self.network_context.network_id(); + let sender = PeerNetworkId::new(network_id, sender); + if let Err(err) = self.inbound_rpcs.handle_inbound_request(handler, ReceivedMessage::new(message, sender)) { + warn!( + NetworkSchema::new(&self.network_context) + .connection_metadata(&self.connection_metadata), + error = %err, + "{} Error handling inbound rpc request: {}", + self.network_context, + err + ); + } + } } }, - NetworkMessage::RpcResponse(response) => { + NetworkMessage::RpcResponse(_) => { + let NetworkMessage::RpcResponse(response) = message else { panic!() }; self.outbound_rpcs.handle_inbound_response(response) }, }; @@ -544,6 +570,7 @@ where /// Handle an inbound DirectSendMsg from the remote peer. There's not much to /// do here other than bump some counters and forward the message up to the /// PeerManager. + #[cfg(obsolete)] fn handle_inbound_direct_send(&mut self, message: DirectSendMsg) { let peer_id = self.remote_peer_id(); let protocol_id = message.protocol_id; @@ -576,6 +603,7 @@ where } /// Updates the inbound direct send metrics (e.g., messages and bytes received) + #[cfg(obsolete)] fn update_inbound_direct_send_metrics(&self, protocol_id: ProtocolId, data_len: u64) { // Update the metrics for the received direct send message counters::direct_send_messages(&self.network_context, RECEIVED_LABEL).inc(); diff --git a/network/framework/src/peer_manager/builder.rs b/network/framework/src/peer_manager/builder.rs index da40894b646f3..3159b27e16225 100644 --- a/network/framework/src/peer_manager/builder.rs +++ b/network/framework/src/peer_manager/builder.rs @@ -8,7 +8,7 @@ use crate::{ noise::{stream::NoiseStream, HandshakeAuthMode}, peer_manager::{ conn_notifs_channel, ConnectionRequest, ConnectionRequestSender, PeerManager, - PeerManagerNotification, PeerManagerRequest, PeerManagerRequestSender, + PeerManagerRequest, PeerManagerRequestSender, }, protocols::{ network::{NetworkClientConfig, NetworkServiceConfig}, @@ -31,6 +31,7 @@ use aptos_time_service::TimeService; use aptos_types::{chain_id::ChainId, network_address::NetworkAddress, PeerId}; use std::{clone::Clone, collections::HashMap, fmt::Debug, sync::Arc}; use tokio::runtime::Handle; +use crate::protocols::network::ReceivedMessage; /// Inbound and Outbound connections are always secured with NoiseIK. The dialer /// will always verify the listener. @@ -70,7 +71,7 @@ struct PeerManagerContext { peers_and_metadata: Arc, upstream_handlers: - HashMap>, + HashMap>, connection_event_handlers: Vec, max_concurrent_network_reqs: usize, @@ -92,7 +93,7 @@ impl PeerManagerContext { peers_and_metadata: Arc, upstream_handlers: HashMap< ProtocolId, - aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, >, connection_event_handlers: Vec, @@ -125,7 +126,7 @@ impl PeerManagerContext { fn add_upstream_handler( &mut self, protocol_id: ProtocolId, - channel: aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + channel: aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, ) -> &mut Self { self.upstream_handlers.insert(protocol_id, channel); self @@ -417,7 +418,7 @@ impl PeerManagerBuilder { pub fn add_service( &mut self, config: &NetworkServiceConfig, - ) -> aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerNotification> { + ) -> aptos_channel::Receiver<(PeerId, ProtocolId), ReceivedMessage> { // Register the direct send and rpc protocols self.transport_context() .add_protocols(&config.direct_send_protocols_and_preferences); diff --git a/network/framework/src/peer_manager/mod.rs b/network/framework/src/peer_manager/mod.rs index ed1b469bf0f43..9c08a297d6b5c 100644 --- a/network/framework/src/peer_manager/mod.rs +++ b/network/framework/src/peer_manager/mod.rs @@ -15,7 +15,7 @@ use crate::{ constants, counters::{self}, logging::*, - peer::{Peer, PeerNotification, PeerRequest}, + peer::{Peer, PeerRequest}, transport::{ Connection, ConnectionId, ConnectionMetadata, TSocket as TransportTSocket, TRANSPORT_TIMEOUT, @@ -62,6 +62,7 @@ use aptos_config::config::PeerRole; use aptos_types::account_address::AccountAddress; pub use senders::*; pub use types::*; +use crate::protocols::network::ReceivedMessage; /// Responsible for handling and maintaining connections to other Peers pub struct PeerManager @@ -93,7 +94,7 @@ where /// Upstream handlers for RPC and DirectSend protocols. The handlers are promised fair delivery /// of messages across (PeerId, ProtocolId). upstream_handlers: - HashMap>, + Arc>>, /// Channels to send NewPeer/LostPeer notifications to. connection_event_handlers: Vec, /// Channel used to send Dial requests to the ConnectionHandler actor @@ -109,7 +110,8 @@ where HashMap>>, /// Pin the transport type corresponding to this PeerManager instance phantom_transport: PhantomData, - /// Maximum concurrent network requests to any peer. + /// Maximum concurrent network requests to any peer. // TODO: bring back usage? + #[allow(dead_code)] max_concurrent_network_reqs: usize, /// Size of channels between different actors. channel_size: usize, @@ -139,7 +141,7 @@ where connection_reqs_rx: aptos_channel::Receiver, upstream_handlers: HashMap< ProtocolId, - aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, >, connection_event_handlers: Vec, channel_size: usize, @@ -182,7 +184,7 @@ where transport_notifs_rx, outstanding_disconnect_requests: HashMap::new(), phantom_transport: PhantomData, - upstream_handlers, + upstream_handlers: Arc::new(upstream_handlers), connection_event_handlers, max_concurrent_network_reqs, channel_size, @@ -650,12 +652,6 @@ where self.channel_size, Some(&counters::PENDING_NETWORK_REQUESTS), ); - // TODO: Add label for peer. - let (peer_notifs_tx, peer_notifs_rx) = aptos_channel::new( - QueueStyle::FIFO, - self.channel_size, - Some(&counters::PENDING_NETWORK_NOTIFICATIONS), - ); // Initialize a new Peer actor for this connection. let peer = Peer::new( @@ -665,7 +661,7 @@ where connection, self.transport_notifs_tx.clone(), peer_reqs_rx, - peer_notifs_tx, + self.upstream_handlers.clone(), Duration::from_millis(constants::INBOUND_RPC_TIMEOUT_MS), constants::MAX_CONCURRENT_INBOUND_RPCS, constants::MAX_CONCURRENT_OUTBOUND_RPCS, @@ -674,9 +670,6 @@ where ); self.executor.spawn(peer.start()); - // Start background task to handle events (RPCs and DirectSend messages) received from - // peer. - self.spawn_peer_network_events_handler(peer_id, peer_notifs_rx); // Save PeerRequest sender to `active_peers`. self.active_peers .insert(peer_id, (conn_meta.clone(), peer_reqs_tx)); @@ -713,6 +706,7 @@ where } } + #[cfg(obsolete)] fn spawn_peer_network_events_handler( &self, peer_id: PeerId, @@ -736,13 +730,14 @@ where } /// A task for consuming inbound network messages +#[cfg(obsolete)] fn handle_inbound_request( network_context: NetworkContext, inbound_event: PeerNotification, peer_id: PeerId, upstream_handlers: &mut HashMap< ProtocolId, - aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, >, ) { let (protocol_id, notification) = match inbound_event { diff --git a/network/framework/src/protocols/network/mod.rs b/network/framework/src/protocols/network/mod.rs index c9c944c8f8d0c..6e3c10d862717 100644 --- a/network/framework/src/protocols/network/mod.rs +++ b/network/framework/src/protocols/network/mod.rs @@ -7,10 +7,12 @@ pub use crate::protocols::rpc::error::RpcError; use crate::{ error::NetworkError, - peer_manager::{ConnectionRequestSender, PeerManagerNotification, PeerManagerRequestSender}, + peer_manager::{ConnectionRequestSender, PeerManagerRequestSender}, ProtocolId, + protocols::wire::messaging::v1::NetworkMessage, }; use aptos_channels::aptos_channel; +use aptos_config::network_id::PeerNetworkId; use aptos_logger::prelude::*; use aptos_short_hex_str::AsShortHexStr; use aptos_types::{network_address::NetworkAddress, PeerId}; @@ -24,6 +26,8 @@ use futures_util::ready; use pin_project::pin_project; use serde::{de::DeserializeOwned, Serialize}; use std::{cmp::min, fmt::Debug, future, marker::PhantomData, pin::Pin, time::Duration}; +use std::sync::Arc; +use crate::protocols::wire::messaging::v1::IncomingRequest; pub trait Message: DeserializeOwned + Serialize {} impl Message for T {} @@ -133,6 +137,60 @@ impl NetworkApplicationConfig { } } +#[derive(Debug, Clone)] +pub struct ReceivedMessage { + pub message: NetworkMessage, + pub sender: PeerNetworkId, + + // unix microseconds + pub rx_at: u64, + + pub rpc_replier: Option>>>, +} + +impl ReceivedMessage { + pub fn new(message: NetworkMessage, sender: PeerNetworkId) -> Self { + let now = std::time::SystemTime::now(); + let rx_at = now + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_micros() as u64; + Self { + message, + sender, + rx_at, + rpc_replier: None, + } + } + + pub fn protocol_id(&self) -> Option { + match &self.message { + NetworkMessage::Error(_e) => None, + NetworkMessage::RpcRequest(req) => Some(req.protocol_id), + NetworkMessage::RpcResponse(_response) => { + // design of RpcResponse lacking ProtocolId requires global rpc counter (or at least per-peer) and requires reply matching globally or per-peer + None + }, + NetworkMessage::DirectSendMsg(msg) => Some(msg.protocol_id), + } + } + + pub fn protocol_id_as_str(&self) -> &'static str { + match &self.message { + NetworkMessage::Error(_) => "error", + NetworkMessage::RpcRequest(rr) => rr.protocol_id.as_str(), + NetworkMessage::RpcResponse(_) => "rpc response", + NetworkMessage::DirectSendMsg(dm) => dm.protocol_id.as_str(), + } + } +} + +impl PartialEq for ReceivedMessage { + fn eq(&self, other: &Self) -> bool { + (self.message == other.message) && (self.rx_at == other.rx_at) && (self.sender == other.sender) + } +} + /// A `Stream` of `Event` from the lower network layer to an upper /// network application that deserializes inbound network direct-send and rpc /// messages into `TMessage`. Inbound messages that fail to deserialize are logged @@ -151,7 +209,7 @@ pub struct NetworkEvents { /// Trait specifying the signature for `new()` `NetworkEvents` pub trait NewNetworkEvents { fn new( - peer_mgr_notifs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerNotification>, + peer_mgr_notifs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), ReceivedMessage>, max_parallel_deserialization_tasks: Option, allow_out_of_order_delivery: bool, ) -> Self; @@ -159,7 +217,7 @@ pub trait NewNetworkEvents { impl NewNetworkEvents for NetworkEvents { fn new( - peer_mgr_notifs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerNotification>, + peer_mgr_notifs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), ReceivedMessage>, max_parallel_deserialization_tasks: Option, allow_out_of_order_delivery: bool, ) -> Self { @@ -167,7 +225,7 @@ impl NewNetworkEvents for NetworkEven let max_parallel_deserialization_tasks = max_parallel_deserialization_tasks.unwrap_or(1); let data_event_stream = peer_mgr_notifs_rx.map(|notification| { - tokio::task::spawn_blocking(move || peer_mgr_notif_to_event(notification)) + tokio::task::spawn_blocking(move || received_message_to_event(notification)) }); let data_event_stream: Pin< @@ -216,29 +274,33 @@ impl Stream for NetworkEvents { /// Deserialize inbound direct send and rpc messages into the application `TMessage` /// type, logging and dropping messages that fail to deserialize. -fn peer_mgr_notif_to_event( - notification: PeerManagerNotification, +fn received_message_to_event( + message: ReceivedMessage, ) -> Option> { - match notification { - PeerManagerNotification::RecvRpc(peer_id, rpc_req) => { + let peer_id = message.sender.peer_id(); + let ReceivedMessage { message, sender: _sender, rx_at: _rx_at, rpc_replier } = message; + match message { + NetworkMessage::RpcRequest(rpc_req) => { + let rpc_replier = Arc::into_inner(rpc_replier.unwrap()).unwrap(); request_to_network_event(peer_id, &rpc_req) - .map(|msg| Event::RpcRequest(peer_id, msg, rpc_req.protocol_id, rpc_req.res_tx)) + .map(|msg| Event::RpcRequest(peer_id, msg, rpc_req.protocol_id, rpc_replier)) }, - PeerManagerNotification::RecvMessage(peer_id, request) => { + NetworkMessage::DirectSendMsg(request) => { request_to_network_event(peer_id, &request).map(|msg| Event::Message(peer_id, msg)) }, + _ => None, } } /// Converts a `SerializedRequest` into a network `Event` for sending to other nodes -fn request_to_network_event( +fn request_to_network_event( peer_id: PeerId, request: &Request, ) -> Option { match request.to_message() { Ok(msg) => Some(msg), Err(err) => { - let data = &request.data(); + let data = request.data(); warn!( SecurityEvent::InvalidNetworkEvent, error = ?err, diff --git a/network/framework/src/protocols/rpc/mod.rs b/network/framework/src/protocols/rpc/mod.rs index 66002fb4ea072..74d5ed1e4a2c3 100644 --- a/network/framework/src/protocols/rpc/mod.rs +++ b/network/framework/src/protocols/rpc/mod.rs @@ -51,7 +51,6 @@ use crate::{ RECEIVED_LABEL, REQUEST_LABEL, RESPONSE_LABEL, SENT_LABEL, }, logging::NetworkSchema, - peer::PeerNotification, protocols::{ network::SerializedRequest, wire::messaging::v1::{NetworkMessage, Priority, RequestId, RpcRequest, RpcResponse}, @@ -75,6 +74,8 @@ use futures::{ }; use serde::Serialize; use std::{cmp::PartialEq, collections::HashMap, fmt::Debug, time::Duration}; +use std::sync::Arc; +use crate::protocols::network::ReceivedMessage; pub mod error; @@ -206,8 +207,8 @@ impl InboundRpcs { /// Handle a new inbound `RpcRequest` message off the wire. pub fn handle_inbound_request( &mut self, - peer_notifs_tx: &mut aptos_channel::Sender, - request: RpcRequest, + peer_notifs_tx: &aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, + mut request: ReceivedMessage, ) -> Result<(), RpcError> { let network_context = &self.network_context; @@ -224,9 +225,13 @@ impl InboundRpcs { return Err(RpcError::TooManyPending(self.max_concurrent_inbound_rpcs)); } - let protocol_id = request.protocol_id; - let request_id = request.request_id; - let priority = request.priority; + let peer_id = request.sender.peer_id(); + let NetworkMessage::RpcRequest(rpc_request) = &request.message else { + return Err(RpcError::InvalidRpcResponse) + }; + let protocol_id = rpc_request.protocol_id; + let request_id = rpc_request.request_id; + let priority = rpc_request.priority; trace!( NetworkSchema::new(network_context).remote_peer(&self.remote_peer_id), @@ -236,19 +241,15 @@ impl InboundRpcs { request_id, protocol_id, ); - self.update_inbound_rpc_request_metrics(protocol_id, request.raw_request.len() as u64); + self.update_inbound_rpc_request_metrics(protocol_id, rpc_request.raw_request.len() as u64); let timer = counters::inbound_rpc_handler_latency(network_context, protocol_id).start_timer(); // Forward request to PeerManager for handling. let (response_tx, response_rx) = oneshot::channel(); - let notif = PeerNotification::RecvRpc(InboundRpcRequest { - protocol_id, - data: Bytes::from(request.raw_request), - res_tx: response_tx, - }); - if let Err(err) = peer_notifs_tx.push(protocol_id, notif) { + request.rpc_replier = Some(Arc::new(response_tx)); + if let Err(err) = peer_notifs_tx.push((peer_id, protocol_id), request) { counters::rpc_messages(network_context, REQUEST_LABEL, INBOUND_LABEL, FAILED_LABEL) .inc(); return Err(err.into()); diff --git a/network/framework/src/protocols/wire/messaging/v1/mod.rs b/network/framework/src/protocols/wire/messaging/v1/mod.rs index b01cc28eee613..bbec524d7a787 100644 --- a/network/framework/src/protocols/wire/messaging/v1/mod.rs +++ b/network/framework/src/protocols/wire/messaging/v1/mod.rs @@ -26,6 +26,7 @@ use std::{ pin::Pin, task::{Context, Poll}, }; +use serde::de::DeserializeOwned; use thiserror::Error; use tokio_util::{ codec::{FramedRead, FramedWrite, LengthDelimitedCodec}, @@ -103,6 +104,17 @@ pub type RequestId = u32; /// Create alias Priority for u8. pub type Priority = u8; +pub trait IncomingRequest { + fn protocol_id(&self) -> crate::ProtocolId; + fn data(&self) -> &Vec; + + /// Converts the `SerializedMessage` into its deserialized version of `TMessage` based on the + /// `ProtocolId`. See: [`crate::ProtocolId::from_bytes`] + fn to_message(&self) -> anyhow::Result { + self.protocol_id().from_bytes(self.data()) + } +} + #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] #[cfg_attr(any(test, feature = "fuzzing"), derive(Arbitrary))] pub struct RpcRequest { @@ -117,6 +129,16 @@ pub struct RpcRequest { pub raw_request: Vec, } +impl IncomingRequest for RpcRequest { + fn protocol_id(&self) -> crate::ProtocolId { + self.protocol_id + } + + fn data(&self) -> &Vec { + &self.raw_request + } +} + #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] #[cfg_attr(any(test, feature = "fuzzing"), derive(Arbitrary))] pub struct RpcResponse { @@ -142,6 +164,16 @@ pub struct DirectSendMsg { pub raw_msg: Vec, } +impl IncomingRequest for DirectSendMsg { + fn protocol_id(&self) -> crate::ProtocolId { + self.protocol_id + } + + fn data(&self) -> &Vec { + &self.raw_msg + } +} + /// Errors from reading and deserializing network messages off the wire. #[derive(Debug, Error)] pub enum ReadError { From 9865dd70734ca84c9a43a46b4a7327faa4fe74bd Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Tue, 9 Jul 2024 11:43:12 -0400 Subject: [PATCH 02/22] clippy passes --- consensus/src/network_tests.rs | 105 ++++-- mempool/src/tests/mod.rs | 1 + mempool/src/tests/node.rs | 98 +++--- mempool/src/tests/test_framework.rs | 67 ++-- network/framework/src/application/tests.rs | 44 ++- network/framework/src/peer/test.rs | 300 +++++++++++++----- network/framework/src/peer_manager/tests.rs | 24 +- .../src/protocols/health_checker/test.rs | 36 ++- network/framework/src/testutils/test_node.rs | 77 +++-- peer-monitoring-service/server/src/tests.rs | 29 +- .../storage-service/server/src/tests/mock.rs | 22 +- 11 files changed, 550 insertions(+), 253 deletions(-) diff --git a/consensus/src/network_tests.rs b/consensus/src/network_tests.rs index 55bc30507cf82..e01e9e64eccff 100644 --- a/consensus/src/network_tests.rs +++ b/consensus/src/network_tests.rs @@ -8,7 +8,7 @@ use crate::{ test_utils::{self, consensus_runtime, placeholder_ledger_info, timed_block_on}, }; use aptos_channels::{self, aptos_channel, message_queues::QueueStyle}; -use aptos_config::network_id::NetworkId; +use aptos_config::network_id::{NetworkId, PeerNetworkId}; use aptos_consensus_types::{ block::{block_test_utils::certificate_for_genesis, Block}, common::Author, @@ -27,7 +27,7 @@ use aptos_network::{ }, protocols::{ network::{NewNetworkEvents, SerializedRequest}, - rpc::InboundRpcRequest, + // rpc::InboundRpcRequest, wire::handshake::v1::ProtocolIdSet, }, ProtocolId, @@ -41,6 +41,8 @@ use std::{ time::Duration, }; use tokio::runtime::Handle; +use aptos_network::protocols::network::ReceivedMessage; +use aptos_network::protocols::wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}; /// `TwinId` is used by the NetworkPlayground to uniquely identify /// nodes, even if they have the same `AccountAddress` (e.g. for Twins) @@ -67,7 +69,7 @@ pub struct NetworkPlayground { /// node_consensus_txs: Arc< Mutex< - HashMap>, + HashMap>, >, >, /// Nodes' outbound handlers forward their outbound non-rpc messages to this @@ -130,7 +132,7 @@ impl NetworkPlayground { Mutex< HashMap< TwinId, - aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, >, >, >, @@ -163,16 +165,27 @@ impl NetworkPlayground { let node_consensus_tx = node_consensus_txs.lock().get(dst_twin_id).unwrap().clone(); - let inbound_req = InboundRpcRequest { - protocol_id: outbound_req.protocol_id, - data: outbound_req.data, - res_tx: outbound_req.res_tx, - }; + // let inbound_req = InboundRpcRequest { + // protocol_id: outbound_req.protocol_id, + // data: outbound_req.data, + // res_tx: outbound_req.res_tx, + // }; node_consensus_tx .push( (src_twin_id.author, ProtocolId::ConsensusRpcBcs), - PeerManagerNotification::RecvRpc(src_twin_id.author, inbound_req), + // PeerManagerNotification::RecvRpc(src_twin_id.author, inbound_req), + ReceivedMessage{ + message: NetworkMessage::RpcRequest(RpcRequest{ + protocol_id: outbound_req.protocol_id, + request_id: 123, // TODO: seq? + priority: 0, + raw_request: outbound_req.data.into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, src_twin_id.author), + rx_at: 0, + rpc_replier: Some(Arc::new(outbound_req.res_tx)), + }, ) .unwrap(); }, @@ -189,7 +202,7 @@ impl NetworkPlayground { pub fn add_node( &mut self, twin_id: TwinId, - consensus_tx: aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + consensus_tx: aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, network_reqs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerRequest>, conn_mgr_reqs_rx: aptos_channels::Receiver, ) { @@ -227,10 +240,20 @@ impl NetworkPlayground { .clone(); // copy message data - let msg_copy = match &msg_notif { + let (source_address, msg, rmsg) = match &msg_notif { PeerManagerNotification::RecvMessage(src, msg) => { + let rmsg = ReceivedMessage{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: msg.protocol_id, + priority: 0, + raw_msg: msg.mdata.clone().into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, *src), + rx_at: 0, + rpc_replier: None, + }; let msg: ConsensusMsg = msg.to_message().unwrap(); - (*src, msg) + (*src, msg, rmsg) }, msg_notif => panic!( "[network playground] Unexpected PeerManagerNotification: {:?}", @@ -239,9 +262,9 @@ impl NetworkPlayground { }; let _ = node_consensus_tx.push( (src_twin_id.author, ProtocolId::ConsensusDirectSendBcs), - msg_notif, + rmsg, ); - msg_copy + (source_address, msg) } /// Wait for exactly `num_messages` to be enqueued and delivered. Return a @@ -268,6 +291,9 @@ impl NetworkPlayground { PeerManagerRequest::SendDirectSend(dst_inner, msg_inner) => { (*dst_inner, msg_inner.clone()) }, + // NetworkMessage::DirectSendMsg(dmsg) => { + // + // }, msg_inner => panic!( "[network playground] Unexpected PeerManagerRequest: {:?}", msg_inner @@ -282,6 +308,12 @@ impl NetworkPlayground { if !self.is_message_dropped(&src_twin_id, dst_twin_id, consensus_msg) { let msg_notif = PeerManagerNotification::RecvMessage(src_twin_id.author, msg.clone()); + // let msg_notif = ReceivedMessage{ + // message: NetworkMessage::, + // sender: (), + // rx_at: 0, + // rpc_replier: None, + // }; let msg_copy = self .deliver_message(src_twin_id, *dst_twin_id, msg_notif) .await; @@ -496,7 +528,7 @@ mod tests { storage::PeersAndMetadata, }, protocols::{ - direct_send::Message, + // direct_send::Message, network, network::{NetworkEvents, NewNetworkSender}, }, @@ -814,10 +846,20 @@ mod tests { let peer_id = PeerId::random(); let protocol_id = ProtocolId::ConsensusDirectSendBcs; - let bad_msg = PeerManagerNotification::RecvMessage(peer_id, Message { - protocol_id, - mdata: Bytes::from_static(b"\xde\xad\xbe\xef"), - }); + // let bad_msg = PeerManagerNotification::RecvMessage(peer_id, Message { + // protocol_id, + // mdata: Bytes::from_static(b"\xde\xad\xbe\xef"), + // }); + let bad_msg = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id, + priority: 0, + raw_msg: Bytes::from_static(b"\xde\xad\xbe\xef").into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, peer_id), + rx_at: 0, + rpc_replier: None, + }; peer_mgr_notifs_tx .push((peer_id, protocol_id), bad_msg) @@ -829,13 +871,24 @@ mod tests { let protocol_id = ProtocolId::ConsensusRpcJson; let (res_tx, _res_rx) = oneshot::channel(); - let liveness_check_msg = PeerManagerNotification::RecvRpc(peer_id, InboundRpcRequest { - protocol_id, - data: Bytes::from(serde_json::to_vec(&liveness_check_msg).unwrap()), - res_tx, - }); + // let liveness_check_msg = PeerManagerNotification::RecvRpc(peer_id, InboundRpcRequest { + // protocol_id, + // data: Bytes::from(serde_json::to_vec(&liveness_check_msg).unwrap()), + // res_tx, + // }); + let liveness_check_msg = ReceivedMessage{ + message: NetworkMessage::RpcRequest(RpcRequest{ + protocol_id, + request_id: 0, // TODO: seq? + priority: 0, + raw_request: Bytes::from(serde_json::to_vec(&liveness_check_msg).unwrap()).into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, peer_id), + rx_at: 0, + rpc_replier: Some(Arc::new(res_tx)), + }; - peer_mgr_notifs_tx + peer_mgr_notifs_tx .push((peer_id, protocol_id), liveness_check_msg) .unwrap(); diff --git a/mempool/src/tests/mod.rs b/mempool/src/tests/mod.rs index 9fee6a1f1d364..7e4b11ffe09d7 100644 --- a/mempool/src/tests/mod.rs +++ b/mempool/src/tests/mod.rs @@ -9,6 +9,7 @@ mod core_mempool_test; #[cfg(test)] mod integration_tests; #[cfg(test)] +#[cfg(wrong_network_abstraction)] mod multi_node_test; #[cfg(test)] mod node; diff --git a/mempool/src/tests/node.rs b/mempool/src/tests/node.rs index ee8097c4e4c60..68e0650971169 100644 --- a/mempool/src/tests/node.rs +++ b/mempool/src/tests/node.rs @@ -2,60 +2,47 @@ // Parts of the project are originally copyright © Meta Platforms, Inc. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - core_mempool::{CoreMempool, TimelineState}, - network::MempoolSyncMsg, - shared_mempool::{start_shared_mempool, types::SharedMempoolNotification}, - tests::common::TestTransaction, -}; -use aptos_channels::{aptos_channel, message_queues::QueueStyle}; +// use crate::{ +// core_mempool::CoreMempool, +// network::MempoolSyncMsg, +// shared_mempool::{start_shared_mempool, types::SharedMempoolNotification}, +// }; +// use aptos_channels::{aptos_channel, message_queues::QueueStyle}; use aptos_config::{ - config::{Identity, NodeConfig, PeerRole, RoleType}, + config::{PeerRole, RoleType}, network_id::{NetworkId, PeerNetworkId}, }; -use aptos_crypto::{x25519::PrivateKey, Uniform}; -use aptos_event_notifications::{ReconfigNotification, ReconfigNotificationListener}; -use aptos_infallible::{Mutex, MutexGuard, RwLock}; -use aptos_netcore::transport::ConnectionOrigin; -use aptos_network::{ - application::{ - interface::{NetworkClient, NetworkServiceEvents}, - storage::PeersAndMetadata, - }, - peer_manager::{ - ConnectionRequestSender, PeerManagerNotification, PeerManagerRequest, - PeerManagerRequestSender, - }, - protocols::{ - network::{NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender}, - wire::handshake::v1::ProtocolId::MempoolDirectSend, - }, - transport::ConnectionMetadata, - ProtocolId, -}; -use aptos_storage_interface::mock::MockDbReaderWriter; +// use aptos_crypto::{x25519::PrivateKey, Uniform}; +// use aptos_event_notifications::{ReconfigNotification, ReconfigNotificationListener}; +// use aptos_infallible::{Mutex, RwLock}; +// use aptos_network::{ +// application::{ +// interface::{NetworkClient, NetworkServiceEvents}, +// storage::PeersAndMetadata, +// }, +// }; +// use aptos_storage_interface::mock::MockDbReaderWriter; use aptos_types::{ - on_chain_config::{InMemoryOnChainConfig, OnChainConfigPayload}, + // on_chain_config::{InMemoryOnChainConfig, OnChainConfigPayload}, PeerId, }; -use aptos_vm_validator::mocks::mock_vm_validator::MockVMValidator; +// use aptos_vm_validator::mocks::mock_vm_validator::MockVMValidator; use enum_dispatch::enum_dispatch; -use futures::{ - channel::mpsc::{self, unbounded, UnboundedReceiver}, - FutureExt, StreamExt, -}; -use rand::rngs::StdRng; +// use futures::{ +// channel::mpsc::{self, unbounded, UnboundedReceiver}, +// }; +// use rand::rngs::StdRng; use std::{ - collections::{HashMap, HashSet}, - sync::Arc, + collections::HashSet, + // sync::Arc, }; -use tokio::runtime::Runtime; +// use tokio::runtime::Runtime; -type MempoolNetworkHandle = ( - NetworkId, - NetworkSender, - NetworkEvents, -); +// type MempoolNetworkHandle = ( +// NetworkId, +// NetworkSender, +// NetworkEvents, +// ); /// This is a simple node identifier for testing /// This keeps track of the `NodeType` and a simple index @@ -65,6 +52,7 @@ pub struct NodeId { num: u32, } +#[cfg(unused)] impl NodeId { pub(crate) fn new(node_type: NodeType, num: u32) -> Self { NodeId { node_type, num } @@ -75,9 +63,9 @@ impl NodeId { /// Validators, ValidatorFullNodes, and FullNodes #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)] pub enum NodeType { - Validator, - ValidatorFullNode, - FullNode, + // Validator, + // ValidatorFullNode, + // FullNode, } /// A union type for all types of simulated nodes @@ -138,6 +126,7 @@ pub struct ValidatorNodeInfo { vfn_peer_id: PeerId, } +#[cfg(unused)] impl ValidatorNodeInfo { fn new(peer_id: PeerId, vfn_peer_id: PeerId) -> Self { ValidatorNodeInfo { @@ -175,6 +164,7 @@ pub struct ValidatorFullNodeInfo { vfn_peer_id: PeerId, } +#[cfg(wrong_network_abstraction)] impl ValidatorFullNodeInfo { fn new(peer_id: PeerId, vfn_peer_id: PeerId) -> Self { ValidatorFullNodeInfo { @@ -212,6 +202,7 @@ pub struct FullNodeInfo { peer_role: PeerRole, } +#[cfg(wrong_network_abstraction)] impl FullNodeInfo { fn new(peer_id: PeerId, peer_role: PeerRole) -> Self { FullNodeInfo { peer_id, peer_role } @@ -241,6 +232,7 @@ impl NodeInfoTrait for FullNodeInfo { } /// Provides a `NodeInfo` and `NodeConfig` for a validator +#[cfg(unused)] pub fn validator_config(rng: &mut StdRng) -> (ValidatorNodeInfo, NodeConfig) { let config = NodeConfig::generate_random_config_with_template( &NodeConfig::get_default_validator_config(), @@ -259,6 +251,7 @@ pub fn validator_config(rng: &mut StdRng) -> (ValidatorNodeInfo, NodeConfig) { } /// Provides a `NodeInfo` and `NodeConfig` for a ValidatorFullNode +#[cfg(wrong_network_abstraction)] pub fn vfn_config(rng: &mut StdRng, peer_id: PeerId) -> (ValidatorFullNodeInfo, NodeConfig) { let mut vfn_config = NodeConfig::generate_random_config_with_template( &NodeConfig::get_default_vfn_config(), @@ -292,6 +285,7 @@ pub fn vfn_config(rng: &mut StdRng, peer_id: PeerId) -> (ValidatorFullNodeInfo, } /// Provides a `NodeInfo` and `NodeConfig` for a public full node +#[cfg(wrong_network_abstraction)] pub fn public_full_node_config( rng: &mut StdRng, peer_role: PeerRole, @@ -311,6 +305,7 @@ pub fn public_full_node_config( } /// A struct representing a node, it's network interfaces, mempool, and a mempool event subscriber +#[cfg(wrong_network_abstraction)] pub struct Node { /// The identifying Node node_info: NodeInfo, @@ -327,6 +322,7 @@ pub struct Node { } /// Reimplement `NodeInfoTrait` for simplicity +#[cfg(wrong_network_abstraction)] impl NodeInfoTrait for Node { fn supported_networks(&self) -> Vec { self.node_info.supported_networks() @@ -345,6 +341,7 @@ impl NodeInfoTrait for Node { } } +#[cfg(wrong_network_abstraction)] impl Node { /// Sets up a single node by starting up mempool and any network handles pub fn new(node: NodeInfo, config: NodeConfig) -> Node { @@ -463,14 +460,16 @@ impl Node { /// A simplistic view of the entire network stack for a given `NetworkId` /// Allows us to mock out the network without dealing with the details +#[cfg(wrong_network_abstraction)] pub struct NodeNetworkInterface { /// Peer request receiver for messages pub(crate) network_reqs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerRequest>, /// Peer notification sender for sending outgoing messages to other peers pub(crate) network_notifs_tx: - aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, } +#[cfg(wrong_network_abstraction)] impl NodeNetworkInterface { fn get_next_network_req(&mut self, runtime: Arc) -> PeerManagerRequest { runtime.block_on(self.network_reqs_rx.next()).unwrap() @@ -491,6 +490,7 @@ impl NodeNetworkInterface { // Below here are static functions to help build a new `Node` /// Sets up the network handles for a `Node` +#[cfg(wrong_network_abstraction)] fn setup_node_network_interfaces( node: &NodeInfo, ) -> ( @@ -534,6 +534,7 @@ fn setup_node_network_interfaces( } /// Builds a single network interface with associated queues, and attaches it to the top level network +#[cfg(wrong_network_abstraction)] fn setup_node_network_interface( peer_network_id: PeerNetworkId, ) -> (NodeNetworkInterface, MempoolNetworkHandle) { @@ -560,6 +561,7 @@ fn setup_node_network_interface( } /// Starts up the mempool resources for a single node +#[cfg(wrong_network_abstraction)] fn start_node_mempool( config: NodeConfig, network_client: NetworkClient, diff --git a/mempool/src/tests/test_framework.rs b/mempool/src/tests/test_framework.rs index cff64cfccfa72..225efaa96789d 100644 --- a/mempool/src/tests/test_framework.rs +++ b/mempool/src/tests/test_framework.rs @@ -23,13 +23,11 @@ use aptos_network::{ storage::PeersAndMetadata, }, peer_manager::{ - ConnectionRequestSender, PeerManagerNotification, PeerManagerRequest, + ConnectionRequestSender, PeerManagerRequest, PeerManagerRequestSender, }, protocols::{ - direct_send::Message, network::{NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender}, - rpc::InboundRpcRequest, wire::handshake::v1::ProtocolId::MempoolDirectSend, }, testutils::{ @@ -54,6 +52,8 @@ use maplit::btreemap; use std::{collections::HashMap, hash::Hash, sync::Arc}; use tokio::{runtime::Handle, time::Duration}; use tokio_stream::StreamExt; +use aptos_network::protocols::network::ReceivedMessage; +use aptos_network::protocols::wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}; /// An individual mempool node that runs in it's own runtime. /// @@ -259,23 +259,44 @@ impl MempoolNode { request_id: batch_id.clone(), transactions: sign_transactions(txns), }; - let data = protocol_id.to_bytes(&msg).unwrap().into(); + let data = protocol_id.to_bytes(&msg).unwrap(); let (notif, maybe_receiver) = match protocol_id { ProtocolId::MempoolDirectSend => ( - PeerManagerNotification::RecvMessage(remote_peer_id, Message { - protocol_id, - mdata: data, - }), + // PeerManagerNotification::RecvMessage(remote_peer_id, Message { + // protocol_id, + // mdata: data, + // }), + ReceivedMessage{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id, + priority: 0, + raw_msg: data, + }), + sender: PeerNetworkId::new(network_id, remote_peer_id), + rx_at: 0, + rpc_replier: None, + }, None, ), ProtocolId::MempoolRpc => { let (res_tx, res_rx) = oneshot::channel(); - let notif = PeerManagerNotification::RecvRpc(remote_peer_id, InboundRpcRequest { - protocol_id, - data, - res_tx, - }); - (notif, Some(res_rx)) + // let notif = PeerManagerNotification::RecvRpc(remote_peer_id, InboundRpcRequest { + // protocol_id, + // data, + // res_tx, + // }); + let rmsg = ReceivedMessage { + message: NetworkMessage::RpcRequest(RpcRequest{ + protocol_id, + request_id: 0, // TODO: a number? + priority: 0, + raw_request: data, + }), + sender: PeerNetworkId::new(network_id, remote_peer_id), + rx_at: 0, + rpc_replier: Some(Arc::new(res_tx)), + }; + (rmsg, Some(res_rx)) }, protocol_id => panic!("Invalid protocol id found: {:?}", protocol_id), @@ -403,10 +424,20 @@ impl MempoolNode { if let Some(rpc_sender) = maybe_rpc_sender { rpc_sender.send(Ok(bytes.into())).unwrap(); } else { - let notif = PeerManagerNotification::RecvMessage(peer_id, Message { - protocol_id, - mdata: bytes.into(), - }); + // let notif = PeerManagerNotification::RecvMessage(peer_id, Message { + // protocol_id, + // mdata: bytes.into(), + // }); + let notif = ReceivedMessage{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id, + priority: 0, + raw_msg: bytes, + }), + sender: PeerNetworkId::new(network_id, peer_id), + rx_at: 0, + rpc_replier: None, + }; inbound_handle .inbound_message_sender .push((peer_id, protocol_id), notif) diff --git a/network/framework/src/application/tests.rs b/network/framework/src/application/tests.rs index e6ac92ce337c5..5af0fd0133d1d 100644 --- a/network/framework/src/application/tests.rs +++ b/network/framework/src/application/tests.rs @@ -10,12 +10,11 @@ use crate::{ storage::PeersAndMetadata, }, peer_manager::{ - ConnectionNotification, ConnectionRequestSender, PeerManagerNotification, + ConnectionNotification, ConnectionRequestSender, PeerManagerRequest, PeerManagerRequestSender, }, protocols::{ - network::{Event, NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender}, - rpc::InboundRpcRequest, + network::{Event, NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender, ReceivedMessage}, wire::handshake::v1::{ProtocolId, ProtocolIdSet}, }, transport::ConnectionMetadata, @@ -27,7 +26,6 @@ use aptos_config::{ }; use aptos_peer_monitoring_service_types::PeerMonitoringMetadata; use aptos_types::{account_address::AccountAddress, PeerId}; -use futures::channel::oneshot; use futures_util::StreamExt; use maplit::hashmap; use serde::{Deserialize, Serialize}; @@ -40,6 +38,7 @@ use std::{ time::Duration, }; use tokio::{sync::mpsc::error::TryRecvError, time::timeout}; +use crate::protocols::wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}; // Useful test constants for timeouts const MAX_CHANNEL_TIMEOUT_SECS: u64 = 1; @@ -1011,7 +1010,7 @@ fn create_network_sender_and_events( HashMap>, NetworkServiceEvents, HashMap>, - HashMap>, + HashMap>, ) { let mut network_senders = HashMap::new(); let mut network_and_events = HashMap::new(); @@ -1178,7 +1177,7 @@ async fn wait_for_network_event( >, inbound_request_senders: &mut HashMap< NetworkId, - aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, >, network_events: &mut NetworkEvents, is_rpc_request: bool, @@ -1206,12 +1205,23 @@ async fn wait_for_network_event( assert_eq!(outbound_rpc_request.timeout, message_wait_time); // Create and return the peer manager notification - let inbound_rpc_request = InboundRpcRequest { - protocol_id: outbound_rpc_request.protocol_id, - data: outbound_rpc_request.data, - res_tx: oneshot::channel().0, + // let inbound_rpc_request = InboundRpcRequest { + // protocol_id: outbound_rpc_request.protocol_id, + // data: outbound_rpc_request.data, + // res_tx: oneshot::channel().0, + // }; + let rmsg = ReceivedMessage { + message: NetworkMessage::RpcRequest(RpcRequest{ + protocol_id: outbound_rpc_request.protocol_id, + request_id: 0, // TODO: rand? seq? + priority: 0, + raw_request: outbound_rpc_request.data.into(), + }), + sender: PeerNetworkId::new(expected_network_id, peer_id), + rx_at: 0, + rpc_replier: Some(Arc::new(outbound_rpc_request.res_tx)), }; - (outbound_rpc_request.protocol_id, PeerManagerNotification::RecvRpc(peer_id, inbound_rpc_request)) + (outbound_rpc_request.protocol_id, rmsg) } PeerManagerRequest::SendDirectSend(peer_id, message) => { // Verify the request is correct @@ -1220,7 +1230,17 @@ async fn wait_for_network_event( assert_eq!(Some(message.protocol_id), expected_direct_send_protocol_id); // Create and return the peer manager notification - (message.protocol_id, PeerManagerNotification::RecvMessage(peer_id, message)) + let rmsg = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: message.protocol_id, + priority: 0, + raw_msg: message.mdata.into(), + }), + sender: PeerNetworkId::new(expected_network_id, peer_id), + rx_at: 0, + rpc_replier: None, + }; + (message.protocol_id, rmsg) } }; diff --git a/network/framework/src/peer/test.rs b/network/framework/src/peer/test.rs index e1b14ce51b148..2324c83f40e74 100644 --- a/network/framework/src/peer/test.rs +++ b/network/framework/src/peer/test.rs @@ -7,11 +7,11 @@ use crate::{ INBOUND_RPC_TIMEOUT_MS, MAX_CONCURRENT_INBOUND_RPCS, MAX_CONCURRENT_OUTBOUND_RPCS, MAX_FRAME_SIZE, MAX_MESSAGE_SIZE, NETWORK_CHANNEL_SIZE, }, - peer::{DisconnectReason, Peer, PeerNotification, PeerRequest}, + peer::{DisconnectReason, Peer, PeerRequest}, peer_manager::TransportNotification, protocols::{ direct_send::Message, - rpc::{error::RpcError, InboundRpcRequest, OutboundRpcRequest}, + rpc::{error::RpcError, OutboundRpcRequest}, wire::{ handshake::v1::{MessagingProtocolVersion, ProtocolIdSet}, messaging::v1::{ @@ -38,10 +38,14 @@ use futures::{ SinkExt, }; use std::{collections::HashSet, str::FromStr, time::Duration}; +use std::collections::HashMap; +use std::sync::Arc; use tokio::runtime::{Handle, Runtime}; use tokio_util::compat::{ FuturesAsyncReadCompatExt, TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt, }; +use aptos_config::network_id::{NetworkId, PeerNetworkId}; +use crate::protocols::network::ReceivedMessage; static PROTOCOL: ProtocolId = ProtocolId::MempoolDirectSend; @@ -49,12 +53,13 @@ fn build_test_peer( executor: Handle, time_service: TimeService, origin: ConnectionOrigin, + upstream_handlers: Arc>>, ) -> ( Peer, PeerHandle, MemorySocket, aptos_channels::Receiver>, - aptos_channel::Receiver, + // aptos_channel::Receiver, ) { let (a, b) = MemorySocket::new_pair(); let peer_id = PeerId::random(); @@ -74,8 +79,10 @@ fn build_test_peer( let (connection_notifs_tx, connection_notifs_rx) = aptos_channels::new_test(1); let (peer_reqs_tx, peer_reqs_rx) = aptos_channel::new(QueueStyle::FIFO, NETWORK_CHANNEL_SIZE, None); - let (peer_notifs_tx, peer_notifs_rx) = - aptos_channel::new(QueueStyle::FIFO, NETWORK_CHANNEL_SIZE, None); + // let (peer_notifs_tx, peer_notifs_rx) = + // aptos_channel::new(QueueStyle::FIFO, NETWORK_CHANNEL_SIZE, None); + // upstream_handlers: Arc>>, + // let upstream_handlers = Arc::new(HashMap::new()); let peer = Peer::new( NetworkContext::mock(), @@ -84,7 +91,7 @@ fn build_test_peer( connection, connection_notifs_tx, peer_reqs_rx, - peer_notifs_tx, + upstream_handlers, Duration::from_millis(INBOUND_RPC_TIMEOUT_MS), MAX_CONCURRENT_INBOUND_RPCS, MAX_CONCURRENT_OUTBOUND_RPCS, @@ -93,34 +100,35 @@ fn build_test_peer( ); let peer_handle = PeerHandle(peer_reqs_tx); - (peer, peer_handle, b, connection_notifs_rx, peer_notifs_rx) + (peer, peer_handle, b, connection_notifs_rx) } fn build_test_connected_peers( executor: Handle, time_service: TimeService, + upstream_handlers_a: Arc>>, + upstream_handlers_b: Arc>>, ) -> ( ( Peer, PeerHandle, aptos_channels::Receiver>, - aptos_channel::Receiver, ), ( Peer, PeerHandle, aptos_channels::Receiver>, - aptos_channel::Receiver, ), ) { - let (peer_a, peer_handle_a, connection_a, connection_notifs_rx_a, peer_notifs_rx_a) = + let (peer_a, peer_handle_a, connection_a, connection_notifs_rx_a) = build_test_peer( executor.clone(), time_service.clone(), ConnectionOrigin::Inbound, + upstream_handlers_a, ); - let (mut peer_b, peer_handle_b, _connection_b, connection_notifs_rx_b, peer_notifs_rx_b) = - build_test_peer(executor, time_service, ConnectionOrigin::Outbound); + let (mut peer_b, peer_handle_b, _connection_b, connection_notifs_rx_b) = + build_test_peer(executor, time_service, ConnectionOrigin::Outbound, upstream_handlers_b); // Make sure both peers are connected peer_b.connection = Some(connection_a); @@ -129,13 +137,11 @@ fn build_test_connected_peers( peer_a, peer_handle_a, connection_notifs_rx_a, - peer_notifs_rx_a, ), ( peer_b, peer_handle_b, connection_notifs_rx_b, - peer_notifs_rx_b, ), ) } @@ -200,11 +206,13 @@ impl PeerHandle { fn peer_send_message() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, mut peer_handle, mut connection, _connection_notifs_rx, _peer_notifs_rx) = + let upstream_handlers = Arc::new(HashMap::new()); + let (peer, mut peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut client_sink, mut client_stream) = build_network_sink_stream(&mut connection); @@ -243,11 +251,16 @@ fn peer_send_message() { fn peer_recv_message() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, _peer_handle, connection, _connection_notifs_rx, mut peer_notifs_rx) = + let mut upstream_handlers = HashMap::new(); + let (sender, mut receiver) = aptos_channel::new(QueueStyle::FIFO, 50, None); + upstream_handlers.insert(PROTOCOL, sender); + let upstream_handlers = Arc::new(upstream_handlers); + let (peer, _peer_handle, connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let send_msg = MultiplexMessage::Message(NetworkMessage::DirectSendMsg(DirectSendMsg { @@ -255,10 +268,22 @@ fn peer_recv_message() { priority: 0, raw_msg: Vec::from("hello world"), })); - let recv_msg = PeerNotification::RecvMessage(Message { - protocol_id: PROTOCOL, - mdata: Bytes::from("hello world"), - }); + // let recv_msg = PeerNotification::RecvMessage(Message { + // protocol_id: PROTOCOL, + // mdata: Bytes::from("hello world"), + // }); + let recv_msg = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: Vec::from("hello world"), + }), + sender: PeerNetworkId::new(NetworkId::Validator,PeerId::random()), + rx_at: 0, + rpc_replier: None, + }; + + //Arc>>, let client = async move { let mut connection = MultiplexMessageSink::new(connection, MAX_FRAME_SIZE); @@ -273,7 +298,7 @@ fn peer_recv_message() { let server = async move { for _ in 0..30 { // Wait to receive notification of DirectSendMsg from Peer. - let received = peer_notifs_rx.next().await.unwrap(); + let received = receiver.next().await.unwrap(); assert_eq!(recv_msg, received); } }; @@ -286,10 +311,18 @@ fn peer_recv_message() { fn peers_send_message_concurrent() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); + let mut upstream_handlers_a = HashMap::new(); + let (prot_a_tx, mut prot_a_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers_a.insert(PROTOCOL, prot_a_tx); + let upstream_handlers_a = Arc::new(upstream_handlers_a); + let mut upstream_handlers_b = HashMap::new(); + let (prot_b_tx, mut prot_b_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers_b.insert(PROTOCOL, prot_b_tx); + let upstream_handlers_b = Arc::new(upstream_handlers_b); let ( - (peer_a, mut peer_handle_a, mut connection_notifs_rx_a, mut peer_notifs_rx_a), - (peer_b, mut peer_handle_b, mut connection_notifs_rx_b, mut peer_notifs_rx_b), - ) = build_test_connected_peers(rt.handle().clone(), TimeService::mock()); + (peer_a, mut peer_handle_a, mut connection_notifs_rx_a), + (peer_b, mut peer_handle_b, mut connection_notifs_rx_b), + ) = build_test_connected_peers(rt.handle().clone(), TimeService::mock(), upstream_handlers_a, upstream_handlers_b); let remote_peer_id_a = peer_a.remote_peer_id(); let remote_peer_id_b = peer_b.remote_peer_id(); @@ -310,10 +343,28 @@ fn peers_send_message_concurrent() { peer_handle_b.send_direct_send(msg_b.clone()); // Check that each peer received the other's message - let notif_a = peer_notifs_rx_a.next().await; - let notif_b = peer_notifs_rx_b.next().await; - assert_eq!(notif_a, Some(PeerNotification::RecvMessage(msg_b))); - assert_eq!(notif_b, Some(PeerNotification::RecvMessage(msg_a))); + let notif_a = prot_a_rx.next().await; + let notif_b = prot_b_rx.next().await; + assert_eq!(notif_a, Some(ReceivedMessage{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_b.mdata.into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, remote_peer_id_b), + rx_at: 0, + rpc_replier: None, + })); + assert_eq!(notif_b, Some(ReceivedMessage{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_a.mdata.into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, remote_peer_id_a), + rx_at: 0, + rpc_replier: None, + })); // Shut one peers and the other should shutdown due to ConnectionLost drop(peer_handle_a); @@ -340,11 +391,16 @@ fn peers_send_message_concurrent() { fn peer_recv_rpc() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, _peer_handle, mut connection, _connection_notifs_rx, mut peer_notifs_rx) = + let mut upstream_handlers = HashMap::new(); + let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers.insert(PROTOCOL, prot_tx); + let upstream_handlers = Arc::new(upstream_handlers); + let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut client_sink, mut client_stream) = build_network_sink_stream(&mut connection); @@ -354,11 +410,11 @@ fn peer_recv_rpc() { priority: 0, raw_request: Vec::from("hello world"), })); - let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - protocol_id: PROTOCOL, - data: Bytes::from("hello world"), - res_tx: oneshot::channel().0, - }); + // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { + // protocol_id: PROTOCOL, + // data: Bytes::from("hello world"), + // res_tx: oneshot::channel().0, + // }); let resp_msg = MultiplexMessage::Message(NetworkMessage::RpcResponse(RpcResponse { request_id: 123, priority: 0, @@ -379,16 +435,23 @@ fn peer_recv_rpc() { let server = async move { for _ in 0..30 { // Wait to receive RpcRequest from Peer. - let received = peer_notifs_rx.next().await.unwrap(); - assert_eq!(recv_msg, received); + let received = prot_rx.next().await.unwrap(); + let ReceivedMessage{ message, sender: _sender, rx_at: _rx_at, rpc_replier } = received; + assert_eq!(message, NetworkMessage::RpcRequest(RpcRequest{ + protocol_id: PROTOCOL, + request_id: 123, + priority: 0, + raw_request: Vec::from("hello world"), + })); // Send response to rpc. - match received { - PeerNotification::RecvRpc(req) => { + match message { + NetworkMessage::RpcRequest(_req) => { let response = Ok(Bytes::from("goodbye world")); - req.res_tx.send(response).unwrap() + let rpc_replier = Arc::into_inner(rpc_replier.expect("rpc without replier")).expect("Arc unpack fail"); + rpc_replier.send(response).expect("rpc reply send fail") }, - _ => panic!("Unexpected PeerNotification: {:?}", received), + msg => panic!("Unexpected PeerNotification: {:?}", msg), } } }; @@ -399,11 +462,16 @@ fn peer_recv_rpc() { fn peer_recv_rpc_concurrent() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, _peer_handle, mut connection, _connection_notifs_rx, mut peer_notifs_rx) = + let mut upstream_handlers = HashMap::new(); + let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers.insert(PROTOCOL, prot_tx); + let upstream_handlers = Arc::new(upstream_handlers); + let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut client_sink, mut client_stream) = build_network_sink_stream(&mut connection); @@ -413,11 +481,11 @@ fn peer_recv_rpc_concurrent() { priority: 0, raw_request: Vec::from("hello world"), })); - let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - protocol_id: PROTOCOL, - data: Bytes::from("hello world"), - res_tx: oneshot::channel().0, - }); + // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { + // protocol_id: PROTOCOL, + // data: Bytes::from("hello world"), + // res_tx: oneshot::channel().0, + // }); let resp_msg = MultiplexMessage::Message(NetworkMessage::RpcResponse(RpcResponse { request_id: 123, priority: 0, @@ -444,10 +512,16 @@ fn peer_recv_rpc_concurrent() { // Wait to receive RpcRequests from Peer. for _ in 0..30 { - let received = peer_notifs_rx.next().await.unwrap(); - assert_eq!(recv_msg, received); - match received { - PeerNotification::RecvRpc(req) => res_txs.push(req.res_tx), + let received = prot_rx.next().await.unwrap(); + // assert_eq!(recv_msg, received); + match &received.message { + // PeerNotification::RecvRpc(req) => res_txs.push(req.res_tx), + NetworkMessage::RpcRequest(req) => { + assert_eq!(Vec::from("hello world"), req.raw_request); + let arcsender = received.rpc_replier.unwrap(); + let sender = Arc::into_inner(arcsender).unwrap(); + res_txs.push(sender) + }, _ => panic!("Unexpected PeerNotification: {:?}", received), }; } @@ -466,11 +540,16 @@ fn peer_recv_rpc_timeout() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let mock_time = MockTimeService::new(); - let (peer, _peer_handle, mut connection, _connection_notifs_rx, mut peer_notifs_rx) = + let mut upstream_handlers = HashMap::new(); + let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers.insert(PROTOCOL, prot_tx); + let upstream_handlers = Arc::new(upstream_handlers); + let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), mock_time.clone().into(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut client_sink, client_stream) = build_network_sink_stream(&mut connection); @@ -480,23 +559,27 @@ fn peer_recv_rpc_timeout() { priority: 0, raw_request: Vec::from("hello world"), })); - let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - protocol_id: PROTOCOL, - data: Bytes::from("hello world"), - res_tx: oneshot::channel().0, - }); + // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { + // protocol_id: PROTOCOL, + // data: Bytes::from("hello world"), + // res_tx: oneshot::channel().0, + // }); let test = async move { // Client sends the rpc request. client_sink.send(&send_msg).await.unwrap(); // Server receives the rpc request from client. - let received = peer_notifs_rx.next().await.unwrap(); - assert_eq!(received, recv_msg); + let received = prot_rx.next().await.unwrap(); + // assert_eq!(received, recv_msg); // Pull out the request completion handle. - let mut res_tx = match received { - PeerNotification::RecvRpc(req) => req.res_tx, + let mut res_tx = match &received.message { + NetworkMessage::RpcRequest(req) => { + assert_eq!(Vec::from("hello world"), req.raw_request); + let arcsender = received.rpc_replier.unwrap(); + Arc::into_inner(arcsender).unwrap() + }, _ => panic!("Unexpected PeerNotification: {:?}", received), }; @@ -524,11 +607,16 @@ fn peer_recv_rpc_timeout() { fn peer_recv_rpc_cancel() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, _peer_handle, mut connection, _connection_notifs_rx, mut peer_notifs_rx) = + let mut upstream_handlers = HashMap::new(); + let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers.insert(PROTOCOL, prot_tx); + let upstream_handlers = Arc::new(upstream_handlers); + let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut client_sink, client_stream) = build_network_sink_stream(&mut connection); @@ -538,23 +626,27 @@ fn peer_recv_rpc_cancel() { priority: 0, raw_request: Vec::from("hello world"), })); - let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - protocol_id: PROTOCOL, - data: Bytes::from("hello world"), - res_tx: oneshot::channel().0, - }); + // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { + // protocol_id: PROTOCOL, + // data: Bytes::from("hello world"), + // res_tx: oneshot::channel().0, + // }); let test = async move { // Client sends the rpc request. client_sink.send(&send_msg).await.unwrap(); // Server receives the rpc request from client. - let received = peer_notifs_rx.next().await.unwrap(); - assert_eq!(received, recv_msg); + let received = prot_rx.next().await.unwrap(); + // assert_eq!(received, recv_msg); // Pull out the request completion handle. - let res_tx = match received { - PeerNotification::RecvRpc(req) => req.res_tx, + let res_tx = match &received.message { + NetworkMessage::RpcRequest(req) => { + assert_eq!(Vec::from("hello world"), req.raw_request); + let arcsender = received.rpc_replier.unwrap(); + Arc::into_inner(arcsender).unwrap() + }, _ => panic!("Unexpected PeerNotification: {:?}", received), }; @@ -578,11 +670,13 @@ fn peer_recv_rpc_cancel() { fn peer_send_rpc() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, mut peer_handle, mut connection, _connection_notifs_rx, _peer_notifs_rx) = + let upstream_handlers = Arc::new(HashMap::new()); + let (peer, mut peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut server_sink, mut server_stream) = build_network_sink_stream(&mut connection); let timeout = Duration::from_millis(10_000); @@ -637,11 +731,13 @@ fn peer_send_rpc() { fn peer_send_rpc_concurrent() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, peer_handle, mut connection, _connection_notifs_rx, _peer_notifs_rx) = + let upstream_handlers = Arc::new(HashMap::new()); + let (peer, peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut server_sink, mut server_stream) = build_network_sink_stream(&mut connection); let timeout = Duration::from_millis(10_000); @@ -706,11 +802,13 @@ fn peer_send_rpc_concurrent() { fn peer_send_rpc_cancel() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, peer_handle, mut connection, _connection_notifs_rx, _peer_notifs_rx) = + let upstream_handlers = Arc::new(HashMap::new()); + let (peer, peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut server_sink, mut server_stream) = build_network_sink_stream(&mut connection); let timeout = Duration::from_millis(10_000); @@ -767,11 +865,13 @@ fn peer_send_rpc_timeout() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let mock_time = MockTimeService::new(); - let (peer, peer_handle, mut connection, _connection_notifs_rx, _peer_notifs_rx) = + let upstream_handlers = Arc::new(HashMap::new()); + let (peer, peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), mock_time.clone().into(), ConnectionOrigin::Inbound, + upstream_handlers, ); let (mut server_sink, mut server_stream) = build_network_sink_stream(&mut connection); let timeout = Duration::from_millis(10_000); @@ -831,11 +931,13 @@ fn peer_send_rpc_timeout() { fn peer_disconnect_request() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, peer_handle, _connection, mut connection_notifs_rx, _peer_notifs_rx) = + let upstream_handlers = Arc::new(HashMap::new()); + let (peer, peer_handle, _connection, mut connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let remote_peer_id = peer.remote_peer_id(); @@ -857,11 +959,13 @@ fn peer_disconnect_request() { fn peer_disconnect_connection_lost() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, _peer_handle, mut connection, mut connection_notifs_rx, _peer_notifs_rx) = + let upstream_handlers = Arc::new(HashMap::new()); + let (peer, _peer_handle, mut connection, mut connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let remote_peer_id = peer.remote_peer_id(); @@ -881,10 +985,12 @@ fn peer_disconnect_connection_lost() { fn peer_terminates_when_request_tx_has_dropped() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let (peer, peer_handle, _connection, _connection_notifs_rx, _peer_notifs_rx) = build_test_peer( + let upstream_handlers = Arc::new(HashMap::new()); + let (peer, peer_handle, _connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), ConnectionOrigin::Inbound, + upstream_handlers, ); let drop = async move { @@ -898,10 +1004,18 @@ fn peer_terminates_when_request_tx_has_dropped() { fn peers_send_multiplex() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); + let mut upstream_handlers_a = HashMap::new(); + let (prot_a_tx, mut prot_a_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers_a.insert(PROTOCOL, prot_a_tx); + let upstream_handlers_a = Arc::new(upstream_handlers_a); + let mut upstream_handlers_b = HashMap::new(); + let (prot_b_tx, mut prot_b_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers_b.insert(PROTOCOL, prot_b_tx); + let upstream_handlers_b = Arc::new(upstream_handlers_b); let ( - (peer_a, mut peer_handle_a, mut connection_notifs_rx_a, mut peer_notifs_rx_a), - (peer_b, mut peer_handle_b, mut connection_notifs_rx_b, mut peer_notifs_rx_b), - ) = build_test_connected_peers(rt.handle().clone(), TimeService::mock()); + (peer_a, mut peer_handle_a, mut connection_notifs_rx_a), + (peer_b, mut peer_handle_b, mut connection_notifs_rx_b), + ) = build_test_connected_peers(rt.handle().clone(), TimeService::mock(), upstream_handlers_a, upstream_handlers_b); let remote_peer_id_a = peer_a.remote_peer_id(); let remote_peer_id_b = peer_b.remote_peer_id(); @@ -922,10 +1036,28 @@ fn peers_send_multiplex() { peer_handle_b.send_direct_send(msg_b.clone()); // Check that each peer received the other's message - let notif_a = peer_notifs_rx_a.next().await; - let notif_b = peer_notifs_rx_b.next().await; - assert_eq!(notif_a, Some(PeerNotification::RecvMessage(msg_b))); - assert_eq!(notif_b, Some(PeerNotification::RecvMessage(msg_a))); + let notif_a = prot_a_rx.next().await; + let notif_b = prot_b_rx.next().await; + assert_eq!(notif_a, Some(ReceivedMessage{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_b.mdata.into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, remote_peer_id_b), + rx_at: 0, + rpc_replier: None, + })); + assert_eq!(notif_b, Some(ReceivedMessage{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_a.mdata.into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, remote_peer_id_a), + rx_at: 0, + rpc_replier: None, + })); // Shut one peers and the other should shutdown due to ConnectionLost drop(peer_handle_a); diff --git a/network/framework/src/peer_manager/tests.rs b/network/framework/src/peer_manager/tests.rs index d9c9c76d7f911..35a7ed693fd21 100644 --- a/network/framework/src/peer_manager/tests.rs +++ b/network/framework/src/peer_manager/tests.rs @@ -8,7 +8,7 @@ use crate::{ peer::DisconnectReason, peer_manager::{ conn_notifs_channel, error::PeerManagerError, ConnectionNotification, ConnectionRequest, - PeerManager, PeerManagerNotification, PeerManagerRequest, TransportNotification, + PeerManager, PeerManagerRequest, TransportNotification, }, protocols::wire::{ handshake::v1::{MessagingProtocolVersion, ProtocolIdSet}, @@ -89,13 +89,13 @@ fn build_test_peer_manager( >, aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerRequest>, aptos_channel::Sender, - aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerNotification>, + // aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerNotification>, conn_notifs_channel::Receiver, ) { let (peer_manager_request_tx, peer_manager_request_rx) = aptos_channel::new(QueueStyle::FIFO, 1, None); let (connection_reqs_tx, connection_reqs_rx) = aptos_channel::new(QueueStyle::FIFO, 1, None); - let (hello_tx, hello_rx) = aptos_channel::new(QueueStyle::FIFO, 1, None); + let (hello_tx, _hello_rx) = aptos_channel::new(QueueStyle::FIFO, 1, None); let (conn_status_tx, conn_status_rx) = conn_notifs_channel::new(); let network_id = NetworkId::Validator; @@ -124,7 +124,7 @@ fn build_test_peer_manager( peer_manager, peer_manager_request_tx, connection_reqs_tx, - hello_rx, + // hello_rx, conn_status_rx, ) } @@ -255,7 +255,7 @@ fn peer_manager_simultaneous_dial_two_inbound() { // Create a list of ordered PeerIds so we can ensure how PeerIds will be compared. let ids = ordered_peer_ids(2); - let (mut peer_manager, _request_tx, _connection_reqs_tx, _hello_rx, _conn_statux_rx) = + let (mut peer_manager, _request_tx, _connection_reqs_tx, _conn_statux_rx) = build_test_peer_manager(runtime.handle().clone(), ids[1]); let test = async move { @@ -305,7 +305,7 @@ fn peer_manager_simultaneous_dial_inbound_outbound_remote_id_larger() { // Create a list of ordered PeerIds so we can ensure how PeerIds will be compared. let ids = ordered_peer_ids(2); - let (mut peer_manager, _request_tx, _connection_reqs_tx, _hello_rx, _conn_status_rx) = + let (mut peer_manager, _request_tx, _connection_reqs_tx, _conn_status_rx) = build_test_peer_manager(runtime.handle().clone(), ids[0]); let test = async move { @@ -356,7 +356,7 @@ fn peer_manager_simultaneous_dial_inbound_outbound_own_id_larger() { // Create a list of ordered PeerIds so we can ensure how PeerIds will be compared. let ids = ordered_peer_ids(2); - let (mut peer_manager, _request_tx, _connection_reqs_tx, _hello_rx, _conn_status_rx) = + let (mut peer_manager, _request_tx, _connection_reqs_tx, _conn_status_rx) = build_test_peer_manager(runtime.handle().clone(), ids[1]); let test = async move { @@ -407,7 +407,7 @@ fn peer_manager_simultaneous_dial_outbound_inbound_remote_id_larger() { // Create a list of ordered PeerIds so we can ensure how PeerIds will be compared. let ids = ordered_peer_ids(2); - let (mut peer_manager, _request_tx, _connection_reqs_tx, _hello_rx, _conn_status_rx) = + let (mut peer_manager, _request_tx, _connection_reqs_tx, _conn_status_rx) = build_test_peer_manager(runtime.handle().clone(), ids[0]); let test = async move { @@ -458,7 +458,7 @@ fn peer_manager_simultaneous_dial_outbound_inbound_own_id_larger() { // Create a list of ordered PeerIds so we can ensure how PeerIds will be compared. let ids = ordered_peer_ids(2); - let (mut peer_manager, _request_tx, _connection_reqs_tx, _hello_rx, _conn_status_rx) = + let (mut peer_manager, _request_tx, _connection_reqs_tx, _conn_status_rx) = build_test_peer_manager(runtime.handle().clone(), ids[1]); let test = async move { @@ -509,7 +509,7 @@ fn peer_manager_simultaneous_dial_two_outbound() { // Create a list of ordered PeerIds so we can ensure how PeerIds will be compared. let ids = ordered_peer_ids(2); - let (mut peer_manager, _request_tx, _connection_reqs_tx, _hello_rx, _conn_status_rx) = + let (mut peer_manager, _request_tx, _connection_reqs_tx, _conn_status_rx) = build_test_peer_manager(runtime.handle().clone(), ids[1]); let test = async move { @@ -556,7 +556,7 @@ fn peer_manager_simultaneous_dial_disconnect_event() { // Create a list of ordered PeerIds so we can ensure how PeerIds will be compared. let ids = ordered_peer_ids(2); - let (mut peer_manager, _request_tx, _connection_reqs_tx, _hello_rx, _conn_status_rx) = + let (mut peer_manager, _request_tx, _connection_reqs_tx, _conn_status_rx) = build_test_peer_manager(runtime.handle().clone(), ids[1]); let test = async move { @@ -600,7 +600,7 @@ fn test_dial_disconnect() { // Create a list of ordered PeerIds so we can ensure how PeerIds will be compared. let ids = ordered_peer_ids(2); - let (mut peer_manager, _request_tx, _connection_reqs_tx, _hello_rx, mut conn_status_rx) = + let (mut peer_manager, _request_tx, _connection_reqs_tx, mut conn_status_rx) = build_test_peer_manager(runtime.handle().clone(), ids[1]); let test = async move { diff --git a/network/framework/src/protocols/health_checker/test.rs b/network/framework/src/protocols/health_checker/test.rs index d415c9aff1ead..a74afb67a4b8b 100644 --- a/network/framework/src/protocols/health_checker/test.rs +++ b/network/framework/src/protocols/health_checker/test.rs @@ -6,12 +6,11 @@ use super::*; use crate::{ application::{interface::NetworkClient, storage::PeersAndMetadata}, peer_manager::{ - self, ConnectionRequest, ConnectionRequestSender, PeerManagerNotification, + self, ConnectionRequest, ConnectionRequestSender, PeerManagerRequest, PeerManagerRequestSender, }, protocols::{ - network::{NetworkSender, NewNetworkEvents, NewNetworkSender}, - rpc::InboundRpcRequest, + network::{NetworkSender, NewNetworkEvents, NewNetworkSender, ReceivedMessage}, wire::handshake::v1::{ProtocolId::HealthCheckerRpc, ProtocolIdSet}, }, transport::ConnectionMetadata, @@ -22,6 +21,8 @@ use aptos_time_service::{MockTimeService, TimeService}; use futures::future; use maplit::hashmap; use std::sync::Arc; +use aptos_config::network_id::NetworkId; +use crate::protocols::wire::messaging::v1::{NetworkMessage, RpcRequest}; const PING_INTERVAL: Duration = Duration::from_secs(1); const PING_TIMEOUT: Duration = Duration::from_millis(500); @@ -29,7 +30,7 @@ const PING_TIMEOUT: Duration = Duration::from_millis(500); struct TestHarness { mock_time: MockTimeService, peer_mgr_reqs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerRequest>, - peer_mgr_notifs_tx: aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>, + peer_mgr_notifs_tx: aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, connection_reqs_rx: aptos_channel::Receiver, connection_notifs_tx: tokio::sync::mpsc::Sender, peers_and_metadata: Arc, @@ -133,20 +134,31 @@ impl TestHarness { ) -> oneshot::Receiver> { let protocol_id = ProtocolId::HealthCheckerRpc; let data = bcs::to_bytes(&HealthCheckerMsg::Ping(Ping(ping))) - .unwrap() - .into(); + .unwrap(); + // .into(); let (res_tx, res_rx) = oneshot::channel(); - let inbound_rpc_req = InboundRpcRequest { - protocol_id, - data, - res_tx, - }; + // let inbound_rpc_req = InboundRpcRequest { + // protocol_id, + // data, + // res_tx, + // }; let key = (peer_id, ProtocolId::HealthCheckerRpc); let (delivered_tx, delivered_rx) = oneshot::channel(); self.peer_mgr_notifs_tx .push_with_feedback( key, - PeerManagerNotification::RecvRpc(peer_id, inbound_rpc_req), + // PeerManagerNotification::RecvRpc(peer_id, inbound_rpc_req), + ReceivedMessage{ + message: NetworkMessage::RpcRequest(RpcRequest{ + protocol_id, + request_id: 0, + priority: 0, + raw_request: data, + }), + sender: PeerNetworkId::new(NetworkId::Validator, peer_id), + rx_at: 0, + rpc_replier: Some(Arc::new(res_tx)), + }, Some(delivered_tx), ) .unwrap(); diff --git a/network/framework/src/testutils/test_node.rs b/network/framework/src/testutils/test_node.rs index f7775e4f97e7f..1751d086de072 100644 --- a/network/framework/src/testutils/test_node.rs +++ b/network/framework/src/testutils/test_node.rs @@ -4,10 +4,10 @@ use crate::{ application::{metadata::ConnectionState, storage::PeersAndMetadata}, - peer_manager::{PeerManagerNotification, PeerManagerRequest}, + peer_manager::PeerManagerRequest, protocols::{ - direct_send::Message, - rpc::{InboundRpcRequest, OutboundRpcRequest}, + network::ReceivedMessage, + rpc::OutboundRpcRequest, }, transport::ConnectionMetadata, ProtocolId, @@ -21,10 +21,11 @@ use aptos_types::PeerId; use async_trait::async_trait; use futures::StreamExt; use std::{collections::HashMap, sync::Arc, time::Duration}; +use crate::protocols::wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}; /// A sender to a node to mock an inbound network message from [`PeerManager`] pub type InboundMessageSender = - aptos_channels::aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerNotification>; + aptos_channels::aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>; /// A sender to a node to mock an inbound connection from [`PeerManager`] pub type ConnectionUpdateSender = crate::peer_manager::conn_notifs_channel::Sender; @@ -306,15 +307,41 @@ pub trait TestNode: ApplicationNode + Sync { async fn send_next_network_msg(&mut self, network_id: NetworkId) { let request = self.get_next_network_msg(network_id).await; - let (remote_peer_id, protocol_id, data, maybe_rpc_info) = match request { - PeerManagerRequest::SendRpc(peer_id, msg) => ( - peer_id, - msg.protocol_id, - msg.data, - Some((msg.timeout, msg.res_tx)), - ), + // let (remote_peer_id, protocol_id, data, maybe_rpc_info) = match request { + let (remote_peer_id, protocol_id, rmsg) = match request { + PeerManagerRequest::SendRpc(peer_id, msg) => { + let rmsg = ReceivedMessage{ + message: NetworkMessage::RpcRequest(RpcRequest{ + protocol_id: msg.protocol_id, + request_id: 0, // TODO: rand? seq? + priority: 0, + raw_request: msg.data.into(), + }), + sender: self.peer_network_id(network_id), + rx_at: 0, + rpc_replier: Some(Arc::new(msg.res_tx)), + }; + ( + peer_id, + msg.protocol_id, + // msg.data, + // Some((msg.timeout, msg.res_tx)), + rmsg, + ) + }, PeerManagerRequest::SendDirectSend(peer_id, msg) => { - (peer_id, msg.protocol_id, msg.mdata, None) + let rmsg = ReceivedMessage{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: msg.protocol_id, + priority: 0, + raw_msg: msg.mdata.into(), + }), + sender: self.peer_network_id(network_id), + rx_at: 0, + rpc_replier: None, + }; + (peer_id, msg.protocol_id, rmsg) + // (peer_id, msg.protocol_id, msg.mdata, None) }, }; @@ -324,21 +351,21 @@ pub trait TestNode: ApplicationNode + Sync { let sender_peer_id = sender_peer_network_id.peer_id(); // TODO: Add timeout functionality - let peer_manager_notif = if let Some((_timeout, res_tx)) = maybe_rpc_info { - PeerManagerNotification::RecvRpc(sender_peer_id, InboundRpcRequest { - protocol_id, - data, - res_tx, - }) - } else { - PeerManagerNotification::RecvMessage(sender_peer_id, Message { - protocol_id, - mdata: data, - }) - }; + // let peer_manager_notif = if let Some((_timeout, res_tx)) = maybe_rpc_info { + // PeerManagerNotification::RecvRpc(sender_peer_id, InboundRpcRequest { + // protocol_id, + // data, + // res_tx, + // }) + // } else { + // PeerManagerNotification::RecvMessage(sender_peer_id, Message { + // protocol_id, + // mdata: data, + // }) + // }; receiver_handle .inbound_message_sender - .push((sender_peer_id, protocol_id), peer_manager_notif) + .push((sender_peer_id, protocol_id), rmsg) .unwrap(); } } diff --git a/peer-monitoring-service/server/src/tests.rs b/peer-monitoring-service/server/src/tests.rs index c9bffc5c703ab..5aad38eda35d5 100644 --- a/peer-monitoring-service/server/src/tests.rs +++ b/peer-monitoring-service/server/src/tests.rs @@ -20,10 +20,10 @@ use aptos_network::{ application::{ interface::NetworkServiceEvents, metadata::ConnectionState, storage::PeersAndMetadata, }, - peer_manager::PeerManagerNotification, + // peer_manager::PeerManagerNotification, protocols::{ network::{NetworkEvents, NewNetworkEvents}, - rpc::InboundRpcRequest, + // rpc::InboundRpcRequest, wire::handshake::v1::{MessagingProtocolVersion, ProtocolId, ProtocolIdSet}, }, transport::{ConnectionId, ConnectionMetadata}, @@ -69,6 +69,8 @@ use std::{ sync::Arc, time::Duration, }; +use aptos_network::protocols::network::ReceivedMessage; +use aptos_network::protocols::wire::messaging::v1::{NetworkMessage, RpcRequest}; // Useful test constants const LOCAL_HOST_NET_ADDR: &str = "/ip4/127.0.0.1/tcp/8081"; @@ -468,7 +470,7 @@ async fn verify_node_information( /// mock client requests to a peer monitoring service server. struct MockClient { peer_manager_notifiers: - HashMap>, + HashMap>, } impl MockClient { @@ -554,12 +556,23 @@ impl MockClient { .to_bytes(&PeerMonitoringServiceMessage::Request(request)) .unwrap(); let (request_sender, request_receiver) = oneshot::channel(); - let inbound_rpc = InboundRpcRequest { - protocol_id, - data: request_data.into(), - res_tx: request_sender, + // let inbound_rpc = InboundRpcRequest { + // protocol_id, + // data: request_data.into(), + // res_tx: request_sender, + // }; + // let request_notification = PeerManagerNotification::RecvRpc(peer_id, inbound_rpc); + let request_notification = ReceivedMessage { + message: NetworkMessage::RpcRequest(RpcRequest{ + protocol_id, + request_id: 42, // TODO? count? rand? + priority: 0, + raw_request: request_data.clone(), + }), + sender: PeerNetworkId::new(network_id, peer_id), + rx_at: 0, + rpc_replier: Some(Arc::new(request_sender)), }; - let request_notification = PeerManagerNotification::RecvRpc(peer_id, inbound_rpc); // Send the request to the peer monitoring service self.peer_manager_notifiers diff --git a/state-sync/storage-service/server/src/tests/mock.rs b/state-sync/storage-service/server/src/tests/mock.rs index f1de92c535213..bf7aa6f3057c5 100644 --- a/state-sync/storage-service/server/src/tests/mock.rs +++ b/state-sync/storage-service/server/src/tests/mock.rs @@ -14,10 +14,8 @@ use aptos_config::{ use aptos_crypto::HashValue; use aptos_network::{ application::{interface::NetworkServiceEvents, storage::PeersAndMetadata}, - peer_manager::PeerManagerNotification, protocols::{ network::{NetworkEvents, NewNetworkEvents}, - rpc::InboundRpcRequest, wire::handshake::v1::ProtocolId, }, }; @@ -51,6 +49,9 @@ use mockall::mock; use rand::{rngs::OsRng, Rng}; use std::{collections::HashMap, sync::Arc, time::Duration}; use tokio::time::timeout; +use aptos_config::network_id::PeerNetworkId; +use aptos_network::protocols::network::ReceivedMessage; +use aptos_network::protocols::wire::messaging::v1::{NetworkMessage, RpcRequest}; // Useful test constants const MAX_RESPONSE_TIMEOUT_SECS: u64 = 60; @@ -59,7 +60,7 @@ const MAX_RESPONSE_TIMEOUT_SECS: u64 = 60; /// mock client requests to a [`StorageServiceServer`]. pub struct MockClient { peer_manager_notifiers: - HashMap>, + HashMap>, } impl MockClient { @@ -161,12 +162,17 @@ impl MockClient { .to_bytes(&StorageServiceMessage::Request(request)) .unwrap(); let (res_tx, res_rx) = oneshot::channel(); - let inbound_rpc = InboundRpcRequest { - protocol_id, - data: data.into(), - res_tx, + let notification = ReceivedMessage { + message: NetworkMessage::RpcRequest(RpcRequest{ + protocol_id, + request_id: 0, // TODO: rand? inc? + priority: 0, + raw_request: data, + }), + sender: PeerNetworkId::new(network_id, peer_id), + rx_at: 0, + rpc_replier: Some(Arc::new(res_tx)), }; - let notification = PeerManagerNotification::RecvRpc(peer_id, inbound_rpc); // Push the request up to the storage service self.peer_manager_notifiers From 9573f16737152a19b09ba8363eeca03a09ef57b4 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Tue, 9 Jul 2024 15:09:21 -0400 Subject: [PATCH 03/22] network test fixes --- network/framework/src/peer/test.rs | 83 +++++++++++------------------- 1 file changed, 30 insertions(+), 53 deletions(-) diff --git a/network/framework/src/peer/test.rs b/network/framework/src/peer/test.rs index 2324c83f40e74..ecaec5bab35f4 100644 --- a/network/framework/src/peer/test.rs +++ b/network/framework/src/peer/test.rs @@ -45,6 +45,7 @@ use tokio_util::compat::{ FuturesAsyncReadCompatExt, TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt, }; use aptos_config::network_id::{NetworkId, PeerNetworkId}; +use aptos_logger::info; use crate::protocols::network::ReceivedMessage; static PROTOCOL: ProtocolId = ProtocolId::MempoolDirectSend; @@ -268,41 +269,37 @@ fn peer_recv_message() { priority: 0, raw_msg: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvMessage(Message { - // protocol_id: PROTOCOL, - // mdata: Bytes::from("hello world"), - // }); - let recv_msg = ReceivedMessage { - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: Vec::from("hello world"), - }), - sender: PeerNetworkId::new(NetworkId::Validator,PeerId::random()), - rx_at: 0, - rpc_replier: None, - }; - - //Arc>>, + let recv_msg = NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: Vec::from("hello world"), + }); let client = async move { + info!("client start"); let mut connection = MultiplexMessageSink::new(connection, MAX_FRAME_SIZE); for _ in 0..30 { // The client should then send the network message. connection.send(&send_msg).await.unwrap(); } + info!("client sent"); // Client then closes connection. connection.close().await.unwrap(); + info!("client exiting"); }; let server = async move { + info!("server start"); for _ in 0..30 { // Wait to receive notification of DirectSendMsg from Peer. let received = receiver.next().await.unwrap(); - assert_eq!(recv_msg, received); + assert_eq!(recv_msg, received.message); } + info!("server exiting"); }; + info!("waiting"); rt.block_on(future::join3(peer.start(), server, client)); + info!("done"); } // Two connected Peer actors should be able to send/recv a DirectSend from each @@ -345,25 +342,15 @@ fn peers_send_message_concurrent() { // Check that each peer received the other's message let notif_a = prot_a_rx.next().await; let notif_b = prot_b_rx.next().await; - assert_eq!(notif_a, Some(ReceivedMessage{ - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: msg_b.mdata.into(), - }), - sender: PeerNetworkId::new(NetworkId::Validator, remote_peer_id_b), - rx_at: 0, - rpc_replier: None, + assert_eq!(notif_a.unwrap().message, NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_b.mdata.into(), })); - assert_eq!(notif_b, Some(ReceivedMessage{ - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: msg_a.mdata.into(), - }), - sender: PeerNetworkId::new(NetworkId::Validator, remote_peer_id_a), - rx_at: 0, - rpc_replier: None, + assert_eq!(notif_b.unwrap().message, NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_a.mdata.into(), })); // Shut one peers and the other should shutdown due to ConnectionLost @@ -1038,25 +1025,15 @@ fn peers_send_multiplex() { // Check that each peer received the other's message let notif_a = prot_a_rx.next().await; let notif_b = prot_b_rx.next().await; - assert_eq!(notif_a, Some(ReceivedMessage{ - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: msg_b.mdata.into(), - }), - sender: PeerNetworkId::new(NetworkId::Validator, remote_peer_id_b), - rx_at: 0, - rpc_replier: None, + assert_eq!(notif_a.unwrap().message, NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_b.mdata.into(), })); - assert_eq!(notif_b, Some(ReceivedMessage{ - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: msg_a.mdata.into(), - }), - sender: PeerNetworkId::new(NetworkId::Validator, remote_peer_id_a), - rx_at: 0, - rpc_replier: None, + assert_eq!(notif_b.unwrap().message, NetworkMessage::DirectSendMsg(DirectSendMsg{ + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_a.mdata.into(), })); // Shut one peers and the other should shutdown due to ConnectionLost From f5b929f5540aa627dddbf6d9edf28a7126b2ae0d Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Tue, 9 Jul 2024 18:11:24 -0400 Subject: [PATCH 04/22] cleanup. fmt. metrics. --- consensus/src/network_tests.rs | 70 ++-- mempool/src/tests/node.rs | 52 +-- mempool/src/tests/test_framework.rs | 35 +- network/framework/src/application/tests.rs | 20 +- network/framework/src/peer/fuzzing.rs | 18 +- network/framework/src/peer/mod.rs | 116 +++---- network/framework/src/peer/test.rs | 308 ++++++++++-------- network/framework/src/peer_manager/builder.rs | 3 +- network/framework/src/peer_manager/mod.rs | 3 +- .../src/protocols/health_checker/test.rs | 21 +- .../framework/src/protocols/network/mod.rs | 17 +- network/framework/src/protocols/rpc/mod.rs | 8 +- .../src/protocols/wire/messaging/v1/mod.rs | 3 +- network/framework/src/testutils/test_node.rs | 10 +- peer-monitoring-service/server/src/tests.rs | 8 +- .../storage-service/server/src/tests/mock.rs | 14 +- 16 files changed, 327 insertions(+), 379 deletions(-) diff --git a/consensus/src/network_tests.rs b/consensus/src/network_tests.rs index e01e9e64eccff..0c783cc48f928 100644 --- a/consensus/src/network_tests.rs +++ b/consensus/src/network_tests.rs @@ -26,9 +26,11 @@ use aptos_network::{ PeerManagerRequestSender, }, protocols::{ - network::{NewNetworkEvents, SerializedRequest}, - // rpc::InboundRpcRequest, - wire::handshake::v1::ProtocolIdSet, + network::{NewNetworkEvents, ReceivedMessage, SerializedRequest}, + wire::{ + handshake::v1::ProtocolIdSet, + messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}, + }, }, ProtocolId, }; @@ -41,8 +43,6 @@ use std::{ time::Duration, }; use tokio::runtime::Handle; -use aptos_network::protocols::network::ReceivedMessage; -use aptos_network::protocols::wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}; /// `TwinId` is used by the NetworkPlayground to uniquely identify /// nodes, even if they have the same `AccountAddress` (e.g. for Twins) @@ -67,11 +67,8 @@ pub struct NetworkPlayground { /// These events will usually be handled by the event loop spawned in /// `ConsensusNetworkImpl`. /// - node_consensus_txs: Arc< - Mutex< - HashMap>, - >, - >, + node_consensus_txs: + Arc>>>, /// Nodes' outbound handlers forward their outbound non-rpc messages to this /// queue. outbound_msgs_tx: mpsc::Sender<(TwinId, PeerManagerRequest)>, @@ -129,12 +126,7 @@ impl NetworkPlayground { mut network_reqs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerRequest>, mut outbound_msgs_tx: mpsc::Sender<(TwinId, PeerManagerRequest)>, node_consensus_txs: Arc< - Mutex< - HashMap< - TwinId, - aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, - >, - >, + Mutex>>, >, author_to_twin_ids: Arc>, ) { @@ -165,24 +157,20 @@ impl NetworkPlayground { let node_consensus_tx = node_consensus_txs.lock().get(dst_twin_id).unwrap().clone(); - // let inbound_req = InboundRpcRequest { - // protocol_id: outbound_req.protocol_id, - // data: outbound_req.data, - // res_tx: outbound_req.res_tx, - // }; - node_consensus_tx .push( (src_twin_id.author, ProtocolId::ConsensusRpcBcs), - // PeerManagerNotification::RecvRpc(src_twin_id.author, inbound_req), - ReceivedMessage{ - message: NetworkMessage::RpcRequest(RpcRequest{ + ReceivedMessage { + message: NetworkMessage::RpcRequest(RpcRequest { protocol_id: outbound_req.protocol_id, - request_id: 123, // TODO: seq? + request_id: 123, priority: 0, raw_request: outbound_req.data.into(), }), - sender: PeerNetworkId::new(NetworkId::Validator, src_twin_id.author), + sender: PeerNetworkId::new( + NetworkId::Validator, + src_twin_id.author, + ), rx_at: 0, rpc_replier: Some(Arc::new(outbound_req.res_tx)), }, @@ -242,7 +230,7 @@ impl NetworkPlayground { // copy message data let (source_address, msg, rmsg) = match &msg_notif { PeerManagerNotification::RecvMessage(src, msg) => { - let rmsg = ReceivedMessage{ + let rmsg = ReceivedMessage { message: NetworkMessage::DirectSendMsg(DirectSendMsg { protocol_id: msg.protocol_id, priority: 0, @@ -291,9 +279,6 @@ impl NetworkPlayground { PeerManagerRequest::SendDirectSend(dst_inner, msg_inner) => { (*dst_inner, msg_inner.clone()) }, - // NetworkMessage::DirectSendMsg(dmsg) => { - // - // }, msg_inner => panic!( "[network playground] Unexpected PeerManagerRequest: {:?}", msg_inner @@ -308,12 +293,6 @@ impl NetworkPlayground { if !self.is_message_dropped(&src_twin_id, dst_twin_id, consensus_msg) { let msg_notif = PeerManagerNotification::RecvMessage(src_twin_id.author, msg.clone()); - // let msg_notif = ReceivedMessage{ - // message: NetworkMessage::, - // sender: (), - // rx_at: 0, - // rpc_replier: None, - // }; let msg_copy = self .deliver_message(src_twin_id, *dst_twin_id, msg_notif) .await; @@ -846,12 +825,8 @@ mod tests { let peer_id = PeerId::random(); let protocol_id = ProtocolId::ConsensusDirectSendBcs; - // let bad_msg = PeerManagerNotification::RecvMessage(peer_id, Message { - // protocol_id, - // mdata: Bytes::from_static(b"\xde\xad\xbe\xef"), - // }); let bad_msg = ReceivedMessage { - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + message: NetworkMessage::DirectSendMsg(DirectSendMsg { protocol_id, priority: 0, raw_msg: Bytes::from_static(b"\xde\xad\xbe\xef").into(), @@ -871,13 +846,8 @@ mod tests { let protocol_id = ProtocolId::ConsensusRpcJson; let (res_tx, _res_rx) = oneshot::channel(); - // let liveness_check_msg = PeerManagerNotification::RecvRpc(peer_id, InboundRpcRequest { - // protocol_id, - // data: Bytes::from(serde_json::to_vec(&liveness_check_msg).unwrap()), - // res_tx, - // }); - let liveness_check_msg = ReceivedMessage{ - message: NetworkMessage::RpcRequest(RpcRequest{ + let liveness_check_msg = ReceivedMessage { + message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, request_id: 0, // TODO: seq? priority: 0, @@ -888,7 +858,7 @@ mod tests { rpc_replier: Some(Arc::new(res_tx)), }; - peer_mgr_notifs_tx + peer_mgr_notifs_tx .push((peer_id, protocol_id), liveness_check_msg) .unwrap(); diff --git a/mempool/src/tests/node.rs b/mempool/src/tests/node.rs index 68e0650971169..91123871c5e9c 100644 --- a/mempool/src/tests/node.rs +++ b/mempool/src/tests/node.rs @@ -2,47 +2,20 @@ // Parts of the project are originally copyright © Meta Platforms, Inc. // SPDX-License-Identifier: Apache-2.0 -// use crate::{ -// core_mempool::CoreMempool, -// network::MempoolSyncMsg, -// shared_mempool::{start_shared_mempool, types::SharedMempoolNotification}, -// }; -// use aptos_channels::{aptos_channel, message_queues::QueueStyle}; use aptos_config::{ config::{PeerRole, RoleType}, network_id::{NetworkId, PeerNetworkId}, }; -// use aptos_crypto::{x25519::PrivateKey, Uniform}; -// use aptos_event_notifications::{ReconfigNotification, ReconfigNotificationListener}; -// use aptos_infallible::{Mutex, RwLock}; -// use aptos_network::{ -// application::{ -// interface::{NetworkClient, NetworkServiceEvents}, -// storage::PeersAndMetadata, -// }, -// }; -// use aptos_storage_interface::mock::MockDbReaderWriter; -use aptos_types::{ - // on_chain_config::{InMemoryOnChainConfig, OnChainConfigPayload}, - PeerId, -}; -// use aptos_vm_validator::mocks::mock_vm_validator::MockVMValidator; +use aptos_types::PeerId; use enum_dispatch::enum_dispatch; -// use futures::{ -// channel::mpsc::{self, unbounded, UnboundedReceiver}, -// }; -// use rand::rngs::StdRng; -use std::{ - collections::HashSet, - // sync::Arc, -}; -// use tokio::runtime::Runtime; +use std::collections::HashSet; -// type MempoolNetworkHandle = ( -// NetworkId, -// NetworkSender, -// NetworkEvents, -// ); +#[cfg(wrong_network_abstraction)] +type MempoolNetworkHandle = ( + NetworkId, + NetworkSender, + NetworkEvents, +); /// This is a simple node identifier for testing /// This keeps track of the `NodeType` and a simple index @@ -52,7 +25,7 @@ pub struct NodeId { num: u32, } -#[cfg(unused)] +#[cfg(wrong_network_abstraction)] impl NodeId { pub(crate) fn new(node_type: NodeType, num: u32) -> Self { NodeId { node_type, num } @@ -126,7 +99,7 @@ pub struct ValidatorNodeInfo { vfn_peer_id: PeerId, } -#[cfg(unused)] +#[cfg(wrong_network_abstraction)] impl ValidatorNodeInfo { fn new(peer_id: PeerId, vfn_peer_id: PeerId) -> Self { ValidatorNodeInfo { @@ -232,7 +205,7 @@ impl NodeInfoTrait for FullNodeInfo { } /// Provides a `NodeInfo` and `NodeConfig` for a validator -#[cfg(unused)] +#[cfg(wrong_network_abstraction)] pub fn validator_config(rng: &mut StdRng) -> (ValidatorNodeInfo, NodeConfig) { let config = NodeConfig::generate_random_config_with_template( &NodeConfig::get_default_validator_config(), @@ -465,8 +438,7 @@ pub struct NodeNetworkInterface { /// Peer request receiver for messages pub(crate) network_reqs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerRequest>, /// Peer notification sender for sending outgoing messages to other peers - pub(crate) network_notifs_tx: - aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, + pub(crate) network_notifs_tx: aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, } #[cfg(wrong_network_abstraction)] diff --git a/mempool/src/tests/test_framework.rs b/mempool/src/tests/test_framework.rs index 225efaa96789d..ee89de6da9615 100644 --- a/mempool/src/tests/test_framework.rs +++ b/mempool/src/tests/test_framework.rs @@ -22,13 +22,15 @@ use aptos_network::{ interface::{NetworkClient, NetworkServiceEvents}, storage::PeersAndMetadata, }, - peer_manager::{ - ConnectionRequestSender, PeerManagerRequest, - PeerManagerRequestSender, - }, + peer_manager::{ConnectionRequestSender, PeerManagerRequest, PeerManagerRequestSender}, protocols::{ - network::{NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender}, - wire::handshake::v1::ProtocolId::MempoolDirectSend, + network::{ + NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender, ReceivedMessage, + }, + wire::{ + handshake::v1::ProtocolId::MempoolDirectSend, + messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}, + }, }, testutils::{ builder::TestFrameworkBuilder, @@ -52,8 +54,6 @@ use maplit::btreemap; use std::{collections::HashMap, hash::Hash, sync::Arc}; use tokio::{runtime::Handle, time::Duration}; use tokio_stream::StreamExt; -use aptos_network::protocols::network::ReceivedMessage; -use aptos_network::protocols::wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}; /// An individual mempool node that runs in it's own runtime. /// @@ -262,12 +262,8 @@ impl MempoolNode { let data = protocol_id.to_bytes(&msg).unwrap(); let (notif, maybe_receiver) = match protocol_id { ProtocolId::MempoolDirectSend => ( - // PeerManagerNotification::RecvMessage(remote_peer_id, Message { - // protocol_id, - // mdata: data, - // }), - ReceivedMessage{ - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg { protocol_id, priority: 0, raw_msg: data, @@ -280,13 +276,8 @@ impl MempoolNode { ), ProtocolId::MempoolRpc => { let (res_tx, res_rx) = oneshot::channel(); - // let notif = PeerManagerNotification::RecvRpc(remote_peer_id, InboundRpcRequest { - // protocol_id, - // data, - // res_tx, - // }); let rmsg = ReceivedMessage { - message: NetworkMessage::RpcRequest(RpcRequest{ + message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, request_id: 0, // TODO: a number? priority: 0, @@ -428,8 +419,8 @@ impl MempoolNode { // protocol_id, // mdata: bytes.into(), // }); - let notif = ReceivedMessage{ - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + let notif = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg { protocol_id, priority: 0, raw_msg: bytes, diff --git a/network/framework/src/application/tests.rs b/network/framework/src/application/tests.rs index 5af0fd0133d1d..d6f41df4b188f 100644 --- a/network/framework/src/application/tests.rs +++ b/network/framework/src/application/tests.rs @@ -10,12 +10,18 @@ use crate::{ storage::PeersAndMetadata, }, peer_manager::{ - ConnectionNotification, ConnectionRequestSender, - PeerManagerRequest, PeerManagerRequestSender, + ConnectionNotification, ConnectionRequestSender, PeerManagerRequest, + PeerManagerRequestSender, }, protocols::{ - network::{Event, NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender, ReceivedMessage}, - wire::handshake::v1::{ProtocolId, ProtocolIdSet}, + network::{ + Event, NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender, + ReceivedMessage, + }, + wire::{ + handshake::v1::{ProtocolId, ProtocolIdSet}, + messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}, + }, }, transport::ConnectionMetadata, }; @@ -38,7 +44,6 @@ use std::{ time::Duration, }; use tokio::{sync::mpsc::error::TryRecvError, time::timeout}; -use crate::protocols::wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}; // Useful test constants for timeouts const MAX_CHANNEL_TIMEOUT_SECS: u64 = 1; @@ -1205,11 +1210,6 @@ async fn wait_for_network_event( assert_eq!(outbound_rpc_request.timeout, message_wait_time); // Create and return the peer manager notification - // let inbound_rpc_request = InboundRpcRequest { - // protocol_id: outbound_rpc_request.protocol_id, - // data: outbound_rpc_request.data, - // res_tx: oneshot::channel().0, - // }; let rmsg = ReceivedMessage { message: NetworkMessage::RpcRequest(RpcRequest{ protocol_id: outbound_rpc_request.protocol_id, diff --git a/network/framework/src/peer/fuzzing.rs b/network/framework/src/peer/fuzzing.rs index 0e70e1d7083f9..9ab4b85bd2e1e 100644 --- a/network/framework/src/peer/fuzzing.rs +++ b/network/framework/src/peer/fuzzing.rs @@ -2,12 +2,16 @@ // Parts of the project are originally copyright © Meta Platforms, Inc. // SPDX-License-Identifier: Apache-2.0 -use std::collections::HashMap; -use std::sync::Arc; -use crate::{constants, peer::Peer, protocols::wire::{ - handshake::v1::{MessagingProtocolVersion, ProtocolIdSet}, - messaging::v1::{MultiplexMessage, MultiplexMessageSink}, -}, testutils::fake_socket::ReadOnlyTestSocketVec, transport::{Connection, ConnectionId, ConnectionMetadata}}; +use crate::{ + constants, + peer::Peer, + protocols::wire::{ + handshake::v1::{MessagingProtocolVersion, ProtocolIdSet}, + messaging::v1::{MultiplexMessage, MultiplexMessageSink}, + }, + testutils::fake_socket::ReadOnlyTestSocketVec, + transport::{Connection, ConnectionId, ConnectionMetadata}, +}; use aptos_channels::{aptos_channel, message_queues::QueueStyle}; use aptos_config::{config::PeerRole, network_id::NetworkContext}; use aptos_memsocket::MemorySocket; @@ -17,7 +21,7 @@ use aptos_time_service::TimeService; use aptos_types::{network_address::NetworkAddress, PeerId}; use futures::{executor::block_on, future, io::AsyncReadExt, sink::SinkExt, stream::StreamExt}; use proptest::{arbitrary::any, collection::vec}; -use std::time::Duration; +use std::{collections::HashMap, sync::Arc, time::Duration}; /// Generate a sequence of `MultiplexMessage`, bcs serialize them, and write them /// out to a buffer using our length-prefixed message codec. diff --git a/network/framework/src/peer/mod.rs b/network/framework/src/peer/mod.rs index 415d996465523..182b2e6eba11d 100644 --- a/network/framework/src/peer/mod.rs +++ b/network/framework/src/peer/mod.rs @@ -17,14 +17,14 @@ use crate::{ counters::{ - self, network_application_outbound_traffic, - FAILED_LABEL, SENT_LABEL, - // network_application_inbound_traffic, RECEIVED_LABEL, // TODO: bring back + self, network_application_inbound_traffic, network_application_outbound_traffic, + DECLINED_LABEL, FAILED_LABEL, RECEIVED_LABEL, SENT_LABEL, }, logging::NetworkSchema, peer_manager::{PeerManagerError, TransportNotification}, protocols::{ direct_send::Message, + network::ReceivedMessage, rpc::{error::RpcError, InboundRpcRequest, InboundRpcs, OutboundRpcRequest, OutboundRpcs}, stream::{InboundStreamBuffer, OutboundStream, StreamMessage}, wire::messaging::v1::{ @@ -50,14 +50,11 @@ use futures::{ }; use futures_util::stream::select; use serde::Serialize; -use std::{fmt, panic, time::Duration}; -use std::collections::HashMap; -use std::sync::Arc; +use std::{collections::HashMap, fmt, panic, sync::Arc, time::Duration}; use tokio::{runtime::Handle, time::timeout}; use tokio_util::compat::{ FuturesAsyncReadCompatExt, TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt, }; -use crate::protocols::network::ReceivedMessage; #[cfg(test)] mod test; @@ -128,7 +125,8 @@ pub struct Peer { /// Channel to receive requests from PeerManager to send messages and rpcs. peer_reqs_rx: aptos_channel::Receiver, /// Where to send inbound messages and rpcs. - upstream_handlers: Arc>>, + upstream_handlers: + Arc>>, /// Inbound rpc request queue for handling requests from remote peer. inbound_rpcs: InboundRpcs, /// Outbound rpc request queue for sending requests to remote peer and handling responses. @@ -155,7 +153,9 @@ where connection: Connection, connection_notifs_tx: aptos_channels::Sender>, peer_reqs_rx: aptos_channel::Receiver, - upstream_handlers: Arc>>, + upstream_handlers: Arc< + HashMap>, + >, inbound_rpc_timeout: Duration, max_concurrent_inbound_rpcs: u32, max_concurrent_outbound_rpcs: u32, @@ -452,19 +452,46 @@ where ) -> Result<(), PeerManagerError> { match &message { NetworkMessage::DirectSendMsg(direct) => { - // self.handle_inbound_direct_send(message) + let data_len = direct.raw_msg.len(); + network_application_inbound_traffic( + self.network_context, + direct.protocol_id, + data_len as u64, + ); match self.upstream_handlers.get(&direct.protocol_id) { None => { - // TODO: count error - } + // TODO: better label than "declined"? more like "garbage-in" + counters::direct_send_messages(&self.network_context, DECLINED_LABEL).inc(); + counters::direct_send_bytes(&self.network_context, DECLINED_LABEL) + .inc_by(data_len as u64); + }, Some(handler) => { let key = (self.connection_metadata.remote_peer_id, direct.protocol_id); let sender = self.connection_metadata.remote_peer_id; let network_id = self.network_context.network_id(); let sender = PeerNetworkId::new(network_id, sender); - // TODO: ensure that this is non-blocking, channel must drop if full - handler.push(key, ReceivedMessage::new(message, sender))? - } + match handler.push(key, ReceivedMessage::new(message, sender)) { + Err(_err) => { + // NOTE: aptos_channel never returns other than Ok(()), but we might switch to tokio::sync::mpsc and then this would work + counters::direct_send_messages( + &self.network_context, + DECLINED_LABEL, + ) + .inc(); + counters::direct_send_bytes(&self.network_context, DECLINED_LABEL) + .inc_by(data_len as u64); + }, + Ok(_) => { + counters::direct_send_messages( + &self.network_context, + RECEIVED_LABEL, + ) + .inc(); + counters::direct_send_bytes(&self.network_context, RECEIVED_LABEL) + .inc_by(data_len as u64); + }, + } + }, } }, NetworkMessage::Error(error_msg) => { @@ -482,12 +509,15 @@ where match self.upstream_handlers.get(&request.protocol_id) { None => { // TODO: count error - } + }, Some(handler) => { let sender = self.connection_metadata.remote_peer_id; let network_id = self.network_context.network_id(); let sender = PeerNetworkId::new(network_id, sender); - if let Err(err) = self.inbound_rpcs.handle_inbound_request(handler, ReceivedMessage::new(message, sender)) { + if let Err(err) = self + .inbound_rpcs + .handle_inbound_request(handler, ReceivedMessage::new(message, sender)) + { warn!( NetworkSchema::new(&self.network_context) .connection_metadata(&self.connection_metadata), @@ -497,11 +527,13 @@ where err ); } - } + }, } }, NetworkMessage::RpcResponse(_) => { - let NetworkMessage::RpcResponse(response) = message else { panic!() }; + let NetworkMessage::RpcResponse(response) = message else { + panic!() + }; self.outbound_rpcs.handle_inbound_response(response) }, }; @@ -567,52 +599,6 @@ where } } - /// Handle an inbound DirectSendMsg from the remote peer. There's not much to - /// do here other than bump some counters and forward the message up to the - /// PeerManager. - #[cfg(obsolete)] - fn handle_inbound_direct_send(&mut self, message: DirectSendMsg) { - let peer_id = self.remote_peer_id(); - let protocol_id = message.protocol_id; - let data = message.raw_msg; - - trace!( - NetworkSchema::new(&self.network_context).remote_peer(&peer_id), - protocol_id = protocol_id, - "{} DirectSend: Received inbound message from peer {} for protocol {:?}", - self.network_context, - peer_id.short_str(), - protocol_id - ); - self.update_inbound_direct_send_metrics(message.protocol_id, data.len() as u64); - - let notif = PeerNotification::RecvMessage(Message { - protocol_id, - mdata: Bytes::from(data), - }); - - if let Err(err) = self.peer_notifs_tx.push(protocol_id, notif) { - warn!( - NetworkSchema::new(&self.network_context), - error = ?err, - "{} Failed to notify PeerManager about inbound DirectSend message. Error: {:?}", - self.network_context, - err - ); - } - } - - /// Updates the inbound direct send metrics (e.g., messages and bytes received) - #[cfg(obsolete)] - fn update_inbound_direct_send_metrics(&self, protocol_id: ProtocolId, data_len: u64) { - // Update the metrics for the received direct send message - counters::direct_send_messages(&self.network_context, RECEIVED_LABEL).inc(); - counters::direct_send_bytes(&self.network_context, RECEIVED_LABEL).inc_by(data_len); - - // Update the general network traffic metrics - network_application_inbound_traffic(self.network_context, protocol_id, data_len); - } - fn handle_outbound_request( &mut self, request: PeerRequest, diff --git a/network/framework/src/peer/test.rs b/network/framework/src/peer/test.rs index ecaec5bab35f4..9750e90e3c4b0 100644 --- a/network/framework/src/peer/test.rs +++ b/network/framework/src/peer/test.rs @@ -11,6 +11,7 @@ use crate::{ peer_manager::TransportNotification, protocols::{ direct_send::Message, + network::ReceivedMessage, rpc::{error::RpcError, OutboundRpcRequest}, wire::{ handshake::v1::{MessagingProtocolVersion, ProtocolIdSet}, @@ -25,6 +26,7 @@ use crate::{ }; use aptos_channels::{self, aptos_channel, message_queues::QueueStyle}; use aptos_config::{config::PeerRole, network_id::NetworkContext}; +use aptos_logger::info; use aptos_memsocket::MemorySocket; use aptos_netcore::transport::ConnectionOrigin; use aptos_time_service::{MockTimeService, TimeService}; @@ -37,16 +39,16 @@ use futures::{ stream::{StreamExt, TryStreamExt}, SinkExt, }; -use std::{collections::HashSet, str::FromStr, time::Duration}; -use std::collections::HashMap; -use std::sync::Arc; +use std::{ + collections::{HashMap, HashSet}, + str::FromStr, + sync::Arc, + time::Duration, +}; use tokio::runtime::{Handle, Runtime}; use tokio_util::compat::{ FuturesAsyncReadCompatExt, TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt, }; -use aptos_config::network_id::{NetworkId, PeerNetworkId}; -use aptos_logger::info; -use crate::protocols::network::ReceivedMessage; static PROTOCOL: ProtocolId = ProtocolId::MempoolDirectSend; @@ -54,7 +56,9 @@ fn build_test_peer( executor: Handle, time_service: TimeService, origin: ConnectionOrigin, - upstream_handlers: Arc>>, + upstream_handlers: Arc< + HashMap>, + >, ) -> ( Peer, PeerHandle, @@ -107,8 +111,12 @@ fn build_test_peer( fn build_test_connected_peers( executor: Handle, time_service: TimeService, - upstream_handlers_a: Arc>>, - upstream_handlers_b: Arc>>, + upstream_handlers_a: Arc< + HashMap>, + >, + upstream_handlers_b: Arc< + HashMap>, + >, ) -> ( ( Peer, @@ -121,29 +129,24 @@ fn build_test_connected_peers( aptos_channels::Receiver>, ), ) { - let (peer_a, peer_handle_a, connection_a, connection_notifs_rx_a) = - build_test_peer( - executor.clone(), - time_service.clone(), - ConnectionOrigin::Inbound, - upstream_handlers_a, - ); - let (mut peer_b, peer_handle_b, _connection_b, connection_notifs_rx_b) = - build_test_peer(executor, time_service, ConnectionOrigin::Outbound, upstream_handlers_b); + let (peer_a, peer_handle_a, connection_a, connection_notifs_rx_a) = build_test_peer( + executor.clone(), + time_service.clone(), + ConnectionOrigin::Inbound, + upstream_handlers_a, + ); + let (mut peer_b, peer_handle_b, _connection_b, connection_notifs_rx_b) = build_test_peer( + executor, + time_service, + ConnectionOrigin::Outbound, + upstream_handlers_b, + ); // Make sure both peers are connected peer_b.connection = Some(connection_a); ( - ( - peer_a, - peer_handle_a, - connection_notifs_rx_a, - ), - ( - peer_b, - peer_handle_b, - connection_notifs_rx_b, - ), + (peer_a, peer_handle_a, connection_notifs_rx_a), + (peer_b, peer_handle_b, connection_notifs_rx_b), ) } @@ -208,13 +211,12 @@ fn peer_send_message() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let upstream_handlers = Arc::new(HashMap::new()); - let (peer, mut peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, mut peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut client_sink, mut client_stream) = build_network_sink_stream(&mut connection); let send_msg = Message { @@ -256,20 +258,19 @@ fn peer_recv_message() { let (sender, mut receiver) = aptos_channel::new(QueueStyle::FIFO, 50, None); upstream_handlers.insert(PROTOCOL, sender); let upstream_handlers = Arc::new(upstream_handlers); - let (peer, _peer_handle, connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, _peer_handle, connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let send_msg = MultiplexMessage::Message(NetworkMessage::DirectSendMsg(DirectSendMsg { protocol_id: PROTOCOL, priority: 0, raw_msg: Vec::from("hello world"), })); - let recv_msg = NetworkMessage::DirectSendMsg(DirectSendMsg{ + let recv_msg = NetworkMessage::DirectSendMsg(DirectSendMsg { protocol_id: PROTOCOL, priority: 0, raw_msg: Vec::from("hello world"), @@ -319,7 +320,12 @@ fn peers_send_message_concurrent() { let ( (peer_a, mut peer_handle_a, mut connection_notifs_rx_a), (peer_b, mut peer_handle_b, mut connection_notifs_rx_b), - ) = build_test_connected_peers(rt.handle().clone(), TimeService::mock(), upstream_handlers_a, upstream_handlers_b); + ) = build_test_connected_peers( + rt.handle().clone(), + TimeService::mock(), + upstream_handlers_a, + upstream_handlers_b, + ); let remote_peer_id_a = peer_a.remote_peer_id(); let remote_peer_id_b = peer_b.remote_peer_id(); @@ -342,16 +348,22 @@ fn peers_send_message_concurrent() { // Check that each peer received the other's message let notif_a = prot_a_rx.next().await; let notif_b = prot_b_rx.next().await; - assert_eq!(notif_a.unwrap().message, NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: msg_b.mdata.into(), - })); - assert_eq!(notif_b.unwrap().message, NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: msg_a.mdata.into(), - })); + assert_eq!( + notif_a.unwrap().message, + NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_b.mdata.into(), + }) + ); + assert_eq!( + notif_b.unwrap().message, + NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_a.mdata.into(), + }) + ); // Shut one peers and the other should shutdown due to ConnectionLost drop(peer_handle_a); @@ -382,13 +394,12 @@ fn peer_recv_rpc() { let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); upstream_handlers.insert(PROTOCOL, prot_tx); let upstream_handlers = Arc::new(upstream_handlers); - let (peer, _peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut client_sink, mut client_stream) = build_network_sink_stream(&mut connection); let send_msg = MultiplexMessage::Message(NetworkMessage::RpcRequest(RpcRequest { @@ -423,19 +434,28 @@ fn peer_recv_rpc() { for _ in 0..30 { // Wait to receive RpcRequest from Peer. let received = prot_rx.next().await.unwrap(); - let ReceivedMessage{ message, sender: _sender, rx_at: _rx_at, rpc_replier } = received; - assert_eq!(message, NetworkMessage::RpcRequest(RpcRequest{ - protocol_id: PROTOCOL, - request_id: 123, - priority: 0, - raw_request: Vec::from("hello world"), - })); + let ReceivedMessage { + message, + sender: _sender, + rx_at: _rx_at, + rpc_replier, + } = received; + assert_eq!( + message, + NetworkMessage::RpcRequest(RpcRequest { + protocol_id: PROTOCOL, + request_id: 123, + priority: 0, + raw_request: Vec::from("hello world"), + }) + ); // Send response to rpc. match message { NetworkMessage::RpcRequest(_req) => { let response = Ok(Bytes::from("goodbye world")); - let rpc_replier = Arc::into_inner(rpc_replier.expect("rpc without replier")).expect("Arc unpack fail"); + let rpc_replier = Arc::into_inner(rpc_replier.expect("rpc without replier")) + .expect("Arc unpack fail"); rpc_replier.send(response).expect("rpc reply send fail") }, msg => panic!("Unexpected PeerNotification: {:?}", msg), @@ -453,13 +473,12 @@ fn peer_recv_rpc_concurrent() { let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); upstream_handlers.insert(PROTOCOL, prot_tx); let upstream_handlers = Arc::new(upstream_handlers); - let (peer, _peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut client_sink, mut client_stream) = build_network_sink_stream(&mut connection); let send_msg = MultiplexMessage::Message(NetworkMessage::RpcRequest(RpcRequest { @@ -531,13 +550,12 @@ fn peer_recv_rpc_timeout() { let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); upstream_handlers.insert(PROTOCOL, prot_tx); let upstream_handlers = Arc::new(upstream_handlers); - let (peer, _peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - mock_time.clone().into(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + mock_time.clone().into(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut client_sink, client_stream) = build_network_sink_stream(&mut connection); let send_msg = MultiplexMessage::Message(NetworkMessage::RpcRequest(RpcRequest { @@ -598,13 +616,12 @@ fn peer_recv_rpc_cancel() { let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); upstream_handlers.insert(PROTOCOL, prot_tx); let upstream_handlers = Arc::new(upstream_handlers); - let (peer, _peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut client_sink, client_stream) = build_network_sink_stream(&mut connection); let send_msg = MultiplexMessage::Message(NetworkMessage::RpcRequest(RpcRequest { @@ -658,13 +675,12 @@ fn peer_send_rpc() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let upstream_handlers = Arc::new(HashMap::new()); - let (peer, mut peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, mut peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut server_sink, mut server_stream) = build_network_sink_stream(&mut connection); let timeout = Duration::from_millis(10_000); @@ -719,13 +735,12 @@ fn peer_send_rpc_concurrent() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let upstream_handlers = Arc::new(HashMap::new()); - let (peer, peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut server_sink, mut server_stream) = build_network_sink_stream(&mut connection); let timeout = Duration::from_millis(10_000); @@ -790,13 +805,12 @@ fn peer_send_rpc_cancel() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let upstream_handlers = Arc::new(HashMap::new()); - let (peer, peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut server_sink, mut server_stream) = build_network_sink_stream(&mut connection); let timeout = Duration::from_millis(10_000); @@ -853,13 +867,12 @@ fn peer_send_rpc_timeout() { let rt = Runtime::new().unwrap(); let mock_time = MockTimeService::new(); let upstream_handlers = Arc::new(HashMap::new()); - let (peer, peer_handle, mut connection, _connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - mock_time.clone().into(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + mock_time.clone().into(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let (mut server_sink, mut server_stream) = build_network_sink_stream(&mut connection); let timeout = Duration::from_millis(10_000); @@ -919,13 +932,12 @@ fn peer_disconnect_request() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let upstream_handlers = Arc::new(HashMap::new()); - let (peer, peer_handle, _connection, mut connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, peer_handle, _connection, mut connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let remote_peer_id = peer.remote_peer_id(); let test = async move { @@ -947,13 +959,12 @@ fn peer_disconnect_connection_lost() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let upstream_handlers = Arc::new(HashMap::new()); - let (peer, _peer_handle, mut connection, mut connection_notifs_rx) = - build_test_peer( - rt.handle().clone(), - TimeService::mock(), - ConnectionOrigin::Inbound, - upstream_handlers, - ); + let (peer, _peer_handle, mut connection, mut connection_notifs_rx) = build_test_peer( + rt.handle().clone(), + TimeService::mock(), + ConnectionOrigin::Inbound, + upstream_handlers, + ); let remote_peer_id = peer.remote_peer_id(); let test = async move { @@ -1002,7 +1013,12 @@ fn peers_send_multiplex() { let ( (peer_a, mut peer_handle_a, mut connection_notifs_rx_a), (peer_b, mut peer_handle_b, mut connection_notifs_rx_b), - ) = build_test_connected_peers(rt.handle().clone(), TimeService::mock(), upstream_handlers_a, upstream_handlers_b); + ) = build_test_connected_peers( + rt.handle().clone(), + TimeService::mock(), + upstream_handlers_a, + upstream_handlers_b, + ); let remote_peer_id_a = peer_a.remote_peer_id(); let remote_peer_id_b = peer_b.remote_peer_id(); @@ -1025,16 +1041,22 @@ fn peers_send_multiplex() { // Check that each peer received the other's message let notif_a = prot_a_rx.next().await; let notif_b = prot_b_rx.next().await; - assert_eq!(notif_a.unwrap().message, NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: msg_b.mdata.into(), - })); - assert_eq!(notif_b.unwrap().message, NetworkMessage::DirectSendMsg(DirectSendMsg{ - protocol_id: PROTOCOL, - priority: 0, - raw_msg: msg_a.mdata.into(), - })); + assert_eq!( + notif_a.unwrap().message, + NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_b.mdata.into(), + }) + ); + assert_eq!( + notif_b.unwrap().message, + NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: PROTOCOL, + priority: 0, + raw_msg: msg_a.mdata.into(), + }) + ); // Shut one peers and the other should shutdown due to ConnectionLost drop(peer_handle_a); diff --git a/network/framework/src/peer_manager/builder.rs b/network/framework/src/peer_manager/builder.rs index 3159b27e16225..9e0d7ad6d0c64 100644 --- a/network/framework/src/peer_manager/builder.rs +++ b/network/framework/src/peer_manager/builder.rs @@ -11,7 +11,7 @@ use crate::{ PeerManagerRequest, PeerManagerRequestSender, }, protocols::{ - network::{NetworkClientConfig, NetworkServiceConfig}, + network::{NetworkClientConfig, NetworkServiceConfig, ReceivedMessage}, wire::handshake::v1::ProtocolIdSet, }, transport::{self, AptosNetTransport, Connection, APTOS_TCP_TRANSPORT}, @@ -31,7 +31,6 @@ use aptos_time_service::TimeService; use aptos_types::{chain_id::ChainId, network_address::NetworkAddress, PeerId}; use std::{clone::Clone, collections::HashMap, fmt::Debug, sync::Arc}; use tokio::runtime::Handle; -use crate::protocols::network::ReceivedMessage; /// Inbound and Outbound connections are always secured with NoiseIK. The dialer /// will always verify the listener. diff --git a/network/framework/src/peer_manager/mod.rs b/network/framework/src/peer_manager/mod.rs index 9c08a297d6b5c..52dd6fa568483 100644 --- a/network/framework/src/peer_manager/mod.rs +++ b/network/framework/src/peer_manager/mod.rs @@ -56,13 +56,12 @@ pub use self::error::PeerManagerError; use crate::{ application::{error::Error, storage::PeersAndMetadata}, peer_manager::transport::{TransportHandler, TransportRequest}, - protocols::network::SerializedRequest, + protocols::network::{ReceivedMessage, SerializedRequest}, }; use aptos_config::config::PeerRole; use aptos_types::account_address::AccountAddress; pub use senders::*; pub use types::*; -use crate::protocols::network::ReceivedMessage; /// Responsible for handling and maintaining connections to other Peers pub struct PeerManager diff --git a/network/framework/src/protocols/health_checker/test.rs b/network/framework/src/protocols/health_checker/test.rs index a74afb67a4b8b..d4978ebcfe4ef 100644 --- a/network/framework/src/protocols/health_checker/test.rs +++ b/network/framework/src/protocols/health_checker/test.rs @@ -6,23 +6,25 @@ use super::*; use crate::{ application::{interface::NetworkClient, storage::PeersAndMetadata}, peer_manager::{ - self, ConnectionRequest, ConnectionRequestSender, - PeerManagerRequest, PeerManagerRequestSender, + self, ConnectionRequest, ConnectionRequestSender, PeerManagerRequest, + PeerManagerRequestSender, }, protocols::{ network::{NetworkSender, NewNetworkEvents, NewNetworkSender, ReceivedMessage}, - wire::handshake::v1::{ProtocolId::HealthCheckerRpc, ProtocolIdSet}, + wire::{ + handshake::v1::{ProtocolId::HealthCheckerRpc, ProtocolIdSet}, + messaging::v1::{NetworkMessage, RpcRequest}, + }, }, transport::ConnectionMetadata, ProtocolId, }; use aptos_channels::{aptos_channel, message_queues::QueueStyle}; +use aptos_config::network_id::NetworkId; use aptos_time_service::{MockTimeService, TimeService}; use futures::future; use maplit::hashmap; use std::sync::Arc; -use aptos_config::network_id::NetworkId; -use crate::protocols::wire::messaging::v1::{NetworkMessage, RpcRequest}; const PING_INTERVAL: Duration = Duration::from_secs(1); const PING_TIMEOUT: Duration = Duration::from_millis(500); @@ -133,9 +135,8 @@ impl TestHarness { ping: u32, ) -> oneshot::Receiver> { let protocol_id = ProtocolId::HealthCheckerRpc; - let data = bcs::to_bytes(&HealthCheckerMsg::Ping(Ping(ping))) - .unwrap(); - // .into(); + let data = bcs::to_bytes(&HealthCheckerMsg::Ping(Ping(ping))).unwrap(); + // .into(); let (res_tx, res_rx) = oneshot::channel(); // let inbound_rpc_req = InboundRpcRequest { // protocol_id, @@ -148,8 +149,8 @@ impl TestHarness { .push_with_feedback( key, // PeerManagerNotification::RecvRpc(peer_id, inbound_rpc_req), - ReceivedMessage{ - message: NetworkMessage::RpcRequest(RpcRequest{ + ReceivedMessage { + message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, request_id: 0, priority: 0, diff --git a/network/framework/src/protocols/network/mod.rs b/network/framework/src/protocols/network/mod.rs index 6e3c10d862717..e83103c83d6d8 100644 --- a/network/framework/src/protocols/network/mod.rs +++ b/network/framework/src/protocols/network/mod.rs @@ -8,8 +8,8 @@ pub use crate::protocols::rpc::error::RpcError; use crate::{ error::NetworkError, peer_manager::{ConnectionRequestSender, PeerManagerRequestSender}, + protocols::wire::messaging::v1::{IncomingRequest, NetworkMessage}, ProtocolId, - protocols::wire::messaging::v1::NetworkMessage, }; use aptos_channels::aptos_channel; use aptos_config::network_id::PeerNetworkId; @@ -25,9 +25,7 @@ use futures::{ use futures_util::ready; use pin_project::pin_project; use serde::{de::DeserializeOwned, Serialize}; -use std::{cmp::min, fmt::Debug, future, marker::PhantomData, pin::Pin, time::Duration}; -use std::sync::Arc; -use crate::protocols::wire::messaging::v1::IncomingRequest; +use std::{cmp::min, fmt::Debug, future, marker::PhantomData, pin::Pin, sync::Arc, time::Duration}; pub trait Message: DeserializeOwned + Serialize {} impl Message for T {} @@ -187,7 +185,9 @@ impl ReceivedMessage { impl PartialEq for ReceivedMessage { fn eq(&self, other: &Self) -> bool { - (self.message == other.message) && (self.rx_at == other.rx_at) && (self.sender == other.sender) + (self.message == other.message) + && (self.rx_at == other.rx_at) + && (self.sender == other.sender) } } @@ -278,7 +278,12 @@ fn received_message_to_event( message: ReceivedMessage, ) -> Option> { let peer_id = message.sender.peer_id(); - let ReceivedMessage { message, sender: _sender, rx_at: _rx_at, rpc_replier } = message; + let ReceivedMessage { + message, + sender: _sender, + rx_at: _rx_at, + rpc_replier, + } = message; match message { NetworkMessage::RpcRequest(rpc_req) => { let rpc_replier = Arc::into_inner(rpc_replier.unwrap()).unwrap(); diff --git a/network/framework/src/protocols/rpc/mod.rs b/network/framework/src/protocols/rpc/mod.rs index 74d5ed1e4a2c3..b948226c4cd70 100644 --- a/network/framework/src/protocols/rpc/mod.rs +++ b/network/framework/src/protocols/rpc/mod.rs @@ -52,7 +52,7 @@ use crate::{ }, logging::NetworkSchema, protocols::{ - network::SerializedRequest, + network::{ReceivedMessage, SerializedRequest}, wire::messaging::v1::{NetworkMessage, Priority, RequestId, RpcRequest, RpcResponse}, }, ProtocolId, @@ -73,9 +73,7 @@ use futures::{ stream::{FuturesUnordered, StreamExt}, }; use serde::Serialize; -use std::{cmp::PartialEq, collections::HashMap, fmt::Debug, time::Duration}; -use std::sync::Arc; -use crate::protocols::network::ReceivedMessage; +use std::{cmp::PartialEq, collections::HashMap, fmt::Debug, sync::Arc, time::Duration}; pub mod error; @@ -227,7 +225,7 @@ impl InboundRpcs { let peer_id = request.sender.peer_id(); let NetworkMessage::RpcRequest(rpc_request) = &request.message else { - return Err(RpcError::InvalidRpcResponse) + return Err(RpcError::InvalidRpcResponse); }; let protocol_id = rpc_request.protocol_id; let request_id = rpc_request.request_id; diff --git a/network/framework/src/protocols/wire/messaging/v1/mod.rs b/network/framework/src/protocols/wire/messaging/v1/mod.rs index bbec524d7a787..b02b792524f9e 100644 --- a/network/framework/src/protocols/wire/messaging/v1/mod.rs +++ b/network/framework/src/protocols/wire/messaging/v1/mod.rs @@ -20,13 +20,12 @@ use futures::{ use pin_project::pin_project; #[cfg(any(test, feature = "fuzzing"))] use proptest_derive::Arbitrary; -use serde::{Deserialize, Serialize}; +use serde::{de::DeserializeOwned, Deserialize, Serialize}; use std::{ io, pin::Pin, task::{Context, Poll}, }; -use serde::de::DeserializeOwned; use thiserror::Error; use tokio_util::{ codec::{FramedRead, FramedWrite, LengthDelimitedCodec}, diff --git a/network/framework/src/testutils/test_node.rs b/network/framework/src/testutils/test_node.rs index 1751d086de072..d323dd8b8c703 100644 --- a/network/framework/src/testutils/test_node.rs +++ b/network/framework/src/testutils/test_node.rs @@ -8,6 +8,7 @@ use crate::{ protocols::{ network::ReceivedMessage, rpc::OutboundRpcRequest, + wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}, }, transport::ConnectionMetadata, ProtocolId, @@ -21,7 +22,6 @@ use aptos_types::PeerId; use async_trait::async_trait; use futures::StreamExt; use std::{collections::HashMap, sync::Arc, time::Duration}; -use crate::protocols::wire::messaging::v1::{DirectSendMsg, NetworkMessage, RpcRequest}; /// A sender to a node to mock an inbound network message from [`PeerManager`] pub type InboundMessageSender = @@ -310,8 +310,8 @@ pub trait TestNode: ApplicationNode + Sync { // let (remote_peer_id, protocol_id, data, maybe_rpc_info) = match request { let (remote_peer_id, protocol_id, rmsg) = match request { PeerManagerRequest::SendRpc(peer_id, msg) => { - let rmsg = ReceivedMessage{ - message: NetworkMessage::RpcRequest(RpcRequest{ + let rmsg = ReceivedMessage { + message: NetworkMessage::RpcRequest(RpcRequest { protocol_id: msg.protocol_id, request_id: 0, // TODO: rand? seq? priority: 0, @@ -330,8 +330,8 @@ pub trait TestNode: ApplicationNode + Sync { ) }, PeerManagerRequest::SendDirectSend(peer_id, msg) => { - let rmsg = ReceivedMessage{ - message: NetworkMessage::DirectSendMsg(DirectSendMsg{ + let rmsg = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg { protocol_id: msg.protocol_id, priority: 0, raw_msg: msg.mdata.into(), diff --git a/peer-monitoring-service/server/src/tests.rs b/peer-monitoring-service/server/src/tests.rs index 5aad38eda35d5..53b125388aa7b 100644 --- a/peer-monitoring-service/server/src/tests.rs +++ b/peer-monitoring-service/server/src/tests.rs @@ -16,6 +16,10 @@ use aptos_config::{ use aptos_crypto::HashValue; use aptos_logger::Level; use aptos_netcore::transport::ConnectionOrigin; +use aptos_network::protocols::{ + network::ReceivedMessage, + wire::messaging::v1::{NetworkMessage, RpcRequest}, +}; use aptos_network::{ application::{ interface::NetworkServiceEvents, metadata::ConnectionState, storage::PeersAndMetadata, @@ -69,8 +73,6 @@ use std::{ sync::Arc, time::Duration, }; -use aptos_network::protocols::network::ReceivedMessage; -use aptos_network::protocols::wire::messaging::v1::{NetworkMessage, RpcRequest}; // Useful test constants const LOCAL_HOST_NET_ADDR: &str = "/ip4/127.0.0.1/tcp/8081"; @@ -563,7 +565,7 @@ impl MockClient { // }; // let request_notification = PeerManagerNotification::RecvRpc(peer_id, inbound_rpc); let request_notification = ReceivedMessage { - message: NetworkMessage::RpcRequest(RpcRequest{ + message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, request_id: 42, // TODO? count? rand? priority: 0, diff --git a/state-sync/storage-service/server/src/tests/mock.rs b/state-sync/storage-service/server/src/tests/mock.rs index bf7aa6f3057c5..aa37005be8948 100644 --- a/state-sync/storage-service/server/src/tests/mock.rs +++ b/state-sync/storage-service/server/src/tests/mock.rs @@ -9,14 +9,17 @@ use anyhow::Result; use aptos_channels::{aptos_channel, message_queues::QueueStyle}; use aptos_config::{ config::{StateSyncConfig, StorageServiceConfig}, - network_id::NetworkId, + network_id::{NetworkId, PeerNetworkId}, }; use aptos_crypto::HashValue; use aptos_network::{ application::{interface::NetworkServiceEvents, storage::PeersAndMetadata}, protocols::{ - network::{NetworkEvents, NewNetworkEvents}, - wire::handshake::v1::ProtocolId, + network::{NetworkEvents, NewNetworkEvents, ReceivedMessage}, + wire::{ + handshake::v1::ProtocolId, + messaging::v1::{NetworkMessage, RpcRequest}, + }, }, }; use aptos_storage_interface::{DbReader, ExecutedTrees, Order}; @@ -49,9 +52,6 @@ use mockall::mock; use rand::{rngs::OsRng, Rng}; use std::{collections::HashMap, sync::Arc, time::Duration}; use tokio::time::timeout; -use aptos_config::network_id::PeerNetworkId; -use aptos_network::protocols::network::ReceivedMessage; -use aptos_network::protocols::wire::messaging::v1::{NetworkMessage, RpcRequest}; // Useful test constants const MAX_RESPONSE_TIMEOUT_SECS: u64 = 60; @@ -163,7 +163,7 @@ impl MockClient { .unwrap(); let (res_tx, res_rx) = oneshot::channel(); let notification = ReceivedMessage { - message: NetworkMessage::RpcRequest(RpcRequest{ + message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, request_id: 0, // TODO: rand? inc? priority: 0, From 3af93ab913ac33fb832cd9815cdf5f6f30558cc3 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Wed, 10 Jul 2024 10:30:01 -0400 Subject: [PATCH 05/22] delete dead code --- mempool/src/tests/test_framework.rs | 4 -- network/framework/src/peer_manager/mod.rs | 67 ------------------- network/framework/src/peer_manager/tests.rs | 1 - .../src/protocols/health_checker/test.rs | 1 - network/framework/src/testutils/test_node.rs | 25 +------ peer-monitoring-service/server/src/tests.rs | 21 ++---- 6 files changed, 8 insertions(+), 111 deletions(-) diff --git a/mempool/src/tests/test_framework.rs b/mempool/src/tests/test_framework.rs index ee89de6da9615..02d1e7e70e568 100644 --- a/mempool/src/tests/test_framework.rs +++ b/mempool/src/tests/test_framework.rs @@ -415,10 +415,6 @@ impl MempoolNode { if let Some(rpc_sender) = maybe_rpc_sender { rpc_sender.send(Ok(bytes.into())).unwrap(); } else { - // let notif = PeerManagerNotification::RecvMessage(peer_id, Message { - // protocol_id, - // mdata: bytes.into(), - // }); let notif = ReceivedMessage { message: NetworkMessage::DirectSendMsg(DirectSendMsg { protocol_id, diff --git a/network/framework/src/peer_manager/mod.rs b/network/framework/src/peer_manager/mod.rs index 52dd6fa568483..90988a5a29c32 100644 --- a/network/framework/src/peer_manager/mod.rs +++ b/network/framework/src/peer_manager/mod.rs @@ -704,71 +704,4 @@ where } } } - - #[cfg(obsolete)] - fn spawn_peer_network_events_handler( - &self, - peer_id: PeerId, - network_events: aptos_channel::Receiver, - ) { - let mut upstream_handlers = self.upstream_handlers.clone(); - let network_context = self.network_context; - self.executor.spawn(network_events.for_each_concurrent( - self.max_concurrent_network_reqs, - move |inbound_event| { - handle_inbound_request( - network_context, - inbound_event, - peer_id, - &mut upstream_handlers, - ); - futures::future::ready(()) - }, - )); - } -} - -/// A task for consuming inbound network messages -#[cfg(obsolete)] -fn handle_inbound_request( - network_context: NetworkContext, - inbound_event: PeerNotification, - peer_id: PeerId, - upstream_handlers: &mut HashMap< - ProtocolId, - aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, - >, -) { - let (protocol_id, notification) = match inbound_event { - PeerNotification::RecvMessage(msg) => ( - msg.protocol_id(), - PeerManagerNotification::RecvMessage(peer_id, msg), - ), - PeerNotification::RecvRpc(req) => ( - req.protocol_id(), - PeerManagerNotification::RecvRpc(peer_id, req), - ), - }; - - if let Some(handler) = upstream_handlers.get_mut(&protocol_id) { - // Send over aptos channel for fairness. - if let Err(err) = handler.push((peer_id, protocol_id), notification) { - warn!( - NetworkSchema::new(&network_context), - error = ?err, - protocol_id = protocol_id, - "{} Upstream handler unable to handle message for protocol: {}. Error: {:?}", - network_context, protocol_id, err - ); - } - } else { - debug!( - NetworkSchema::new(&network_context), - protocol_id = protocol_id, - message = format!("{:?}", notification), - "{} Received network message for unregistered protocol: {:?}", - network_context, - notification, - ); - } } diff --git a/network/framework/src/peer_manager/tests.rs b/network/framework/src/peer_manager/tests.rs index 35a7ed693fd21..4b95ec1e7658b 100644 --- a/network/framework/src/peer_manager/tests.rs +++ b/network/framework/src/peer_manager/tests.rs @@ -89,7 +89,6 @@ fn build_test_peer_manager( >, aptos_channel::Sender<(PeerId, ProtocolId), PeerManagerRequest>, aptos_channel::Sender, - // aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerNotification>, conn_notifs_channel::Receiver, ) { let (peer_manager_request_tx, peer_manager_request_rx) = diff --git a/network/framework/src/protocols/health_checker/test.rs b/network/framework/src/protocols/health_checker/test.rs index d4978ebcfe4ef..eeef6d2492464 100644 --- a/network/framework/src/protocols/health_checker/test.rs +++ b/network/framework/src/protocols/health_checker/test.rs @@ -148,7 +148,6 @@ impl TestHarness { self.peer_mgr_notifs_tx .push_with_feedback( key, - // PeerManagerNotification::RecvRpc(peer_id, inbound_rpc_req), ReceivedMessage { message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, diff --git a/network/framework/src/testutils/test_node.rs b/network/framework/src/testutils/test_node.rs index d323dd8b8c703..bb95b8cb9a10b 100644 --- a/network/framework/src/testutils/test_node.rs +++ b/network/framework/src/testutils/test_node.rs @@ -307,13 +307,12 @@ pub trait TestNode: ApplicationNode + Sync { async fn send_next_network_msg(&mut self, network_id: NetworkId) { let request = self.get_next_network_msg(network_id).await; - // let (remote_peer_id, protocol_id, data, maybe_rpc_info) = match request { let (remote_peer_id, protocol_id, rmsg) = match request { PeerManagerRequest::SendRpc(peer_id, msg) => { let rmsg = ReceivedMessage { message: NetworkMessage::RpcRequest(RpcRequest { protocol_id: msg.protocol_id, - request_id: 0, // TODO: rand? seq? + request_id: 0, priority: 0, raw_request: msg.data.into(), }), @@ -321,13 +320,7 @@ pub trait TestNode: ApplicationNode + Sync { rx_at: 0, rpc_replier: Some(Arc::new(msg.res_tx)), }; - ( - peer_id, - msg.protocol_id, - // msg.data, - // Some((msg.timeout, msg.res_tx)), - rmsg, - ) + (peer_id, msg.protocol_id, rmsg) }, PeerManagerRequest::SendDirectSend(peer_id, msg) => { let rmsg = ReceivedMessage { @@ -341,7 +334,6 @@ pub trait TestNode: ApplicationNode + Sync { rpc_replier: None, }; (peer_id, msg.protocol_id, rmsg) - // (peer_id, msg.protocol_id, msg.mdata, None) }, }; @@ -350,19 +342,6 @@ pub trait TestNode: ApplicationNode + Sync { let receiver_handle = self.get_inbound_handle_for_peer(receiver_peer_network_id); let sender_peer_id = sender_peer_network_id.peer_id(); - // TODO: Add timeout functionality - // let peer_manager_notif = if let Some((_timeout, res_tx)) = maybe_rpc_info { - // PeerManagerNotification::RecvRpc(sender_peer_id, InboundRpcRequest { - // protocol_id, - // data, - // res_tx, - // }) - // } else { - // PeerManagerNotification::RecvMessage(sender_peer_id, Message { - // protocol_id, - // mdata: data, - // }) - // }; receiver_handle .inbound_message_sender .push((sender_peer_id, protocol_id), rmsg) diff --git a/peer-monitoring-service/server/src/tests.rs b/peer-monitoring-service/server/src/tests.rs index 53b125388aa7b..0e65c551f8478 100644 --- a/peer-monitoring-service/server/src/tests.rs +++ b/peer-monitoring-service/server/src/tests.rs @@ -16,19 +16,16 @@ use aptos_config::{ use aptos_crypto::HashValue; use aptos_logger::Level; use aptos_netcore::transport::ConnectionOrigin; -use aptos_network::protocols::{ - network::ReceivedMessage, - wire::messaging::v1::{NetworkMessage, RpcRequest}, -}; use aptos_network::{ application::{ interface::NetworkServiceEvents, metadata::ConnectionState, storage::PeersAndMetadata, }, - // peer_manager::PeerManagerNotification, protocols::{ - network::{NetworkEvents, NewNetworkEvents}, - // rpc::InboundRpcRequest, - wire::handshake::v1::{MessagingProtocolVersion, ProtocolId, ProtocolIdSet}, + network::{NetworkEvents, NewNetworkEvents, ReceivedMessage}, + wire::{ + handshake::v1::{MessagingProtocolVersion, ProtocolId, ProtocolIdSet}, + messaging::v1::{NetworkMessage, RpcRequest}, + }, }, transport::{ConnectionId, ConnectionMetadata}, }; @@ -558,16 +555,10 @@ impl MockClient { .to_bytes(&PeerMonitoringServiceMessage::Request(request)) .unwrap(); let (request_sender, request_receiver) = oneshot::channel(); - // let inbound_rpc = InboundRpcRequest { - // protocol_id, - // data: request_data.into(), - // res_tx: request_sender, - // }; - // let request_notification = PeerManagerNotification::RecvRpc(peer_id, inbound_rpc); let request_notification = ReceivedMessage { message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, - request_id: 42, // TODO? count? rand? + request_id: 42, priority: 0, raw_request: request_data.clone(), }), From b645b2e3813bed2de74f140a3d3956113088357f Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Wed, 10 Jul 2024 11:38:31 -0400 Subject: [PATCH 06/22] PeerNotification goes away --- network/framework/src/peer/mod.rs | 11 +------ network/framework/src/peer/test.rs | 31 +++---------------- .../framework/src/protocols/network/mod.rs | 14 +++------ 3 files changed, 9 insertions(+), 47 deletions(-) diff --git a/network/framework/src/peer/mod.rs b/network/framework/src/peer/mod.rs index 182b2e6eba11d..eaa1ffe0e8f01 100644 --- a/network/framework/src/peer/mod.rs +++ b/network/framework/src/peer/mod.rs @@ -25,7 +25,7 @@ use crate::{ protocols::{ direct_send::Message, network::ReceivedMessage, - rpc::{error::RpcError, InboundRpcRequest, InboundRpcs, OutboundRpcRequest, OutboundRpcs}, + rpc::{error::RpcError, InboundRpcs, OutboundRpcRequest, OutboundRpcs}, stream::{InboundStreamBuffer, OutboundStream, StreamMessage}, wire::messaging::v1::{ DirectSendMsg, ErrorCode, MultiplexMessage, MultiplexMessageSink, @@ -71,15 +71,6 @@ pub enum PeerRequest { SendDirectSend(Message), } -/// Notifications that [`Peer`] sends to the [`PeerManager`](crate::peer_manager::PeerManager). -#[derive(Debug, PartialEq)] -pub enum PeerNotification { - /// A new RPC request has been received from peer. - RecvRpc(InboundRpcRequest), - /// A new message has been received from peer. - RecvMessage(Message), -} - /// The reason for closing a connection. /// /// For example, if the remote peer closed the connection or the connection was diff --git a/network/framework/src/peer/test.rs b/network/framework/src/peer/test.rs index 9750e90e3c4b0..8daac6db9f6bd 100644 --- a/network/framework/src/peer/test.rs +++ b/network/framework/src/peer/test.rs @@ -64,7 +64,6 @@ fn build_test_peer( PeerHandle, MemorySocket, aptos_channels::Receiver>, - // aptos_channel::Receiver, ) { let (a, b) = MemorySocket::new_pair(); let peer_id = PeerId::random(); @@ -408,11 +407,6 @@ fn peer_recv_rpc() { priority: 0, raw_request: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - // protocol_id: PROTOCOL, - // data: Bytes::from("hello world"), - // res_tx: oneshot::channel().0, - // }); let resp_msg = MultiplexMessage::Message(NetworkMessage::RpcResponse(RpcResponse { request_id: 123, priority: 0, @@ -458,7 +452,7 @@ fn peer_recv_rpc() { .expect("Arc unpack fail"); rpc_replier.send(response).expect("rpc reply send fail") }, - msg => panic!("Unexpected PeerNotification: {:?}", msg), + msg => panic!("Unexpected NetworkMessage: {:?}", msg), } } }; @@ -487,11 +481,6 @@ fn peer_recv_rpc_concurrent() { priority: 0, raw_request: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - // protocol_id: PROTOCOL, - // data: Bytes::from("hello world"), - // res_tx: oneshot::channel().0, - // }); let resp_msg = MultiplexMessage::Message(NetworkMessage::RpcResponse(RpcResponse { request_id: 123, priority: 0, @@ -519,16 +508,14 @@ fn peer_recv_rpc_concurrent() { // Wait to receive RpcRequests from Peer. for _ in 0..30 { let received = prot_rx.next().await.unwrap(); - // assert_eq!(recv_msg, received); match &received.message { - // PeerNotification::RecvRpc(req) => res_txs.push(req.res_tx), NetworkMessage::RpcRequest(req) => { assert_eq!(Vec::from("hello world"), req.raw_request); let arcsender = received.rpc_replier.unwrap(); let sender = Arc::into_inner(arcsender).unwrap(); res_txs.push(sender) }, - _ => panic!("Unexpected PeerNotification: {:?}", received), + _ => panic!("Unexpected NetworkMessage: {:?}", received), }; } @@ -564,11 +551,6 @@ fn peer_recv_rpc_timeout() { priority: 0, raw_request: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - // protocol_id: PROTOCOL, - // data: Bytes::from("hello world"), - // res_tx: oneshot::channel().0, - // }); let test = async move { // Client sends the rpc request. @@ -585,7 +567,7 @@ fn peer_recv_rpc_timeout() { let arcsender = received.rpc_replier.unwrap(); Arc::into_inner(arcsender).unwrap() }, - _ => panic!("Unexpected PeerNotification: {:?}", received), + _ => panic!("Unexpected NetworkMessage: {:?}", received), }; // The rpc response channel should still be open since we haven't timed out yet. @@ -630,11 +612,6 @@ fn peer_recv_rpc_cancel() { priority: 0, raw_request: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - // protocol_id: PROTOCOL, - // data: Bytes::from("hello world"), - // res_tx: oneshot::channel().0, - // }); let test = async move { // Client sends the rpc request. @@ -651,7 +628,7 @@ fn peer_recv_rpc_cancel() { let arcsender = received.rpc_replier.unwrap(); Arc::into_inner(arcsender).unwrap() }, - _ => panic!("Unexpected PeerNotification: {:?}", received), + _ => panic!("Unexpected NetworkMessage: {:?}", received), }; // The rpc response channel should still be open since we haven't timed out yet. diff --git a/network/framework/src/protocols/network/mod.rs b/network/framework/src/protocols/network/mod.rs index e83103c83d6d8..ead5d136960f6 100644 --- a/network/framework/src/protocols/network/mod.rs +++ b/network/framework/src/protocols/network/mod.rs @@ -32,13 +32,10 @@ impl Message for T {} /// Events received by network clients in a validator /// -/// An enumeration of the various types of messages that the network will be sending -/// to its clients. This differs from [`PeerNotification`] since the contents are deserialized -/// into the type `TMessage` over which `Event` is generic. Note that we assume here that for every -/// consumer of this API there's a singleton message type, `TMessage`, which encapsulates all the -/// messages and RPCs that are received by that consumer. -/// -/// [`PeerNotification`]: crate::peer::PeerNotification +/// We assume here that for every consumer of this API there's a singleton message type, `TMessage`, +/// which encapsulates all the messages and RPCs that are received by that consumer. +/// This struct is received by application code after a message of bytes has been deserialized +/// into the application code's TMessage struct type. #[derive(Debug)] pub enum Event { /// New inbound direct-send message from peer. @@ -195,9 +192,6 @@ impl PartialEq for ReceivedMessage { /// network application that deserializes inbound network direct-send and rpc /// messages into `TMessage`. Inbound messages that fail to deserialize are logged /// and dropped. -/// -/// `NetworkEvents` is really just a thin wrapper around a -/// `channel::Receiver` that deserializes inbound messages. #[pin_project] pub struct NetworkEvents { #[pin] From 841d9092e04e82e51bd2c140ddc0e348f6fc639b Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Wed, 10 Jul 2024 11:38:31 -0400 Subject: [PATCH 07/22] PeerNotification goes away --- network/framework/src/peer/mod.rs | 11 +------ network/framework/src/peer/test.rs | 31 +++---------------- .../framework/src/protocols/network/mod.rs | 14 +++------ 3 files changed, 9 insertions(+), 47 deletions(-) diff --git a/network/framework/src/peer/mod.rs b/network/framework/src/peer/mod.rs index 182b2e6eba11d..eaa1ffe0e8f01 100644 --- a/network/framework/src/peer/mod.rs +++ b/network/framework/src/peer/mod.rs @@ -25,7 +25,7 @@ use crate::{ protocols::{ direct_send::Message, network::ReceivedMessage, - rpc::{error::RpcError, InboundRpcRequest, InboundRpcs, OutboundRpcRequest, OutboundRpcs}, + rpc::{error::RpcError, InboundRpcs, OutboundRpcRequest, OutboundRpcs}, stream::{InboundStreamBuffer, OutboundStream, StreamMessage}, wire::messaging::v1::{ DirectSendMsg, ErrorCode, MultiplexMessage, MultiplexMessageSink, @@ -71,15 +71,6 @@ pub enum PeerRequest { SendDirectSend(Message), } -/// Notifications that [`Peer`] sends to the [`PeerManager`](crate::peer_manager::PeerManager). -#[derive(Debug, PartialEq)] -pub enum PeerNotification { - /// A new RPC request has been received from peer. - RecvRpc(InboundRpcRequest), - /// A new message has been received from peer. - RecvMessage(Message), -} - /// The reason for closing a connection. /// /// For example, if the remote peer closed the connection or the connection was diff --git a/network/framework/src/peer/test.rs b/network/framework/src/peer/test.rs index 9750e90e3c4b0..8daac6db9f6bd 100644 --- a/network/framework/src/peer/test.rs +++ b/network/framework/src/peer/test.rs @@ -64,7 +64,6 @@ fn build_test_peer( PeerHandle, MemorySocket, aptos_channels::Receiver>, - // aptos_channel::Receiver, ) { let (a, b) = MemorySocket::new_pair(); let peer_id = PeerId::random(); @@ -408,11 +407,6 @@ fn peer_recv_rpc() { priority: 0, raw_request: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - // protocol_id: PROTOCOL, - // data: Bytes::from("hello world"), - // res_tx: oneshot::channel().0, - // }); let resp_msg = MultiplexMessage::Message(NetworkMessage::RpcResponse(RpcResponse { request_id: 123, priority: 0, @@ -458,7 +452,7 @@ fn peer_recv_rpc() { .expect("Arc unpack fail"); rpc_replier.send(response).expect("rpc reply send fail") }, - msg => panic!("Unexpected PeerNotification: {:?}", msg), + msg => panic!("Unexpected NetworkMessage: {:?}", msg), } } }; @@ -487,11 +481,6 @@ fn peer_recv_rpc_concurrent() { priority: 0, raw_request: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - // protocol_id: PROTOCOL, - // data: Bytes::from("hello world"), - // res_tx: oneshot::channel().0, - // }); let resp_msg = MultiplexMessage::Message(NetworkMessage::RpcResponse(RpcResponse { request_id: 123, priority: 0, @@ -519,16 +508,14 @@ fn peer_recv_rpc_concurrent() { // Wait to receive RpcRequests from Peer. for _ in 0..30 { let received = prot_rx.next().await.unwrap(); - // assert_eq!(recv_msg, received); match &received.message { - // PeerNotification::RecvRpc(req) => res_txs.push(req.res_tx), NetworkMessage::RpcRequest(req) => { assert_eq!(Vec::from("hello world"), req.raw_request); let arcsender = received.rpc_replier.unwrap(); let sender = Arc::into_inner(arcsender).unwrap(); res_txs.push(sender) }, - _ => panic!("Unexpected PeerNotification: {:?}", received), + _ => panic!("Unexpected NetworkMessage: {:?}", received), }; } @@ -564,11 +551,6 @@ fn peer_recv_rpc_timeout() { priority: 0, raw_request: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - // protocol_id: PROTOCOL, - // data: Bytes::from("hello world"), - // res_tx: oneshot::channel().0, - // }); let test = async move { // Client sends the rpc request. @@ -585,7 +567,7 @@ fn peer_recv_rpc_timeout() { let arcsender = received.rpc_replier.unwrap(); Arc::into_inner(arcsender).unwrap() }, - _ => panic!("Unexpected PeerNotification: {:?}", received), + _ => panic!("Unexpected NetworkMessage: {:?}", received), }; // The rpc response channel should still be open since we haven't timed out yet. @@ -630,11 +612,6 @@ fn peer_recv_rpc_cancel() { priority: 0, raw_request: Vec::from("hello world"), })); - // let recv_msg = PeerNotification::RecvRpc(InboundRpcRequest { - // protocol_id: PROTOCOL, - // data: Bytes::from("hello world"), - // res_tx: oneshot::channel().0, - // }); let test = async move { // Client sends the rpc request. @@ -651,7 +628,7 @@ fn peer_recv_rpc_cancel() { let arcsender = received.rpc_replier.unwrap(); Arc::into_inner(arcsender).unwrap() }, - _ => panic!("Unexpected PeerNotification: {:?}", received), + _ => panic!("Unexpected NetworkMessage: {:?}", received), }; // The rpc response channel should still be open since we haven't timed out yet. diff --git a/network/framework/src/protocols/network/mod.rs b/network/framework/src/protocols/network/mod.rs index e83103c83d6d8..ead5d136960f6 100644 --- a/network/framework/src/protocols/network/mod.rs +++ b/network/framework/src/protocols/network/mod.rs @@ -32,13 +32,10 @@ impl Message for T {} /// Events received by network clients in a validator /// -/// An enumeration of the various types of messages that the network will be sending -/// to its clients. This differs from [`PeerNotification`] since the contents are deserialized -/// into the type `TMessage` over which `Event` is generic. Note that we assume here that for every -/// consumer of this API there's a singleton message type, `TMessage`, which encapsulates all the -/// messages and RPCs that are received by that consumer. -/// -/// [`PeerNotification`]: crate::peer::PeerNotification +/// We assume here that for every consumer of this API there's a singleton message type, `TMessage`, +/// which encapsulates all the messages and RPCs that are received by that consumer. +/// This struct is received by application code after a message of bytes has been deserialized +/// into the application code's TMessage struct type. #[derive(Debug)] pub enum Event { /// New inbound direct-send message from peer. @@ -195,9 +192,6 @@ impl PartialEq for ReceivedMessage { /// network application that deserializes inbound network direct-send and rpc /// messages into `TMessage`. Inbound messages that fail to deserialize are logged /// and dropped. -/// -/// `NetworkEvents` is really just a thin wrapper around a -/// `channel::Receiver` that deserializes inbound messages. #[pin_project] pub struct NetworkEvents { #[pin] From dac4777435f2d03c7b19ea06874608121e9faa0a Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Wed, 10 Jul 2024 13:32:09 -0400 Subject: [PATCH 08/22] comment deprecate PeerManagerNotification --- network/framework/src/peer_manager/types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/network/framework/src/peer_manager/types.rs b/network/framework/src/peer_manager/types.rs index c41d2c4c3f82e..36e661017d7a5 100644 --- a/network/framework/src/peer_manager/types.rs +++ b/network/framework/src/peer_manager/types.rs @@ -26,6 +26,7 @@ pub enum PeerManagerRequest { } /// Notifications sent by PeerManager to upstream actors. +/// TODO: PeerManagerNotification now only exists in test code and should be deleted; probably use `ReceivedMessage` #[derive(Debug)] pub enum PeerManagerNotification { /// A new RPC request has been received from a remote peer. From 62b6dff8c2ea902d6c6d98782325a4ac18bbef66 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Wed, 10 Jul 2024 15:13:21 -0400 Subject: [PATCH 09/22] add inbound queue delay metric aptos_network_inbound_queue_time --- network/framework/src/counters.rs | 14 ++++++++++++++ network/framework/src/protocols/network/mod.rs | 10 +++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/network/framework/src/counters.rs b/network/framework/src/counters.rs index 233bee9c00062..96eec3f284c97 100644 --- a/network/framework/src/counters.rs +++ b/network/framework/src/counters.rs @@ -638,3 +638,17 @@ pub static OP_MEASURE: Lazy = Lazy::new(|| { ) .unwrap() }); + +pub static INBOUND_QUEUE_DELAY: Lazy = Lazy::new(|| { + register_histogram_vec!( + "aptos_network_inbound_queue_time", + "Time a message sits in queue between peer socket and app code", + &["protocol_id"], + exponential_buckets(/*start=*/ 1e-6, /*factor=*/ 2.0, /*count=*/ 20).unwrap(), + ) + .unwrap() +}); + +pub fn inbound_queue_delay_observe(protocol_id: ProtocolId, micros: u64) { + INBOUND_QUEUE_DELAY.with_label_values(&[protocol_id.as_str()]).observe((micros as f64) / 1000000.0) +} diff --git a/network/framework/src/protocols/network/mod.rs b/network/framework/src/protocols/network/mod.rs index ead5d136960f6..7b446f1a3def4 100644 --- a/network/framework/src/protocols/network/mod.rs +++ b/network/framework/src/protocols/network/mod.rs @@ -275,16 +275,24 @@ fn received_message_to_event( let ReceivedMessage { message, sender: _sender, - rx_at: _rx_at, + rx_at, rpc_replier, } = message; + let now = std::time::SystemTime::now(); + let dequeue_at = now + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_micros() as u64; + let dt_micros = dequeue_at - rx_at; match message { NetworkMessage::RpcRequest(rpc_req) => { + crate::counters::inbound_queue_delay_observe(rpc_req.protocol_id, dt_micros); let rpc_replier = Arc::into_inner(rpc_replier.unwrap()).unwrap(); request_to_network_event(peer_id, &rpc_req) .map(|msg| Event::RpcRequest(peer_id, msg, rpc_req.protocol_id, rpc_replier)) }, NetworkMessage::DirectSendMsg(request) => { + crate::counters::inbound_queue_delay_observe(request.protocol_id, dt_micros); request_to_network_event(peer_id, &request).map(|msg| Event::Message(peer_id, msg)) }, _ => None, From 026c14a30b2a2259e1f4c676007ecd641b0c0d8b Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Wed, 10 Jul 2024 16:12:45 -0400 Subject: [PATCH 10/22] fmt --- network/framework/src/counters.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/network/framework/src/counters.rs b/network/framework/src/counters.rs index 96eec3f284c97..057b7fd9e3940 100644 --- a/network/framework/src/counters.rs +++ b/network/framework/src/counters.rs @@ -646,9 +646,11 @@ pub static INBOUND_QUEUE_DELAY: Lazy = Lazy::new(|| { &["protocol_id"], exponential_buckets(/*start=*/ 1e-6, /*factor=*/ 2.0, /*count=*/ 20).unwrap(), ) - .unwrap() + .unwrap() }); pub fn inbound_queue_delay_observe(protocol_id: ProtocolId, micros: u64) { - INBOUND_QUEUE_DELAY.with_label_values(&[protocol_id.as_str()]).observe((micros as f64) / 1000000.0) + INBOUND_QUEUE_DELAY + .with_label_values(&[protocol_id.as_str()]) + .observe((micros as f64) / 1000000.0) } From 085901a0a8648af152e18dd7d1f2c4e02599de85 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Thu, 11 Jul 2024 14:11:10 -0400 Subject: [PATCH 11/22] fmt --- consensus/src/network_tests.rs | 1 - network/framework/src/counters.rs | 4 ++-- network/framework/src/protocols/health_checker/test.rs | 5 ----- network/framework/src/protocols/network/mod.rs | 5 +++-- 4 files changed, 5 insertions(+), 10 deletions(-) diff --git a/consensus/src/network_tests.rs b/consensus/src/network_tests.rs index 0c783cc48f928..075d5b4960176 100644 --- a/consensus/src/network_tests.rs +++ b/consensus/src/network_tests.rs @@ -507,7 +507,6 @@ mod tests { storage::PeersAndMetadata, }, protocols::{ - // direct_send::Message, network, network::{NetworkEvents, NewNetworkSender}, }, diff --git a/network/framework/src/counters.rs b/network/framework/src/counters.rs index 057b7fd9e3940..1d748bfb4c038 100644 --- a/network/framework/src/counters.rs +++ b/network/framework/src/counters.rs @@ -649,8 +649,8 @@ pub static INBOUND_QUEUE_DELAY: Lazy = Lazy::new(|| { .unwrap() }); -pub fn inbound_queue_delay_observe(protocol_id: ProtocolId, micros: u64) { +pub fn inbound_queue_delay_observe(protocol_id: ProtocolId, seconds: f64) { INBOUND_QUEUE_DELAY .with_label_values(&[protocol_id.as_str()]) - .observe((micros as f64) / 1000000.0) + .observe(seconds) } diff --git a/network/framework/src/protocols/health_checker/test.rs b/network/framework/src/protocols/health_checker/test.rs index eeef6d2492464..0159bb3a147a3 100644 --- a/network/framework/src/protocols/health_checker/test.rs +++ b/network/framework/src/protocols/health_checker/test.rs @@ -138,11 +138,6 @@ impl TestHarness { let data = bcs::to_bytes(&HealthCheckerMsg::Ping(Ping(ping))).unwrap(); // .into(); let (res_tx, res_rx) = oneshot::channel(); - // let inbound_rpc_req = InboundRpcRequest { - // protocol_id, - // data, - // res_tx, - // }; let key = (peer_id, ProtocolId::HealthCheckerRpc); let (delivered_tx, delivered_rx) = oneshot::channel(); self.peer_mgr_notifs_tx diff --git a/network/framework/src/protocols/network/mod.rs b/network/framework/src/protocols/network/mod.rs index 7b446f1a3def4..d707fb0189dcb 100644 --- a/network/framework/src/protocols/network/mod.rs +++ b/network/framework/src/protocols/network/mod.rs @@ -284,15 +284,16 @@ fn received_message_to_event( .unwrap() .as_micros() as u64; let dt_micros = dequeue_at - rx_at; + let dt_seconds = (dt_micros as f64) / 1000000.0; match message { NetworkMessage::RpcRequest(rpc_req) => { - crate::counters::inbound_queue_delay_observe(rpc_req.protocol_id, dt_micros); + crate::counters::inbound_queue_delay_observe(rpc_req.protocol_id, dt_seconds); let rpc_replier = Arc::into_inner(rpc_replier.unwrap()).unwrap(); request_to_network_event(peer_id, &rpc_req) .map(|msg| Event::RpcRequest(peer_id, msg, rpc_req.protocol_id, rpc_replier)) }, NetworkMessage::DirectSendMsg(request) => { - crate::counters::inbound_queue_delay_observe(request.protocol_id, dt_micros); + crate::counters::inbound_queue_delay_observe(request.protocol_id, dt_seconds); request_to_network_event(peer_id, &request).map(|msg| Event::Message(peer_id, msg)) }, _ => None, From aec0b060972aa8cc0d36d7aee8bbfb7f544f813d Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Thu, 11 Jul 2024 14:55:27 -0400 Subject: [PATCH 12/22] rx_at -> receive_timestamp_micros --- consensus/src/network_tests.rs | 8 ++++---- mempool/src/tests/test_framework.rs | 6 +++--- network/framework/src/application/tests.rs | 4 ++-- network/framework/src/peer/test.rs | 2 +- network/framework/src/protocols/health_checker/test.rs | 2 +- network/framework/src/protocols/network/mod.rs | 8 ++++---- network/framework/src/testutils/test_node.rs | 4 ++-- peer-monitoring-service/server/src/tests.rs | 2 +- state-sync/storage-service/server/src/tests/mock.rs | 2 +- 9 files changed, 19 insertions(+), 19 deletions(-) diff --git a/consensus/src/network_tests.rs b/consensus/src/network_tests.rs index 075d5b4960176..be9ff4ac3db63 100644 --- a/consensus/src/network_tests.rs +++ b/consensus/src/network_tests.rs @@ -171,7 +171,7 @@ impl NetworkPlayground { NetworkId::Validator, src_twin_id.author, ), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: Some(Arc::new(outbound_req.res_tx)), }, ) @@ -237,7 +237,7 @@ impl NetworkPlayground { raw_msg: msg.mdata.clone().into(), }), sender: PeerNetworkId::new(NetworkId::Validator, *src), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: None, }; let msg: ConsensusMsg = msg.to_message().unwrap(); @@ -831,7 +831,7 @@ mod tests { raw_msg: Bytes::from_static(b"\xde\xad\xbe\xef").into(), }), sender: PeerNetworkId::new(NetworkId::Validator, peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: None, }; @@ -853,7 +853,7 @@ mod tests { raw_request: Bytes::from(serde_json::to_vec(&liveness_check_msg).unwrap()).into(), }), sender: PeerNetworkId::new(NetworkId::Validator, peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: Some(Arc::new(res_tx)), }; diff --git a/mempool/src/tests/test_framework.rs b/mempool/src/tests/test_framework.rs index 02d1e7e70e568..d45bb6a14dae5 100644 --- a/mempool/src/tests/test_framework.rs +++ b/mempool/src/tests/test_framework.rs @@ -269,7 +269,7 @@ impl MempoolNode { raw_msg: data, }), sender: PeerNetworkId::new(network_id, remote_peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: None, }, None, @@ -284,7 +284,7 @@ impl MempoolNode { raw_request: data, }), sender: PeerNetworkId::new(network_id, remote_peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: Some(Arc::new(res_tx)), }; (rmsg, Some(res_rx)) @@ -422,7 +422,7 @@ impl MempoolNode { raw_msg: bytes, }), sender: PeerNetworkId::new(network_id, peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: None, }; inbound_handle diff --git a/network/framework/src/application/tests.rs b/network/framework/src/application/tests.rs index d6f41df4b188f..91fb0650b04e8 100644 --- a/network/framework/src/application/tests.rs +++ b/network/framework/src/application/tests.rs @@ -1218,7 +1218,7 @@ async fn wait_for_network_event( raw_request: outbound_rpc_request.data.into(), }), sender: PeerNetworkId::new(expected_network_id, peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: Some(Arc::new(outbound_rpc_request.res_tx)), }; (outbound_rpc_request.protocol_id, rmsg) @@ -1237,7 +1237,7 @@ async fn wait_for_network_event( raw_msg: message.mdata.into(), }), sender: PeerNetworkId::new(expected_network_id, peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: None, }; (message.protocol_id, rmsg) diff --git a/network/framework/src/peer/test.rs b/network/framework/src/peer/test.rs index 8daac6db9f6bd..e0d28e3e7a91f 100644 --- a/network/framework/src/peer/test.rs +++ b/network/framework/src/peer/test.rs @@ -431,7 +431,7 @@ fn peer_recv_rpc() { let ReceivedMessage { message, sender: _sender, - rx_at: _rx_at, + receive_timestamp_micros: _rx_at, rpc_replier, } = received; assert_eq!( diff --git a/network/framework/src/protocols/health_checker/test.rs b/network/framework/src/protocols/health_checker/test.rs index 0159bb3a147a3..7eb71c9d3195a 100644 --- a/network/framework/src/protocols/health_checker/test.rs +++ b/network/framework/src/protocols/health_checker/test.rs @@ -151,7 +151,7 @@ impl TestHarness { raw_request: data, }), sender: PeerNetworkId::new(NetworkId::Validator, peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: Some(Arc::new(res_tx)), }, Some(delivered_tx), diff --git a/network/framework/src/protocols/network/mod.rs b/network/framework/src/protocols/network/mod.rs index d707fb0189dcb..eed3aedcb3214 100644 --- a/network/framework/src/protocols/network/mod.rs +++ b/network/framework/src/protocols/network/mod.rs @@ -138,7 +138,7 @@ pub struct ReceivedMessage { pub sender: PeerNetworkId, // unix microseconds - pub rx_at: u64, + pub receive_timestamp_micros: u64, pub rpc_replier: Option>>>, } @@ -153,7 +153,7 @@ impl ReceivedMessage { Self { message, sender, - rx_at, + receive_timestamp_micros: rx_at, rpc_replier: None, } } @@ -183,7 +183,7 @@ impl ReceivedMessage { impl PartialEq for ReceivedMessage { fn eq(&self, other: &Self) -> bool { (self.message == other.message) - && (self.rx_at == other.rx_at) + && (self.receive_timestamp_micros == other.receive_timestamp_micros) && (self.sender == other.sender) } } @@ -275,7 +275,7 @@ fn received_message_to_event( let ReceivedMessage { message, sender: _sender, - rx_at, + receive_timestamp_micros: rx_at, rpc_replier, } = message; let now = std::time::SystemTime::now(); diff --git a/network/framework/src/testutils/test_node.rs b/network/framework/src/testutils/test_node.rs index bb95b8cb9a10b..03b69d34ccbd1 100644 --- a/network/framework/src/testutils/test_node.rs +++ b/network/framework/src/testutils/test_node.rs @@ -317,7 +317,7 @@ pub trait TestNode: ApplicationNode + Sync { raw_request: msg.data.into(), }), sender: self.peer_network_id(network_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: Some(Arc::new(msg.res_tx)), }; (peer_id, msg.protocol_id, rmsg) @@ -330,7 +330,7 @@ pub trait TestNode: ApplicationNode + Sync { raw_msg: msg.mdata.into(), }), sender: self.peer_network_id(network_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: None, }; (peer_id, msg.protocol_id, rmsg) diff --git a/peer-monitoring-service/server/src/tests.rs b/peer-monitoring-service/server/src/tests.rs index 0e65c551f8478..7e43211d53322 100644 --- a/peer-monitoring-service/server/src/tests.rs +++ b/peer-monitoring-service/server/src/tests.rs @@ -563,7 +563,7 @@ impl MockClient { raw_request: request_data.clone(), }), sender: PeerNetworkId::new(network_id, peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: Some(Arc::new(request_sender)), }; diff --git a/state-sync/storage-service/server/src/tests/mock.rs b/state-sync/storage-service/server/src/tests/mock.rs index aa37005be8948..b5d049de78e6a 100644 --- a/state-sync/storage-service/server/src/tests/mock.rs +++ b/state-sync/storage-service/server/src/tests/mock.rs @@ -170,7 +170,7 @@ impl MockClient { raw_request: data, }), sender: PeerNetworkId::new(network_id, peer_id), - rx_at: 0, + receive_timestamp_micros: 0, rpc_replier: Some(Arc::new(res_tx)), }; From adce73282680ed56c604700e7fc10e8625688087 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Mon, 15 Jul 2024 09:19:58 -0400 Subject: [PATCH 13/22] less PeerManagerNotification --- consensus/src/network_tests.rs | 73 ++++++++++--------- .../src/protocols/health_checker/mod.rs | 6 +- 2 files changed, 40 insertions(+), 39 deletions(-) diff --git a/consensus/src/network_tests.rs b/consensus/src/network_tests.rs index f11a3cfb094fe..f1d18d90d6e7c 100644 --- a/consensus/src/network_tests.rs +++ b/consensus/src/network_tests.rs @@ -21,10 +21,7 @@ use aptos_consensus_types::{ use aptos_infallible::{Mutex, RwLock}; use aptos_network::{ application::storage::PeersAndMetadata, - peer_manager::{ - ConnectionRequestSender, PeerManagerNotification, PeerManagerRequest, - PeerManagerRequestSender, - }, + peer_manager::{ConnectionRequestSender, PeerManagerRequest, PeerManagerRequestSender}, protocols::{ network::{NewNetworkEvents, ReceivedMessage, RpcError, SerializedRequest}, wire::{ @@ -232,7 +229,7 @@ impl NetworkPlayground { &mut self, src_twin_id: TwinId, dst_twin_id: TwinId, - msg_notif: PeerManagerNotification, + rmsg: ReceivedMessage, ) -> (Author, ConsensusMsg) { let node_consensus_tx = self .node_consensus_txs @@ -242,31 +239,24 @@ impl NetworkPlayground { .clone(); // copy message data - let (source_address, msg, rmsg) = match &msg_notif { - PeerManagerNotification::RecvMessage(src, msg) => { - let rmsg = ReceivedMessage { - message: NetworkMessage::DirectSendMsg(DirectSendMsg { - protocol_id: msg.protocol_id, - priority: 0, - raw_msg: msg.mdata.clone().into(), - }), - sender: PeerNetworkId::new(NetworkId::Validator, *src), - receive_timestamp_micros: 0, - rpc_replier: None, - }; - let msg: ConsensusMsg = msg.to_message().unwrap(); - (*src, msg, rmsg) + let source_address = rmsg.sender.peer_id(); + let consensus_msg = match &rmsg.message { + NetworkMessage::DirectSendMsg(dmsg) => dmsg + .protocol_id + .from_bytes(dmsg.raw_msg.as_slice()) + .unwrap(), + wrong_message => { + panic!( + "[network playground] Unexpected ReceivedMessage: {:?}", + wrong_message + ); }, - msg_notif => panic!( - "[network playground] Unexpected PeerManagerNotification: {:?}", - msg_notif - ), }; let _ = node_consensus_tx.push( (src_twin_id.author, ProtocolId::ConsensusDirectSendBcs), rmsg, ); - (source_address, msg) + (source_address, consensus_msg) } /// Wait for exactly `num_messages` to be enqueued and delivered. Return a @@ -287,7 +277,7 @@ impl NetworkPlayground { let (src_twin_id, net_req) = self.outbound_msgs_rx.next().await .expect("[network playground] waiting for messages, but message queue has shutdown unexpectedly"); - // Convert PeerManagerRequest to corresponding PeerManagerNotification, + // Convert PeerManagerRequest to corresponding ReceivedMessage, // and extract destination peer let (dst, msg) = match &net_req { PeerManagerRequest::SendDirectSend(dst_inner, msg_inner) => { @@ -305,11 +295,17 @@ impl NetworkPlayground { // Deliver and copy message if it's not dropped if !self.is_message_dropped(&src_twin_id, dst_twin_id, consensus_msg) { - let msg_notif = - PeerManagerNotification::RecvMessage(src_twin_id.author, msg.clone()); - let msg_copy = self - .deliver_message(src_twin_id, *dst_twin_id, msg_notif) - .await; + let rmsg = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: msg.protocol_id, + priority: 0, + raw_msg: msg.mdata.clone().into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, src_twin_id.author), + receive_timestamp_micros: 0, + rpc_replier: None, + }; + let msg_copy = self.deliver_message(src_twin_id, *dst_twin_id, rmsg).await; // Only insert msg_copy once for twins (if delivered) if idx == 0 && msg_inspector(&msg_copy) { @@ -406,7 +402,7 @@ impl NetworkPlayground { pub async fn start(mut self) { // Take the next queued message while let Some((src_twin_id, net_req)) = self.outbound_msgs_rx.next().await { - // Convert PeerManagerRequest to corresponding PeerManagerNotification, + // Convert PeerManagerRequest to corresponding ReceivedMessage, // and extract destination peer let (dst, msg) = match &net_req { PeerManagerRequest::SendDirectSend(dst_inner, msg_inner) => { @@ -421,14 +417,21 @@ impl NetworkPlayground { let dst_twin_ids = self.get_twin_ids(dst); for dst_twin_id in dst_twin_ids.iter() { - let msg_notif = - PeerManagerNotification::RecvMessage(src_twin_id.author, msg.clone()); + let rmsg = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: msg.protocol_id, + priority: 0, + raw_msg: msg.mdata.clone().into(), + }), + sender: PeerNetworkId::new(NetworkId::Validator, src_twin_id.author), + receive_timestamp_micros: 0, + rpc_replier: None, + }; let consensus_msg = msg.to_message().unwrap(); // Deliver and copy message it if it's not dropped if !self.is_message_dropped(&src_twin_id, dst_twin_id, consensus_msg) { - self.deliver_message(src_twin_id, *dst_twin_id, msg_notif) - .await; + self.deliver_message(src_twin_id, *dst_twin_id, rmsg).await; } } } diff --git a/network/framework/src/protocols/health_checker/mod.rs b/network/framework/src/protocols/health_checker/mod.rs index a5522fb876fa9..fea7da738dd95 100644 --- a/network/framework/src/protocols/health_checker/mod.rs +++ b/network/framework/src/protocols/health_checker/mod.rs @@ -57,10 +57,8 @@ mod test; /// The interface from Network to HealthChecker layer. /// -/// `HealthCheckerNetworkEvents` is a `Stream` of `PeerManagerNotification` where the -/// raw `Bytes` rpc messages are deserialized into -/// `HealthCheckerMsg` types. `HealthCheckerNetworkEvents` is a thin wrapper -/// around an `channel::Receiver`. +/// `HealthCheckerNetworkEvents` is a `Stream` of `HealthCheckerMsg`. +/// (Behind the scenes, network messages are being deserialized) pub type HealthCheckerNetworkEvents = NetworkEvents; /// Returns a network application config for the health check client and service From c8a822c3042d45ab5b605027f5379cff873f70c3 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Mon, 15 Jul 2024 09:40:58 -0400 Subject: [PATCH 14/22] PR cleanup --- network/framework/src/peer/test.rs | 6 ------ network/framework/src/protocols/health_checker/test.rs | 1 - 2 files changed, 7 deletions(-) diff --git a/network/framework/src/peer/test.rs b/network/framework/src/peer/test.rs index e0d28e3e7a91f..8c2c46745863b 100644 --- a/network/framework/src/peer/test.rs +++ b/network/framework/src/peer/test.rs @@ -83,10 +83,6 @@ fn build_test_peer( let (connection_notifs_tx, connection_notifs_rx) = aptos_channels::new_test(1); let (peer_reqs_tx, peer_reqs_rx) = aptos_channel::new(QueueStyle::FIFO, NETWORK_CHANNEL_SIZE, None); - // let (peer_notifs_tx, peer_notifs_rx) = - // aptos_channel::new(QueueStyle::FIFO, NETWORK_CHANNEL_SIZE, None); - // upstream_handlers: Arc>>, - // let upstream_handlers = Arc::new(HashMap::new()); let peer = Peer::new( NetworkContext::mock(), @@ -558,7 +554,6 @@ fn peer_recv_rpc_timeout() { // Server receives the rpc request from client. let received = prot_rx.next().await.unwrap(); - // assert_eq!(received, recv_msg); // Pull out the request completion handle. let mut res_tx = match &received.message { @@ -619,7 +614,6 @@ fn peer_recv_rpc_cancel() { // Server receives the rpc request from client. let received = prot_rx.next().await.unwrap(); - // assert_eq!(received, recv_msg); // Pull out the request completion handle. let res_tx = match &received.message { diff --git a/network/framework/src/protocols/health_checker/test.rs b/network/framework/src/protocols/health_checker/test.rs index 7eb71c9d3195a..0027655e57a9c 100644 --- a/network/framework/src/protocols/health_checker/test.rs +++ b/network/framework/src/protocols/health_checker/test.rs @@ -136,7 +136,6 @@ impl TestHarness { ) -> oneshot::Receiver> { let protocol_id = ProtocolId::HealthCheckerRpc; let data = bcs::to_bytes(&HealthCheckerMsg::Ping(Ping(ping))).unwrap(); - // .into(); let (res_tx, res_rx) = oneshot::channel(); let key = (peer_id, ProtocolId::HealthCheckerRpc); let (delivered_tx, delivered_rx) = oneshot::channel(); From f93522881c893d7a98d57a0c0464b414a0119d5b Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Mon, 15 Jul 2024 10:40:31 -0400 Subject: [PATCH 15/22] cleanup and PR refactor --- mempool/src/tests/node.rs | 10 ++-- mempool/src/tests/test_framework.rs | 2 +- network/framework/src/application/tests.rs | 2 +- network/framework/src/peer/mod.rs | 3 +- network/framework/src/peer/test.rs | 56 ++++++++-------------- 5 files changed, 31 insertions(+), 42 deletions(-) diff --git a/mempool/src/tests/node.rs b/mempool/src/tests/node.rs index 91123871c5e9c..f6a999d1b702c 100644 --- a/mempool/src/tests/node.rs +++ b/mempool/src/tests/node.rs @@ -34,11 +34,15 @@ impl NodeId { /// Yet another type, to determine the differences between /// Validators, ValidatorFullNodes, and FullNodes +/// TODO: much code is currently disabled under [cfg(wrong_network_abstraction)], NodeType may or may not come back when that changes? #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)] pub enum NodeType { - // Validator, - // ValidatorFullNode, - // FullNode, + #[allow(unused)] + Validator, + #[allow(unused)] + ValidatorFullNode, + #[allow(unused)] + FullNode, } /// A union type for all types of simulated nodes diff --git a/mempool/src/tests/test_framework.rs b/mempool/src/tests/test_framework.rs index d45bb6a14dae5..69234cf290334 100644 --- a/mempool/src/tests/test_framework.rs +++ b/mempool/src/tests/test_framework.rs @@ -279,7 +279,7 @@ impl MempoolNode { let rmsg = ReceivedMessage { message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, - request_id: 0, // TODO: a number? + request_id: 0, priority: 0, raw_request: data, }), diff --git a/network/framework/src/application/tests.rs b/network/framework/src/application/tests.rs index 91fb0650b04e8..e98232a2da0be 100644 --- a/network/framework/src/application/tests.rs +++ b/network/framework/src/application/tests.rs @@ -1213,7 +1213,7 @@ async fn wait_for_network_event( let rmsg = ReceivedMessage { message: NetworkMessage::RpcRequest(RpcRequest{ protocol_id: outbound_rpc_request.protocol_id, - request_id: 0, // TODO: rand? seq? + request_id: 0, priority: 0, raw_request: outbound_rpc_request.data.into(), }), diff --git a/network/framework/src/peer/mod.rs b/network/framework/src/peer/mod.rs index eaa1ffe0e8f01..20276834abfb0 100644 --- a/network/framework/src/peer/mod.rs +++ b/network/framework/src/peer/mod.rs @@ -522,8 +522,9 @@ where } }, NetworkMessage::RpcResponse(_) => { + // non-reference cast identical to this match case let NetworkMessage::RpcResponse(response) = message else { - panic!() + unreachable!("NetworkMessage type changed between match and let") }; self.outbound_rpcs.handle_inbound_response(response) }, diff --git a/network/framework/src/peer/test.rs b/network/framework/src/peer/test.rs index 8c2c46745863b..cf70f40bbe618 100644 --- a/network/framework/src/peer/test.rs +++ b/network/framework/src/peer/test.rs @@ -243,16 +243,24 @@ fn peer_send_message() { rt.block_on(future::join3(peer.start(), server, client)); } +fn test_upstream_handlers() -> ( + Arc>>, + aptos_channel::Receiver<(PeerId, ProtocolId), ReceivedMessage>, +) { + let mut upstream_handlers = HashMap::new(); + let (sender, receiver) = aptos_channel::new(QueueStyle::FIFO, 100, None); + upstream_handlers.insert(PROTOCOL, sender); + let upstream_handlers = Arc::new(upstream_handlers); + (upstream_handlers, receiver) +} + // Reading an inbound DirectSendMsg off the wire should notify the PeerManager of // an inbound DirectSend. #[test] fn peer_recv_message() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let mut upstream_handlers = HashMap::new(); - let (sender, mut receiver) = aptos_channel::new(QueueStyle::FIFO, 50, None); - upstream_handlers.insert(PROTOCOL, sender); - let upstream_handlers = Arc::new(upstream_handlers); + let (upstream_handlers, mut receiver) = test_upstream_handlers(); let (peer, _peer_handle, connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), @@ -304,14 +312,8 @@ fn peer_recv_message() { fn peers_send_message_concurrent() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let mut upstream_handlers_a = HashMap::new(); - let (prot_a_tx, mut prot_a_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); - upstream_handlers_a.insert(PROTOCOL, prot_a_tx); - let upstream_handlers_a = Arc::new(upstream_handlers_a); - let mut upstream_handlers_b = HashMap::new(); - let (prot_b_tx, mut prot_b_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); - upstream_handlers_b.insert(PROTOCOL, prot_b_tx); - let upstream_handlers_b = Arc::new(upstream_handlers_b); + let (upstream_handlers_a, mut prot_a_rx) = test_upstream_handlers(); + let (upstream_handlers_b, mut prot_b_rx) = test_upstream_handlers(); let ( (peer_a, mut peer_handle_a, mut connection_notifs_rx_a), (peer_b, mut peer_handle_b, mut connection_notifs_rx_b), @@ -385,10 +387,7 @@ fn peers_send_message_concurrent() { fn peer_recv_rpc() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let mut upstream_handlers = HashMap::new(); - let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); - upstream_handlers.insert(PROTOCOL, prot_tx); - let upstream_handlers = Arc::new(upstream_handlers); + let (upstream_handlers, mut prot_rx) = test_upstream_handlers(); let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), @@ -459,10 +458,7 @@ fn peer_recv_rpc() { fn peer_recv_rpc_concurrent() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let mut upstream_handlers = HashMap::new(); - let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); - upstream_handlers.insert(PROTOCOL, prot_tx); - let upstream_handlers = Arc::new(upstream_handlers); + let (upstream_handlers, mut prot_rx) = test_upstream_handlers(); let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), @@ -529,10 +525,7 @@ fn peer_recv_rpc_timeout() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); let mock_time = MockTimeService::new(); - let mut upstream_handlers = HashMap::new(); - let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); - upstream_handlers.insert(PROTOCOL, prot_tx); - let upstream_handlers = Arc::new(upstream_handlers); + let (upstream_handlers, mut prot_rx) = test_upstream_handlers(); let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), mock_time.clone().into(), @@ -589,10 +582,7 @@ fn peer_recv_rpc_timeout() { fn peer_recv_rpc_cancel() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let mut upstream_handlers = HashMap::new(); - let (prot_tx, mut prot_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); - upstream_handlers.insert(PROTOCOL, prot_tx); - let upstream_handlers = Arc::new(upstream_handlers); + let (upstream_handlers, mut prot_rx) = test_upstream_handlers(); let (peer, _peer_handle, mut connection, _connection_notifs_rx) = build_test_peer( rt.handle().clone(), TimeService::mock(), @@ -973,14 +963,8 @@ fn peer_terminates_when_request_tx_has_dropped() { fn peers_send_multiplex() { ::aptos_logger::Logger::init_for_testing(); let rt = Runtime::new().unwrap(); - let mut upstream_handlers_a = HashMap::new(); - let (prot_a_tx, mut prot_a_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); - upstream_handlers_a.insert(PROTOCOL, prot_a_tx); - let upstream_handlers_a = Arc::new(upstream_handlers_a); - let mut upstream_handlers_b = HashMap::new(); - let (prot_b_tx, mut prot_b_rx) = aptos_channel::new(QueueStyle::FIFO, 100, None); - upstream_handlers_b.insert(PROTOCOL, prot_b_tx); - let upstream_handlers_b = Arc::new(upstream_handlers_b); + let (upstream_handlers_a, mut prot_a_rx) = test_upstream_handlers(); + let (upstream_handlers_b, mut prot_b_rx) = test_upstream_handlers(); let ( (peer_a, mut peer_handle_a, mut connection_notifs_rx_a), (peer_b, mut peer_handle_b, mut connection_notifs_rx_b), From 7e4d5bbffbd40a5eba6193800ba0eb376254bd82 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Mon, 15 Jul 2024 10:46:57 -0400 Subject: [PATCH 16/22] unix_micros() util --- .../framework/src/protocols/network/mod.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/network/framework/src/protocols/network/mod.rs b/network/framework/src/protocols/network/mod.rs index eed3aedcb3214..786b9ff033800 100644 --- a/network/framework/src/protocols/network/mod.rs +++ b/network/framework/src/protocols/network/mod.rs @@ -145,11 +145,7 @@ pub struct ReceivedMessage { impl ReceivedMessage { pub fn new(message: NetworkMessage, sender: PeerNetworkId) -> Self { - let now = std::time::SystemTime::now(); - let rx_at = now - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_micros() as u64; + let rx_at = unix_micros(); Self { message, sender, @@ -266,6 +262,13 @@ impl Stream for NetworkEvents { } } +fn unix_micros() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_micros() as u64 +} + /// Deserialize inbound direct send and rpc messages into the application `TMessage` /// type, logging and dropping messages that fail to deserialize. fn received_message_to_event( @@ -278,11 +281,7 @@ fn received_message_to_event( receive_timestamp_micros: rx_at, rpc_replier, } = message; - let now = std::time::SystemTime::now(); - let dequeue_at = now - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_micros() as u64; + let dequeue_at = unix_micros(); let dt_micros = dequeue_at - rx_at; let dt_seconds = (dt_micros as f64) / 1000000.0; match message { From 26ec28982c4a7bf89ed5bf4b60993f6a0f1da7b2 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Mon, 15 Jul 2024 10:49:20 -0400 Subject: [PATCH 17/22] cleanup --- state-sync/storage-service/server/src/tests/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/state-sync/storage-service/server/src/tests/mock.rs b/state-sync/storage-service/server/src/tests/mock.rs index b5d049de78e6a..1554bfb8cd3a6 100644 --- a/state-sync/storage-service/server/src/tests/mock.rs +++ b/state-sync/storage-service/server/src/tests/mock.rs @@ -165,7 +165,7 @@ impl MockClient { let notification = ReceivedMessage { message: NetworkMessage::RpcRequest(RpcRequest { protocol_id, - request_id: 0, // TODO: rand? inc? + request_id: 0, priority: 0, raw_request: data, }), From 834ab52fdd1ddd1d8a7199d2a6b3e936e8fcee6b Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Mon, 15 Jul 2024 13:10:05 -0400 Subject: [PATCH 18/22] fix mempool tests --- mempool/src/tests/mod.rs | 1 - mempool/src/tests/multi_node_test.rs | 39 ++++++++++---- mempool/src/tests/node.rs | 77 +++++++++++++++++----------- 3 files changed, 75 insertions(+), 42 deletions(-) diff --git a/mempool/src/tests/mod.rs b/mempool/src/tests/mod.rs index 7e4b11ffe09d7..9fee6a1f1d364 100644 --- a/mempool/src/tests/mod.rs +++ b/mempool/src/tests/mod.rs @@ -9,7 +9,6 @@ mod core_mempool_test; #[cfg(test)] mod integration_tests; #[cfg(test)] -#[cfg(wrong_network_abstraction)] mod multi_node_test; #[cfg(test)] mod node; diff --git a/mempool/src/tests/multi_node_test.rs b/mempool/src/tests/multi_node_test.rs index 220c971a871ca..d42556913baf0 100644 --- a/mempool/src/tests/multi_node_test.rs +++ b/mempool/src/tests/multi_node_test.rs @@ -20,7 +20,11 @@ use aptos_config::{ }; use aptos_netcore::transport::ConnectionOrigin; use aptos_network::{ - peer_manager::{PeerManagerNotification, PeerManagerRequest}, + peer_manager::PeerManagerRequest, + protocols::{ + network::ReceivedMessage, + wire::messaging::v1::{DirectSendMsg, NetworkMessage}, + }, ProtocolId, }; use aptos_types::{transaction::SignedTransaction, PeerId}; @@ -362,12 +366,18 @@ impl TestHarness { let receiver_id = *self.peer_to_node_id.get(&lookup_peer_network_id).unwrap(); let receiver = self.mut_node(&receiver_id); + let rmsg = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: msg.protocol_id, + priority: 0, + raw_msg: msg.mdata.into(), + }), + sender: PeerNetworkId::new(network_id, sender_peer_id), + receive_timestamp_micros: 0, + rpc_replier: None, + }; - receiver.send_network_req( - network_id, - ProtocolId::MempoolDirectSend, - PeerManagerNotification::RecvMessage(sender_peer_id, msg), - ); + receiver.send_network_req(network_id, ProtocolId::MempoolDirectSend, rmsg); receiver.wait_for_event(SharedMempoolNotification::NewTransactions); // Verify transaction was inserted into Mempool @@ -432,11 +442,18 @@ impl TestHarness { let receiver_id = *self.peer_to_node_id.get(&lookup_peer_network_id).unwrap(); let receiver = self.mut_node(&receiver_id); - receiver.send_network_req( - network_id, - ProtocolId::MempoolDirectSend, - PeerManagerNotification::RecvMessage(sender_peer_id, msg), - ); + let rmsg = ReceivedMessage { + message: NetworkMessage::DirectSendMsg(DirectSendMsg { + protocol_id: msg.protocol_id, + priority: 0, + raw_msg: msg.mdata.into(), + }), + sender: PeerNetworkId::new(network_id, sender_peer_id), + receive_timestamp_micros: 0, + rpc_replier: None, + }; + + receiver.send_network_req(network_id, ProtocolId::MempoolDirectSend, rmsg); }, request => panic!( "did not receive expected broadcast ACK, instead got {:?}", diff --git a/mempool/src/tests/node.rs b/mempool/src/tests/node.rs index f6a999d1b702c..48a1ef4eb9556 100644 --- a/mempool/src/tests/node.rs +++ b/mempool/src/tests/node.rs @@ -2,15 +2,54 @@ // Parts of the project are originally copyright © Meta Platforms, Inc. // SPDX-License-Identifier: Apache-2.0 +use crate::{ + core_mempool::{CoreMempool, TimelineState}, + network::MempoolSyncMsg, + shared_mempool::{start_shared_mempool, types::SharedMempoolNotification}, + tests::common::TestTransaction, +}; +use aptos_channels::{aptos_channel, message_queues::QueueStyle}; use aptos_config::{ - config::{PeerRole, RoleType}, + config::{Identity, NodeConfig, PeerRole, RoleType}, network_id::{NetworkId, PeerNetworkId}, }; -use aptos_types::PeerId; +use aptos_crypto::{x25519::PrivateKey, Uniform}; +use aptos_event_notifications::{ReconfigNotification, ReconfigNotificationListener}; +use aptos_infallible::{Mutex, MutexGuard, RwLock}; +use aptos_netcore::transport::ConnectionOrigin; +use aptos_network::{ + application::{ + interface::{NetworkClient, NetworkServiceEvents}, + storage::PeersAndMetadata, + }, + peer_manager::{ConnectionRequestSender, PeerManagerRequest, PeerManagerRequestSender}, + protocols::{ + network::{ + NetworkEvents, NetworkSender, NewNetworkEvents, NewNetworkSender, ReceivedMessage, + }, + wire::handshake::v1::ProtocolId::MempoolDirectSend, + }, + transport::ConnectionMetadata, + ProtocolId, +}; +use aptos_storage_interface::mock::MockDbReaderWriter; +use aptos_types::{ + on_chain_config::{InMemoryOnChainConfig, OnChainConfigPayload}, + PeerId, +}; +use aptos_vm_validator::mocks::mock_vm_validator::MockVMValidator; use enum_dispatch::enum_dispatch; -use std::collections::HashSet; +use futures::{ + channel::mpsc::{self, unbounded, UnboundedReceiver}, + FutureExt, StreamExt, +}; +use rand::rngs::StdRng; +use std::{ + collections::{HashMap, HashSet}, + sync::Arc, +}; +use tokio::runtime::Runtime; -#[cfg(wrong_network_abstraction)] type MempoolNetworkHandle = ( NetworkId, NetworkSender, @@ -25,7 +64,6 @@ pub struct NodeId { num: u32, } -#[cfg(wrong_network_abstraction)] impl NodeId { pub(crate) fn new(node_type: NodeType, num: u32) -> Self { NodeId { node_type, num } @@ -34,14 +72,10 @@ impl NodeId { /// Yet another type, to determine the differences between /// Validators, ValidatorFullNodes, and FullNodes -/// TODO: much code is currently disabled under [cfg(wrong_network_abstraction)], NodeType may or may not come back when that changes? #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)] pub enum NodeType { - #[allow(unused)] Validator, - #[allow(unused)] ValidatorFullNode, - #[allow(unused)] FullNode, } @@ -103,7 +137,6 @@ pub struct ValidatorNodeInfo { vfn_peer_id: PeerId, } -#[cfg(wrong_network_abstraction)] impl ValidatorNodeInfo { fn new(peer_id: PeerId, vfn_peer_id: PeerId) -> Self { ValidatorNodeInfo { @@ -141,7 +174,6 @@ pub struct ValidatorFullNodeInfo { vfn_peer_id: PeerId, } -#[cfg(wrong_network_abstraction)] impl ValidatorFullNodeInfo { fn new(peer_id: PeerId, vfn_peer_id: PeerId) -> Self { ValidatorFullNodeInfo { @@ -179,7 +211,6 @@ pub struct FullNodeInfo { peer_role: PeerRole, } -#[cfg(wrong_network_abstraction)] impl FullNodeInfo { fn new(peer_id: PeerId, peer_role: PeerRole) -> Self { FullNodeInfo { peer_id, peer_role } @@ -209,7 +240,6 @@ impl NodeInfoTrait for FullNodeInfo { } /// Provides a `NodeInfo` and `NodeConfig` for a validator -#[cfg(wrong_network_abstraction)] pub fn validator_config(rng: &mut StdRng) -> (ValidatorNodeInfo, NodeConfig) { let config = NodeConfig::generate_random_config_with_template( &NodeConfig::get_default_validator_config(), @@ -228,7 +258,6 @@ pub fn validator_config(rng: &mut StdRng) -> (ValidatorNodeInfo, NodeConfig) { } /// Provides a `NodeInfo` and `NodeConfig` for a ValidatorFullNode -#[cfg(wrong_network_abstraction)] pub fn vfn_config(rng: &mut StdRng, peer_id: PeerId) -> (ValidatorFullNodeInfo, NodeConfig) { let mut vfn_config = NodeConfig::generate_random_config_with_template( &NodeConfig::get_default_vfn_config(), @@ -262,7 +291,6 @@ pub fn vfn_config(rng: &mut StdRng, peer_id: PeerId) -> (ValidatorFullNodeInfo, } /// Provides a `NodeInfo` and `NodeConfig` for a public full node -#[cfg(wrong_network_abstraction)] pub fn public_full_node_config( rng: &mut StdRng, peer_role: PeerRole, @@ -282,7 +310,6 @@ pub fn public_full_node_config( } /// A struct representing a node, it's network interfaces, mempool, and a mempool event subscriber -#[cfg(wrong_network_abstraction)] pub struct Node { /// The identifying Node node_info: NodeInfo, @@ -299,7 +326,6 @@ pub struct Node { } /// Reimplement `NodeInfoTrait` for simplicity -#[cfg(wrong_network_abstraction)] impl NodeInfoTrait for Node { fn supported_networks(&self) -> Vec { self.node_info.supported_networks() @@ -318,7 +344,6 @@ impl NodeInfoTrait for Node { } } -#[cfg(wrong_network_abstraction)] impl Node { /// Sets up a single node by starting up mempool and any network handles pub fn new(node: NodeInfo, config: NodeConfig) -> Node { @@ -423,12 +448,12 @@ impl Node { .get_next_network_req(runtime) } - /// Send network request `PeerManagerNotification` from a remote peer to the local node + /// Send network request `ReceivedMessage` from a remote peer to the local node pub fn send_network_req( &mut self, network_id: NetworkId, protocol: ProtocolId, - notif: PeerManagerNotification, + notif: ReceivedMessage, ) { self.get_network_interface(network_id) .send_network_req(protocol, notif); @@ -437,7 +462,6 @@ impl Node { /// A simplistic view of the entire network stack for a given `NetworkId` /// Allows us to mock out the network without dealing with the details -#[cfg(wrong_network_abstraction)] pub struct NodeNetworkInterface { /// Peer request receiver for messages pub(crate) network_reqs_rx: aptos_channel::Receiver<(PeerId, ProtocolId), PeerManagerRequest>, @@ -445,17 +469,13 @@ pub struct NodeNetworkInterface { pub(crate) network_notifs_tx: aptos_channel::Sender<(PeerId, ProtocolId), ReceivedMessage>, } -#[cfg(wrong_network_abstraction)] impl NodeNetworkInterface { fn get_next_network_req(&mut self, runtime: Arc) -> PeerManagerRequest { runtime.block_on(self.network_reqs_rx.next()).unwrap() } - fn send_network_req(&mut self, protocol: ProtocolId, message: PeerManagerNotification) { - let remote_peer_id = match &message { - PeerManagerNotification::RecvRpc(peer_id, _) => *peer_id, - PeerManagerNotification::RecvMessage(peer_id, _) => *peer_id, - }; + fn send_network_req(&mut self, protocol: ProtocolId, message: ReceivedMessage) { + let remote_peer_id = message.sender.peer_id(); self.network_notifs_tx .push((remote_peer_id, protocol), message) @@ -466,7 +486,6 @@ impl NodeNetworkInterface { // Below here are static functions to help build a new `Node` /// Sets up the network handles for a `Node` -#[cfg(wrong_network_abstraction)] fn setup_node_network_interfaces( node: &NodeInfo, ) -> ( @@ -510,7 +529,6 @@ fn setup_node_network_interfaces( } /// Builds a single network interface with associated queues, and attaches it to the top level network -#[cfg(wrong_network_abstraction)] fn setup_node_network_interface( peer_network_id: PeerNetworkId, ) -> (NodeNetworkInterface, MempoolNetworkHandle) { @@ -537,7 +555,6 @@ fn setup_node_network_interface( } /// Starts up the mempool resources for a single node -#[cfg(wrong_network_abstraction)] fn start_node_mempool( config: NodeConfig, network_client: NetworkClient, From 504162b37c3e8ee3d96e72c7240eb000a78b9d2c Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Thu, 18 Jul 2024 20:46:26 -0400 Subject: [PATCH 19/22] drop unused max_concurrent_network_reqs --- aptos-node/src/lib.rs | 2 -- config/src/config/network_config.rs | 4 ---- network/builder/src/builder.rs | 9 ++------- network/framework/src/constants.rs | 1 - network/framework/src/peer_manager/builder.rs | 6 ------ network/framework/src/peer_manager/mod.rs | 5 ----- network/framework/src/peer_manager/tests.rs | 1 - 7 files changed, 2 insertions(+), 26 deletions(-) diff --git a/aptos-node/src/lib.rs b/aptos-node/src/lib.rs index 79f847c97f461..80ed4f44bc7a4 100644 --- a/aptos-node/src/lib.rs +++ b/aptos-node/src/lib.rs @@ -512,7 +512,6 @@ where // Configure the validator network let validator_network = node_config.validator_network.as_mut().unwrap(); - validator_network.max_concurrent_network_reqs = 1; validator_network.connectivity_check_interval_ms = 10000; validator_network.max_connection_delay_ms = 10000; validator_network.ping_interval_ms = 10000; @@ -520,7 +519,6 @@ where // Configure the fullnode network let fullnode_network = node_config.full_node_networks.get_mut(0).unwrap(); - fullnode_network.max_concurrent_network_reqs = 1; fullnode_network.connectivity_check_interval_ms = 10000; fullnode_network.max_connection_delay_ms = 10000; fullnode_network.ping_interval_ms = 10000; diff --git a/config/src/config/network_config.rs b/config/src/config/network_config.rs index aa69c13453b88..8ecd0964a6723 100644 --- a/config/src/config/network_config.rs +++ b/config/src/config/network_config.rs @@ -40,7 +40,6 @@ pub const PING_INTERVAL_MS: u64 = 10_000; pub const PING_TIMEOUT_MS: u64 = 20_000; pub const PING_FAILURES_TOLERATED: u64 = 3; pub const CONNECTIVITY_CHECK_INTERVAL_MS: u64 = 5000; -pub const MAX_CONCURRENT_NETWORK_REQS: usize = 100; pub const MAX_CONNECTION_DELAY_MS: u64 = 60_000; /* 1 minute */ pub const MAX_FULLNODE_OUTBOUND_CONNECTIONS: usize = 6; pub const MAX_INBOUND_CONNECTIONS: usize = 100; @@ -65,8 +64,6 @@ pub struct NetworkConfig { pub connectivity_check_interval_ms: u64, /// Size of all network channels pub network_channel_size: usize, - /// Maximum number of concurrent network requests - pub max_concurrent_network_reqs: usize, /// Choose a protocol to discover and dial out to other peers on this network. /// `DiscoveryMethod::None` disables discovery and dialing out (unless you have /// seed peers configured). @@ -153,7 +150,6 @@ impl NetworkConfig { max_connection_delay_ms: MAX_CONNECTION_DELAY_MS, connectivity_check_interval_ms: CONNECTIVITY_CHECK_INTERVAL_MS, network_channel_size: NETWORK_CHANNEL_SIZE, - max_concurrent_network_reqs: MAX_CONCURRENT_NETWORK_REQS, connection_backoff_base: CONNECTION_BACKOFF_BASE, ping_interval_ms: PING_INTERVAL_MS, ping_timeout_ms: PING_TIMEOUT_MS, diff --git a/network/builder/src/builder.rs b/network/builder/src/builder.rs index 484766af02fed..a4b941cef6a22 100644 --- a/network/builder/src/builder.rs +++ b/network/builder/src/builder.rs @@ -13,9 +13,8 @@ use aptos_config::{ config::{ DiscoveryMethod, NetworkConfig, Peer, PeerRole, PeerSet, RoleType, CONNECTION_BACKOFF_BASE, - CONNECTIVITY_CHECK_INTERVAL_MS, MAX_CONCURRENT_NETWORK_REQS, MAX_CONNECTION_DELAY_MS, - MAX_FRAME_SIZE, MAX_FULLNODE_OUTBOUND_CONNECTIONS, MAX_INBOUND_CONNECTIONS, - NETWORK_CHANNEL_SIZE, + CONNECTIVITY_CHECK_INTERVAL_MS, MAX_CONNECTION_DELAY_MS, MAX_FRAME_SIZE, + MAX_FULLNODE_OUTBOUND_CONNECTIONS, MAX_INBOUND_CONNECTIONS, NETWORK_CHANNEL_SIZE, }, network_id::NetworkContext, }; @@ -84,7 +83,6 @@ impl NetworkBuilder { max_message_size: usize, enable_proxy_protocol: bool, network_channel_size: usize, - max_concurrent_network_reqs: usize, inbound_connection_limit: usize, tcp_buffer_cfg: TCPBufferCfg, ) -> Self { @@ -98,7 +96,6 @@ impl NetworkBuilder { peers_and_metadata.clone(), authentication_mode, network_channel_size, - max_concurrent_network_reqs, max_frame_size, max_message_size, enable_proxy_protocol, @@ -141,7 +138,6 @@ impl NetworkBuilder { MAX_MESSAGE_SIZE, false, /* Disable proxy protocol */ NETWORK_CHANNEL_SIZE, - MAX_CONCURRENT_NETWORK_REQS, MAX_INBOUND_CONNECTIONS, TCPBufferCfg::default(), ); @@ -192,7 +188,6 @@ impl NetworkBuilder { config.max_message_size, config.enable_proxy_protocol, config.network_channel_size, - config.max_concurrent_network_reqs, config.max_inbound_connections, TCPBufferCfg::new_configs( config.inbound_rx_buffer_size_bytes, diff --git a/network/framework/src/constants.rs b/network/framework/src/constants.rs index 6cdcc779f4194..ad4954f58278f 100644 --- a/network/framework/src/constants.rs +++ b/network/framework/src/constants.rs @@ -20,5 +20,4 @@ pub const MAX_CONCURRENT_INBOUND_RPCS: u32 = 100; pub const NETWORK_CHANNEL_SIZE: usize = 1024; pub const MAX_FRAME_SIZE: usize = 4 * 1024 * 1024; /* 4 MiB */ pub const MAX_MESSAGE_SIZE: usize = 64 * 1024 * 1024; /* 64 MiB */ -pub const MAX_CONCURRENT_NETWORK_REQS: usize = 100; pub const MAX_CONCURRENT_NETWORK_NOTIFS: usize = 100; diff --git a/network/framework/src/peer_manager/builder.rs b/network/framework/src/peer_manager/builder.rs index 9e0d7ad6d0c64..6277ef9637bf3 100644 --- a/network/framework/src/peer_manager/builder.rs +++ b/network/framework/src/peer_manager/builder.rs @@ -73,7 +73,6 @@ struct PeerManagerContext { HashMap>, connection_event_handlers: Vec, - max_concurrent_network_reqs: usize, channel_size: usize, max_frame_size: usize, max_message_size: usize, @@ -96,7 +95,6 @@ impl PeerManagerContext { >, connection_event_handlers: Vec, - max_concurrent_network_reqs: usize, channel_size: usize, max_frame_size: usize, max_message_size: usize, @@ -113,7 +111,6 @@ impl PeerManagerContext { upstream_handlers, connection_event_handlers, - max_concurrent_network_reqs, channel_size, max_frame_size, max_message_size, @@ -171,7 +168,6 @@ impl PeerManagerBuilder { peers_and_metadata: Arc, authentication_mode: AuthenticationMode, channel_size: usize, - max_concurrent_network_reqs: usize, max_frame_size: usize, max_message_size: usize, enable_proxy_protocol: bool, @@ -206,7 +202,6 @@ impl PeerManagerBuilder { peers_and_metadata, HashMap::new(), Vec::new(), - max_concurrent_network_reqs, channel_size, max_frame_size, max_message_size, @@ -342,7 +337,6 @@ impl PeerManagerBuilder { pm_context.upstream_handlers, pm_context.connection_event_handlers, pm_context.channel_size, - pm_context.max_concurrent_network_reqs, pm_context.max_frame_size, pm_context.max_message_size, pm_context.inbound_connection_limit, diff --git a/network/framework/src/peer_manager/mod.rs b/network/framework/src/peer_manager/mod.rs index 90988a5a29c32..4ee50778121fb 100644 --- a/network/framework/src/peer_manager/mod.rs +++ b/network/framework/src/peer_manager/mod.rs @@ -109,9 +109,6 @@ where HashMap>>, /// Pin the transport type corresponding to this PeerManager instance phantom_transport: PhantomData, - /// Maximum concurrent network requests to any peer. // TODO: bring back usage? - #[allow(dead_code)] - max_concurrent_network_reqs: usize, /// Size of channels between different actors. channel_size: usize, /// Max network frame size @@ -144,7 +141,6 @@ where >, connection_event_handlers: Vec, channel_size: usize, - max_concurrent_network_reqs: usize, max_frame_size: usize, max_message_size: usize, inbound_connection_limit: usize, @@ -185,7 +181,6 @@ where phantom_transport: PhantomData, upstream_handlers: Arc::new(upstream_handlers), connection_event_handlers, - max_concurrent_network_reqs, channel_size, max_frame_size, max_message_size, diff --git a/network/framework/src/peer_manager/tests.rs b/network/framework/src/peer_manager/tests.rs index 4b95ec1e7658b..4ac6b381af77f 100644 --- a/network/framework/src/peer_manager/tests.rs +++ b/network/framework/src/peer_manager/tests.rs @@ -113,7 +113,6 @@ fn build_test_peer_manager( .collect(), vec![conn_status_tx], constants::NETWORK_CHANNEL_SIZE, - constants::MAX_CONCURRENT_NETWORK_REQS, constants::MAX_FRAME_SIZE, constants::MAX_MESSAGE_SIZE, MAX_INBOUND_CONNECTIONS, From f5dfe7dce72fb9858c5128a7f2007f22efde07be Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Thu, 18 Jul 2024 20:48:58 -0400 Subject: [PATCH 20/22] drop unused PeerManagerNotification --- network/framework/src/peer_manager/types.rs | 25 +-------------------- 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/network/framework/src/peer_manager/types.rs b/network/framework/src/peer_manager/types.rs index 36e661017d7a5..006465c7e9073 100644 --- a/network/framework/src/peer_manager/types.rs +++ b/network/framework/src/peer_manager/types.rs @@ -4,10 +4,7 @@ use crate::{ peer::DisconnectReason, peer_manager::PeerManagerError, - protocols::{ - direct_send::Message, - rpc::{InboundRpcRequest, OutboundRpcRequest}, - }, + protocols::{direct_send::Message, rpc::OutboundRpcRequest}, transport::{Connection, ConnectionMetadata}, }; use aptos_config::network_id::NetworkId; @@ -25,26 +22,6 @@ pub enum PeerManagerRequest { SendDirectSend(PeerId, #[serde(skip)] Message), } -/// Notifications sent by PeerManager to upstream actors. -/// TODO: PeerManagerNotification now only exists in test code and should be deleted; probably use `ReceivedMessage` -#[derive(Debug)] -pub enum PeerManagerNotification { - /// A new RPC request has been received from a remote peer. - RecvRpc(PeerId, InboundRpcRequest), - /// A new message has been received from a remote peer. - RecvMessage(PeerId, Message), -} - -impl PeerManagerNotification { - /// Returns the peer ID of the notification - pub fn get_peer_id(&self) -> PeerId { - match self { - PeerManagerNotification::RecvRpc(peer_id, _) => *peer_id, - PeerManagerNotification::RecvMessage(peer_id, _) => *peer_id, - } - } -} - #[derive(Debug, Serialize)] pub enum ConnectionRequest { DialPeer( From 9298a594a84ee8c1c0255f26cf312f74a00a36cf Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Fri, 19 Jul 2024 09:22:14 -0400 Subject: [PATCH 21/22] count UNKNOWN_LABEL inbound messages --- network/framework/src/counters.rs | 1 + network/framework/src/peer/mod.rs | 11 ++++++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/network/framework/src/counters.rs b/network/framework/src/counters.rs index 1d748bfb4c038..472056b296797 100644 --- a/network/framework/src/counters.rs +++ b/network/framework/src/counters.rs @@ -26,6 +26,7 @@ pub const RECEIVED_LABEL: &str = "received"; pub const SENT_LABEL: &str = "sent"; pub const SUCCEEDED_LABEL: &str = "succeeded"; pub const FAILED_LABEL: &str = "failed"; +pub const UNKNOWN_LABEL: &str = "unk"; // Direction labels pub const INBOUND_LABEL: &str = "inbound"; diff --git a/network/framework/src/peer/mod.rs b/network/framework/src/peer/mod.rs index 20276834abfb0..094e3d70c0421 100644 --- a/network/framework/src/peer/mod.rs +++ b/network/framework/src/peer/mod.rs @@ -18,7 +18,7 @@ use crate::{ counters::{ self, network_application_inbound_traffic, network_application_outbound_traffic, - DECLINED_LABEL, FAILED_LABEL, RECEIVED_LABEL, SENT_LABEL, + DECLINED_LABEL, FAILED_LABEL, RECEIVED_LABEL, SENT_LABEL, UNKNOWN_LABEL, }, logging::NetworkSchema, peer_manager::{PeerManagerError, TransportNotification}, @@ -451,9 +451,8 @@ where ); match self.upstream_handlers.get(&direct.protocol_id) { None => { - // TODO: better label than "declined"? more like "garbage-in" - counters::direct_send_messages(&self.network_context, DECLINED_LABEL).inc(); - counters::direct_send_bytes(&self.network_context, DECLINED_LABEL) + counters::direct_send_messages(&self.network_context, UNKNOWN_LABEL).inc(); + counters::direct_send_bytes(&self.network_context, UNKNOWN_LABEL) .inc_by(data_len as u64); }, Some(handler) => { @@ -499,7 +498,9 @@ where NetworkMessage::RpcRequest(request) => { match self.upstream_handlers.get(&request.protocol_id) { None => { - // TODO: count error + counters::direct_send_messages(&self.network_context, UNKNOWN_LABEL).inc(); + counters::direct_send_bytes(&self.network_context, UNKNOWN_LABEL) + .inc_by(request.raw_request.len() as u64); }, Some(handler) => { let sender = self.connection_metadata.remote_peer_id; From 5634764423ef39264cf65a5c12ddc4edf8f0c9c9 Mon Sep 17 00:00:00 2001 From: Brian Olson Date: Fri, 19 Jul 2024 16:00:17 -0400 Subject: [PATCH 22/22] PR cleanup --- network/framework/src/counters.rs | 2 +- network/framework/src/peer_manager/tests.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/network/framework/src/counters.rs b/network/framework/src/counters.rs index 472056b296797..d381264061383 100644 --- a/network/framework/src/counters.rs +++ b/network/framework/src/counters.rs @@ -26,7 +26,7 @@ pub const RECEIVED_LABEL: &str = "received"; pub const SENT_LABEL: &str = "sent"; pub const SUCCEEDED_LABEL: &str = "succeeded"; pub const FAILED_LABEL: &str = "failed"; -pub const UNKNOWN_LABEL: &str = "unk"; +pub const UNKNOWN_LABEL: &str = "unknown"; // Direction labels pub const INBOUND_LABEL: &str = "inbound"; diff --git a/network/framework/src/peer_manager/tests.rs b/network/framework/src/peer_manager/tests.rs index 4ac6b381af77f..712ab9160aa9f 100644 --- a/network/framework/src/peer_manager/tests.rs +++ b/network/framework/src/peer_manager/tests.rs @@ -122,7 +122,6 @@ fn build_test_peer_manager( peer_manager, peer_manager_request_tx, connection_reqs_tx, - // hello_rx, conn_status_rx, ) }