From 5135a77e38cc2b5a13a543d1b8537538e27e0f7e Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 7 May 2024 02:55:12 +0900 Subject: [PATCH] Move sync lookup trait function to its caller (#5704) * Move sync lookup trait function to its caller * lint --- .../network/src/sync/block_lookups/common.rs | 55 +------------------ .../sync/block_lookups/single_block_lookup.rs | 50 +++++++++++++---- 2 files changed, 41 insertions(+), 64 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index 06e00ea6d1..fa63e37c1b 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -1,9 +1,7 @@ use crate::sync::block_lookups::single_block_lookup::{ LookupRequestError, SingleBlockLookup, SingleLookupRequestState, }; -use crate::sync::block_lookups::{ - BlobRequestState, BlockRequestState, PeerId, SINGLE_BLOCK_LOOKUP_MAX_ATTEMPTS, -}; +use crate::sync::block_lookups::{BlobRequestState, BlockRequestState, PeerId}; use crate::sync::manager::{BlockProcessType, Id, SLOT_IMPORT_TOLERANCE}; use crate::sync::network_context::SyncNetworkContext; use beacon_chain::block_verification_types::RpcBlock; @@ -26,11 +24,6 @@ pub enum ResponseType { /// is further back than the most recent head slot. pub(crate) const PARENT_DEPTH_TOLERANCE: usize = SLOT_IMPORT_TOLERANCE * 2; -/// Wrapper around bool to prevent mixing this argument with `BlockIsProcessed` -pub(crate) struct AwaitingParent(pub bool); -/// Wrapper around bool to prevent mixing this argument with `AwaitingParent` -pub(crate) struct BlockIsProcessed(pub bool); - /// This trait unifies common single block lookup functionality across blocks and blobs. This /// includes making requests, verifying responses, and handling processing results. A /// `SingleBlockLookup` includes both a `BlockRequestState` and a `BlobRequestState`, this trait is @@ -43,52 +36,6 @@ pub trait RequestState { /// The type created after validation. type VerifiedResponseType: Clone; - /// Potentially makes progress on this request if it's in a progress-able state - fn continue_request( - &mut self, - id: Id, - awaiting_parent: AwaitingParent, - downloaded_block_expected_blobs: Option, - block_is_processed: BlockIsProcessed, - cx: &mut SyncNetworkContext, - ) -> Result<(), LookupRequestError> { - // Attempt to progress awaiting downloads - if self.get_state().is_awaiting_download() { - // Verify the current request has not exceeded the maximum number of attempts. - let request_state = self.get_state(); - if request_state.failed_attempts() >= SINGLE_BLOCK_LOOKUP_MAX_ATTEMPTS { - let cannot_process = request_state.more_failed_processing_attempts(); - return Err(LookupRequestError::TooManyAttempts { cannot_process }); - } - - let peer_id = self - .get_state_mut() - .use_rand_available_peer() - .ok_or(LookupRequestError::NoPeers)?; - - // make_request returns true only if a request needs to be made - if self.make_request(id, peer_id, downloaded_block_expected_blobs, cx)? { - self.get_state_mut().on_download_start()?; - } else { - self.get_state_mut().on_completed_request()?; - } - - // Otherwise, attempt to progress awaiting processing - // If this request is awaiting a parent lookup to be processed, do not send for processing. - // The request will be rejected with unknown parent error. - } else if !awaiting_parent.0 - && (block_is_processed.0 || matches!(Self::response_type(), ResponseType::Block)) - { - // maybe_start_processing returns Some if state == AwaitingProcess. This pattern is - // useful to conditionally access the result data. - if let Some(result) = self.get_state_mut().maybe_start_processing() { - return Self::send_for_processing(id, result, cx); - } - } - - Ok(()) - } - /// Request the network context to prepare a request of a component of `block_root`. If the /// request is not necessary because the component is already known / processed, return false. /// Return true if it sent a request and we can expect an event back from the network. diff --git a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs index e5a79e8129..a5729f3906 100644 --- a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs @@ -1,5 +1,5 @@ -use super::common::{AwaitingParent, BlockIsProcessed}; -use super::{BlockComponent, PeerId}; +use super::common::ResponseType; +use super::{BlockComponent, PeerId, SINGLE_BLOCK_LOOKUP_MAX_ATTEMPTS}; use crate::sync::block_lookups::common::RequestState; use crate::sync::block_lookups::Id; use crate::sync::network_context::SyncNetworkContext; @@ -150,7 +150,7 @@ impl SingleBlockLookup { } } - /// Wrapper around `RequestState::continue_request` to inject lookup data + /// Potentially makes progress on this request if it's in a progress-able state pub fn continue_request>( &mut self, cx: &mut SyncNetworkContext, @@ -163,13 +163,43 @@ impl SingleBlockLookup { .peek_downloaded_data() .map(|block| block.num_expected_blobs()); let block_is_processed = self.block_request_state.state.is_processed(); - R::request_state_mut(self).continue_request( - id, - AwaitingParent(awaiting_parent), - downloaded_block_expected_blobs, - BlockIsProcessed(block_is_processed), - cx, - ) + let request = R::request_state_mut(self); + + // Attempt to progress awaiting downloads + if request.get_state().is_awaiting_download() { + // Verify the current request has not exceeded the maximum number of attempts. + let request_state = request.get_state(); + if request_state.failed_attempts() >= SINGLE_BLOCK_LOOKUP_MAX_ATTEMPTS { + let cannot_process = request_state.more_failed_processing_attempts(); + return Err(LookupRequestError::TooManyAttempts { cannot_process }); + } + + let peer_id = request + .get_state_mut() + .use_rand_available_peer() + .ok_or(LookupRequestError::NoPeers)?; + + // make_request returns true only if a request needs to be made + if request.make_request(id, peer_id, downloaded_block_expected_blobs, cx)? { + request.get_state_mut().on_download_start()?; + } else { + request.get_state_mut().on_completed_request()?; + } + + // Otherwise, attempt to progress awaiting processing + // If this request is awaiting a parent lookup to be processed, do not send for processing. + // The request will be rejected with unknown parent error. + } else if !awaiting_parent + && (block_is_processed || matches!(R::response_type(), ResponseType::Block)) + { + // maybe_start_processing returns Some if state == AwaitingProcess. This pattern is + // useful to conditionally access the result data. + if let Some(result) = request.get_state_mut().maybe_start_processing() { + return R::send_for_processing(id, result, cx); + } + } + + Ok(()) } /// Add peer to all request states. The peer must be able to serve this request.