From 1dccd1cb1ad7acec8fffae3ea6fa75509479ef3c Mon Sep 17 00:00:00 2001 From: Jay Geng Date: Fri, 15 Sep 2023 12:03:57 -0400 Subject: [PATCH] Code cleanup around PRNG (#1064) * Remove dead and move around some code * [prng] avoid using core::mem:size_of * fixup! [prng] avoid using core::mem:size_of --- soroban-env-host/src/host.rs | 70 +------------------- soroban-env-host/src/host/error.rs | 79 +++++++++++++++++++++-- soroban-env-host/src/host/frame.rs | 28 -------- soroban-env-host/src/host/invoker_type.rs | 28 -------- soroban-env-host/src/host/prng.rs | 11 ++-- 5 files changed, 80 insertions(+), 136 deletions(-) delete mode 100644 soroban-env-host/src/host/invoker_type.rs diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs index 5553f3c5c..6a7ddc471 100644 --- a/soroban-env-host/src/host.rs +++ b/soroban-env-host/src/host.rs @@ -34,7 +34,6 @@ mod data_helper; pub(crate) mod declared_size; pub(crate) mod error; pub(crate) mod frame; -pub(crate) mod invoker_type; pub(crate) mod ledger_info_helper; mod mem_helper; pub(crate) mod metered_clone; @@ -939,73 +938,6 @@ impl Host { Ok(()) } - - fn decorate_contract_data_storage_error(&self, err: HostError, key: Val) -> HostError { - if !err.error.is_type(ScErrorType::Storage) { - return err; - } - if err.error.is_code(ScErrorCode::ExceededLimit) { - return self.err( - ScErrorType::Storage, - ScErrorCode::ExceededLimit, - "trying to access contract storage key outside of the correct footprint", - &[key], - ); - } - if err.error.is_code(ScErrorCode::MissingValue) { - return self.err( - ScErrorType::Storage, - ScErrorCode::MissingValue, - "trying to get non-existing value for contract storage key", - &[key], - ); - } - err - } - - pub(crate) fn decorate_contract_instance_storage_error( - &self, - err: HostError, - contract_id: &Hash, - ) -> HostError { - if err.error.is_type(ScErrorType::Storage) && err.error.is_code(ScErrorCode::ExceededLimit) - { - return self.err( - ScErrorType::Storage, - ScErrorCode::ExceededLimit, - "trying to access contract instance key outside of the correct footprint", - // No need for metered clone here as we are on the unrecoverable - // error path. - &[self - .add_host_object(ScAddress::Contract(contract_id.clone())) - .map(|a| a.into()) - .unwrap_or(Val::VOID.into())], - ); - } - err - } - - pub(crate) fn decorate_contract_code_storage_error( - &self, - err: HostError, - wasm_hash: &Hash, - ) -> HostError { - if err.error.is_type(ScErrorType::Storage) && err.error.is_code(ScErrorCode::ExceededLimit) - { - return self.err( - ScErrorType::Storage, - ScErrorCode::ExceededLimit, - "trying to access contract code key outside of the correct footprint", - // No need for metered clone here as we are on the unrecoverable - // error path. - &[self - .add_host_object(self.scbytes_from_hash(wasm_hash).unwrap_or_default()) - .map(|a| a.into()) - .unwrap_or(Val::VOID.into())], - ); - } - err - } } // Notes on metering: these are called from the guest and thus charged on the VM instructions. @@ -3105,7 +3037,7 @@ impl VmCallerEnv for Host { ) -> Result { self.visit_obj(seed, |bytes: &ScBytes| { let slice: &[u8] = bytes.as_ref(); - self.charge_budget(ContractCostType::HostMemCpy, Some(prng::SEED_BYTES as u64))?; + self.charge_budget(ContractCostType::HostMemCpy, Some(prng::SEED_BYTES))?; if let Ok(seed32) = slice.try_into() { self.with_current_prng(|prng| { *prng = Prng::new_from_seed(seed32); diff --git a/soroban-env-host/src/host/error.rs b/soroban-env-host/src/host/error.rs index 5af9c735c..2453b25ce 100644 --- a/soroban-env-host/src/host/error.rs +++ b/soroban-env-host/src/host/error.rs @@ -1,15 +1,11 @@ use crate::{ budget::AsBudget, events::Events, - xdr::{self, ScError}, - EnvBase, Error, Host, + xdr::{self, Hash, ScAddress, ScError, ScErrorCode, ScErrorType}, + ConversionError, EnvBase, Error, Host, TryFromVal, U32Val, Val, }; use backtrace::{Backtrace, BacktraceFrame}; use core::fmt::Debug; -use soroban_env_common::{ - xdr::{ScErrorCode, ScErrorType}, - ConversionError, TryFromVal, U32Val, Val, -}; use std::{ cell::{Ref, RefCell, RefMut}, ops::DerefMut, @@ -326,6 +322,77 @@ impl Host { } }) } + + pub(crate) fn decorate_contract_data_storage_error( + &self, + err: HostError, + key: Val, + ) -> HostError { + if !err.error.is_type(ScErrorType::Storage) { + return err; + } + if err.error.is_code(ScErrorCode::ExceededLimit) { + return self.err( + ScErrorType::Storage, + ScErrorCode::ExceededLimit, + "trying to access contract storage key outside of the correct footprint", + &[key], + ); + } + if err.error.is_code(ScErrorCode::MissingValue) { + return self.err( + ScErrorType::Storage, + ScErrorCode::MissingValue, + "trying to get non-existing value for contract storage key", + &[key], + ); + } + err + } + + pub(crate) fn decorate_contract_instance_storage_error( + &self, + err: HostError, + contract_id: &Hash, + ) -> HostError { + if err.error.is_type(ScErrorType::Storage) && err.error.is_code(ScErrorCode::ExceededLimit) + { + return self.err( + ScErrorType::Storage, + ScErrorCode::ExceededLimit, + "trying to access contract instance key outside of the correct footprint", + // No need for metered clone here as we are on the unrecoverable + // error path. + &[self + .add_host_object(ScAddress::Contract(contract_id.clone())) + .map(|a| a.into()) + .unwrap_or(Val::VOID.into())], + ); + } + err + } + + pub(crate) fn decorate_contract_code_storage_error( + &self, + err: HostError, + wasm_hash: &Hash, + ) -> HostError { + if err.error.is_type(ScErrorType::Storage) && err.error.is_code(ScErrorCode::ExceededLimit) + { + return self.err( + ScErrorType::Storage, + ScErrorCode::ExceededLimit, + "trying to access contract code key outside of the correct footprint", + // No need for metered clone here as we are on the unrecoverable + // error path. + &[self + .add_host_object(self.scbytes_from_hash(wasm_hash).unwrap_or_default()) + .map(|a| a.into()) + .unwrap_or(Val::VOID.into())], + ); + } + err + } } pub(crate) trait DebugArg { diff --git a/soroban-env-host/src/host/frame.rs b/soroban-env-host/src/host/frame.rs index 688daefeb..9d4d83aab 100644 --- a/soroban-env-host/src/host/frame.rs +++ b/soroban-env-host/src/host/frame.rs @@ -21,7 +21,6 @@ use std::rc::Rc; use crate::Vm; use super::{ - invoker_type::InvokerType, metered_clone::{MeteredClone, MeteredContainer, MeteredIterator}, prng::Prng, }; @@ -426,33 +425,6 @@ impl Host { Ok(hash) } - // Metering: mostly free or already covered by components (e.g. err_general) - pub(super) fn get_invoker_type(&self) -> Result { - let frames = self.try_borrow_context()?; - // If the previous frame exists and is a contract, return its ID, otherwise return - // the account invoking. - let st = match frames.as_slice() { - // There are always two frames when WASM is executed in the VM. - [.., c2, _] => match &c2.frame { - Frame::ContractVM { .. } => Ok(InvokerType::Contract), - Frame::HostFunction(_) => Ok(InvokerType::Account), - Frame::Token(..) => Ok(InvokerType::Contract), - #[cfg(any(test, feature = "testutils"))] - Frame::TestContract(_) => Ok(InvokerType::Contract), - }, - // In tests contracts are executed with a single frame. - // TODO: Investigate this discrepancy: https://github.com/stellar/rs-soroban-env/issues/485. - [_] => Ok(InvokerType::Account), - _ => Err(self.err( - ScErrorType::Context, - ScErrorCode::InternalError, - "no frames to derive the invoker from", - &[], - )), - }?; - Ok(st as u64) - } - /// Pushes a test contract [`Frame`], runs a closure, and then pops the /// frame, rolling back if the closure returned an error. Returns the result /// that the closure returned (or any error caused during the frame diff --git a/soroban-env-host/src/host/invoker_type.rs b/soroban-env-host/src/host/invoker_type.rs deleted file mode 100644 index 29c3faa07..000000000 --- a/soroban-env-host/src/host/invoker_type.rs +++ /dev/null @@ -1,28 +0,0 @@ -use soroban_env_common::ConversionError; - -#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] -#[repr(u64)] -pub(crate) enum InvokerType { - Account = 0, - Contract = 1, -} - -impl From for u64 { - fn from(v: InvokerType) -> Self { - v as u64 - } -} - -impl TryFrom for InvokerType { - type Error = ConversionError; - - fn try_from(v: u64) -> Result { - const ACCOUNT: u64 = InvokerType::Account as u64; - const CONTRACT: u64 = InvokerType::Contract as u64; - match v { - ACCOUNT => Ok(Self::Account), - CONTRACT => Ok(Self::Contract), - _ => Err(ConversionError), - } - } -} diff --git a/soroban-env-host/src/host/prng.rs b/soroban-env-host/src/host/prng.rs index 8003b0caa..51fd9a9a1 100644 --- a/soroban-env-host/src/host/prng.rs +++ b/soroban-env-host/src/host/prng.rs @@ -1,3 +1,4 @@ +use super::declared_size::DeclaredSizeForMetering; use crate::{ budget::Budget, host::metered_clone::MeteredClone, @@ -80,7 +81,7 @@ use std::ops::RangeInclusive; pub(crate) struct Prng(ChaCha20Rng); pub type Seed = ::Seed; -pub const SEED_BYTES: usize = core::mem::size_of::(); +pub const SEED_BYTES: u64 = ::DECLARED_SIZE; static_assertions::const_assert_eq!(SEED_BYTES, 32); impl Prng { @@ -101,7 +102,7 @@ impl Prng { // We over-estimate the number of bytes drawn by a factor of 2, to // account for the fact that a range sample is rejection-sampling which // is expected to only do one draw but might do more than one. - self.charge_prng_bytes(budget, (2 * core::mem::size_of::()) as u64)?; + self.charge_prng_bytes(budget, 2 * ::DECLARED_SIZE)?; let u = Uniform::from(range); Ok(u.sample(&mut self.0)) } @@ -136,10 +137,10 @@ impl Prng { } pub(crate) fn sub_prng(&mut self, budget: &Budget) -> Result { - let mut new_seed: Seed = [0; SEED_BYTES]; - self.charge_prng_bytes(budget, SEED_BYTES as u64)?; + let mut new_seed: Seed = [0; SEED_BYTES as usize]; + self.charge_prng_bytes(budget, SEED_BYTES)?; self.0.fill_bytes(&mut new_seed); - budget.charge(ContractCostType::HostMemCpy, Some(SEED_BYTES as u64))?; + budget.charge(ContractCostType::HostMemCpy, Some(SEED_BYTES))?; Ok(Self(ChaCha20Rng::from_seed(new_seed))) } }