From c64fe8853d9a81887ee4764bd66149979ddfa6e2 Mon Sep 17 00:00:00 2001 From: Rostislav Rumenov Date: Fri, 8 Dec 2023 14:03:33 +0000 Subject: [PATCH] refactor: don't use the ArtifactChunk type in the old P2P; remove the UnitChunk variant as well. --- rs/p2p/src/download_management.rs | 56 ++------------------ rs/p2p/src/gossip_protocol.rs | 22 ++------ rs/p2p/src/gossip_types.rs | 26 +++------ rs/p2p/state_sync_manager/tests/common.rs | 12 ++--- rs/protobuf/def/types/v1/p2p.proto | 12 +---- rs/protobuf/src/gen/types/types.v1.rs | 27 ++-------- rs/state_manager/src/state_sync/chunkable.rs | 11 +--- rs/state_manager/tests/common/mod.rs | 6 --- rs/types/types/src/chunkable.rs | 56 +------------------- rs/types/types/src/single_chunked.rs | 37 ++----------- 10 files changed, 32 insertions(+), 233 deletions(-) diff --git a/rs/p2p/src/download_management.rs b/rs/p2p/src/download_management.rs index 31d4ac6e656..51849d0b01e 100644 --- a/rs/p2p/src/download_management.rs +++ b/rs/p2p/src/download_management.rs @@ -83,7 +83,6 @@ use ic_types::{ CanisterHttpArtifact, CertificationArtifact, ConsensusArtifact, DkgArtifact, EcdsaArtifact, IngressArtifact, }, - chunkable::ArtifactChunkData, chunkable::ChunkId, chunkable::CHUNKID_UNIT_CHUNK, crypto::CryptoHash, @@ -211,7 +210,7 @@ impl GossipImpl { // Allowing the rest of the artifact to be downloaded and // skipping only the affected chunk increase overall // resilience. - if let Err(error) = gossip_chunk.artifact_chunk { + if let Err(error) = gossip_chunk.artifact { self.metrics.chunks_not_served_from_peer.inc(); trace!( self.log, @@ -262,12 +261,7 @@ impl GossipImpl { let artifact_tracker = artifact_tracker.unwrap(); // Feed the chunk to the tracker. - let completed_artifact = match gossip_chunk.artifact_chunk.unwrap().artifact_chunk_data { - ArtifactChunkData::UnitChunkData(artifact) => artifact, - _ => { - return; - } - }; + let completed_artifact = gossip_chunk.artifact.unwrap(); // Record metrics. self.metrics.artifacts_received.inc(); @@ -1007,7 +1001,6 @@ pub mod tests { types::ids::{node_test_id, subnet_test_id}, }; use ic_test_utilities_registry::{add_subnet_record, SubnetRecordBuilder}; - use ic_types::chunkable::ArtifactErrorCode; use ic_types::consensus::dkg::{DealingContent, DkgMessageId, Message as DkgMessage}; use ic_types::crypto::{ threshold_sig::ni_dkg::{NiDkgDealing, NiDkgId, NiDkgTag, NiDkgTargetSubnet}, @@ -1018,7 +1011,7 @@ pub mod tests { use ic_types::{ artifact, artifact::{Artifact, ArtifactAttribute, ArtifactPriorityFn, Priority}, - chunkable::{ArtifactChunk, ArtifactChunkData, Chunkable, ChunkableArtifact}, + chunkable::ChunkableArtifact, Height, NodeId, PrincipalId, }; use ic_types::{artifact::ArtifactKind, artifact_kind::ConsensusArtifact, consensus::*}; @@ -1037,43 +1030,6 @@ pub mod tests { #[derive(Default)] pub(crate) struct TestArtifactManager {} - /// The test artifact. - struct TestArtifact { - /// The number of chunks. - num_chunks: u32, - /// The list of artifact chunks. - chunks: Vec, - } - - /// `TestArtifact` implements the `Chunkable` trait. - impl Chunkable for TestArtifact { - /// The method returns an Iterator over the chunks to download. - fn chunks_to_download(&self) -> Box> { - Box::new( - (0..self.num_chunks) - .map(ChunkId::from) - .collect::>() - .into_iter(), - ) - } - - /// The method adds the given chunk. - fn add_chunk( - &mut self, - artifact_chunk: ArtifactChunk, - ) -> Result { - self.chunks.push(artifact_chunk.clone()); - if self.chunks.len() == self.num_chunks as usize { - match artifact_chunk.artifact_chunk_data { - ArtifactChunkData::UnitChunkData(artifact) => Ok(artifact), - _ => Err(ArtifactErrorCode::ChunkVerificationFailed), - } - } else { - Err(ArtifactErrorCode::ChunksMoreNeeded) - } - } - } - /// The `TestArtifactManager` implements the `TestArtifact` trait. impl ArtifactManager for TestArtifactManager { /// The method ignores the artifact and always returns Ok(()). @@ -1555,10 +1511,6 @@ pub mod tests { integrity_hash: CryptoHash, ) -> GossipChunk { let payload = Artifact::DkgMessage(receive_check_test_create_message(number)); - let artifact_chunk = ArtifactChunk { - chunk_id, - artifact_chunk_data: ArtifactChunkData::UnitChunkData(payload), - }; let request = GossipChunkRequest { artifact_id, @@ -1567,7 +1519,7 @@ pub mod tests { }; GossipChunk { request, - artifact_chunk: Ok(artifact_chunk), + artifact: Ok(payload), } } diff --git a/rs/p2p/src/gossip_protocol.rs b/rs/p2p/src/gossip_protocol.rs index c1aeab555d2..fd4ebe06f72 100644 --- a/rs/p2p/src/gossip_protocol.rs +++ b/rs/p2p/src/gossip_protocol.rs @@ -68,10 +68,7 @@ use ic_logger::{info, replica_logger::ReplicaLogger}; use ic_metrics::MetricsRegistry; use ic_protobuf::registry::subnet::v1::GossipConfig; use ic_registry_client_helpers::subnet::SubnetRegistry; -use ic_types::{ - artifact::ArtifactFilter, chunkable::ArtifactChunk, chunkable::ArtifactChunkData, - crypto::CryptoHash, p2p::GossipAdvert, NodeId, SubnetId, -}; +use ic_types::{artifact::ArtifactFilter, crypto::CryptoHash, p2p::GossipAdvert, NodeId, SubnetId}; use lru::LruCache; use parking_lot::{Mutex, RwLock}; use std::collections::HashMap; @@ -261,22 +258,11 @@ impl Gossip for GossipImpl { .with_label_values(&["in_chunk_request"]) .start_timer(); - let artifact_chunk = match self + let artifact = match self .artifact_manager .get_validated_by_identifier(&chunk_request.artifact_id) { - Some(artifact) => artifact - .get_chunk(chunk_request.chunk_id) - .map(|a| ArtifactChunk { - chunk_id: chunk_request.chunk_id, - artifact_chunk_data: ArtifactChunkData::UnitChunkData(a), - }) - .ok_or_else(|| { - self.gossip_metrics.requested_chunks_not_found.inc(); - P2PError { - p2p_error_code: P2PErrorCode::NotFound, - } - }), + Some(artifact) => Ok(artifact.get_chunk()), None => { self.gossip_metrics.requested_chunks_not_found.inc(); Err(P2PError { @@ -287,7 +273,7 @@ impl Gossip for GossipImpl { let gossip_chunk = GossipChunk { request: chunk_request, - artifact_chunk, + artifact, }; let message = GossipMessage::Chunk(gossip_chunk); self.transport_send(message, node_id); diff --git a/rs/p2p/src/gossip_types.rs b/rs/p2p/src/gossip_types.rs index 220dd912a81..a7075289e76 100644 --- a/rs/p2p/src/gossip_types.rs +++ b/rs/p2p/src/gossip_types.rs @@ -5,8 +5,8 @@ use ic_protobuf::types::v1 as pb; use ic_protobuf::types::v1::gossip_chunk::Response; use ic_protobuf::types::v1::gossip_message::Body; use ic_types::{ - artifact::{ArtifactFilter, ArtifactId}, - chunkable::{ArtifactChunk, ChunkId}, + artifact::{Artifact, ArtifactFilter, ArtifactId}, + chunkable::ChunkId, crypto::CryptoHash, p2p::GossipAdvert, }; @@ -31,7 +31,7 @@ pub(crate) struct GossipChunk { /// The request which resulted in the 'artifact_chunk'. pub(crate) request: GossipChunkRequest, /// The artifact chunk, encapsulated in a `P2PResult`. - pub(crate) artifact_chunk: P2PResult, + pub(crate) artifact: P2PResult, } /// This is the message exchanged on the wire with other peers. This @@ -135,12 +135,9 @@ impl TryFrom for GossipChunkRequest { impl From for pb::GossipChunk { /// The function converts the given chunk into the Protobuf equivalent. fn from(gossip_chunk: GossipChunk) -> Self { - let GossipChunk { - request, - artifact_chunk, - } = gossip_chunk; - let response = match artifact_chunk { - Ok(artifact_chunk) => Some(Response::Chunk(artifact_chunk.into())), + let GossipChunk { request, artifact } = gossip_chunk; + let response = match artifact { + Ok(artifact_chunk) => Some(Response::Artifact(artifact_chunk.into())), // Add additional cases as required. Err(_) => Some(Response::Error(pb::P2pError::NotFound as i32)), }; @@ -160,18 +157,11 @@ impl TryFrom for GossipChunk { let request = gossip_chunk.request.ok_or(ProxyDecodeError::MissingField( "The 'request' field is missing", ))?; - let chunk_id = ChunkId::from(request.chunk_id); let request = GossipChunkRequest::try_from(request)?; Ok(Self { request, - artifact_chunk: match response { - Response::Chunk(c) => { - let artifact_chunk: ArtifactChunk = c.try_into()?; - Ok(ArtifactChunk { - chunk_id, - artifact_chunk_data: artifact_chunk.artifact_chunk_data, - }) - } + artifact: match response { + Response::Artifact(c) => Ok(c.try_into()?), Response::Error(_e) => Err(P2PError { p2p_error_code: P2PErrorCode::NotFound, }), diff --git a/rs/p2p/state_sync_manager/tests/common.rs b/rs/p2p/state_sync_manager/tests/common.rs index de9822433b8..7d03fd11f82 100644 --- a/rs/p2p/state_sync_manager/tests/common.rs +++ b/rs/p2p/state_sync_manager/tests/common.rs @@ -293,14 +293,10 @@ impl Chunkable for FakeChunkable { // Add chunk to state if not part of manifest if !is_manifest_chunk(artifact_chunk.chunk_id) { - if let ArtifactChunkData::SemiStructuredChunkData(data) = - artifact_chunk.artifact_chunk_data - { - self.local_state - .add_chunk(artifact_chunk.chunk_id, data.len()) - } else { - panic!("Bug: Wrong artifact data type.") - } + let ArtifactChunkData::SemiStructuredChunkData(data) = + artifact_chunk.artifact_chunk_data; + self.local_state + .add_chunk(artifact_chunk.chunk_id, data.len()) } let elems = self.chunk_sets.iter().map(|set| set.len()).sum::(); diff --git a/rs/protobuf/def/types/v1/p2p.proto b/rs/protobuf/def/types/v1/p2p.proto index bd121840758..f78fcc99cfa 100644 --- a/rs/protobuf/def/types/v1/p2p.proto +++ b/rs/protobuf/def/types/v1/p2p.proto @@ -36,10 +36,10 @@ message GossipChunkRequest { message GossipChunk { GossipChunkRequest request = 6; oneof response { - ArtifactChunk chunk = 3; + Artifact artifact = 7; P2PError error = 4; } - reserved 1, 2, 5; + reserved 1, 2, 3, 5; } enum P2PError { @@ -77,11 +77,3 @@ message Artifact { FileTreeSyncArtifact file_tree_sync = 7; } } - -message ArtifactChunk { - oneof data { - bytes chunk = 3; - Artifact artifact = 4; - } - reserved 1, 2; -} diff --git a/rs/protobuf/src/gen/types/types.v1.rs b/rs/protobuf/src/gen/types/types.v1.rs index 606d8c16094..34e080f36be 100644 --- a/rs/protobuf/src/gen/types/types.v1.rs +++ b/rs/protobuf/src/gen/types/types.v1.rs @@ -1570,7 +1570,7 @@ pub struct GossipChunkRequest { pub struct GossipChunk { #[prost(message, optional, tag = "6")] pub request: ::core::option::Option, - #[prost(oneof = "gossip_chunk::Response", tags = "3, 4")] + #[prost(oneof = "gossip_chunk::Response", tags = "7, 4")] pub response: ::core::option::Option, } /// Nested message and enum types in `GossipChunk`. @@ -1580,8 +1580,8 @@ pub mod gossip_chunk { #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Oneof)] pub enum Response { - #[prost(message, tag = "3")] - Chunk(super::ArtifactChunk), + #[prost(message, tag = "7")] + Artifact(super::Artifact), #[prost(enumeration = "super::P2pError", tag = "4")] Error(i32), } @@ -1651,27 +1651,6 @@ pub mod artifact { FileTreeSync(super::FileTreeSyncArtifact), } } -#[derive(serde::Serialize, serde::Deserialize)] -#[allow(clippy::large_enum_variant)] -#[allow(clippy::derive_partial_eq_without_eq)] -#[derive(Clone, PartialEq, ::prost::Message)] -pub struct ArtifactChunk { - #[prost(oneof = "artifact_chunk::Data", tags = "3, 4")] - pub data: ::core::option::Option, -} -/// Nested message and enum types in `ArtifactChunk`. -pub mod artifact_chunk { - #[derive(serde::Serialize, serde::Deserialize)] - #[allow(clippy::large_enum_variant)] - #[allow(clippy::derive_partial_eq_without_eq)] - #[derive(Clone, PartialEq, ::prost::Oneof)] - pub enum Data { - #[prost(bytes, tag = "3")] - Chunk(::prost::alloc::vec::Vec), - #[prost(message, tag = "4")] - Artifact(super::Artifact), - } -} #[derive( serde::Serialize, serde::Deserialize, diff --git a/rs/state_manager/src/state_sync/chunkable.rs b/rs/state_manager/src/state_sync/chunkable.rs index defe95d76fd..1e072b7e3ad 100644 --- a/rs/state_manager/src/state_sync/chunkable.rs +++ b/rs/state_manager/src/state_sync/chunkable.rs @@ -1159,15 +1159,8 @@ impl Chunkable for IncompleteState { fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result { let ix = artifact_chunk.chunk_id.get(); - - let payload = match artifact_chunk.artifact_chunk_data { - ArtifactChunkData::SemiStructuredChunkData(ref payload) => payload, - other => { - warn!(self.log, "State sync chunk has wrong shape {:?}", other); - return Err(ChunkVerificationFailed); - } - }; - + let ArtifactChunkData::SemiStructuredChunkData(ref payload) = + artifact_chunk.artifact_chunk_data; match &mut self.state { DownloadState::Complete(ref artifact) => { debug!( diff --git a/rs/state_manager/tests/common/mod.rs b/rs/state_manager/tests/common/mod.rs index 59ea150bdd5..e976bfffd71 100644 --- a/rs/state_manager/tests/common/mod.rs +++ b/rs/state_manager/tests/common/mod.rs @@ -396,12 +396,6 @@ pub fn pipe_state_sync(src: StateSyncMessage, mut dst: Box) -> St fn alter_chunk_data(chunk: &mut ArtifactChunk) { let mut chunk_data = match &chunk.artifact_chunk_data { - ArtifactChunkData::UnitChunkData(_) => { - panic!( - "Unexpected artifact chunk data type: {:?}", - chunk.artifact_chunk_data - ); - } ArtifactChunkData::SemiStructuredChunkData(chunk_data) => chunk_data.clone(), }; match chunk_data.last_mut() { diff --git a/rs/types/types/src/chunkable.rs b/rs/types/types/src/chunkable.rs index 5e8752919fc..be8111b9c4e 100644 --- a/rs/types/types/src/chunkable.rs +++ b/rs/types/types/src/chunkable.rs @@ -12,12 +12,8 @@ //! For more context please check `` //! use crate::artifact::Artifact; -use bincode::serialize; use ic_protobuf::p2p::v1 as p2p_pb; -use ic_protobuf::proxy::ProxyDecodeError; -use ic_protobuf::types::v1 as pb; use phantom_newtype::Id; -use std::convert::TryFrom; pub struct Chunk(Vec); @@ -48,7 +44,6 @@ pub const CHUNKID_UNIT_CHUNK: u32 = 0; #[derive(Clone, Debug, PartialEq, Eq, Hash)] #[allow(clippy::large_enum_variant)] pub enum ArtifactChunkData { - UnitChunkData(Artifact), // Unit chunk data has 1:1 mapping with real artifacts SemiStructuredChunkData(Vec), } @@ -61,66 +56,19 @@ pub struct ArtifactChunk { pub artifact_chunk_data: ArtifactChunkData, } -/// Interface providing access to artifact chunks. +/// Interface providing access to artifact. pub trait ChunkableArtifact { - /// Retrieves the artifact chunk with the given ID. - /// - /// The chunk ID for single-chunked artifacts must be - /// `CHUNKID_UNIT_CHUNK`. - fn get_chunk(self: Box, chunk_id: ChunkId) -> Option; + fn get_chunk(self: Box) -> Artifact; } -/// Basic chunking interface for [`crate::single_chunked::SingleChunked`] artifact tracker. pub trait Chunkable { fn chunks_to_download(&self) -> Box>; fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result; } -impl From for pb::ArtifactChunk { - fn from(chunk: ArtifactChunk) -> Self { - let data: pb::artifact_chunk::Data = match chunk.artifact_chunk_data { - ArtifactChunkData::UnitChunkData(artifact) => { - pb::artifact_chunk::Data::Artifact(artifact.into()) - } - ArtifactChunkData::SemiStructuredChunkData(chunk_data) => { - pb::artifact_chunk::Data::Chunk(chunk_data) - } - }; - Self { data: Some(data) } - } -} - -impl TryFrom for ArtifactChunk { - type Error = ProxyDecodeError; - - fn try_from(chunk: pb::ArtifactChunk) -> Result { - let artifact_chunk_data = match chunk.data { - None => { - return Err(ProxyDecodeError::Other( - "unable to deserialize ArtifactChunk.data".to_string(), - )) - } - Some(d) => match d { - pb::artifact_chunk::Data::Artifact(a) => { - ArtifactChunkData::UnitChunkData(a.try_into()?) - } - pb::artifact_chunk::Data::Chunk(d) => ArtifactChunkData::SemiStructuredChunkData(d), - }, - }; - Ok(Self { - // On the wire chunk_id is passed in GossipChunk. - chunk_id: ChunkId::from(CHUNKID_UNIT_CHUNK), - artifact_chunk_data, - }) - } -} - impl From for p2p_pb::StateSyncChunkResponse { fn from(chunk: ArtifactChunk) -> Self { match chunk.artifact_chunk_data { - ArtifactChunkData::UnitChunkData(artifact) => Self { - data: serialize(&artifact).unwrap(), - }, ArtifactChunkData::SemiStructuredChunkData(chunk_data) => Self { data: chunk_data }, } } diff --git a/rs/types/types/src/single_chunked.rs b/rs/types/types/src/single_chunked.rs index 770703dbdd8..4283be05835 100644 --- a/rs/types/types/src/single_chunked.rs +++ b/rs/types/types/src/single_chunked.rs @@ -6,7 +6,7 @@ use crate::{ artifact::Artifact, canister_http::CanisterHttpResponseShare, - chunkable::{ChunkId, ChunkableArtifact, CHUNKID_UNIT_CHUNK}, + chunkable::ChunkableArtifact, consensus::{ certification::CertificationMessage, dkg::Message as DkgMessage, ecdsa::EcdsaMessage, ConsensusMessage, @@ -14,42 +14,11 @@ use crate::{ messages::SignedIngress, }; -// Static polymorphic dispatch for chunk tracking. -// -// Chunk trackers give a polymorphic interface over client chunk tracking logic. -// For artifacts consisting of a single chunk, P2P provides a default -// [`Chunkable`] trait implementation. Artifact types for which this default -// chunking logic is sufficient are marked using the [`SingleChunked`] marker -// trait. -// -// Why Trackers: Rust doesn't allow objects to be partially -// initialized, i.e we cannot track an under construction -// `ConsensusArtifact` using the same type as assembled -// `Artifact`. Tracker types provide an abstract control point that enables us -// to implement a polymorphic dispatch to per client tracking logic. -// -// Trackers are created from adverts and implement From trait. - -/// Artifact types composed of a single chunk. -pub enum SingleChunked { - CanisterHttp, - Consensus, - Ingress, - Certification, - Dkg, - Ecdsa, -} - macro_rules! chunkable_artifact_impl { ($id:path, |$self:ident| $v:expr) => { impl ChunkableArtifact for $id { - fn get_chunk($self: Box, chunk_id: ChunkId) -> Option { - if chunk_id != ChunkId::from(CHUNKID_UNIT_CHUNK) { - // Single chunked in identified only chunk CHUNKID_UNIT_CHUNK - None - } else { - Some($v) - } + fn get_chunk($self: Box) -> Artifact { + $v } } };