Super tiny RPC refactor (#1187)

* wip: mwake the request id optional

* make the request_id optional

* cleanup

* address clippy lints inside rpc

* WIP: Separate sent RPC events from received ones

* WIP: Separate sent RPC events from received ones

* cleanup

* Separate request ids from substream ids

* Make RPC's message handling independent of RequestIds

* Change behaviour RPC events to be more outside-crate friendly

* Propage changes across the network + router + processor

* Propage changes across the network + router + processor

* fmt

* "tiny" refactor

* more tiny refactors

* fmt eth2-libp2p

* wip: propagating changes

* wip: propagating changes

* cleaning up

* more cleanup

* fmt

* tests HOT fix

Co-authored-by: Age Manning <Age@AgeManning.com>

beacon_node/network/src/sync/manager.rs

@@ -37,9 +37,10 @@ use super::block_processor::{spawn_block_processor, BatchProcessResult, ProcessI
 use super::network_context::SyncNetworkContext;
 use super::peer_sync_info::{PeerSyncInfo, PeerSyncType};
 use super::range_sync::{BatchId, ChainId, RangeSync};
+use super::RequestId;
 use crate::service::NetworkMessage;
 use beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome};
-use eth2_libp2p::rpc::{methods::*, RequestId};
+use eth2_libp2p::rpc::BlocksByRootRequest;
 use eth2_libp2p::types::NetworkGlobals;
 use eth2_libp2p::PeerId;
 use fnv::FnvHashMap;

beacon_node/network/src/sync/mod.rs

@@ -9,3 +9,6 @@ mod range_sync;
 
 pub use manager::SyncMessage;
 pub use peer_sync_info::PeerSyncInfo;
+
+/// Type of id of rpc requests sent by sync
+pub type RequestId = usize;

beacon_node/network/src/sync/network_context.rs

@@ -4,9 +4,8 @@
 use crate::router::processor::status_message;
 use crate::service::NetworkMessage;
 use beacon_chain::{BeaconChain, BeaconChainTypes};
-use eth2_libp2p::rpc::methods::*;
-use eth2_libp2p::rpc::{RPCEvent, RPCRequest, RequestId};
-use eth2_libp2p::{Client, NetworkGlobals, PeerId};
+use eth2_libp2p::rpc::{BlocksByRangeRequest, BlocksByRootRequest, GoodbyeReason, RequestId};
+use eth2_libp2p::{Client, NetworkGlobals, PeerId, Request};
 use slog::{debug, trace, warn};
 use std::sync::Arc;
 use tokio::sync::mpsc;
@@ -22,7 +21,7 @@ pub struct SyncNetworkContext<T: EthSpec> {
     network_globals: Arc<NetworkGlobals<T>>,
 
     /// A sequential ID for all RPC requests.
-    request_id: RequestId,
+    request_id: usize,
     /// Logger for the `SyncNetworkContext`.
     log: slog::Logger,
 }
@@ -68,7 +67,7 @@ impl<T: EthSpec> SyncNetworkContext<T> {
                 "head_slot" => format!("{}", status_message.head_slot),
             );
 
-            let _ = self.send_rpc_request(peer_id, RPCRequest::Status(status_message));
+            let _ = self.send_rpc_request(peer_id, Request::Status(status_message));
         }
     }
 
@@ -76,7 +75,7 @@ impl<T: EthSpec> SyncNetworkContext<T> {
         &mut self,
         peer_id: PeerId,
         request: BlocksByRangeRequest,
-    ) -> Result<RequestId, &'static str> {
+    ) -> Result<usize, &'static str> {
         trace!(
             self.log,
             "Sending BlocksByRange Request";
@@ -84,14 +83,14 @@ impl<T: EthSpec> SyncNetworkContext<T> {
             "count" => request.count,
             "peer" => format!("{:?}", peer_id)
         );
-        self.send_rpc_request(peer_id, RPCRequest::BlocksByRange(request))
+        self.send_rpc_request(peer_id, Request::BlocksByRange(request))
     }
 
     pub fn blocks_by_root_request(
         &mut self,
         peer_id: PeerId,
         request: BlocksByRootRequest,
-    ) -> Result<RequestId, &'static str> {
+    ) -> Result<usize, &'static str> {
         trace!(
             self.log,
             "Sending BlocksByRoot Request";
@@ -99,7 +98,7 @@ impl<T: EthSpec> SyncNetworkContext<T> {
             "count" => request.block_roots.len(),
             "peer" => format!("{:?}", peer_id)
         );
-        self.send_rpc_request(peer_id, RPCRequest::BlocksByRoot(request))
+        self.send_rpc_request(peer_id, Request::BlocksByRoot(request))
     }
 
     pub fn downvote_peer(&mut self, peer_id: PeerId) {
@@ -109,6 +108,10 @@ impl<T: EthSpec> SyncNetworkContext<T> {
             "peer" => format!("{:?}", peer_id)
         );
         // TODO: Implement reputation
+        // TODO: what if we first close the channel sending a response
+        // RPCResponseErrorCode::InvalidRequest (or something)
+        // and then disconnect the peer? either request dc or let the behaviour have that logic
+        // itself
         self.disconnect(peer_id, GoodbyeReason::Fault);
     }
 
@@ -121,7 +124,7 @@ impl<T: EthSpec> SyncNetworkContext<T> {
         );
 
         // ignore the error if the channel send fails
-        let _ = self.send_rpc_request(peer_id.clone(), RPCRequest::Goodbye(reason));
+        let _ = self.send_rpc_request(peer_id.clone(), Request::Goodbye(reason));
         self.network_send
             .send(NetworkMessage::Disconnect { peer_id })
             .unwrap_or_else(|_| {
@@ -135,27 +138,22 @@ impl<T: EthSpec> SyncNetworkContext<T> {
     pub fn send_rpc_request(
         &mut self,
         peer_id: PeerId,
-        rpc_request: RPCRequest<T>,
-    ) -> Result<RequestId, &'static str> {
+        request: Request,
+    ) -> Result<usize, &'static str> {
         let request_id = self.request_id;
         self.request_id += 1;
-        self.send_rpc_event(peer_id, RPCEvent::Request(request_id, rpc_request))?;
+        self.send_network_msg(NetworkMessage::SendRequest {
+            peer_id,
+            request_id: RequestId::Sync(request_id),
+            request,
+        })?;
         Ok(request_id)
     }
 
-    fn send_rpc_event(
-        &mut self,
-        peer_id: PeerId,
-        rpc_event: RPCEvent<T>,
-    ) -> Result<(), &'static str> {
-        self.network_send
-            .send(NetworkMessage::RPC(peer_id, rpc_event))
-            .map_err(|_| {
-                debug!(
-                    self.log,
-                    "Could not send RPC message to the network service"
-                );
-                "Network channel send Failed"
-            })
+    fn send_network_msg(&mut self, msg: NetworkMessage<T>) -> Result<(), &'static str> {
+        self.network_send.send(msg).map_err(|_| {
+            debug!(self.log, "Could not send message to the network service");
+            "Network channel send Failed"
+        })
     }
 }

beacon_node/network/src/sync/peer_sync_info.rs

@@ -1,7 +1,7 @@
 use super::manager::SLOT_IMPORT_TOLERANCE;
 use crate::router::processor::status_message;
 use beacon_chain::{BeaconChain, BeaconChainTypes};
-use eth2_libp2p::rpc::methods::*;
+use eth2_libp2p::rpc::*;
 use eth2_libp2p::SyncInfo;
 use std::ops::Sub;
 use std::sync::Arc;

beacon_node/network/src/sync/range_sync/batch.rs

@@ -1,6 +1,5 @@
 use super::chain::EPOCHS_PER_BATCH;
 use eth2_libp2p::rpc::methods::*;
-use eth2_libp2p::rpc::RequestId;
 use eth2_libp2p::PeerId;
 use fnv::FnvHashMap;
 use ssz::Encode;
@@ -112,9 +111,9 @@ impl<T: EthSpec> PartialOrd for Batch<T> {
 /// This is used to optimise searches for idle peers (peers that have no outbound batch requests).
 pub struct PendingBatches<T: EthSpec> {
     /// The current pending batches.
-    batches: FnvHashMap<RequestId, Batch<T>>,
+    batches: FnvHashMap<usize, Batch<T>>,
     /// A mapping of peers to the number of pending requests.
-    peer_requests: HashMap<PeerId, HashSet<RequestId>>,
+    peer_requests: HashMap<PeerId, HashSet<usize>>,
 }
 
 impl<T: EthSpec> PendingBatches<T> {
@@ -125,7 +124,7 @@ impl<T: EthSpec> PendingBatches<T> {
         }
     }
 
-    pub fn insert(&mut self, request_id: RequestId, batch: Batch<T>) -> Option<Batch<T>> {
+    pub fn insert(&mut self, request_id: usize, batch: Batch<T>) -> Option<Batch<T>> {
         let peer_request = batch.current_peer.clone();
         self.peer_requests
             .entry(peer_request)
@@ -134,7 +133,7 @@ impl<T: EthSpec> PendingBatches<T> {
         self.batches.insert(request_id, batch)
     }
 
-    pub fn remove(&mut self, request_id: RequestId) -> Option<Batch<T>> {
+    pub fn remove(&mut self, request_id: usize) -> Option<Batch<T>> {
         if let Some(batch) = self.batches.remove(&request_id) {
             if let Entry::Occupied(mut entry) = self.peer_requests.entry(batch.current_peer.clone())
             {
@@ -157,7 +156,7 @@ impl<T: EthSpec> PendingBatches<T> {
 
     /// Adds a block to the batches if the request id exists. Returns None if there is no batch
     /// matching the request id.
-    pub fn add_block(&mut self, request_id: RequestId, block: SignedBeaconBlock<T>) -> Option<()> {
+    pub fn add_block(&mut self, request_id: usize, block: SignedBeaconBlock<T>) -> Option<()> {
         let batch = self.batches.get_mut(&request_id)?;
         batch.downloaded_blocks.push(block);
         Some(())

beacon_node/network/src/sync/range_sync/chain.rs

@@ -1,9 +1,8 @@
 use super::batch::{Batch, BatchId, PendingBatches};
 use crate::sync::block_processor::{spawn_block_processor, BatchProcessResult, ProcessId};
 use crate::sync::network_context::SyncNetworkContext;
-use crate::sync::SyncMessage;
+use crate::sync::{RequestId, SyncMessage};
 use beacon_chain::{BeaconChain, BeaconChainTypes};
-use eth2_libp2p::rpc::RequestId;
 use eth2_libp2p::PeerId;
 use rand::prelude::*;
 use slog::{crit, debug, warn};

beacon_node/network/src/sync/range_sync/range.rs

@@ -47,8 +47,8 @@ use crate::sync::block_processor::BatchProcessResult;
 use crate::sync::manager::SyncMessage;
 use crate::sync::network_context::SyncNetworkContext;
 use crate::sync::PeerSyncInfo;
+use crate::sync::RequestId;
 use beacon_chain::{BeaconChain, BeaconChainTypes};
-use eth2_libp2p::rpc::RequestId;
 use eth2_libp2p::{NetworkGlobals, PeerId};
 use slog::{debug, error, trace};
 use std::collections::HashSet;