diff --git a/crates/namada/src/ledger/native_vp/ibc/context.rs b/crates/namada/src/ledger/native_vp/ibc/context.rs index 1c44b638fff..edd5f06ea65 100644 --- a/crates/namada/src/ledger/native_vp/ibc/context.rs +++ b/crates/namada/src/ledger/native_vp/ibc/context.rs @@ -90,11 +90,6 @@ where )?; Ok(None) } - Some(StorageModification::Temp { .. }) => { - Err(StorageError::new_const( - "Temp shouldn't be inserted in an IBC transaction", - )) - } Some(StorageModification::InitAccount { .. }) => Err( StorageError::new_const("InitAccount shouldn't be inserted"), ), diff --git a/crates/namada/src/ledger/vp_host_fns.rs b/crates/namada/src/ledger/vp_host_fns.rs index 393e18d01f7..e88780c8019 100644 --- a/crates/namada/src/ledger/vp_host_fns.rs +++ b/crates/namada/src/ledger/vp_host_fns.rs @@ -86,9 +86,8 @@ where // Read the VP of a new account Ok(Some(vp_code_hash.to_vec())) } - Some(&write_log::StorageModification::Temp { .. }) | None => { - // When not found in write log or only found a temporary value, try - // to read from the storage + None => { + // When not found in write log, try to read from the storage let (value, gas) = state.db_read(key).map_err(RuntimeError::StorageError)?; add_gas(gas_meter, gas)?; @@ -108,19 +107,17 @@ where S: StateRead + Debug, { // Try to read from the write log first - let (log_val, gas) = state.write_log().read_persistent(key); + let (log_val, gas) = state.write_log().read(key); add_gas(gas_meter, gas)?; match log_val { - Some(write_log::PersistentStorageModification::Write { value }) => { + Some(write_log::StorageModification::Write { value }) => { Ok(Some(value.clone())) } - Some(write_log::PersistentStorageModification::Delete) => { + Some(write_log::StorageModification::Delete) => { // Given key has been deleted Ok(None) } - Some(write_log::PersistentStorageModification::InitAccount { - vp_code_hash, - }) => { + Some(write_log::StorageModification::InitAccount { vp_code_hash }) => { // Read the VP code hash of a new account Ok(Some(vp_code_hash.to_vec())) } @@ -145,14 +142,9 @@ pub fn read_temp( where S: StateRead + Debug, { - let (log_val, gas) = state.write_log().read(key); + let (log_val, gas) = state.write_log().read_temp(key); add_gas(gas_meter, gas)?; - match log_val { - Some(write_log::StorageModification::Temp { ref value }) => { - Ok(Some(value.clone())) - } - _ => Ok(None), - } + Ok(log_val.cloned()) } /// Storage `has_key` in prior state (before tx execution). It will try to read @@ -175,9 +167,8 @@ where Ok(false) } Some(&write_log::StorageModification::InitAccount { .. }) => Ok(true), - Some(&write_log::StorageModification::Temp { .. }) | None => { - // When not found in write log or only found a temporary value, try - // to check the storage + None => { + // When not found in write log, try to check the storage let (present, gas) = state.db_has_key(key).map_err(RuntimeError::StorageError)?; add_gas(gas_meter, gas)?; @@ -197,19 +188,15 @@ where S: StateRead + Debug, { // Try to read from the write log first - let (log_val, gas) = state.write_log().read_persistent(key); + let (log_val, gas) = state.write_log().read(key); add_gas(gas_meter, gas)?; match log_val { - Some(write_log::PersistentStorageModification::Write { .. }) => { - Ok(true) - } - Some(write_log::PersistentStorageModification::Delete) => { + Some(write_log::StorageModification::Write { .. }) => Ok(true), + Some(write_log::StorageModification::Delete) => { // The given key has been deleted Ok(false) } - Some(write_log::PersistentStorageModification::InitAccount { - .. - }) => Ok(true), + Some(write_log::StorageModification::InitAccount { .. }) => Ok(true), None => { // When not found in write log, try // to check the storage diff --git a/crates/namada/src/vm/host_env.rs b/crates/namada/src/vm/host_env.rs index 9327a8838e5..c316a712df3 100644 --- a/crates/namada/src/vm/host_env.rs +++ b/crates/namada/src/vm/host_env.rs @@ -67,8 +67,6 @@ pub enum TxRuntimeError { StorageError(#[from] StorageError), #[error("Storage data error: {0}")] StorageDataError(crate::storage::Error), - #[error("Trying to read a permanent value with read_temp")] - ReadPermanentValueError, #[error("Encoding error: {0}")] EncodingError(std::io::Error), #[error("Address error: {0}")] @@ -714,23 +712,16 @@ where let key = Key::parse(key).map_err(TxRuntimeError::StorageDataError)?; let write_log = unsafe { env.ctx.write_log.get() }; - let (log_val, gas) = write_log.read(&key); + let (log_val, gas) = write_log.read_temp(&key); tx_charge_gas::(env, gas)?; - let value = match log_val { - Some(write_log::StorageModification::Temp { ref value }) => { - Ok(Some(value.clone())) - } - None => Ok(None), - _ => Err(TxRuntimeError::ReadPermanentValueError), - }?; - match value { + match log_val { Some(value) => { let len: i64 = value .len() .try_into() .map_err(TxRuntimeError::NumConversionError)?; let result_buffer = unsafe { env.ctx.result_buffer.get() }; - result_buffer.replace(value); + result_buffer.replace(value.clone()); Ok(len) } None => Ok(HostEnvResult::Fail.to_i64()), @@ -850,10 +841,6 @@ where // a VP of a new account doesn't need to be iterated continue; } - Some(write_log::StorageModification::Temp { .. }) => { - // temporary values are not returned by the iterator - continue; - } None => { let key_val = borsh::to_vec(&KeyVal { key, val }) .map_err(TxRuntimeError::EncodingError)?; diff --git a/crates/state/src/lib.rs b/crates/state/src/lib.rs index 79e34b972a6..daf984d1937 100644 --- a/crates/state/src/lib.rs +++ b/crates/state/src/lib.rs @@ -194,14 +194,14 @@ macro_rules! impl_storage_read { key: &storage::Key, ) -> namada_storage::Result>> { // try to read from the write log first - let (log_val, gas) = self.write_log().read_persistent(key); + let (log_val, gas) = self.write_log().read(key); self.charge_gas(gas).into_storage_result()?; match log_val { - Some(write_log::PersistentStorageModification::Write { value }) => { + Some(write_log::StorageModification::Write { value }) => { Ok(Some(value.clone())) } - Some(write_log::PersistentStorageModification::Delete) => Ok(None), - Some(write_log::PersistentStorageModification::InitAccount { + Some(write_log::StorageModification::Delete) => Ok(None), + Some(write_log::StorageModification::InitAccount { ref vp_code_hash, }) => Ok(Some(vp_code_hash.to_vec())), None => { @@ -224,8 +224,8 @@ macro_rules! impl_storage_read { // the given key has been deleted Ok(false) } - Some(&write_log::StorageModification::Temp { .. }) | None => { - // when not found in write log or only found a temporary value, try to check the storage + None => { + // when not found in write log try to check the storage let (present, gas) = self.db_has_key(key).into_storage_result()?; self.charge_gas(gas).into_storage_result()?; Ok(present) @@ -407,8 +407,6 @@ pub fn merklize_all_keys(_key: &storage::Key) -> bool { pub enum Error { #[error("TEMPORARY error: {error}")] Temporary { error: String }, - #[error("Found an unknown key: {key}")] - UnknownKey { key: String }, #[error("Storage key error {0}")] KeyError(namada_core::storage::Error), #[error("Coding error: {0}")] @@ -554,8 +552,7 @@ where let gas = vp_code_hash.len() as u64; return Some((key, vp_code_hash.to_vec(), gas)); } - write_log::StorageModification::Delete - | write_log::StorageModification::Temp { .. } => { + write_log::StorageModification::Delete => { continue; } } diff --git a/crates/state/src/wl_state.rs b/crates/state/src/wl_state.rs index 4934647624c..48ffe259d0b 100644 --- a/crates/state/src/wl_state.rs +++ b/crates/state/src/wl_state.rs @@ -208,8 +208,6 @@ where StorageModification::InitAccount { vp_code_hash } => { self.batch_write_subspace_val(batch, &key, vp_code_hash)?; } - // temporary value isn't persisted - StorageModification::Temp { .. } => {} } } debug_assert!(self.0.write_log.block_write_log.is_empty()); @@ -1274,18 +1272,15 @@ where &self, key: &storage::Key, ) -> Result> { - let (log_val, _) = self.write_log().read(key); + let (log_val, _) = self.write_log().read_temp(key); match log_val { - Some(crate::write_log::StorageModification::Temp { value }) => { + Some(value) => { let value = namada_core::borsh::BorshDeserialize::try_from_slice(value) .map_err(Error::BorshCodingError)?; Ok(Some(value)) } None => Ok(None), - _ => Err(Error::UnknownKey { - key: key.to_string(), - }), } } } diff --git a/crates/state/src/write_log.rs b/crates/state/src/write_log.rs index 44f28c96190..ee7c56ca39d 100644 --- a/crates/state/src/write_log.rs +++ b/crates/state/src/write_log.rs @@ -32,10 +32,6 @@ pub enum Error { WriteTempAfterWrite, #[error("Replay protection key: {0}")] ReplayProtection(String), - #[error( - "Trying to cast a temporary write to a persistent storage modification" - )] - TempToPersistentModificationCast, } /// Result for functions that may fail @@ -58,57 +54,6 @@ pub enum StorageModification { /// Validity predicate hash bytes vp_code_hash: Hash, }, - /// Temporary value. This value will be never written to the storage. After - /// writing a temporary value, it can't be mutated with normal write. - Temp { - /// Value bytes - value: Vec, - }, -} - -/// A persistent storage modification. Associated data is present as a reference -/// to the corresponding [`StorageModification`] present in the write log -#[derive(Clone, Debug, PartialEq, Eq)] -pub enum PersistentStorageModification<'wl> { - /// Write a new value - Write { - /// Value bytes - value: &'wl Vec, - }, - /// Delete an existing key-value - Delete, - /// Initialize a new account with established address and a given validity - /// predicate hash. The key for `InitAccount` inside the [`WriteLog`] must - /// point to its validity predicate. - InitAccount { - /// Validity predicate hash bytes - vp_code_hash: &'wl Hash, - }, -} - -impl<'wl> TryFrom<&'wl StorageModification> - for PersistentStorageModification<'wl> -{ - type Error = Error; - - fn try_from( - value: &'wl StorageModification, - ) -> std::prelude::v1::Result { - match value { - StorageModification::Write { value } => { - Ok(PersistentStorageModification::Write { value }) - } - StorageModification::Delete => { - Ok(PersistentStorageModification::Delete) - } - StorageModification::InitAccount { vp_code_hash } => { - Ok(PersistentStorageModification::InitAccount { vp_code_hash }) - } - StorageModification::Temp { value: _ } => { - Err(Error::TempToPersistentModificationCast) - } - } - } } #[derive(Debug, Clone, PartialEq, Eq)] @@ -132,6 +77,9 @@ pub struct WriteLog { pub(crate) block_write_log: HashMap, /// The storage modifications for the current transaction pub(crate) tx_write_log: HashMap, + /// Temporary key-values for the current transaction that are dropped after + /// tx and its verifying VPs execution is done + pub(crate) tx_temp_log: HashMap>, /// A precommit bucket for the `tx_write_log`. This is useful for /// validation when a clean `tx_write_log` is needed without committing any /// modification already in there. These modifications can be temporarily @@ -171,6 +119,7 @@ impl Default for WriteLog { address_gen: None, block_write_log: HashMap::with_capacity(100_000), tx_write_log: HashMap::with_capacity(100), + tx_temp_log: HashMap::with_capacity(1), tx_precommit_write_log: HashMap::with_capacity(100), ibc_events: BTreeSet::new(), replay_protection: HashMap::with_capacity(1_000), @@ -206,9 +155,6 @@ impl WriteLog { StorageModification::InitAccount { ref vp_code_hash } => { key.len() + vp_code_hash.len() } - StorageModification::Temp { ref value } => { - key.len() + value.len() - } }; (Some(v), gas as u64 * MEMORY_ACCESS_GAS_PER_BYTE) } @@ -219,10 +165,10 @@ impl WriteLog { /// Read a non-temporary value at the given key and return the value and the /// gas cost, returns [`None`] if the key is not present in the write /// log - pub fn read_persistent( + pub fn read( &self, key: &storage::Key, - ) -> (Option, u64) { + ) -> (Option, u64) { for bucket in [ &self.tx_write_log, &self.tx_precommit_write_log, @@ -237,12 +183,9 @@ impl WriteLog { StorageModification::InitAccount { ref vp_code_hash } => { key.len() + vp_code_hash.len() } - StorageModification::Temp { .. } => continue, }; return ( - Some(modification.try_into().expect( - "Temporary value should have been filtered out", - )), + Some(modification.clone()), gas as u64 * MEMORY_ACCESS_GAS_PER_BYTE, ); } @@ -268,9 +211,6 @@ impl WriteLog { StorageModification::InitAccount { ref vp_code_hash } => { key.len() + vp_code_hash.len() } - StorageModification::Temp { ref value } => { - key.len() + value.len() - } }; (Some(v), gas as u64 * MEMORY_ACCESS_GAS_PER_BYTE) } @@ -278,6 +218,21 @@ impl WriteLog { } } + /// Read a temp value at the given key and return the value and the gas + /// cost, returns [`None`] if the key is not present in the temp write + /// log + pub fn read_temp(&self, key: &storage::Key) -> (Option<&Vec>, u64) { + // try to read from tx write log first + match self.tx_temp_log.get(key) { + Some(value) => { + let gas = key.len() + value.len(); + + (Some(value), gas as u64 * MEMORY_ACCESS_GAS_PER_BYTE) + } + None => (None, key.len() as u64 * MEMORY_ACCESS_GAS_PER_BYTE), + } + } + /// Write a key and a value and return the gas cost and the size difference /// Fails with [`Error::UpdateVpOfNewAccount`] when attempting to update a /// validity predicate of a new account that's not yet committed to storage. @@ -290,6 +245,9 @@ impl WriteLog { ) -> Result<(u64, i64)> { let len = value.len(); let gas = key.len() + len; + if self.tx_temp_log.contains_key(key) { + return Err(Error::UpdateTemporaryValue); + } let size_diff = match self .tx_write_log .get(key) @@ -308,9 +266,6 @@ impl WriteLog { // anyway and this cannot be exploited to run the vm forever return Err(Error::UpdateVpOfNewAccount); } - StorageModification::Temp { .. } => { - return Err(Error::UpdateTemporaryValue); - } }, // set just the length of the value because we don't know if // the previous value exists on the storage @@ -333,6 +288,9 @@ impl WriteLog { key: &storage::Key, value: Vec, ) -> Result<()> { + if self.tx_temp_log.contains_key(key) { + return Err(Error::UpdateTemporaryValue); + } if let Some(prev) = self .block_write_log .insert(key.clone(), StorageModification::Write { value }) @@ -341,9 +299,6 @@ impl WriteLog { StorageModification::InitAccount { .. } => { return Err(Error::UpdateVpOfNewAccount); } - StorageModification::Temp { .. } => { - return Err(Error::UpdateTemporaryValue); - } StorageModification::Write { .. } | StorageModification::Delete => {} } @@ -363,14 +318,12 @@ impl WriteLog { key: &storage::Key, value: Vec, ) -> Result<(u64, i64)> { - let len = value.len(); - let gas = key.len() + len; - let size_diff = match self + if let Some(prev) = self .tx_write_log .get(key) .or_else(|| self.tx_precommit_write_log.get(key)) { - Some(prev) => match prev { + match prev { StorageModification::Write { .. } => { // Cannot overwrite a write request with a temporary one return Err(Error::WriteTempAfterWrite); @@ -381,17 +334,19 @@ impl WriteLog { StorageModification::InitAccount { .. } => { return Err(Error::UpdateVpOfNewAccount); } - StorageModification::Temp { ref value } => { - len as i64 - value.len() as i64 - } - }, + } + } + + let len = value.len(); + let gas = key.len() + len; + let size_diff = match self.tx_temp_log.get(key) { + Some(prev) => len as i64 - prev.len() as i64, // set just the length of the value because we don't know if // the previous value exists on the storage None => len as i64, }; - self.tx_write_log - .insert(key.clone(), StorageModification::Temp { value }); + self.tx_temp_log.insert(key.clone(), value); // Temp writes are not propagated to db so just charge the cost of // accessing storage @@ -417,7 +372,6 @@ impl WriteLog { StorageModification::InitAccount { .. } => { return Err(Error::DeleteVp); } - StorageModification::Temp { ref value } => value.len() as i64, }, // set 0 because we don't know if the previous value exists on the // storage @@ -446,8 +400,7 @@ impl WriteLog { return Err(Error::DeleteVp); } StorageModification::Write { .. } - | StorageModification::Delete - | StorageModification::Temp { .. } => {} + | StorageModification::Delete => {} } }; Ok(()) @@ -491,13 +444,7 @@ impl WriteLog { pub fn get_keys(&self) -> BTreeSet { self.tx_write_log .iter() - .filter_map(|(key, modification)| match modification { - StorageModification::Write { .. } => Some(key.clone()), - StorageModification::Delete => Some(key.clone()), - StorageModification::InitAccount { .. } => Some(key.clone()), - // Skip temporary storage changes - they are never committed - StorageModification::Temp { .. } => None, - }) + .map(|(key, _modification)| key.clone()) .collect() } @@ -570,30 +517,29 @@ impl WriteLog { /// Commit the current transaction's write log and precommit log to the /// block when it's accepted by all the triggered validity predicates. - /// Starts a new transaction write log. + /// Starts a new transaction write log a clears the temp write log. pub fn commit_tx(&mut self) { // First precommit everything self.precommit_tx(); // Then commit to block - self.tx_precommit_write_log.retain(|_, v| { - !matches!(v, StorageModification::Temp { value: _ }) - }); let tx_precommit_write_log = std::mem::replace( &mut self.tx_precommit_write_log, HashMap::with_capacity(100), ); self.block_write_log.extend(tx_precommit_write_log); + self.tx_temp_log.clear(); self.take_ibc_events(); } /// Drop the current transaction's write log and IBC events and precommit /// when it's declined by any of the triggered validity predicates. - /// Starts a new transaction write log. + /// Starts a new transaction write log a clears the temp write log. pub fn drop_tx(&mut self) { self.tx_precommit_write_log.clear(); self.tx_write_log.clear(); + self.tx_temp_log.clear(); self.ibc_events.clear(); } @@ -1219,8 +1165,6 @@ pub mod testing { vp_code_hash: Hash(hash), } }), - any::>() - .prop_map(|value| StorageModification::Temp { value }), ] .boxed() } else { @@ -1228,8 +1172,6 @@ pub mod testing { any::>() .prop_map(|value| StorageModification::Write { value }), Just(StorageModification::Delete), - any::>() - .prop_map(|value| StorageModification::Temp { value }), ] .boxed() }