From 09fcb7b7949e1389c07fdda414d2fde5853c8f12 Mon Sep 17 00:00:00 2001 From: Rostislav Rumenov Date: Wed, 13 Dec 2023 13:06:54 +0000 Subject: [PATCH] refactor: Don't use the Artifact aggregation type for the new state sync. --- rs/p2p/state_sync_manager/src/ongoing.rs | 8 ++------ rs/p2p/state_sync_manager/tests/common.rs | 18 ++++++++++++------ rs/p2p/test_utils/src/mocks.rs | 4 ++-- rs/state_manager/src/state_sync/chunkable.rs | 15 +++++++++------ .../src/state_sync/chunkable/cache/tests.rs | 4 ++-- rs/state_manager/tests/common/mod.rs | 17 ++++------------- rs/types/types/src/chunkable.rs | 7 +++++-- 7 files changed, 36 insertions(+), 37 deletions(-) diff --git a/rs/p2p/state_sync_manager/src/ongoing.rs b/rs/p2p/state_sync_manager/src/ongoing.rs index 529bb26b9c6..85dbc0a5354 100644 --- a/rs/p2p/state_sync_manager/src/ongoing.rs +++ b/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 { diff --git a/rs/p2p/state_sync_manager/tests/common.rs b/rs/p2p/state_sync_manager/tests/common.rs index 221baa1b03e..464822b24c8 100644 --- a/rs/p2p/state_sync_manager/tests/common.rs +++ b/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 { + fn add_chunk( + &mut self, + artifact_chunk: ArtifactChunk, + ) -> Result { 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 { + fn add_chunk( + &mut self, + artifact_chunk: ArtifactChunk, + ) -> Result { 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( diff --git a/rs/p2p/test_utils/src/mocks.rs b/rs/p2p/test_utils/src/mocks.rs index d7aca626d41..3d8d99aab10 100644 --- a/rs/p2p/test_utils/src/mocks.rs +++ b/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>; - fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result; + fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result; } } diff --git a/rs/state_manager/src/state_sync/chunkable.rs b/rs/state_manager/src/state_sync/chunkable.rs index 59dca89bec8..637a98e2423 100644 --- a/rs/state_manager/src/state_sync/chunkable.rs +++ b/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), + Complete(Box), } /// 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 { + fn add_chunk( + &mut self, + artifact_chunk: ArtifactChunk, + ) -> Result { let ix = artifact_chunk.chunk_id.get(); let payload = &artifact_chunk.chunk; match &mut self.state { diff --git a/rs/state_manager/src/state_sync/chunkable/cache/tests.rs b/rs/state_manager/src/state_sync/chunkable/cache/tests.rs index 9eb3b20e1a4..1053402f6f2 100644 --- a/rs/state_manager/src/state_sync/chunkable/cache/tests.rs +++ b/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)) } diff --git a/rs/state_manager/tests/common/mod.rs b/rs/state_manager/tests/common/mod.rs index f12fac2e6ba..538723cfa8e 100644 --- a/rs/state_manager/tests/common/mod.rs +++ b/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) diff --git a/rs/types/types/src/chunkable.rs b/rs/types/types/src/chunkable.rs index d6aa75bf070..5021a36cf73 100644 --- a/rs/types/types/src/chunkable.rs +++ b/rs/types/types/src/chunkable.rs @@ -11,7 +11,7 @@ //! //! For more context please check `` //! -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>; - fn add_chunk(&mut self, artifact_chunk: ArtifactChunk) -> Result; + fn add_chunk( + &mut self, + artifact_chunk: ArtifactChunk, + ) -> Result; } impl From for p2p_pb::StateSyncChunkResponse {