Custom RPC request management for sync (#3029)

## Proposed Changes
Make `lighthouse_network` generic over request ids, now usable by sync

beacon_node/lighthouse_network/src/rpc/handler.rs

@@ -1,12 +1,10 @@
 #![allow(clippy::type_complexity)]
 #![allow(clippy::cognitive_complexity)]
 
-use super::methods::{
-    GoodbyeReason, RPCCodedResponse, RPCResponseErrorCode, RequestId, ResponseTermination,
-};
+use super::methods::{GoodbyeReason, RPCCodedResponse, RPCResponseErrorCode, ResponseTermination};
 use super::outbound::OutboundRequestContainer;
 use super::protocol::{max_rpc_size, InboundRequest, Protocol, RPCError, RPCProtocol};
-use super::{RPCReceived, RPCSend};
+use super::{RPCReceived, RPCSend, ReqId};
 use crate::rpc::outbound::{OutboundFramed, OutboundRequest};
 use crate::rpc::protocol::InboundFramed;
 use fnv::FnvHashMap;
@@ -49,11 +47,11 @@ pub struct SubstreamId(usize);
 type InboundSubstream<TSpec> = InboundFramed<NegotiatedSubstream, TSpec>;
 
 /// Events the handler emits to the behaviour.
-type HandlerEvent<T> = Result<RPCReceived<T>, HandlerErr>;
+pub type HandlerEvent<Id, T> = Result<RPCReceived<Id, T>, HandlerErr<Id>>;
 
 /// An error encountered by the handler.
 #[derive(Debug)]
-pub enum HandlerErr {
+pub enum HandlerErr<Id> {
     /// An error occurred for this peer's request. This can occur during protocol negotiation,
     /// message passing, or if the handler identifies that we are sending an error response to the peer.
     Inbound {
@@ -69,7 +67,7 @@ pub enum HandlerErr {
     /// indicates an error.
     Outbound {
         /// Application-given Id of the request for which an error occurred.
-        id: RequestId,
+        id: Id,
         /// Information of the protocol.
         proto: Protocol,
         /// The error that occurred.
@@ -78,7 +76,7 @@ pub enum HandlerErr {
 }
 
 /// Implementation of `ConnectionHandler` for the RPC protocol.
-pub struct RPCHandler<TSpec>
+pub struct RPCHandler<Id, TSpec>
 where
     TSpec: EthSpec,
 {
@@ -86,10 +84,10 @@ where
     listen_protocol: SubstreamProtocol<RPCProtocol<TSpec>, ()>,
 
     /// Queue of events to produce in `poll()`.
-    events_out: SmallVec<[HandlerEvent<TSpec>; 4]>,
+    events_out: SmallVec<[HandlerEvent<Id, TSpec>; 4]>,
 
     /// Queue of outbound substreams to open.
-    dial_queue: SmallVec<[(RequestId, OutboundRequest<TSpec>); 4]>,
+    dial_queue: SmallVec<[(Id, OutboundRequest<TSpec>); 4]>,
 
     /// Current number of concurrent outbound substreams being opened.
     dial_negotiated: u32,
@@ -101,7 +99,7 @@ where
     inbound_substreams_delay: DelayQueue<SubstreamId>,
 
     /// Map of outbound substreams that need to be driven to completion.
-    outbound_substreams: FnvHashMap<SubstreamId, OutboundInfo<TSpec>>,
+    outbound_substreams: FnvHashMap<SubstreamId, OutboundInfo<Id, TSpec>>,
 
     /// Inbound substream `DelayQueue` which keeps track of when an inbound substream will timeout.
     outbound_substreams_delay: DelayQueue<SubstreamId>,
@@ -163,7 +161,7 @@ struct InboundInfo<TSpec: EthSpec> {
 }
 
 /// Contains the information the handler keeps on established outbound substreams.
-struct OutboundInfo<TSpec: EthSpec> {
+struct OutboundInfo<Id, TSpec: EthSpec> {
     /// State of the substream.
     state: OutboundSubstreamState<TSpec>,
     /// Key to keep track of the substream's timeout via `self.outbound_substreams_delay`.
@@ -172,8 +170,8 @@ struct OutboundInfo<TSpec: EthSpec> {
     proto: Protocol,
     /// Number of chunks to be seen from the peer's response.
     remaining_chunks: Option<u64>,
-    /// `RequestId` as given by the application that sent the request.
-    req_id: RequestId,
+    /// `Id` as given by the application that sent the request.
+    req_id: Id,
 }
 
 /// State of an inbound substream connection.
@@ -204,7 +202,7 @@ pub enum OutboundSubstreamState<TSpec: EthSpec> {
     Poisoned,
 }
 
-impl<TSpec> RPCHandler<TSpec>
+impl<Id, TSpec> RPCHandler<Id, TSpec>
 where
     TSpec: EthSpec,
 {
@@ -235,7 +233,7 @@ where
 
     /// Initiates the handler's shutdown process, sending an optional Goodbye message to the
     /// peer.
-    fn shutdown(&mut self, goodbye_reason: Option<GoodbyeReason>) {
+    fn shutdown(&mut self, goodbye_reason: Option<(Id, GoodbyeReason)>) {
         if matches!(self.state, HandlerState::Active) {
             if !self.dial_queue.is_empty() {
                 debug!(self.log, "Starting handler shutdown"; "unsent_queued_requests" => self.dial_queue.len());
@@ -250,9 +248,8 @@ where
             }
 
             // Queue our goodbye message.
-            if let Some(reason) = goodbye_reason {
-                self.dial_queue
-                    .push((RequestId::Router, OutboundRequest::Goodbye(reason)));
+            if let Some((id, reason)) = goodbye_reason {
+                self.dial_queue.push((id, OutboundRequest::Goodbye(reason)));
             }
 
             self.state = HandlerState::ShuttingDown(Box::new(sleep_until(
@@ -262,7 +259,7 @@ where
     }
 
     /// Opens an outbound substream with a request.
-    fn send_request(&mut self, id: RequestId, req: OutboundRequest<TSpec>) {
+    fn send_request(&mut self, id: Id, req: OutboundRequest<TSpec>) {
         match self.state {
             HandlerState::Active => {
                 self.dial_queue.push((id, req));
@@ -310,16 +307,17 @@ where
     }
 }
 
-impl<TSpec> ConnectionHandler for RPCHandler<TSpec>
+impl<Id, TSpec> ConnectionHandler for RPCHandler<Id, TSpec>
 where
     TSpec: EthSpec,
+    Id: ReqId,
 {
-    type InEvent = RPCSend<TSpec>;
-    type OutEvent = HandlerEvent<TSpec>;
+    type InEvent = RPCSend<Id, TSpec>;
+    type OutEvent = HandlerEvent<Id, TSpec>;
     type Error = RPCError;
     type InboundProtocol = RPCProtocol<TSpec>;
     type OutboundProtocol = OutboundRequestContainer<TSpec>;
-    type OutboundOpenInfo = (RequestId, OutboundRequest<TSpec>); // Keep track of the id and the request
+    type OutboundOpenInfo = (Id, OutboundRequest<TSpec>); // Keep track of the id and the request
     type InboundOpenInfo = ();
 
     fn listen_protocol(&self) -> SubstreamProtocol<Self::InboundProtocol, ()> {
@@ -432,7 +430,7 @@ where
         match rpc_event {
             RPCSend::Request(id, req) => self.send_request(id, req),
             RPCSend::Response(inbound_id, response) => self.send_response(inbound_id, response),
-            RPCSend::Shutdown(reason) => self.shutdown(Some(reason)),
+            RPCSend::Shutdown(id, reason) => self.shutdown(Some((id, reason))),
         }
         // In any case, we need the handler to process the event.
         if let Some(waker) = &self.waker {

beacon_node/lighthouse_network/src/rpc/methods.rs

@@ -56,17 +56,6 @@ impl ToString for ErrorType {
 
 /* Requests */
 
-/// Identifier of a request.
-///
-// NOTE: The handler stores the `RequestId` to inform back of responses and errors, but it's execution
-// is independent of the contents on this type.
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub enum RequestId {
-    Router,
-    Sync(usize),
-    Behaviour,
-}
-
 /// The STATUS request/response handshake message.
 #[derive(Encode, Decode, Clone, Debug, PartialEq)]
 pub struct StatusMessage {
@@ -432,18 +421,3 @@ impl slog::KV for StatusMessage {
         slog::Result::Ok(())
     }
 }
-
-impl slog::Value for RequestId {
-    fn serialize(
-        &self,
-        record: &slog::Record,
-        key: slog::Key,
-        serializer: &mut dyn slog::Serializer,
-    ) -> slog::Result {
-        match self {
-            RequestId::Behaviour => slog::Value::serialize("Behaviour", record, key, serializer),
-            RequestId::Router => slog::Value::serialize("Router", record, key, serializer),
-            RequestId::Sync(ref id) => slog::Value::serialize(id, record, key, serializer),
-        }
-    }
-}

beacon_node/lighthouse_network/src/rpc/mod.rs

@@ -5,13 +5,13 @@
 //! syncing.
 
 use futures::future::FutureExt;
-use handler::RPCHandler;
-use libp2p::core::{connection::ConnectionId, ConnectedPoint};
+use handler::{HandlerEvent, RPCHandler};
+use libp2p::core::connection::ConnectionId;
 use libp2p::swarm::{
     handler::ConnectionHandler, NetworkBehaviour, NetworkBehaviourAction, NotifyHandler,
     PollParameters, SubstreamProtocol,
 };
-use libp2p::{Multiaddr, PeerId};
+use libp2p::PeerId;
 use rate_limiter::{RPCRateLimiter as RateLimiter, RPCRateLimiterBuilder, RateLimitedErr};
 use slog::{crit, debug, o};
 use std::marker::PhantomData;
@@ -27,7 +27,7 @@ pub(crate) use protocol::{InboundRequest, RPCProtocol};
 pub use handler::SubstreamId;
 pub use methods::{
     BlocksByRangeRequest, BlocksByRootRequest, GoodbyeReason, MaxRequestBlocks,
-    RPCResponseErrorCode, RequestId, ResponseTermination, StatusMessage, MAX_REQUEST_BLOCKS,
+    RPCResponseErrorCode, ResponseTermination, StatusMessage, MAX_REQUEST_BLOCKS,
 };
 pub(crate) use outbound::OutboundRequest;
 pub use protocol::{max_rpc_size, Protocol, RPCError};
@@ -39,14 +39,18 @@ mod outbound;
 mod protocol;
 mod rate_limiter;
 
+/// Composite trait for a request id.
+pub trait ReqId: Send + 'static + std::fmt::Debug + Copy + Clone {}
+impl<T> ReqId for T where T: Send + 'static + std::fmt::Debug + Copy + Clone {}
+
 /// RPC events sent from Lighthouse.
 #[derive(Debug, Clone)]
-pub enum RPCSend<TSpec: EthSpec> {
+pub enum RPCSend<Id, TSpec: EthSpec> {
     /// A request sent from Lighthouse.
     ///
-    /// The `RequestId` is given by the application making the request. These
+    /// The `Id` is given by the application making the request. These
     /// go over *outbound* connections.
-    Request(RequestId, OutboundRequest<TSpec>),
+    Request(Id, OutboundRequest<TSpec>),
     /// A response sent from Lighthouse.
     ///
     /// The `SubstreamId` must correspond to the RPC-given ID of the original request received from the
@@ -54,12 +58,12 @@ pub enum RPCSend<TSpec: EthSpec> {
     /// connections.
     Response(SubstreamId, RPCCodedResponse<TSpec>),
     /// Lighthouse has requested to terminate the connection with a goodbye message.
-    Shutdown(GoodbyeReason),
+    Shutdown(Id, GoodbyeReason),
 }
 
 /// RPC events received from outside Lighthouse.
 #[derive(Debug, Clone)]
-pub enum RPCReceived<T: EthSpec> {
+pub enum RPCReceived<Id, T: EthSpec> {
     /// A request received from the outside.
     ///
     /// The `SubstreamId` is given by the `RPCHandler` as it identifies this request with the
@@ -67,47 +71,47 @@ pub enum RPCReceived<T: EthSpec> {
     Request(SubstreamId, InboundRequest<T>),
     /// A response received from the outside.
     ///
-    /// The `RequestId` corresponds to the application given ID of the original request sent to the
+    /// The `Id` corresponds to the application given ID of the original request sent to the
     /// peer. The second parameter is a single chunk of a response. These go over *outbound*
     /// connections.
-    Response(RequestId, RPCResponse<T>),
+    Response(Id, RPCResponse<T>),
     /// Marks a request as completed
-    EndOfStream(RequestId, ResponseTermination),
+    EndOfStream(Id, ResponseTermination),
 }
 
-impl<T: EthSpec> std::fmt::Display for RPCSend<T> {
+impl<T: EthSpec, Id: std::fmt::Debug> std::fmt::Display for RPCSend<Id, T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             RPCSend::Request(id, req) => write!(f, "RPC Request(id: {:?}, {})", id, req),
             RPCSend::Response(id, res) => write!(f, "RPC Response(id: {:?}, {})", id, res),
-            RPCSend::Shutdown(reason) => write!(f, "Sending Goodbye: {}", reason),
+            RPCSend::Shutdown(_id, reason) => write!(f, "Sending Goodbye: {}", reason),
         }
     }
 }
 
 /// Messages sent to the user from the RPC protocol.
-pub struct RPCMessage<TSpec: EthSpec> {
+pub struct RPCMessage<Id, TSpec: EthSpec> {
     /// The peer that sent the message.
     pub peer_id: PeerId,
     /// Handler managing this message.
     pub conn_id: ConnectionId,
     /// The message that was sent.
-    pub event: <RPCHandler<TSpec> as ConnectionHandler>::OutEvent,
+    pub event: HandlerEvent<Id, TSpec>,
 }
 
 /// Implements the libp2p `NetworkBehaviour` trait and therefore manages network-level
 /// logic.
-pub struct RPC<TSpec: EthSpec> {
+pub struct RPC<Id: ReqId, TSpec: EthSpec> {
     /// Rate limiter
     limiter: RateLimiter,
     /// Queue of events to be processed.
-    events: Vec<NetworkBehaviourAction<RPCMessage<TSpec>, RPCHandler<TSpec>>>,
+    events: Vec<NetworkBehaviourAction<RPCMessage<Id, TSpec>, RPCHandler<Id, TSpec>>>,
     fork_context: Arc<ForkContext>,
     /// Slog logger for RPC behaviour.
     log: slog::Logger,
 }
 
-impl<TSpec: EthSpec> RPC<TSpec> {
+impl<Id: ReqId, TSpec: EthSpec> RPC<Id, TSpec> {
     pub fn new(fork_context: Arc<ForkContext>, log: slog::Logger) -> Self {
         let log = log.new(o!("service" => "libp2p_rpc"));
         let limiter = RPCRateLimiterBuilder::new()
@@ -150,12 +154,7 @@ impl<TSpec: EthSpec> RPC<TSpec> {
     /// Submits an RPC request.
     ///
     /// The peer must be connected for this to succeed.
-    pub fn send_request(
-        &mut self,
-        peer_id: PeerId,
-        request_id: RequestId,
-        event: OutboundRequest<TSpec>,
-    ) {
+    pub fn send_request(&mut self, peer_id: PeerId, request_id: Id, event: OutboundRequest<TSpec>) {
         self.events.push(NetworkBehaviourAction::NotifyHandler {
             peer_id,
             handler: NotifyHandler::Any,
@@ -165,21 +164,22 @@ impl<TSpec: EthSpec> RPC<TSpec> {
 
     /// Lighthouse wishes to disconnect from this peer by sending a Goodbye message. This
     /// gracefully terminates the RPC behaviour with a goodbye message.
-    pub fn shutdown(&mut self, peer_id: PeerId, reason: GoodbyeReason) {
+    pub fn shutdown(&mut self, peer_id: PeerId, id: Id, reason: GoodbyeReason) {
         self.events.push(NetworkBehaviourAction::NotifyHandler {
             peer_id,
             handler: NotifyHandler::Any,
-            event: RPCSend::Shutdown(reason),
+            event: RPCSend::Shutdown(id, reason),
         });
     }
 }
 
-impl<TSpec> NetworkBehaviour for RPC<TSpec>
+impl<Id, TSpec> NetworkBehaviour for RPC<Id, TSpec>
 where
     TSpec: EthSpec,
+    Id: ReqId,
 {
-    type ConnectionHandler = RPCHandler<TSpec>;
-    type OutEvent = RPCMessage<TSpec>;
+    type ConnectionHandler = RPCHandler<Id, TSpec>;
+    type OutEvent = RPCMessage<Id, TSpec>;
 
     fn new_handler(&mut self) -> Self::ConnectionHandler {
         RPCHandler::new(
@@ -196,33 +196,6 @@ where
         )
     }
 
-    // handled by discovery
-    fn addresses_of_peer(&mut self, _peer_id: &PeerId) -> Vec<Multiaddr> {
-        Vec::new()
-    }
-
-    // Use connection established/closed instead of these currently
-    fn inject_connection_established(
-        &mut self,
-        peer_id: &PeerId,
-        _connection_id: &ConnectionId,
-        _endpoint: &ConnectedPoint,
-        _failed_addresses: Option<&Vec<Multiaddr>>,
-        other_established: usize,
-    ) {
-        if other_established == 0 {
-            // find the peer's meta-data
-            debug!(self.log, "Requesting new peer's metadata"; "peer_id" => %peer_id);
-            let rpc_event =
-                RPCSend::Request(RequestId::Behaviour, OutboundRequest::MetaData(PhantomData));
-            self.events.push(NetworkBehaviourAction::NotifyHandler {
-                peer_id: *peer_id,
-                handler: NotifyHandler::Any,
-                event: rpc_event,
-            });
-        }
-    }
-
     fn inject_event(
         &mut self,
         peer_id: PeerId,