From 68b174e08844d2e033a1bfa3d1e0a2c2b2c8a67e Mon Sep 17 00:00:00 2001 From: Thomas Coratger <60488569+tcoratger@users.noreply.github.com> Date: Wed, 27 Dec 2023 15:02:10 +0100 Subject: [PATCH] refactor: small refactoring and documentation in interfaces crate (#5866) --- crates/interfaces/src/consensus.rs | 15 +- crates/interfaces/src/error.rs | 12 +- .../interfaces/src/test_utils/full_block.rs | 144 +++++++++++++----- 3 files changed, 128 insertions(+), 43 deletions(-) diff --git a/crates/interfaces/src/consensus.rs b/crates/interfaces/src/consensus.rs index 166111cb72..0adc63a50d 100644 --- a/crates/interfaces/src/consensus.rs +++ b/crates/interfaces/src/consensus.rs @@ -37,13 +37,14 @@ pub trait Consensus: Debug + Send + Sync { /// /// Note: this expects that the headers are in natural order (ascending block number) fn validate_header_range(&self, headers: &[SealedHeader]) -> Result<(), ConsensusError> { - let mut headers = headers.iter(); - let Some(mut parent) = headers.next() else { return Ok(()) }; - self.validate_header(parent)?; - for child in headers { - self.validate_header(child)?; - self.validate_header_against_parent(child, parent)?; - parent = child; + if let Some((initial_header, remaining_headers)) = headers.split_first() { + self.validate_header(initial_header)?; + let mut parent = initial_header; + for child in remaining_headers { + self.validate_header(child)?; + self.validate_header_against_parent(child, parent)?; + parent = child; + } } Ok(()) } diff --git a/crates/interfaces/src/error.rs b/crates/interfaces/src/error.rs index 58bef122b4..dad569390d 100644 --- a/crates/interfaces/src/error.rs +++ b/crates/interfaces/src/error.rs @@ -12,27 +12,37 @@ use reth_primitives::fs::FsPathError; pub type RethResult = Result; /// Core error variants possible when interacting with the blockchain. +/// +/// This enum encapsulates various error types that can occur during blockchain interactions. +/// +/// It allows for structured error handling based on the nature of the encountered issue. #[derive(Debug, thiserror::Error, Clone, PartialEq, Eq)] -#[allow(missing_docs)] pub enum RethError { + /// Error encountered during block execution. #[error(transparent)] Execution(#[from] BlockExecutionError), + /// Consensus-related errors. #[error(transparent)] Consensus(#[from] ConsensusError), + /// Database-related errors. #[error(transparent)] Database(#[from] DatabaseError), + /// Errors originating from providers. #[error(transparent)] Provider(#[from] ProviderError), + /// Errors related to networking. #[error(transparent)] Network(#[from] NetworkError), + /// Canonical errors encountered. #[error(transparent)] Canonical(#[from] CanonicalError), + /// Custom error message. #[error("{0}")] Custom(String), } diff --git a/crates/interfaces/src/test_utils/full_block.rs b/crates/interfaces/src/test_utils/full_block.rs index a0e3ed59d2..a971049199 100644 --- a/crates/interfaces/src/test_utils/full_block.rs +++ b/crates/interfaces/src/test_utils/full_block.rs @@ -17,29 +17,70 @@ use std::{collections::HashMap, sync::Arc}; #[non_exhaustive] pub struct NoopFullBlockClient; +/// Implements the `DownloadClient` trait for the `NoopFullBlockClient` struct. impl DownloadClient for NoopFullBlockClient { + /// Reports a bad message received from a peer. + /// + /// # Arguments + /// + /// * `_peer_id` - Identifier for the peer sending the bad message (unused in this + /// implementation). fn report_bad_message(&self, _peer_id: PeerId) {} + /// Retrieves the number of connected peers. + /// + /// # Returns + /// + /// The number of connected peers, which is always zero in this implementation. fn num_connected_peers(&self) -> usize { 0 } } +/// Implements the `BodiesClient` trait for the `NoopFullBlockClient` struct. impl BodiesClient for NoopFullBlockClient { + /// Defines the output type of the function. type Output = futures::future::Ready>>; + /// Retrieves block bodies based on provided hashes and priority. + /// + /// # Arguments + /// + /// * `_hashes` - A vector of block hashes (unused in this implementation). + /// * `_priority` - Priority level for block body retrieval (unused in this implementation). + /// + /// # Returns + /// + /// A future containing an empty vector of block bodies and a randomly generated `PeerId`. fn get_block_bodies_with_priority( &self, _hashes: Vec, _priority: Priority, ) -> Self::Output { + // Create a future that immediately returns an empty vector of block bodies and a random + // PeerId. futures::future::ready(Ok(WithPeerId::new(PeerId::random(), vec![]))) } } impl HeadersClient for NoopFullBlockClient { + /// The output type representing a future containing a peer request result with a vector of + /// headers. type Output = futures::future::Ready>>; + /// Retrieves headers with a specified priority level. + /// + /// This implementation does nothing and returns an empty vector of headers. + /// + /// # Arguments + /// + /// * `_request` - A request for headers (unused in this implementation). + /// * `_priority` - The priority level for the headers request (unused in this implementation). + /// + /// # Returns + /// + /// Always returns a ready future with an empty vector of headers wrapped in a + /// `PeerRequestResult`. fn get_headers_with_priority( &self, _request: HeadersRequest, @@ -75,8 +116,7 @@ impl TestFullBlockClient { /// Insert a header and body into the client maps. pub fn insert(&self, header: SealedHeader, body: BlockBody) { let hash = header.hash(); - let header = header.unseal(); - self.headers.lock().insert(hash, header); + self.headers.lock().insert(hash, header.unseal()); self.bodies.lock().insert(hash, body); } @@ -87,31 +127,52 @@ impl TestFullBlockClient { /// Get the block with the highest block number. pub fn highest_block(&self) -> Option { - let headers = self.headers.lock(); - let (hash, header) = headers.iter().max_by_key(|(hash, header)| header.number)?; - let bodies = self.bodies.lock(); - let body = bodies.get(hash)?; - Some(SealedBlock::new(header.clone().seal(*hash), body.clone())) + self.headers.lock().iter().max_by_key(|(_, header)| header.number).and_then( + |(hash, header)| { + self.bodies + .lock() + .get(hash) + .map(|body| SealedBlock::new(header.clone().seal(*hash), body.clone())) + }, + ) } } impl DownloadClient for TestFullBlockClient { + /// Reports a bad message from a specific peer. fn report_bad_message(&self, _peer_id: PeerId) {} + /// Retrieves the number of connected peers. + /// + /// Returns the number of connected peers in the test scenario (1). fn num_connected_peers(&self) -> usize { 1 } } +/// Implements the `HeadersClient` trait for the `TestFullBlockClient` struct. impl HeadersClient for TestFullBlockClient { + /// Specifies the associated output type. type Output = futures::future::Ready>>; + /// Retrieves headers with a given priority level. + /// + /// # Arguments + /// + /// * `request` - A `HeadersRequest` indicating the headers to retrieve. + /// * `_priority` - A `Priority` level for the request. + /// + /// # Returns + /// + /// A `Ready` future containing a `PeerRequestResult` with a vector of retrieved headers. fn get_headers_with_priority( &self, request: HeadersRequest, _priority: Priority, ) -> Self::Output { let headers = self.headers.lock(); + + // Initializes the block hash or number. let mut block: BlockHashOrNumber = match request.start { BlockHashOrNumber::Hash(hash) => headers.get(&hash).cloned(), BlockHashOrNumber::Number(num) => headers.values().find(|h| h.number == num).cloned(), @@ -119,48 +180,61 @@ impl HeadersClient for TestFullBlockClient { .map(|h| h.number.into()) .unwrap(); - let mut resp = Vec::new(); - - for _ in 0..request.limit { - // fetch from storage - if let Some((_, header)) = headers.iter().find(|(hash, header)| { - BlockNumHash::new(header.number, **hash).matches_block_or_num(&block) - }) { - match request.direction { - HeadersDirection::Falling => block = header.parent_hash.into(), - HeadersDirection::Rising => { - let next = header.number + 1; - block = next.into() + // Retrieves headers based on the provided limit and request direction. + let resp = (0..request.limit) + .filter_map(|_| { + headers.iter().find_map(|(hash, header)| { + // Checks if the header matches the specified block or number. + if BlockNumHash::new(header.number, *hash).matches_block_or_num(&block) { + match request.direction { + HeadersDirection::Falling => block = header.parent_hash.into(), + HeadersDirection::Rising => block = (header.number + 1).into(), + } + Some(header.clone()) + } else { + None } - } - resp.push(header.clone()); - } else { - break - } - } + }) + }) + .collect::>(); + + // Returns a future containing the retrieved headers with a random peer ID. futures::future::ready(Ok(WithPeerId::new(PeerId::random(), resp))) } } +/// Implements the `BodiesClient` trait for the `TestFullBlockClient` struct. impl BodiesClient for TestFullBlockClient { + /// Defines the output type of the function. type Output = futures::future::Ready>>; + /// Retrieves block bodies corresponding to provided hashes with a given priority. + /// + /// # Arguments + /// + /// * `hashes` - A vector of block hashes to retrieve bodies for. + /// * `_priority` - Priority level for block body retrieval (unused in this implementation). + /// + /// # Returns + /// + /// A future containing the result of the block body retrieval operation. fn get_block_bodies_with_priority( &self, hashes: Vec, _priority: Priority, ) -> Self::Output { + // Acquire a lock on the bodies. let bodies = self.bodies.lock(); - let mut all_bodies = Vec::new(); - for hash in hashes { - if let Some(body) = bodies.get(&hash) { - all_bodies.push(body.clone()); - } - if all_bodies.len() == self.soft_limit { - break - } - } - futures::future::ready(Ok(WithPeerId::new(PeerId::random(), all_bodies))) + // Create a future that immediately returns the result of the block body retrieval + // operation. + futures::future::ready(Ok(WithPeerId::new( + PeerId::random(), + hashes + .iter() + .filter_map(|hash| bodies.get(hash).cloned()) + .take(self.soft_limit) + .collect(), + ))) } }