From 2084e05c41b348fb9cc0cc3a15feff3dd45b0b69 Mon Sep 17 00:00:00 2001 From: Jay Geng Date: Tue, 5 Sep 2023 15:26:00 -0400 Subject: [PATCH 1/4] wip --- soroban-env-host/src/storage.rs | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/soroban-env-host/src/storage.rs b/soroban-env-host/src/storage.rs index 84dfab49a..2322d3cec 100644 --- a/soroban-env-host/src/storage.rs +++ b/soroban-env-host/src/storage.rs @@ -10,7 +10,7 @@ use std::rc::Rc; use soroban_env_common::xdr::{ScErrorCode, ScErrorType}; -use soroban_env_common::{Compare, Env, Val}; +use soroban_env_common::{Env, Val}; use crate::budget::Budget; use crate::xdr::{LedgerEntry, LedgerKey}; @@ -47,22 +47,6 @@ pub enum AccessType { ReadWrite, } -impl Compare for Host { - type Error = HostError; - - fn compare(&self, a: &AccessType, b: &AccessType) -> Result { - Ok(a.cmp(b)) - } -} - -impl Compare for Budget { - type Error = HostError; - - fn compare(&self, a: &AccessType, b: &AccessType) -> Result { - Ok(a.cmp(b)) - } -} - /// A helper type used by [FootprintMode::Recording] to provide access /// to a stable read-snapshot of a ledger. pub trait SnapshotSource { @@ -224,7 +208,7 @@ impl Storage { /// /// In [FootprintMode::Enforcing] mode, succeeds only if the read /// [LedgerKey] has been declared in the [Footprint]. - pub fn get_with_expiration( + pub(crate) fn get_with_expiration( &mut self, key: &Rc, budget: &Budget, From cad0ff0d1a0a46eba7cf4e1e6cd5eac62623f90b Mon Sep 17 00:00:00 2001 From: Jay Geng Date: Thu, 7 Sep 2023 17:18:29 -0400 Subject: [PATCH 2/4] [Storage] Move testing code to tests --- soroban-env-host/src/storage.rs | 140 --------------------------- soroban-env-host/src/test/storage.rs | 107 +++++++++++++++++++- soroban-env-host/src/test/util.rs | 29 +++++- 3 files changed, 131 insertions(+), 145 deletions(-) diff --git a/soroban-env-host/src/storage.rs b/soroban-env-host/src/storage.rs index 2322d3cec..3a7a2a27c 100644 --- a/soroban-env-host/src/storage.rs +++ b/soroban-env-host/src/storage.rs @@ -402,143 +402,3 @@ impl Storage { Ok(()) } } - -#[cfg(test)] -mod test_footprint { - - use soroban_env_common::xdr::ScAddress; - - use super::*; - use crate::budget::Budget; - use crate::xdr::{ContractDataDurability, LedgerKeyContractData, ScVal}; - - #[test] - fn footprint_record_access() -> Result<(), HostError> { - let budget = Budget::default(); - budget.reset_unlimited()?; - let mut fp = Footprint::default(); - // record when key not exist - let key = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { - contract: ScAddress::Contract([0; 32].into()), - key: ScVal::I32(0), - durability: ContractDataDurability::Persistent, - })); - fp.record_access(&key, AccessType::ReadOnly, &budget)?; - assert_eq!(fp.0.contains_key::(&key, &budget)?, true); - assert_eq!( - fp.0.get::(&key, &budget)?, - Some(&AccessType::ReadOnly) - ); - // record and change access - fp.record_access(&key, AccessType::ReadWrite, &budget)?; - assert_eq!( - fp.0.get::(&key, &budget)?, - Some(&AccessType::ReadWrite) - ); - fp.record_access(&key, AccessType::ReadOnly, &budget)?; - assert_eq!( - fp.0.get::(&key, &budget)?, - Some(&AccessType::ReadWrite) - ); - Ok(()) - } - - #[test] - fn footprint_enforce_access() -> Result<(), HostError> { - let budget = Budget::default(); - let key = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { - contract: ScAddress::Contract([0; 32].into()), - key: ScVal::I32(0), - durability: ContractDataDurability::Persistent, - })); - - // Key not in footprint. Only difference is type_ - let key2 = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { - contract: ScAddress::Contract([0; 32].into()), - key: ScVal::I32(0), - durability: ContractDataDurability::Temporary, - })); - - let om = [(Rc::clone(&key), AccessType::ReadOnly)].into(); - let mom = MeteredOrdMap::from_map(om, &budget)?; - let mut fp = Footprint(mom); - assert!(fp - .enforce_access(&key2, AccessType::ReadOnly, &budget) - .is_err()); - fp.enforce_access(&key, AccessType::ReadOnly, &budget)?; - fp.0 = - fp.0.insert(Rc::clone(&key), AccessType::ReadWrite, &budget)?; - fp.enforce_access(&key, AccessType::ReadOnly, &budget)?; - fp.enforce_access(&key, AccessType::ReadWrite, &budget)?; - Ok(()) - } - - #[test] - fn footprint_enforce_access_not_exist() -> Result<(), HostError> { - let budget = Budget::default(); - let mut fp = Footprint::default(); - let key = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { - contract: ScAddress::Contract([0; 32].into()), - key: ScVal::I32(0), - durability: ContractDataDurability::Persistent, - })); - let res = fp.enforce_access(&key, AccessType::ReadOnly, &budget); - assert!(HostError::result_matches_err( - res, - (ScErrorType::Storage, ScErrorCode::ExceededLimit) - )); - Ok(()) - } - - #[test] - fn footprint_attempt_to_write_readonly_entry() -> Result<(), HostError> { - let budget = Budget::default(); - let key = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { - contract: ScAddress::Contract([0; 32].into()), - key: ScVal::I32(0), - durability: ContractDataDurability::Persistent, - })); - let om = [(Rc::clone(&key), AccessType::ReadOnly)].into(); - let mom = MeteredOrdMap::from_map(om, &budget)?; - let mut fp = Footprint(mom); - let res = fp.enforce_access(&key, AccessType::ReadWrite, &budget); - assert!(HostError::result_matches_err( - res, - (ScErrorType::Storage, ScErrorCode::ExceededLimit) - )); - Ok(()) - } -} - -#[cfg(test)] -pub(crate) mod test_storage { - use std::collections::BTreeMap; - - use soroban_env_common::Error; - - use super::*; - #[allow(dead_code)] - pub(crate) struct MockSnapshotSource(BTreeMap, (Rc, Option)>); - #[allow(dead_code)] - impl MockSnapshotSource { - pub(crate) fn new() -> Self { - Self(BTreeMap::, (Rc, Option)>::new()) - } - } - impl SnapshotSource for MockSnapshotSource { - fn get(&self, key: &Rc) -> Result<(Rc, Option), HostError> { - if let Some(val) = self.0.get(key) { - Ok((Rc::clone(&val.0), val.1)) - } else { - Err( - Error::from_type_and_code(ScErrorType::Storage, ScErrorCode::MissingValue) - .into(), - ) - } - } - - fn has(&self, key: &Rc) -> Result { - Ok(self.0.contains_key(key)) - } - } -} diff --git a/soroban-env-host/src/test/storage.rs b/soroban-env-host/src/test/storage.rs index c82354302..2007ce6ed 100644 --- a/soroban-env-host/src/test/storage.rs +++ b/soroban-env-host/src/test/storage.rs @@ -1,8 +1,113 @@ +use std::rc::Rc; + +use crate::budget::Budget; use crate::native_contract::testutils::HostVec; -use crate::{host_vec, Host}; +use crate::storage::{AccessType, Footprint}; +use crate::xdr::{ + ContractDataDurability, LedgerKey, LedgerKeyContractData, ScAddress, ScErrorCode, ScErrorType, + ScVal, +}; +use crate::{host_vec, Host, HostError, MeteredOrdMap}; use soroban_env_common::{AddressObject, Env, Symbol, TryFromVal, TryIntoVal}; use soroban_test_wasms::CONTRACT_STORAGE; +#[test] +fn footprint_record_access() -> Result<(), HostError> { + let budget = Budget::default(); + budget.reset_unlimited()?; + let mut fp = Footprint::default(); + // record when key not exist + let key = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { + contract: ScAddress::Contract([0; 32].into()), + key: ScVal::I32(0), + durability: ContractDataDurability::Persistent, + })); + fp.record_access(&key, AccessType::ReadOnly, &budget)?; + assert_eq!(fp.0.contains_key::(&key, &budget)?, true); + assert_eq!( + fp.0.get::(&key, &budget)?, + Some(&AccessType::ReadOnly) + ); + // record and change access + fp.record_access(&key, AccessType::ReadWrite, &budget)?; + assert_eq!( + fp.0.get::(&key, &budget)?, + Some(&AccessType::ReadWrite) + ); + fp.record_access(&key, AccessType::ReadOnly, &budget)?; + assert_eq!( + fp.0.get::(&key, &budget)?, + Some(&AccessType::ReadWrite) + ); + Ok(()) +} + +#[test] +fn footprint_enforce_access() -> Result<(), HostError> { + let budget = Budget::default(); + let key = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { + contract: ScAddress::Contract([0; 32].into()), + key: ScVal::I32(0), + durability: ContractDataDurability::Persistent, + })); + + // Key not in footprint. Only difference is type_ + let key2 = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { + contract: ScAddress::Contract([0; 32].into()), + key: ScVal::I32(0), + durability: ContractDataDurability::Temporary, + })); + + let om = [(Rc::clone(&key), AccessType::ReadOnly)].into(); + let mom = MeteredOrdMap::from_map(om, &budget)?; + let mut fp = Footprint(mom); + assert!(fp + .enforce_access(&key2, AccessType::ReadOnly, &budget) + .is_err()); + fp.enforce_access(&key, AccessType::ReadOnly, &budget)?; + fp.0 = + fp.0.insert(Rc::clone(&key), AccessType::ReadWrite, &budget)?; + fp.enforce_access(&key, AccessType::ReadOnly, &budget)?; + fp.enforce_access(&key, AccessType::ReadWrite, &budget)?; + Ok(()) +} + +#[test] +fn footprint_enforce_access_not_exist() -> Result<(), HostError> { + let budget = Budget::default(); + let mut fp = Footprint::default(); + let key = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { + contract: ScAddress::Contract([0; 32].into()), + key: ScVal::I32(0), + durability: ContractDataDurability::Persistent, + })); + let res = fp.enforce_access(&key, AccessType::ReadOnly, &budget); + assert!(HostError::result_matches_err( + res, + (ScErrorType::Storage, ScErrorCode::ExceededLimit) + )); + Ok(()) +} + +#[test] +fn footprint_attempt_to_write_readonly_entry() -> Result<(), HostError> { + let budget = Budget::default(); + let key = Rc::new(LedgerKey::ContractData(LedgerKeyContractData { + contract: ScAddress::Contract([0; 32].into()), + key: ScVal::I32(0), + durability: ContractDataDurability::Persistent, + })); + let om = [(Rc::clone(&key), AccessType::ReadOnly)].into(); + let mom = MeteredOrdMap::from_map(om, &budget)?; + let mut fp = Footprint(mom); + let res = fp.enforce_access(&key, AccessType::ReadWrite, &budget); + assert!(HostError::result_matches_err( + res, + (ScErrorType::Storage, ScErrorCode::ExceededLimit) + )); + Ok(()) +} + fn storage_fn_name(host: &Host, fn_name: &str, storage: &str) -> Symbol { Symbol::try_from_val(host, &format!("{}_{}", fn_name, storage).as_str()).unwrap() } diff --git a/soroban-env-host/src/test/util.rs b/soroban-env-host/src/test/util.rs index c1e2d263c..a517775a0 100644 --- a/soroban-env-host/src/test/util.rs +++ b/soroban-env-host/src/test/util.rs @@ -1,10 +1,10 @@ -use std::rc::Rc; +use std::{collections::BTreeMap, rc::Rc}; use rand::{thread_rng, RngCore}; use soroban_env_common::{ xdr::{ AccountEntry, AccountId, ContractCostType, LedgerEntry, LedgerEntryData, LedgerKey, - PublicKey, ScAddress, ScVal, ScVec, Uint256, + PublicKey, ScAddress, ScErrorCode, ScErrorType, ScVal, ScVec, Uint256, }, AddressObject, BytesObject, Env, EnvBase, Symbol, Val, VecObject, }; @@ -12,8 +12,8 @@ use soroban_synth_wasm::{Arity, ModEmitter, Operand}; use crate::{ budget::{AsBudget, Budget}, - storage::{test_storage::MockSnapshotSource, Storage}, - xdr, Host, HostError, LedgerInfo, + storage::{SnapshotSource, Storage}, + xdr, Error, Host, HostError, LedgerInfo, }; use soroban_bench_utils::HostTracker; @@ -68,6 +68,27 @@ pub(crate) fn wasm_module_with_4n_insns(n: usize) -> Vec { fe.finish_and_export("test").finish() } +pub(crate) struct MockSnapshotSource(BTreeMap, (Rc, Option)>); + +impl MockSnapshotSource { + pub(crate) fn new() -> Self { + Self(BTreeMap::, (Rc, Option)>::new()) + } +} +impl SnapshotSource for MockSnapshotSource { + fn get(&self, key: &Rc) -> Result<(Rc, Option), HostError> { + if let Some(val) = self.0.get(key) { + Ok((Rc::clone(&val.0), val.1)) + } else { + Err(Error::from_type_and_code(ScErrorType::Storage, ScErrorCode::MissingValue).into()) + } + } + + fn has(&self, key: &Rc) -> Result { + Ok(self.0.contains_key(key)) + } +} + #[allow(dead_code)] impl Host { pub(crate) fn test_host() -> Self { From dba043c8007e9aee9b713119f44b911d957520f1 Mon Sep 17 00:00:00 2001 From: Jay Geng Date: Thu, 7 Sep 2023 17:18:38 -0400 Subject: [PATCH 3/4] Update Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 7ddfca70e..1a1df864b 100644 --- a/Makefile +++ b/Makefile @@ -24,7 +24,7 @@ publish: publish-dry-run: ./publish-dry-run.sh -# Requires: cargo-install llvm-cov +# Requires: `cargo install cargo-llvm-cov` coverage: rm -f lcov.info cargo llvm-cov test --all-features --tests --lcov --output-path=lcov.info From f276ed87622e4791582c4669bc9d2ac1f4368d83 Mon Sep 17 00:00:00 2001 From: Jay Geng Date: Thu, 7 Sep 2023 18:46:00 -0400 Subject: [PATCH 4/4] [Storage] code cleanup/comments around instance storage --- soroban-env-host/src/host.rs | 62 ++++-------------------- soroban-env-host/src/host/data_helper.rs | 41 ++++++++++++++-- soroban-env-host/src/host/frame.rs | 6 ++- soroban-env-host/src/storage.rs | 4 ++ 4 files changed, 56 insertions(+), 57 deletions(-) diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs index 22a4a0033..b52170960 100644 --- a/soroban-env-host/src/host.rs +++ b/soroban-env-host/src/host.rs @@ -2220,32 +2220,11 @@ impl VmCallerEnv for Host { high_expiration_watermark: U32Val, ) -> Result { let contract_id = self.get_current_contract_id_internal()?; - let key = self.contract_instance_ledger_key(&contract_id)?; - self.try_borrow_storage_mut()? - .bump( - self, - key.metered_clone(self)?, - low_expiration_watermark.into(), - high_expiration_watermark.into(), - ) - .map_err(|e| self.decorate_contract_instance_storage_error(e, &contract_id))?; - match self - .retrieve_contract_instance_from_storage(&key)? - .executable - { - ContractExecutable::Wasm(wasm_hash) => { - let key = self.contract_code_ledger_key(&wasm_hash)?; - self.try_borrow_storage_mut()? - .bump( - self, - key, - low_expiration_watermark.into(), - high_expiration_watermark.into(), - ) - .map_err(|e| self.decorate_contract_code_storage_error(e, &wasm_hash))?; - } - ContractExecutable::Token => {} - } + self.bump_contract_instance_and_code_from_contract_id( + &contract_id, + low_expiration_watermark.into(), + high_expiration_watermark.into(), + )?; Ok(Val::VOID) } @@ -2257,32 +2236,11 @@ impl VmCallerEnv for Host { high_expiration_watermark: U32Val, ) -> Result { let contract_id = self.contract_id_from_address(contract)?; - let key = self.contract_instance_ledger_key(&contract_id)?; - self.try_borrow_storage_mut()? - .bump( - self, - key.metered_clone(self)?, - low_expiration_watermark.into(), - high_expiration_watermark.into(), - ) - .map_err(|e| self.decorate_contract_instance_storage_error(e, &contract_id))?; - match self - .retrieve_contract_instance_from_storage(&key)? - .executable - { - ContractExecutable::Wasm(wasm_hash) => { - let key = self.contract_code_ledger_key(&wasm_hash)?; - self.try_borrow_storage_mut()? - .bump( - self, - key, - low_expiration_watermark.into(), - high_expiration_watermark.into(), - ) - .map_err(|e| self.decorate_contract_code_storage_error(e, &wasm_hash))?; - } - ContractExecutable::Token => {} - } + self.bump_contract_instance_and_code_from_contract_id( + &contract_id, + low_expiration_watermark.into(), + high_expiration_watermark.into(), + )?; Ok(Val::VOID) } diff --git a/soroban-env-host/src/host/data_helper.rs b/soroban-env-host/src/host/data_helper.rs index ef68865e2..182753e30 100644 --- a/soroban-env-host/src/host/data_helper.rs +++ b/soroban-env-host/src/host/data_helper.rs @@ -2,8 +2,8 @@ use core::cmp::min; use std::rc::Rc; use soroban_env_common::xdr::{ - BytesM, ContractDataDurability, ContractIdPreimage, ExtensionPoint, HashIdPreimageContractId, - ScAddress, ScContractInstance, ScErrorCode, ScErrorType, + BytesM, ContractDataDurability, ContractExecutable, ContractIdPreimage, ExtensionPoint, + HashIdPreimageContractId, ScAddress, ScContractInstance, ScErrorCode, ScErrorType, }; use soroban_env_common::{AddressObject, Env, U32Val}; @@ -110,7 +110,7 @@ impl Host { let (current, expiration_ledger) = self .try_borrow_storage_mut()? .get_with_expiration(key, self.as_budget())?; - let mut current = (*current).metered_clone(self.as_budget())?; + let mut current = (*current).metered_clone(self)?; match current.data { LedgerEntryData::ContractData(ref mut entry) => { @@ -153,6 +153,41 @@ impl Host { Ok(()) } + pub(crate) fn bump_contract_instance_and_code_from_contract_id( + &self, + contract_id: &Hash, + low_expiration_watermark: u32, + high_expiration_watermark: u32, + ) -> Result<(), HostError> { + let key = self.contract_instance_ledger_key(&contract_id)?; + self.try_borrow_storage_mut()? + .bump( + self, + key.metered_clone(self)?, + low_expiration_watermark, + high_expiration_watermark, + ) + .map_err(|e| self.decorate_contract_instance_storage_error(e, &contract_id))?; + match self + .retrieve_contract_instance_from_storage(&key)? + .executable + { + ContractExecutable::Wasm(wasm_hash) => { + let key = self.contract_code_ledger_key(&wasm_hash)?; + self.try_borrow_storage_mut()? + .bump( + self, + key, + low_expiration_watermark, + high_expiration_watermark, + ) + .map_err(|e| self.decorate_contract_code_storage_error(e, &wasm_hash))?; + } + ContractExecutable::Token => {} + } + Ok(()) + } + // metering: covered by components pub fn get_full_contract_id_preimage( &self, diff --git a/soroban-env-host/src/host/frame.rs b/soroban-env-host/src/host/frame.rs index 28de225fb..688daefeb 100644 --- a/soroban-env-host/src/host/frame.rs +++ b/soroban-env-host/src/host/frame.rs @@ -149,7 +149,7 @@ impl Host { // instead of snapshotting it and rolling it back, we just flush the // changes only when rollback is not needed. if orp.is_none() { - self.flush_instance_storage()?; + self.persist_instance_storage()?; } self.try_borrow_context_mut()? .pop() @@ -787,7 +787,9 @@ impl Host { Ok(()) } - fn flush_instance_storage(&self) -> Result<(), HostError> { + // Make the in-memory instance storage persist into the `Storage` by writing + // its updated contents into corresponding `ContractData` ledger entry. + fn persist_instance_storage(&self) -> Result<(), HostError> { let updated_instance = self.with_current_context_mut(|ctx| { if let Some(storage) = &ctx.storage { if !storage.is_modified { diff --git a/soroban-env-host/src/storage.rs b/soroban-env-host/src/storage.rs index 3a7a2a27c..b76d89814 100644 --- a/soroban-env-host/src/storage.rs +++ b/soroban-env-host/src/storage.rs @@ -19,6 +19,10 @@ use crate::{host::metered_map::MeteredOrdMap, HostError}; pub type FootprintMap = MeteredOrdMap, AccessType, Budget>; pub type StorageMap = MeteredOrdMap, Option<(Rc, Option)>, Budget>; + +/// The in-memory instance storage of the current running contract. Initially +/// contains entries from the `ScMap` of the corresponding `ScContractInstance` +/// contract data entry. #[derive(Clone)] pub(crate) struct InstanceStorageMap { pub(crate) map: MeteredOrdMap,