diff --git a/beacon_node/network/src/router.rs b/beacon_node/network/src/router.rs index a0dd0665acf..2f7a92e91ed 100644 --- a/beacon_node/network/src/router.rs +++ b/beacon_node/network/src/router.rs @@ -493,10 +493,7 @@ impl Router { ) { let request_id = match request_id { RequestId::Sync(sync_id) => match sync_id { - SyncId::SingleBlock { .. } - | SyncId::SingleBlob { .. } - | SyncId::ParentLookup { .. } - | SyncId::ParentLookupBlob { .. } => { + SyncId::SingleBlock { .. } | SyncId::SingleBlob { .. } => { crit!(self.log, "Block lookups do not request BBRange requests"; "peer_id" => %peer_id); return; } @@ -561,7 +558,7 @@ impl Router { ) { let request_id = match request_id { RequestId::Sync(sync_id) => match sync_id { - id @ (SyncId::SingleBlock { .. } | SyncId::ParentLookup { .. }) => id, + id @ SyncId::SingleBlock { .. } => id, SyncId::BackFillBlocks { .. } | SyncId::RangeBlocks { .. } | SyncId::RangeBlockAndBlobs { .. } @@ -569,7 +566,7 @@ impl Router { crit!(self.log, "Batch syncing do not request BBRoot requests"; "peer_id" => %peer_id); return; } - SyncId::SingleBlob { .. } | SyncId::ParentLookupBlob { .. } => { + SyncId::SingleBlob { .. } => { crit!(self.log, "Blob response to block by roots request"; "peer_id" => %peer_id); return; } @@ -602,8 +599,8 @@ impl Router { ) { let request_id = match request_id { RequestId::Sync(sync_id) => match sync_id { - id @ (SyncId::SingleBlob { .. } | SyncId::ParentLookupBlob { .. }) => id, - SyncId::SingleBlock { .. } | SyncId::ParentLookup { .. } => { + id @ SyncId::SingleBlob { .. } => id, + SyncId::SingleBlock { .. } => { crit!(self.log, "Block response to blobs by roots request"; "peer_id" => %peer_id); return; } diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index 08d0758c7e8..a46e7d09376 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -25,7 +25,7 @@ pub enum ResponseType { Blob, } -#[derive(Debug, Copy, Clone)] +#[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)] pub enum LookupType { Current, Parent, @@ -119,6 +119,7 @@ pub trait RequestState { let id = SingleLookupReqId { id, req_counter: self.get_state().req_counter, + lookup_type: L::lookup_type(), }; Self::make_request(id, peer_id, request, cx) } @@ -265,7 +266,7 @@ impl RequestState for BlockRequestState request: Self::RequestType, cx: &SyncNetworkContext, ) -> Result<(), LookupRequestError> { - cx.block_lookup_request(id, peer_id, request, L::lookup_type()) + cx.block_lookup_request(id, peer_id, request) .map_err(LookupRequestError::SendFailed) } @@ -365,7 +366,7 @@ impl RequestState for BlobRequestState, ) -> Result<(), LookupRequestError> { - cx.blob_lookup_request(id, peer_id, request, L::lookup_type()) + cx.blob_lookup_request(id, peer_id, request) .map_err(LookupRequestError::SendFailed) } diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index f3c4a768ffe..949e1762ac7 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -691,7 +691,7 @@ impl BlockLookups { pub fn parent_lookup_failed>( &mut self, id: SingleLookupReqId, - peer_id: PeerId, + peer_id: &PeerId, cx: &SyncNetworkContext, error: RPCError, ) { diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 275976cae0d..6d50fe63200 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -296,7 +296,7 @@ impl TestRig { beacon_block: Option>>, ) { self.send_sync_message(SyncMessage::RpcBlock { - request_id: SyncRequestId::ParentLookup { id }, + request_id: SyncRequestId::SingleBlock { id }, peer_id, beacon_block, seen_timestamp: D, @@ -324,7 +324,7 @@ impl TestRig { blob_sidecar: Option>>, ) { self.send_sync_message(SyncMessage::RpcBlob { - request_id: SyncRequestId::ParentLookupBlob { id }, + request_id: SyncRequestId::SingleBlob { id }, peer_id, blob_sidecar, seen_timestamp: D, @@ -348,7 +348,7 @@ impl TestRig { fn parent_lookup_failed(&mut self, id: SingleLookupReqId, peer_id: PeerId, error: RPCError) { self.send_sync_message(SyncMessage::RpcError { peer_id, - request_id: SyncRequestId::ParentLookup { id }, + request_id: SyncRequestId::SingleBlock { id }, error, }) } @@ -409,7 +409,11 @@ impl TestRig { peer_id: _, request: Request::BlocksByRoot(request), request_id: RequestId::Sync(SyncRequestId::SingleBlock { id }), - } if request.block_roots().to_vec().contains(&for_block) => Some(*id), + } if id.lookup_type == LookupType::Current + && request.block_roots().to_vec().contains(&for_block) => + { + Some(*id) + } _ => None, }) .unwrap_or_else(|e| panic!("Expected block request for {for_block:?}: {e}")) @@ -422,11 +426,12 @@ impl TestRig { peer_id: _, request: Request::BlobsByRoot(request), request_id: RequestId::Sync(SyncRequestId::SingleBlob { id }), - } if request - .blob_ids - .to_vec() - .iter() - .any(|r| r.block_root == for_block) => + } if id.lookup_type == LookupType::Current + && request + .blob_ids + .to_vec() + .iter() + .any(|r| r.block_root == for_block) => { Some(*id) } @@ -441,8 +446,12 @@ impl TestRig { NetworkMessage::SendRequest { peer_id: _, request: Request::BlocksByRoot(request), - request_id: RequestId::Sync(SyncRequestId::ParentLookup { id }), - } if request.block_roots().to_vec().contains(&for_block) => Some(*id), + request_id: RequestId::Sync(SyncRequestId::SingleBlock { id }), + } if id.lookup_type == LookupType::Parent + && request.block_roots().to_vec().contains(&for_block) => + { + Some(*id) + } _ => None, }) .unwrap_or_else(|e| panic!("Expected block parent request for {for_block:?}: {e}")) @@ -454,12 +463,13 @@ impl TestRig { NetworkMessage::SendRequest { peer_id: _, request: Request::BlobsByRoot(request), - request_id: RequestId::Sync(SyncRequestId::ParentLookupBlob { id }), - } if request - .blob_ids - .to_vec() - .iter() - .all(|r| r.block_root == for_block) => + request_id: RequestId::Sync(SyncRequestId::SingleBlob { id }), + } if id.lookup_type == LookupType::Parent + && request + .blob_ids + .to_vec() + .iter() + .all(|r| r.block_root == for_block) => { Some(*id) } @@ -1974,7 +1984,7 @@ mod deneb_only { return; }; let (block, blobs) = r.rand_block_and_blobs(NumBlobs::Number(2)); - let parent_root = block.parent_root(); + let block_root = block.canonical_root(); let blob_0 = blobs[0].clone(); let blob_1 = blobs[1].clone(); let peer_a = r.new_connected_peer(); @@ -1982,7 +1992,7 @@ mod deneb_only { // Send unknown parent block lookup r.trigger_unknown_parent_block(peer_a, block.into()); // Expect network request for blobs - let id = r.expect_blob_parent_request(parent_root); + let id = r.expect_blob_lookup_request(block_root); // Peer responses with blob 0 r.single_lookup_blob_response(id, peer_a, Some(blob_0.into())); // Blob 1 is received via gossip unknown parent blob from a different peer diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 0084829e63b..ca4d0a2183d 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -34,6 +34,7 @@ //! search for the block and subsequently search for parents if needed. use super::backfill_sync::{BackFillSync, ProcessResult, SyncStart}; +use super::block_lookups::common::LookupType; use super::block_lookups::BlockLookups; use super::network_context::{BlockOrBlob, SyncNetworkContext}; use super::peer_sync_info::{remote_sync_type, PeerSyncType}; @@ -80,6 +81,7 @@ pub type Id = u32; pub struct SingleLookupReqId { pub id: Id, pub req_counter: Id, + pub lookup_type: LookupType, } /// Id of rpc requests sent by sync to the network. @@ -89,12 +91,6 @@ pub enum RequestId { SingleBlock { id: SingleLookupReqId }, /// Request searching for a set of blobs given a hash. SingleBlob { id: SingleLookupReqId }, - /// Request searching for a block's parent. The id is the chain, share with the corresponding - /// blob id. - ParentLookup { id: SingleLookupReqId }, - /// Request searching for a block's parent blobs. The id is the chain, shared with the corresponding - /// block id. - ParentLookupBlob { id: SingleLookupReqId }, /// Request was from the backfill sync algorithm. BackFillBlocks { id: Id }, /// Backfill request that is composed by both a block range request and a blob range request. @@ -331,42 +327,42 @@ impl SyncManager { fn inject_error(&mut self, peer_id: PeerId, request_id: RequestId, error: RPCError) { trace!(self.log, "Sync manager received a failed RPC"); match request_id { - RequestId::SingleBlock { id } => { - self.block_lookups + RequestId::SingleBlock { id } => match id.lookup_type { + LookupType::Current => self + .block_lookups .single_block_lookup_failed::>( id, &peer_id, &self.network, error, - ); - } - RequestId::SingleBlob { id } => { - self.block_lookups - .single_block_lookup_failed::>( + ), + LookupType::Parent => self + .block_lookups + .parent_lookup_failed::>( id, &peer_id, &self.network, error, - ); - } - RequestId::ParentLookup { id } => { - self.block_lookups - .parent_lookup_failed::>( + ), + }, + RequestId::SingleBlob { id } => match id.lookup_type { + LookupType::Current => self + .block_lookups + .single_block_lookup_failed::>( id, - peer_id, + &peer_id, &self.network, error, - ); - } - RequestId::ParentLookupBlob { id } => { - self.block_lookups + ), + LookupType::Parent => self + .block_lookups .parent_lookup_failed::>( id, - peer_id, + &peer_id, &self.network, error, - ); - } + ), + }, RequestId::BackFillBlocks { id } => { if let Some(batch_id) = self .network @@ -882,30 +878,29 @@ impl SyncManager { seen_timestamp: Duration, ) { match request_id { - RequestId::SingleBlock { id } => self - .block_lookups - .single_lookup_response::>( - id, - peer_id, - block, - seen_timestamp, - &self.network, - ), + RequestId::SingleBlock { id } => match id.lookup_type { + LookupType::Current => self + .block_lookups + .single_lookup_response::>( + id, + peer_id, + block, + seen_timestamp, + &self.network, + ), + LookupType::Parent => self + .block_lookups + .parent_lookup_response::>( + id, + peer_id, + block, + seen_timestamp, + &self.network, + ), + }, RequestId::SingleBlob { .. } => { crit!(self.log, "Block received during blob request"; "peer_id" => %peer_id ); } - RequestId::ParentLookup { id } => self - .block_lookups - .parent_lookup_response::>( - id, - peer_id, - block, - seen_timestamp, - &self.network, - ), - RequestId::ParentLookupBlob { id: _ } => { - crit!(self.log, "Block received during parent blob request"; "peer_id" => %peer_id ); - } RequestId::BackFillBlocks { id } => { let is_stream_terminator = block.is_none(); if let Some(batch_id) = self @@ -966,28 +961,26 @@ impl SyncManager { RequestId::SingleBlock { .. } => { crit!(self.log, "Single blob received during block request"; "peer_id" => %peer_id ); } - RequestId::SingleBlob { id } => self - .block_lookups - .single_lookup_response::>( - id, - peer_id, - blob, - seen_timestamp, - &self.network, - ), - - RequestId::ParentLookup { id: _ } => { - crit!(self.log, "Single blob received during parent block request"; "peer_id" => %peer_id ); - } - RequestId::ParentLookupBlob { id } => self - .block_lookups - .parent_lookup_response::>( - id, - peer_id, - blob, - seen_timestamp, - &self.network, - ), + RequestId::SingleBlob { id } => match id.lookup_type { + LookupType::Current => self + .block_lookups + .single_lookup_response::>( + id, + peer_id, + blob, + seen_timestamp, + &self.network, + ), + LookupType::Parent => self + .block_lookups + .parent_lookup_response::>( + id, + peer_id, + blob, + seen_timestamp, + &self.network, + ), + }, RequestId::BackFillBlocks { id: _ } => { crit!(self.log, "Blob received during backfill block request"; "peer_id" => %peer_id ); } diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index f15105781f0..29effb7bce0 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -7,7 +7,6 @@ use super::range_sync::{BatchId, ByRangeRequestType, ChainId}; use crate::network_beacon_processor::NetworkBeaconProcessor; use crate::service::{NetworkMessage, RequestId}; use crate::status::ToStatusMessage; -use crate::sync::block_lookups::common::LookupType; use crate::sync::manager::SingleLookupReqId; use beacon_chain::block_verification_types::RpcBlock; use beacon_chain::{BeaconChain, BeaconChainTypes, EngineState}; @@ -437,27 +436,20 @@ impl SyncNetworkContext { id: SingleLookupReqId, peer_id: PeerId, request: BlocksByRootRequest, - lookup_type: LookupType, ) -> Result<(), &'static str> { - let sync_id = match lookup_type { - LookupType::Current => SyncRequestId::SingleBlock { id }, - LookupType::Parent => SyncRequestId::ParentLookup { id }, - }; - let request_id = RequestId::Sync(sync_id); - debug!( self.log, "Sending BlocksByRoot Request"; "method" => "BlocksByRoot", "block_roots" => ?request.block_roots().to_vec(), "peer" => %peer_id, - "lookup_type" => ?lookup_type + "id" => ?id ); self.send_network_msg(NetworkMessage::SendRequest { peer_id, request: Request::BlocksByRoot(request), - request_id, + request_id: RequestId::Sync(SyncRequestId::SingleBlock { id }), })?; Ok(()) } @@ -467,14 +459,7 @@ impl SyncNetworkContext { id: SingleLookupReqId, blob_peer_id: PeerId, blob_request: BlobsByRootRequest, - lookup_type: LookupType, ) -> Result<(), &'static str> { - let sync_id = match lookup_type { - LookupType::Current => SyncRequestId::SingleBlob { id }, - LookupType::Parent => SyncRequestId::ParentLookupBlob { id }, - }; - let request_id = RequestId::Sync(sync_id); - if let Some(block_root) = blob_request .blob_ids .as_slice() @@ -494,13 +479,13 @@ impl SyncNetworkContext { "block_root" => ?block_root, "blob_indices" => ?indices, "peer" => %blob_peer_id, - "lookup_type" => ?lookup_type + "id" => ?id ); self.send_network_msg(NetworkMessage::SendRequest { peer_id: blob_peer_id, request: Request::BlobsByRoot(blob_request), - request_id, + request_id: RequestId::Sync(SyncRequestId::SingleBlob { id }), })?; } Ok(())