Merge branch 'rumenov/rmasdchr' into 'master'

refactor: remove the ArtifactChunkData type

 

See merge request dfinity-lab/public/ic!16605

rs/p2p/src/download_management.rs

@@ -84,7 +84,6 @@ use ic_types::{
         IngressArtifact,
     },
     chunkable::ChunkId,
-    chunkable::CHUNKID_UNIT_CHUNK,
     crypto::CryptoHash,
     p2p::GossipAdvert,
     NodeId, RegistryVersion,
@@ -97,6 +96,8 @@ use std::{
     time::{Instant, SystemTime},
 };
 
+const CHUNKID_UNIT_CHUNK: u32 = 0;
+
 /// `DownloadManagerImpl` implements the `DownloadManager` trait.
 impl GossipImpl {
     /// The method downloads chunks for adverts with the highest priority from

rs/p2p/state_sync_manager/src/routes/chunk.rs

@@ -56,7 +56,7 @@ pub(crate) async fn state_sync_chunk_handler(
         tokio::task::spawn_blocking(
             move || match state.state_sync.chunk(&artifact_id, chunk_id) {
                 Some(data) => {
-                    let pb_chunk = pb::StateSyncChunkResponse { data: data.into() };
+                    let pb_chunk = pb::StateSyncChunkResponse { data };
                     let mut raw = BytesMut::with_capacity(pb_chunk.encoded_len());
                     pb_chunk.encode(&mut raw).expect("Allocated enough memory");
                     let raw = raw.freeze();
@@ -129,8 +129,7 @@ pub(crate) fn parse_chunk_handler_response(
 
             let chunk = ArtifactChunk {
                 chunk_id,
-                artifact_chunk_data:
-                    ic_types::chunkable::ArtifactChunkData::SemiStructuredChunkData(pb.data),
+                chunk: pb.data,
             };
             Ok(chunk)
         }

rs/p2p/state_sync_manager/tests/common.rs

@@ -16,7 +16,7 @@ use ic_metrics::MetricsRegistry;
 use ic_p2p_test_utils::mocks::{MockChunkable, MockStateSync};
 use ic_types::{
     artifact::{Artifact, StateSyncArtifactId, StateSyncMessage},
-    chunkable::{ArtifactChunk, ArtifactChunkData, ArtifactErrorCode, Chunk, ChunkId, Chunkable},
+    chunkable::{ArtifactChunk, ArtifactErrorCode, Chunk, ChunkId, Chunkable},
     crypto::CryptoHash,
     state_sync::{Manifest, MetaManifest, StateSyncVersion},
     CryptoHashOfState, Height, NodeId, PrincipalId,
@@ -221,10 +221,10 @@ impl StateSyncClient for FakeStateSync {
         }
 
         if is_manifest_chunk(chunk_id) {
-            return Some(vec![0; 100].into());
+            return Some(vec![0; 100]);
         }
 
-        self.global_state.chunk(chunk_id).map(|c| c.into())
+        self.global_state.chunk(chunk_id)
     }
 
     fn deliver_state_sync(&self, msg: StateSyncMessage) {
@@ -293,10 +293,8 @@ impl Chunkable for FakeChunkable {
 
         // Add chunk to state if not part of manifest
         if !is_manifest_chunk(artifact_chunk.chunk_id) {
-            let ArtifactChunkData::SemiStructuredChunkData(data) =
-                artifact_chunk.artifact_chunk_data;
             self.local_state
-                .add_chunk(artifact_chunk.chunk_id, data.len())
+                .add_chunk(artifact_chunk.chunk_id, artifact_chunk.chunk.len())
         }
 
         let elems = self.chunk_sets.iter().map(|set| set.len()).sum::<usize>();

rs/p2p/state_sync_manager/tests/test.rs

@@ -654,9 +654,7 @@ fn test_state_sync_abortion() {
                 ]
             });
         s1.get_mut().expect_available_states().return_const(vec![]);
-        s1.get_mut()
-            .expect_chunk()
-            .returning(|_, _| Some(vec![].into()));
+        s1.get_mut().expect_chunk().returning(|_, _| Some(vec![]));
 
         // Verify that peers got expected number of adverts. The last advert on node 2
         // is used to start the state sync.
@@ -699,9 +697,7 @@ fn test_state_sync_abortion() {
 
         // Add Node 3 to state sync by advertising the state once.
         s3.get_mut().checkpoint();
-        s3.get_mut()
-            .expect_chunk()
-            .returning(|_, _| Some(vec![].into()));
+        s3.get_mut().expect_chunk().returning(|_, _| Some(vec![]));
         s3.get_mut()
             .expect_available_states()
             .once()

rs/state_manager/src/state_sync/chunkable.rs

@@ -13,7 +13,7 @@ use ic_sys::mmap::ScopedMmap;
 use ic_types::{
     artifact::{Artifact, StateSyncMessage},
     chunkable::{
-        ArtifactChunk, ArtifactChunkData,
+        ArtifactChunk,
         ArtifactErrorCode::{self, ChunkVerificationFailed, ChunksMoreNeeded},
         ChunkId, Chunkable,
     },
@@ -1159,8 +1159,7 @@ impl Chunkable for IncompleteState {
 
     fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result<Artifact, ArtifactErrorCode> {
         let ix = artifact_chunk.chunk_id.get();
-        let ArtifactChunkData::SemiStructuredChunkData(ref payload) =
-            artifact_chunk.artifact_chunk_data;
+        let payload = &artifact_chunk.chunk;
         match &mut self.state {
             DownloadState::Complete(ref artifact) => {
                 debug!(

rs/state_manager/tests/common/mod.rs

@@ -22,7 +22,7 @@ use ic_test_utilities_tmpdir::tmpdir;
 use ic_types::{
     artifact::{Artifact, StateSyncMessage},
     chunkable::{
-        ArtifactChunk, ArtifactChunkData,
+        ArtifactChunk,
         ArtifactErrorCode::{ChunkVerificationFailed, ChunksMoreNeeded},
         ChunkId, Chunkable,
     },
@@ -395,9 +395,7 @@ pub fn pipe_state_sync(src: StateSyncMessage, mut dst: Box<dyn Chunkable>) -> St
 }
 
 fn alter_chunk_data(chunk: &mut ArtifactChunk) {
-    let mut chunk_data = match &chunk.artifact_chunk_data {
-        ArtifactChunkData::SemiStructuredChunkData(chunk_data) => chunk_data.clone(),
-    };
+    let mut chunk_data = chunk.chunk.clone();
     match chunk_data.last_mut() {
         Some(last) => {
             // Alter the last element of chunk_data.
@@ -408,7 +406,7 @@ fn alter_chunk_data(chunk: &mut ArtifactChunk) {
             chunk_data = vec![9; 100];
         }
     }
-    chunk.artifact_chunk_data = ArtifactChunkData::SemiStructuredChunkData(chunk_data);
+    chunk.chunk = chunk_data;
 }
 
 /// Pipe the meta-manifest (chunk 0) from src to dest.
@@ -427,12 +425,10 @@ pub fn pipe_meta_manifest(
 
     let mut chunk = ArtifactChunk {
         chunk_id: id,
-        artifact_chunk_data: ArtifactChunkData::SemiStructuredChunkData(
-            src.clone()
-                .get_chunk(id)
-                .unwrap_or_else(|| panic!("Requested unknown chunk {}", id))
-                .into(),
-        ),
+        chunk: src
+            .clone()
+            .get_chunk(id)
+            .unwrap_or_else(|| panic!("Requested unknown chunk {}", id)),
     };
 
     if use_bad_chunk {
@@ -469,12 +465,10 @@ pub fn pipe_manifest(
     for (index, id) in ids.iter().enumerate() {
         let mut chunk = ArtifactChunk {
             chunk_id: *id,
-            artifact_chunk_data: ArtifactChunkData::SemiStructuredChunkData(
-                src.clone()
-                    .get_chunk(*id)
-                    .unwrap_or_else(|| panic!("Requested unknown chunk {}", id))
-                    .into(),
-            ),
+            chunk: src
+                .clone()
+                .get_chunk(*id)
+                .unwrap_or_else(|| panic!("Requested unknown chunk {}", id)),
         };
 
         if use_bad_chunk && index == ids.len() / 2 {
@@ -520,12 +514,10 @@ pub fn pipe_partial_state_sync(
             }
             let mut chunk = ArtifactChunk {
                 chunk_id: *id,
-                artifact_chunk_data: ArtifactChunkData::SemiStructuredChunkData(
-                    src.clone()
-                        .get_chunk(*id)
-                        .unwrap_or_else(|| panic!("Requested unknown chunk {}", id))
-                        .into(),
-                ),
+                chunk: src
+                    .clone()
+                    .get_chunk(*id)
+                    .unwrap_or_else(|| panic!("Requested unknown chunk {}", id)),
             };
 
             if use_bad_chunk && index == ids.len() / 2 {

rs/types/types/src/artifact.rs

@@ -637,7 +637,7 @@ impl StateSyncMessage {
                 }
             }
 
-            Some(payload.into())
+            Some(payload)
         }
     }
 }

rs/types/types/src/chunkable.rs

@@ -15,19 +15,7 @@ use crate::artifact::Artifact;
 use ic_protobuf::p2p::v1 as p2p_pb;
 use phantom_newtype::Id;
 
-pub struct Chunk(Vec<u8>);
-
-impl From<Chunk> for Vec<u8> {
-    fn from(chunk: Chunk) -> Vec<u8> {
-        chunk.0
-    }
-}
-
-impl From<Vec<u8>> for Chunk {
-    fn from(chunk: Vec<u8>) -> Chunk {
-        Chunk(chunk)
-    }
-}
+pub type Chunk = Vec<u8>;
 
 /// Error codes returned by the `Chunkable` interface.
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
@@ -38,22 +26,14 @@ pub enum ArtifactErrorCode {
 
 /// The chunk type.
 pub type ChunkId = Id<ArtifactChunk, u32>;
-pub const CHUNKID_UNIT_CHUNK: u32 = 0;
-
-/// The data contained in an artifact chunk.
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
-#[allow(clippy::large_enum_variant)]
-pub enum ArtifactChunkData {
-    SemiStructuredChunkData(Vec<u8>),
-}
 
 /// An artifact chunk.
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct ArtifactChunk {
     // Chunk number/id for this chunk
     pub chunk_id: ChunkId,
     // Payload for the chunk
-    pub artifact_chunk_data: ArtifactChunkData,
+    pub chunk: Chunk,
 }
 
 /// Interface providing access to artifact.
@@ -68,8 +48,6 @@ pub trait Chunkable {
 
 impl From<ArtifactChunk> for p2p_pb::StateSyncChunkResponse {
     fn from(chunk: ArtifactChunk) -> Self {
-        match chunk.artifact_chunk_data {
-            ArtifactChunkData::SemiStructuredChunkData(chunk_data) => Self { data: chunk_data },
-        }
+        Self { data: chunk.chunk }
     }
 }