Merge branch 'rumenov/rmsdjfromf' into 'master'

refactor: Don't use the Artifact aggregation type for the new state sync.

Artifact type will be deleted with the rollout of the new P2P for consensus 

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

rs/p2p/state_sync_manager/src/ongoing.rs

@@ -24,7 +24,7 @@ use ic_interfaces::p2p::state_sync::StateSyncClient;
 use ic_logger::{error, info, ReplicaLogger};
 use ic_quic_transport::Transport;
 use ic_types::{
-    artifact::{Artifact, StateSyncArtifactId, StateSyncMessage},
+    artifact::{StateSyncArtifactId, StateSyncMessage},
     chunkable::ChunkId,
     chunkable::{ArtifactErrorCode, Chunkable},
     NodeId,
@@ -339,11 +339,7 @@ impl OngoingStateSync {
         .and_then(std::convert::identity);
 
         let result = match chunk_add_result {
-            Ok(Ok(Artifact::StateSync(msg))) => Ok(Some(msg)),
-            Ok(Ok(_)) => {
-                //TODO: (NET-1448) With new protobufs this condition will redundant.
-                panic!("Should not happen");
-            }
+            Ok(Ok(msg)) => Ok(Some(msg)),
             Ok(Err(ArtifactErrorCode::ChunksMoreNeeded)) => Ok(None),
             Ok(Err(ArtifactErrorCode::ChunkVerificationFailed)) => {
                 Err(DownloadChunkError::RequestError {

rs/p2p/state_sync_manager/tests/common.rs

@@ -15,7 +15,7 @@ use ic_memory_transport::TransportRouter;
 use ic_metrics::MetricsRegistry;
 use ic_p2p_test_utils::mocks::{MockChunkable, MockStateSync};
 use ic_types::{
-    artifact::{Artifact, StateSyncArtifactId, StateSyncMessage},
+    artifact::{StateSyncArtifactId, StateSyncMessage},
     chunkable::{ArtifactChunk, ArtifactErrorCode, Chunk, ChunkId, Chunkable},
     crypto::CryptoHash,
     state_sync::{Manifest, MetaManifest, StateSyncVersion},
@@ -279,7 +279,10 @@ impl Chunkable for FakeChunkable {
         Box::new(to_download.into_iter().map(ChunkId::from))
     }
 
-    fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result<Artifact, ArtifactErrorCode> {
+    fn add_chunk(
+        &mut self,
+        artifact_chunk: ArtifactChunk,
+    ) -> Result<StateSyncMessage, ArtifactErrorCode> {
         for set in self.chunk_sets.iter_mut() {
             if set.is_empty() {
                 continue;
@@ -337,7 +340,10 @@ impl Chunkable for SharableMockChunkable {
         self.chunks_to_download_calls.fetch_add(1, Ordering::SeqCst);
         self.mock.lock().unwrap().chunks_to_download()
     }
-    fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result<Artifact, ArtifactErrorCode> {
+    fn add_chunk(
+        &mut self,
+        artifact_chunk: ArtifactChunk,
+    ) -> Result<StateSyncMessage, ArtifactErrorCode> {
         self.add_chunks_calls.fetch_add(1, Ordering::SeqCst);
         self.mock.lock().unwrap().add_chunk(artifact_chunk)
     }
@@ -411,21 +417,21 @@ pub fn latency_30ms_throughput_1000mbits() -> (Duration, usize) {
     (Duration::from_millis(30), 3_750_000)
 }
 
-fn state_sync_artifact(id: StateSyncArtifactId) -> Artifact {
+fn state_sync_artifact(id: StateSyncArtifactId) -> StateSyncMessage {
     let manifest = Manifest::new(StateSyncVersion::V0, vec![], vec![]);
     let meta_manifest = MetaManifest {
         version: StateSyncVersion::V0,
         sub_manifest_hashes: vec![],
     };
 
-    Artifact::StateSync(StateSyncMessage {
+    StateSyncMessage {
         height: id.height,
         root_hash: id.hash,
         checkpoint_root: PathBuf::new(),
         manifest,
         meta_manifest: Arc::new(meta_manifest),
         state_sync_file_group: Default::default(),
-    })
+    }
 }
 
 pub fn create_node(

rs/p2p/test_utils/src/mocks.rs

@@ -9,7 +9,7 @@ use ic_interfaces::p2p::{
 use ic_quic_transport::{ConnId, SendError, Transport};
 use ic_types::{artifact::PriorityFn, chunkable::ArtifactChunk};
 use ic_types::{
-    artifact::{Artifact, StateSyncArtifactId, StateSyncMessage},
+    artifact::{StateSyncArtifactId, StateSyncMessage},
     chunkable::{ArtifactErrorCode, Chunkable},
     chunkable::{Chunk, ChunkId},
     NodeId,
@@ -61,7 +61,7 @@ mock! {
 
     impl Chunkable for Chunkable{
         fn chunks_to_download(&self) -> Box<dyn Iterator<Item = ChunkId>>;
-        fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result<Artifact, ArtifactErrorCode>;
+        fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result<StateSyncMessage, ArtifactErrorCode>;
     }
 }
 

rs/state_manager/src/state_sync/chunkable.rs

@@ -11,7 +11,7 @@ use ic_state_layout::utils::do_copy_overwrite;
 use ic_state_layout::{error::LayoutError, CheckpointLayout, ReadOnly, RwPolicy, StateLayout};
 use ic_sys::mmap::ScopedMmap;
 use ic_types::{
-    artifact::{Artifact, StateSyncMessage},
+    artifact::StateSyncMessage,
     chunkable::{
         ArtifactChunk,
         ArtifactErrorCode::{self, ChunkVerificationFailed, ChunksMoreNeeded},
@@ -72,7 +72,7 @@ enum DownloadState {
     },
     /// Successfully completed and returned the artifact to P2P, nothing else to
     /// do.
-    Complete(Box<Artifact>),
+    Complete(Box<StateSyncMessage>),
 }
 
 /// An implementation of Chunkable trait that represents a (on-disk) state under
@@ -776,8 +776,8 @@ impl IncompleteState {
         root_hash: CryptoHashOfState,
         manifest: &Manifest,
         meta_manifest: &MetaManifest,
-    ) -> Artifact {
-        Artifact::StateSync(StateSyncMessage {
+    ) -> StateSyncMessage {
+        StateSyncMessage {
             height,
             root_hash,
             checkpoint_root: state_layout
@@ -790,7 +790,7 @@ impl IncompleteState {
             // `state_sync_file_group` and `checkpoint_root` are not included in the integrity hash of this artifact.
             // Therefore it is OK to pass a default value here as it is only used when fetching chunks.
             state_sync_file_group: Default::default(),
-        })
+        }
     }
 
     fn make_checkpoint(
@@ -1157,7 +1157,10 @@ impl Chunkable for IncompleteState {
         }
     }
 
-    fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result<Artifact, ArtifactErrorCode> {
+    fn add_chunk(
+        &mut self,
+        artifact_chunk: ArtifactChunk,
+    ) -> Result<StateSyncMessage, ArtifactErrorCode> {
         let ix = artifact_chunk.chunk_id.get();
         let payload = &artifact_chunk.chunk;
         match &mut self.state {

rs/state_manager/src/state_sync/chunkable/cache/tests.rs

@@ -74,14 +74,14 @@ fn fake_complete() -> DownloadState {
         version: StateSyncVersion::V0,
         sub_manifest_hashes: vec![],
     };
-    let artifact = Artifact::StateSync(StateSyncMessage {
+    let artifact = StateSyncMessage {
         height: Height::new(0),
         root_hash: CryptoHashOfState::from(CryptoHash(vec![0; 32])),
         checkpoint_root: PathBuf::new(),
         manifest,
         meta_manifest: Arc::new(meta_manifest),
         state_sync_file_group: Default::default(),
-    });
+    };
     DownloadState::Complete(Box::new(artifact))
 }
 

rs/state_manager/tests/common/mod.rs

@@ -20,7 +20,7 @@ use ic_test_utilities::{
 use ic_test_utilities_logger::with_test_replica_logger;
 use ic_test_utilities_tmpdir::tmpdir;
 use ic_types::{
-    artifact::{Artifact, StateSyncMessage},
+    artifact::StateSyncMessage,
     chunkable::{
         ArtifactChunk,
         ArtifactErrorCode::{ChunkVerificationFailed, ChunksMoreNeeded},
@@ -436,10 +436,7 @@ pub fn pipe_meta_manifest(
     }
 
     match dst.add_chunk(chunk) {
-        Ok(Artifact::StateSync(msg)) => Ok(msg),
-        Ok(artifact) => {
-            panic!("Unexpected artifact type: {:?}", artifact);
-        }
+        Ok(msg) => Ok(msg),
         Err(ChunksMoreNeeded) => Err(StateSyncErrorCode::ChunksMoreNeeded),
         Err(ChunkVerificationFailed) => Err(StateSyncErrorCode::MetaManifestVerificationFailed),
     }
@@ -476,12 +473,9 @@ pub fn pipe_manifest(
         }
 
         match dst.add_chunk(chunk) {
-            Ok(Artifact::StateSync(msg)) => {
+            Ok(msg) => {
                 return Ok(msg);
             }
-            Ok(artifact) => {
-                panic!("Unexpected artifact type: {:?}", artifact);
-            }
             Err(ChunksMoreNeeded) => (),
             Err(ChunkVerificationFailed) => {
                 return Err(StateSyncErrorCode::ManifestVerificationFailed)
@@ -525,12 +519,9 @@ pub fn pipe_partial_state_sync(
             }
 
             match dst.add_chunk(chunk) {
-                Ok(Artifact::StateSync(msg)) => {
+                Ok(msg) => {
                     return Ok(msg);
                 }
-                Ok(artifact) => {
-                    panic!("Unexpected artifact type: {:?}", artifact);
-                }
                 Err(ChunksMoreNeeded) => (),
                 Err(ChunkVerificationFailed) => {
                     return Err(StateSyncErrorCode::OtherChunkVerificationFailed)

rs/types/types/src/chunkable.rs

@@ -11,7 +11,7 @@
 //!
 //! For more context please check `<https://youtu.be/WaNJINjGleg>`
 //!
-use crate::artifact::Artifact;
+use crate::artifact::{Artifact, StateSyncMessage};
 use ic_protobuf::p2p::v1 as p2p_pb;
 use phantom_newtype::Id;
 
@@ -43,7 +43,10 @@ pub trait ChunkableArtifact {
 
 pub trait Chunkable {
     fn chunks_to_download(&self) -> Box<dyn Iterator<Item = ChunkId>>;
-    fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result<Artifact, ArtifactErrorCode>;
+    fn add_chunk(
+        &mut self,
+        artifact_chunk: ArtifactChunk,
+    ) -> Result<StateSyncMessage, ArtifactErrorCode>;
 }
 
 impl From<ArtifactChunk> for p2p_pb::StateSyncChunkResponse {