Fixes following PR comments

core-primitives/settings/src/lib.rs

@@ -53,7 +53,7 @@ pub mod files {
 	pub static RA_API_KEY_FILE: &str = "key.txt";
 
 	pub const SPID_MIN_LENGTH: usize = 32;
-	pub const STATE_SNAPSHOTS_CACHE_SIZE: usize = 40;
+	pub const STATE_SNAPSHOTS_CACHE_SIZE: usize = 120;
 }
 
 /// Settings concerning the worker

core-primitives/stf-executor/src/executor_tests.rs

@@ -133,7 +133,7 @@ pub fn propose_state_update_always_executes_preprocessing_step() {
 	// given
 	let shard = ShardIdentifier::default();
 	let (stf_executor, _, state_handler) = stf_executor();
-	let _init_hash = state_handler.initialize_shard(&shard).unwrap();
+	let _init_hash = state_handler.initialize_shard(shard).unwrap();
 	let key = "my_key".encode();
 	let value = "my_value".encode();
 	let old_state_hash = state_hash(&state_handler.load(&shard).unwrap());
@@ -239,7 +239,7 @@ pub fn execute_update_works() {
 	// given
 	let shard = ShardIdentifier::default();
 	let (stf_executor, _ocall_api, state_handler) = stf_executor();
-	let _init_hash = state_handler.initialize_shard(&shard).unwrap();
+	let _init_hash = state_handler.initialize_shard(shard).unwrap();
 	let key = "my_key".encode();
 	let value = "my_value".encode();
 	let old_state_hash = state_hash(&state_handler.load(&shard).unwrap());
@@ -303,7 +303,7 @@ fn init_state_and_shard_with_state_handler<S: HandleState<StateT = State>>(
 	state_handler: &S,
 ) -> (State, ShardIdentifier) {
 	let shard = ShardIdentifier::default();
-	let _hash = state_handler.initialize_shard(&shard).unwrap();
+	let _hash = state_handler.initialize_shard(shard).unwrap();
 
 	let (lock, mut state) = state_handler.load_for_mutation(&shard).unwrap();
 	test_genesis_setup(&mut state);

core-primitives/stf-state-handler/src/file_io.rs

@@ -245,12 +245,11 @@ pub mod sgx {
 			Ok(directory_items
 				.iter()
 				.flat_map(|item| {
-					extract_state_id_from_file_name(item.as_str())
-						// Maybe there is a better way? Need to call a function in case of `None`.
-						.ok_or_else(|| {
-							warn!("Found item ({}) that does not match state snapshot naming pattern, ignoring it", item)
-						})
-						.ok()
+					let maybe_state_id = extract_state_id_from_file_name(item.as_str());
+					if maybe_state_id.is_none() {
+						warn!("Found item ({}) that does not match state snapshot naming pattern, ignoring it", item)
+					}
+					maybe_state_id
 				})
 				.collect())
 		}
@@ -291,6 +290,7 @@ fn file_for_state_exists(shard: &ShardIdentifier, state_id: StateId) -> bool {
 }
 
 #[allow(unused)]
+/// Returns true if a shard directory for a given identifier exists AND contains at least one state file.
 pub(crate) fn shard_exists(shard: &ShardIdentifier) -> bool {
 	let shard_path = shard_path(shard);
 	if !shard_path.exists() {
@@ -351,6 +351,12 @@ mod tests {
 		assert!(to_file_name(generate_current_timestamp_state_id())
 			.strip_suffix(format!("_{}", ENCRYPTED_STATE_FILE).as_str())
 			.is_some());
+
+		let now_time_stamp = generate_current_timestamp_state_id();
+		assert_eq!(
+			extract_state_id_from_file_name(to_file_name(now_time_stamp).as_str()).unwrap(),
+			now_time_stamp
+		);
 	}
 
 	#[test]

core-primitives/stf-state-handler/src/handle_state.rs

@@ -33,7 +33,7 @@ pub trait HandleState {
 	/// Initialize a new shard.
 	///
 	/// Initializes a default state for the shard and returns its hash.
-	fn initialize_shard(&self, shard: &ShardIdentifier) -> Result<Self::HashType>;
+	fn initialize_shard(&self, shard: ShardIdentifier) -> Result<Self::HashType>;
 
 	/// Load the state for a given shard.
 	///

core-primitives/stf-state-handler/src/in_memory_state_file_io.rs

@@ -120,7 +120,7 @@ where
 		states_for_shard
 			.get(&state_id)
 			.map(|(_, s)| -> State { s.clone() })
-			.ok_or_else(|| Error::InvalidStateId(state_id))
+			.ok_or(Error::InvalidStateId(state_id))
 	}
 
 	fn compute_hash(
@@ -179,7 +179,7 @@ where
 
 		states_for_shard
 			.remove(&state_id)
-			.ok_or_else(|| Error::InvalidStateId(state_id))
+			.ok_or(Error::InvalidStateId(state_id))
 			.map(|_| {})
 	}
 

core-primitives/stf-state-handler/src/state_handler.rs

@@ -53,7 +53,7 @@ where
 	type StateT = Repository::StateType;
 	type HashType = Repository::HashType;
 
-	fn initialize_shard(&self, shard: &ShardIdentifier) -> Result<Self::HashType> {
+	fn initialize_shard(&self, shard: ShardIdentifier) -> Result<Self::HashType> {
 		let mut state_write_lock =
 			self.state_snapshot_repository.write().map_err(|_| Error::LockPoisoning)?;
 		state_write_lock.initialize_new_shard(shard)

core-primitives/stf-state-handler/src/state_snapshot_repository.rs

@@ -51,10 +51,8 @@ pub trait VersionedStateAccess {
 	) -> Result<Self::StateType>;
 
 	/// Initialize a new shard.
-	fn initialize_new_shard(
-		&mut self,
-		shard_identifier: &ShardIdentifier,
-	) -> Result<Self::HashType>;
+	fn initialize_new_shard(&mut self, shard_identifier: ShardIdentifier)
+		-> Result<Self::HashType>;
 
 	/// Checks if a shard for a given identifier exists.
 	fn shard_exists(&self, shard_identifier: &ShardIdentifier) -> bool;
@@ -248,19 +246,19 @@ where
 
 	fn initialize_new_shard(
 		&mut self,
-		shard_identifier: &ShardIdentifier,
+		shard_identifier: ShardIdentifier,
 	) -> Result<Self::HashType> {
-		if let Some(state_snapshots) = self.snapshot_history.get(shard_identifier) {
+		if let Some(state_snapshots) = self.snapshot_history.get(&shard_identifier) {
 			warn!("Shard ({:?}) already exists, will not initialize again", shard_identifier);
 			return state_snapshots.front().map(|s| s.state_hash).ok_or(Error::EmptyRepository)
 		}
 
 		let snapshot_metadata =
-			initialize_shard_with_snapshot(shard_identifier, self.file_io.as_ref())?;
+			initialize_shard_with_snapshot(&shard_identifier, self.file_io.as_ref())?;
 
 		let state_hash = snapshot_metadata.state_hash;
 		self.snapshot_history
-			.insert(*shard_identifier, VecDeque::from([snapshot_metadata]));
+			.insert(shard_identifier, VecDeque::from([snapshot_metadata]));
 		Ok(state_hash)
 	}
 
@@ -413,7 +411,7 @@ mod tests {
 		assert!(state_snapshot_repository.load_latest(&shard_id).is_err());
 		assert!(state_snapshot_repository.list_shards().unwrap().is_empty());
 
-		let _hash = state_snapshot_repository.initialize_new_shard(&shard_id).unwrap();
+		let _hash = state_snapshot_repository.initialize_new_shard(shard_id).unwrap();
 
 		assert!(state_snapshot_repository.load_latest(&shard_id).is_ok());
 		assert_eq!(1, state_snapshot_repository.list_shards().unwrap().len());
@@ -424,7 +422,7 @@ mod tests {
 		let shard_id = ShardIdentifier::random();
 		let (_, mut state_snapshot_repository) = create_state_snapshot_repository(&[shard_id], 2);
 
-		let _hash = state_snapshot_repository.initialize_new_shard(&shard_id).unwrap();
+		let _hash = state_snapshot_repository.initialize_new_shard(shard_id).unwrap();
 
 		assert!(state_snapshot_repository.load_latest(&shard_id).is_ok());
 		assert_eq!(1, state_snapshot_repository.list_shards().unwrap().len());

core-primitives/stf-state-handler/src/state_snapshot_repository_loader.rs

@@ -174,22 +174,6 @@ mod tests {
 		assert_latest_state_id(&snapshot_history, &shards[2], 14_000_000);
 	}
 
-	#[test]
-	fn ignore_invalid_file_names() {
-		let shard = ShardIdentifier::random();
-		let (file_io, loader) = create_test_fixtures(&[shard]);
-
-		// Only 2 of these file names are valid
-		file_io.create_initialized(&shard, 12).unwrap();
-		file_io.create_initialized(&shard, 13).unwrap();
-		file_io.create_initialized(&shard, 8_000_000).unwrap();
-		file_io.create_initialized(&shard, 4_000_000).unwrap();
-
-		let snapshot_history = loader.load_state_snapshot_history().unwrap();
-		assert_eq!(2, snapshot_history.get(&shard).unwrap().len());
-		assert_latest_state_id(&snapshot_history, &shard, 8_000_000);
-	}
-
 	fn add_state_snapshots(file_io: &TestFileIo, shard: &ShardIdentifier, state_ids: &[StateId]) {
 		for state_id in state_ids {
 			add_snapshot_with_state_ids(file_io, shard, *state_id);

core-primitives/stf-state-handler/src/test/mocks/versioned_state_access_mock.rs

@@ -81,9 +81,9 @@ where
 
 	fn initialize_new_shard(
 		&mut self,
-		shard_identifier: &ShardIdentifier,
+		shard_identifier: ShardIdentifier,
 	) -> Result<Self::HashType> {
-		self.state_history.insert(*shard_identifier, VecDeque::from([State::default()]));
+		self.state_history.insert(shard_identifier, VecDeque::from([State::default()]));
 		Ok(Hash::default())
 	}
 

core-primitives/stf-state-handler/src/test/sgx_tests.rs

@@ -29,7 +29,7 @@ use crate::{
 use codec::{Decode, Encode};
 use ita_stf::{State as StfState, StateType as StfStateType};
 use itp_sgx_crypto::{Aes, AesSeal, StateCrypto};
-use itp_sgx_io::SealedIO;
+use itp_sgx_io::{write, SealedIO};
 use itp_types::{ShardIdentifier, H256};
 use sgx_externalities::SgxExternalitiesTrait;
 use sp_core::hashing::blake2_256;
@@ -268,6 +268,21 @@ pub fn test_state_files_from_handler_can_be_loaded_again() {
 	);
 }
 
+pub fn test_list_state_ids_ignores_files_not_matching_the_pattern() {
+	let shard: ShardIdentifier = [21u8; 32].into();
+	let _shard_dir_handle = ShardDirectoryHandle::new(shard).unwrap();
+
+	let file_io = TestStateFileIo::new(AesSeal::unseal().unwrap());
+
+	let mut invalid_state_file_path = shard_path(&shard);
+	invalid_state_file_path.push("invalid-state.bin");
+	write(&[0, 1, 2, 3, 4, 5], invalid_state_file_path).unwrap();
+
+	file_io.create_initialized(&shard, 1234).unwrap();
+
+	assert_eq!(1, file_io.list_state_ids_for_shard(&shard).unwrap().len());
+}
+
 fn initialize_state_handler_with_directory_handle(
 	shard: &ShardIdentifier,
 ) -> (Arc<TestStateHandler>, ShardDirectoryHandle) {

core-primitives/test/src/mock/handle_state_mock.rs

@@ -41,7 +41,7 @@ pub struct HandleStateMock {
 }
 
 impl HandleStateMock {
-	pub fn from_shard(shard: &ShardIdentifier) -> Result<Self> {
+	pub fn from_shard(shard: ShardIdentifier) -> Result<Self> {
 		let state_handler = HandleStateMock { state_map: Default::default() };
 		state_handler.initialize_shard(shard)?;
 		Ok(state_handler)
@@ -53,15 +53,15 @@ impl HandleState for HandleStateMock {
 	type StateT = StfState;
 	type HashType = H256;
 
-	fn initialize_shard(&self, shard: &ShardIdentifier) -> Result<Self::HashType> {
-		let maybe_state = self.state_map.read().unwrap().get(shard).cloned();
+	fn initialize_shard(&self, shard: ShardIdentifier) -> Result<Self::HashType> {
+		let maybe_state = self.state_map.read().unwrap().get(&shard).cloned();
 
 		return match maybe_state {
 			// Initialize with default state, if it doesn't exist yet.
 			None => {
 				let state = StfState::default();
 				let state_hash = state.using_encoded(blake2_256).into();
-				self.state_map.write().unwrap().insert(*shard, state);
+				self.state_map.write().unwrap().insert(shard, state);
 				Ok(state_hash)
 			},
 			Some(s) => Ok(s.using_encoded(blake2_256).into()),
@@ -127,7 +127,7 @@ pub mod tests {
 	pub fn shard_exists_after_inserting() {
 		let state_handler = HandleStateMock::default();
 		let shard = ShardIdentifier::default();
-		state_handler.initialize_shard(&shard).unwrap();
+		state_handler.initialize_shard(shard).unwrap();
 
 		assert!(state_handler.load(&shard).is_ok());
 		assert!(state_handler.shard_exists(&shard).unwrap());
@@ -136,7 +136,7 @@ pub mod tests {
 	pub fn initialize_creates_default_state() {
 		let state_handler = HandleStateMock::default();
 		let shard = ShardIdentifier::default();
-		state_handler.initialize_shard(&shard).unwrap();
+		state_handler.initialize_shard(shard).unwrap();
 
 		let loaded_state_result = state_handler.load(&shard);
 
@@ -146,7 +146,7 @@ pub mod tests {
 	pub fn load_mutate_and_write_works() {
 		let state_handler = HandleStateMock::default();
 		let shard = ShardIdentifier::default();
-		state_handler.initialize_shard(&shard).unwrap();
+		state_handler.initialize_shard(shard).unwrap();
 
 		let (lock, mut state) = state_handler.load_for_mutation(&shard).unwrap();
 
@@ -165,7 +165,7 @@ pub mod tests {
 	pub fn ensure_subsequent_state_loads_have_same_hash() {
 		let state_handler = HandleStateMock::default();
 		let shard = ShardIdentifier::default();
-		state_handler.initialize_shard(&shard).unwrap();
+		state_handler.initialize_shard(shard).unwrap();
 
 		let (lock, _) = state_handler.load_for_mutation(&shard).unwrap();
 		let initial_state = Stf::init_state();

enclave-runtime/src/initialization.rs

@@ -246,7 +246,7 @@ pub(crate) fn init_direct_invocation_server(server_addr: String) -> EnclaveResul
 	Ok(())
 }
 
-pub(crate) fn init_shard(shard: &ShardIdentifier) -> EnclaveResult<()> {
+pub(crate) fn init_shard(shard: ShardIdentifier) -> EnclaveResult<()> {
 	let state_handler = GLOBAL_STATE_HANDLER_COMPONENT.get()?;
 	let _ = state_handler.initialize_shard(shard)?;
 	Ok(())

enclave-runtime/src/lib.rs

@@ -433,7 +433,7 @@ pub unsafe extern "C" fn init_shard(shard: *const u8, shard_size: u32) -> sgx_st
 	let shard_identifier =
 		ShardIdentifier::from_slice(slice::from_raw_parts(shard, shard_size as usize));
 
-	if let Err(e) = initialization::init_shard(&shard_identifier) {
+	if let Err(e) = initialization::init_shard(shard_identifier) {
 		error!("Failed to initialize shard ({:?}): {:?}", shard_identifier, e);
 		return sgx_status_t::SGX_ERROR_UNEXPECTED
 	}

enclave-runtime/src/test/fixtures/initialize_test_state.rs

@@ -27,7 +27,7 @@ pub fn init_state<S: HandleState<StateT = SgxExternalities>>(
 ) -> (State, ShardIdentifier) {
 	let shard = ShardIdentifier::default();
 
-	let _hash = state_handler.initialize_shard(&shard).unwrap();
+	let _hash = state_handler.initialize_shard(shard).unwrap();
 	let (lock, _) = state_handler.load_for_mutation(&shard).unwrap();
 
 	let mut state = Stf::init_state();

enclave-runtime/src/tests.rs

@@ -94,6 +94,7 @@ pub extern "C" fn test_main_entrance() -> size_t {
 		itp_stf_state_handler::test::sgx_tests::test_multiple_state_updates_create_snapshots_up_to_cache_size,
 		itp_stf_state_handler::test::sgx_tests::test_state_files_from_handler_can_be_loaded_again,
 		itp_stf_state_handler::test::sgx_tests::test_file_io_get_state_hash_works,
+		itp_stf_state_handler::test::sgx_tests::test_list_state_ids_ignores_files_not_matching_the_pattern,
 		test_compose_block_and_confirmation,
 		test_submit_trusted_call_to_top_pool,
 		test_submit_trusted_getter_to_top_pool,

enclave-runtime/src/tls_ra/seal_handler.rs

@@ -192,7 +192,7 @@ pub mod test {
 		let seal_handler = SealHandlerMock::default();
 		let state = <HandleStateMock as HandleState>::StateT::default();
 		let shard = ShardIdentifier::default();
-		let _init_hash = seal_handler.state_handler.initialize_shard(&shard).unwrap();
+		let _init_hash = seal_handler.state_handler.initialize_shard(shard).unwrap();
 
 		let result = seal_handler.seal_state(&state.encode(), &shard);
 
@@ -211,7 +211,7 @@ pub mod test {
 	pub fn unseal_seal_state_works() {
 		let seal_handler = SealHandlerMock::default();
 		let shard = ShardIdentifier::default();
-		seal_handler.state_handler.initialize_shard(&shard).unwrap();
+		seal_handler.state_handler.initialize_shard(shard).unwrap();
 		// Fill our mock state:
 		let (lock, mut state) = seal_handler.state_handler.load_for_mutation(&shard).unwrap();
 		let (key, value) = ("my_key", "my_value");

sidechain/consensus/aura/src/test/block_importer_tests.rs

@@ -75,7 +75,7 @@ fn default_authority() -> Pair {
 fn test_fixtures(
 	parentchain_block_import_trigger: Arc<TestParentchainBlockImportTrigger>,
 ) -> (TestBlockImporter, Arc<HandleStateMock>, Arc<TestTopPoolCallOperator>) {
-	let state_handler = Arc::new(HandleStateMock::from_shard(&shard()).unwrap());
+	let state_handler = Arc::new(HandleStateMock::from_shard(shard()).unwrap());
 	let top_pool_call_operator = Arc::new(TestTopPoolCallOperator::default());
 	let ocall_api = Arc::new(
 		OnchainMock::default()

sidechain/top-pool-rpc-author/src/author_tests.rs

@@ -104,7 +104,7 @@ fn create_author_with_filter<F: Filter<Value = TrustedOperation>>(
 	let top_pool = Arc::new(TrustedOperationPoolMock::default());
 
 	let shard_id = shard_id();
-	let state_facade = HandleStateMock::from_shard(&shard_id).unwrap();
+	let state_facade = HandleStateMock::from_shard(shard_id).unwrap();
 	let _ = state_facade.load(&shard_id).unwrap();
 
 	let encryption_key = ShieldingCryptoMock::default();