From 5fbfe0c401cf348d1b27129d7eb5901333609531 Mon Sep 17 00:00:00 2001 From: Thomas Letan Date: Fri, 6 Dec 2024 15:40:00 +0100 Subject: [PATCH] Rust SDK: Do not use `store_has` in the happy path Some (most?) function from the `Runtime` trait are calling `store_has` on the input path before doing anything. In practice, this adds an overhead in every execution branches, even in the ones where the return of the next host function is enough te deduce the correct error to return. In this patch, we propose to delay the call to `store_has` to only when it is needed. This is the approach implemented in the native execution runtime, and we have replayed the full history of Bifrost on Mainnet as evidence that the semantics is correct. --- src/kernel_sdk/CHANGES.md | 2 + src/kernel_sdk/host/src/runtime.rs | 90 +++++++++++++++++++----------- 2 files changed, 60 insertions(+), 32 deletions(-) diff --git a/src/kernel_sdk/CHANGES.md b/src/kernel_sdk/CHANGES.md index cefd8de2f97f..5241a6af0e71 100644 --- a/src/kernel_sdk/CHANGES.md +++ b/src/kernel_sdk/CHANGES.md @@ -3,6 +3,7 @@ ## Version next ### SDK + - Add experimental support for compiling kernels to a Hermit RISC-V image behind the `proto-alpha` flag. - Add an experimental rollup host for RISC-V with an in-memory store behind the `experimental-host-in-memory-store` flag. - Add an `OutboxQueue` that can be used when more than 100 outbox messages are produced at a given level. @@ -30,6 +31,7 @@ - Remove redundant `testing` feature flag from `entrypoint` crate. - Remove redundant `testing` feature flag from `host` crate. - Disable `testing` as default features of the main `sdk` crate. +- Change `Runtime::store_value_size` to return `PathNotFound` when the input is the path of a directory. ### Installer client/kernel diff --git a/src/kernel_sdk/host/src/runtime.rs b/src/kernel_sdk/host/src/runtime.rs index 269e5b21d31b..66ac91c0a63f 100644 --- a/src/kernel_sdk/host/src/runtime.rs +++ b/src/kernel_sdk/host/src/runtime.rs @@ -325,8 +325,6 @@ where ) -> Result, RuntimeError> { use tezos_smart_rollup_core::MAX_FILE_CHUNK_SIZE; - check_path_has_value(self, path)?; - let mut buffer = Vec::with_capacity(max_bytes); unsafe { @@ -340,7 +338,9 @@ where // returns the total bytes written. buffer.set_len(usize::min(MAX_FILE_CHUNK_SIZE, max_bytes)); - let size = self.store_read_slice(path, from_offset, &mut buffer)?; + let size = self + .store_read_slice(path, from_offset, &mut buffer) + .map_err(check_path_has_value(self, path))?; // SAFETY: // We ensure that we set the length of the vector to the @@ -484,7 +484,9 @@ where } fn store_delete(&mut self, path: &T) -> Result<(), RuntimeError> { - check_path_exists(self, path)?; + if let Ok(None) = Runtime::store_has(self, path) { + return Err(RuntimeError::PathNotFound); + } let res = unsafe { SmartRollupCore::store_delete(self, path.as_ptr(), path.size()) }; @@ -520,8 +522,6 @@ where from_path: &impl Path, to_path: &impl Path, ) -> Result<(), RuntimeError> { - check_path_exists(self, from_path)?; - let res = unsafe { SmartRollupCore::store_move( self, @@ -533,7 +533,9 @@ where }; match Error::wrap(res) { Ok(_) => Ok(()), - Err(e) => Err(RuntimeError::HostErr(e)), + Err(e) => { + Err(RuntimeError::HostErr(e)).map_err(check_path_exists(self, from_path)) + } } } @@ -542,8 +544,6 @@ where from_path: &impl Path, to_path: &impl Path, ) -> Result<(), RuntimeError> { - check_path_exists(self, from_path)?; - let res = unsafe { SmartRollupCore::store_copy( self, @@ -555,7 +555,9 @@ where }; match Error::wrap(res) { Ok(_) => Ok(()), - Err(e) => Err(RuntimeError::HostErr(e)), + Err(e) => { + Err(RuntimeError::HostErr(e)).map_err(check_path_exists(self, from_path)) + } } } @@ -664,13 +666,14 @@ where } fn store_value_size(&self, path: &impl Path) -> Result { - check_path_exists(self, path)?; let res = unsafe { SmartRollupCore::store_value_size(self, path.as_ptr(), path.size()) }; match Error::wrap(res) { Ok(size) => Ok(size), - Err(e) => Err(RuntimeError::HostErr(e)), + Err(e) => { + Err(RuntimeError::HostErr(e)).map_err(check_path_has_value(self, path)) + } } } @@ -724,28 +727,31 @@ where } } -#[cfg(feature = "alloc")] -fn check_path_has_value( - runtime: &impl Runtime, - path: &T, -) -> Result<(), RuntimeError> { - if let Ok(Some(ValueType::Value | ValueType::ValueWithSubtree)) = - runtime.store_has(path) - { - Ok(()) - } else { - Err(RuntimeError::PathNotFound) +fn check_path_has_value<'a>( + runtime: &'a impl Runtime, + path: &'a impl Path, +) -> impl FnOnce(RuntimeError) -> RuntimeError + 'a { + |err| { + if let Ok(Some(ValueType::Value | ValueType::ValueWithSubtree)) = + runtime.store_has(path) + { + err + } else { + RuntimeError::PathNotFound + } } } -fn check_path_exists( - runtime: &impl Runtime, - path: &T, -) -> Result<(), RuntimeError> { - if let Ok(Some(_)) = runtime.store_has(path) { - Ok(()) - } else { - Err(RuntimeError::PathNotFound) +fn check_path_exists<'a, T: Path>( + runtime: &'a impl Runtime, + path: &'a T, +) -> impl FnOnce(RuntimeError) -> RuntimeError + 'a { + |err| { + if let Ok(Some(_)) = runtime.store_has(path) { + err + } else { + RuntimeError::PathNotFound + } } } @@ -873,10 +879,28 @@ mod tests { fn mock_path_not_existing(path_bytes: Vec) -> MockSmartRollupCore { let mut mock = MockSmartRollupCore::new(); + let path_bytes_delete = path_bytes.clone(); + let path_bytes_read = path_bytes.clone(); + let path_bytes_has = path_bytes.clone(); + + mock.expect_store_delete() + .withf(move |ptr, size| { + let bytes = unsafe { from_raw_parts(*ptr, *size) }; + path_bytes_delete == bytes + }) + .return_const(tezos_smart_rollup_core::STORE_NOT_A_NODE); + + mock.expect_store_read() + .withf(move |ptr, size, _, _, _| { + let bytes = unsafe { from_raw_parts(*ptr, *size) }; + path_bytes_read == bytes + }) + .return_const(tezos_smart_rollup_core::STORE_NOT_A_NODE); + mock.expect_store_has() .withf(move |ptr, size| { let bytes = unsafe { from_raw_parts(*ptr, *size) }; - path_bytes == bytes + path_bytes_has == bytes }) .return_const(tezos_smart_rollup_core::VALUE_TYPE_NONE); @@ -1306,6 +1330,8 @@ mod tests { fn store_value_size_path_not_found() { let mut mock = MockSmartRollupCore::new(); const PATH: RefPath<'static> = RefPath::assert_from(b"/prefix/of/other/paths"); + mock.expect_store_value_size() + .return_const(tezos_smart_rollup_core::STORE_NOT_A_VALUE); mock.expect_store_has() .return_const(tezos_smart_rollup_core::VALUE_TYPE_NONE); -- GitLab