Fix temporary values read

.changelog/unreleased/bug-fixes/2735-fix-vps-api.md

@@ -0,0 +1,2 @@
+- Fixed the `StorageRead` implementation and vp host functions to ignore
+  temporary writes. ([\#2735](https://github.com/anoma/namada/pull/2735))

crates/namada/src/ledger/vp_host_fns.rs

@@ -37,10 +37,6 @@
     NumConversionError(TryFromIntError),
     #[error("Memory error: {0}")]
     MemoryError(Box<dyn std::error::Error + Sync + Send + 'static>),
-    #[error("Trying to read a temporary value with read_post")]
-    ReadTemporaryValueError,
-    #[error("Trying to read a permanent value with read_temp")]
-    ReadPermanentValueError,
     #[error("Invalid transaction code hash")]
     InvalidCodeHash,
     #[error("No value found in result buffer")]
@@ -90,11 +86,9 @@
             // Read the VP of a new account
             Ok(Some(vp_code_hash.to_vec()))
         }
-        Some(&write_log::StorageModification::Temp { .. }) => {
-            Err(RuntimeError::ReadTemporaryValueError)
-        }
-        None => {
-            // When not found in write log, try to read from the storage
+        Some(&write_log::StorageModification::Temp { .. }) | None => {
+            // When not found in write log or only found a temporary value, try
+            // to read from the storage
             let (value, gas) =
                 state.db_read(key).map_err(RuntimeError::StorageError)?;
             add_gas(gas_meter, gas, sentinel)?;
@@ -115,27 +109,25 @@
     S: StateRead + Debug,
 {
     // Try to read from the write log first
-    let (log_val, gas) = state.write_log().read(key);
+    let (log_val, gas) = state.write_log().read_persistent(key);
     add_gas(gas_meter, gas, sentinel)?;
     match log_val {
-        Some(write_log::StorageModification::Write { ref value }) => {
+        Some(write_log::PersistentStorageModification::Write { value }) => {
             Ok(Some(value.clone()))
         }
-        Some(&write_log::StorageModification::Delete) => {
+        Some(write_log::PersistentStorageModification::Delete) => {
             // Given key has been deleted
             Ok(None)
         }
-        Some(write_log::StorageModification::InitAccount {
-            ref vp_code_hash,
+        Some(write_log::PersistentStorageModification::InitAccount {
+            vp_code_hash,
         }) => {
             // Read the VP code hash of a new account
             Ok(Some(vp_code_hash.to_vec()))
         }
-        Some(&write_log::StorageModification::Temp { .. }) => {
-            Err(RuntimeError::ReadTemporaryValueError)
-        }
         None => {
-            // When not found in write log, try to read from the storage
+            // 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, sentinel)?;
@@ -155,15 +147,13 @@
 where
     S: StateRead + Debug,
 {
-    // Try to read from the write log first
     let (log_val, gas) = state.write_log().read(key);
     add_gas(gas_meter, gas, sentinel)?;
     match log_val {
         Some(write_log::StorageModification::Temp { ref value }) => {
             Ok(Some(value.clone()))
         }
-        None => Ok(None),
-        _ => Err(RuntimeError::ReadPermanentValueError),
+        _ => Ok(None),
     }
 }
 
@@ -188,9 +178,9 @@
             Ok(false)
         }
         Some(&write_log::StorageModification::InitAccount { .. }) => Ok(true),
-        Some(&write_log::StorageModification::Temp { .. }) => Ok(true),
-        None => {
-            // When not found in write log, try to check the storage
+        Some(&write_log::StorageModification::Temp { .. }) | None => {
+            // When not found in write log or only found a temporary value, try
+            // to check the storage
             let (present, gas) =
                 state.db_has_key(key).map_err(RuntimeError::StorageError)?;
             add_gas(gas_meter, gas, sentinel)?;
@@ -211,18 +201,22 @@
     S: StateRead + Debug,
 {
     // Try to read from the write log first
-    let (log_val, gas) = state.write_log().read(key);
+    let (log_val, gas) = state.write_log().read_persistent(key);
     add_gas(gas_meter, gas, sentinel)?;
     match log_val {
-        Some(&write_log::StorageModification::Write { .. }) => Ok(true),
-        Some(&write_log::StorageModification::Delete) => {
+        Some(write_log::PersistentStorageModification::Write { .. }) => {
+            Ok(true)
+        }
+        Some(write_log::PersistentStorageModification::Delete) => {
             // The given key has been deleted
             Ok(false)
         }
-        Some(&write_log::StorageModification::InitAccount { .. }) => Ok(true),
-        Some(&write_log::StorageModification::Temp { .. }) => Ok(true),
+        Some(write_log::PersistentStorageModification::InitAccount {
+            ..
+        }) => Ok(true),
         None => {
-            // When not found in write log, try to check the storage
+            // 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, sentinel)?;

crates/namada/src/vm/host_env.rs

@@ -796,19 +796,9 @@
                 // a VP of a new account doesn't need to be iterated
                 continue;
             }
-            Some(write_log::StorageModification::Temp { ref value }) => {
-                let key_val = borsh::to_vec(&KeyVal {
-                    key,
-                    val: value.clone(),
-                })
-                .map_err(TxRuntimeError::EncodingError)?;
-                let len: i64 = key_val
-                    .len()
-                    .try_into()
-                    .map_err(TxRuntimeError::NumConversionError)?;
-                let result_buffer = unsafe { env.ctx.result_buffer.get() };
-                result_buffer.replace(key_val);
-                return Ok(len);
+            Some(write_log::StorageModification::Temp { .. }) => {
+                // temporary values are not returned by the iterator
+                continue;
             }
             None => {
                 let key_val = borsh::to_vec(&KeyVal { key, val })
@@ -938,25 +928,16 @@
             continue;
         }
         let vp_key = Key::validity_predicate(&addr);
-        let (vp, gas) = state.write_log().read(&vp_key);
-        tx_charge_gas::<MEM, D, H, CA>(env, gas)?;
-        // just check the existence because the write log should not have the
-        // delete log of the VP
-        if vp.is_none() {
-            let (is_present, gas) = state
-                .db_has_key(&vp_key)
-                .map_err(TxRuntimeError::StateError)?;
-            tx_charge_gas::<MEM, D, H, CA>(env, gas)?;
-            if !is_present {
-                tracing::info!(
-                    "Trying to write into storage with a key containing an \
-                     address that doesn't exist: {}",
-                    addr
-                );
-                return Err(TxRuntimeError::UnknownAddressStorageModification(
-                    addr,
-                ));
-            }
+        let is_present = state.has_key(&vp_key)?;
+        if !is_present {
+            tracing::info!(
+                "Trying to write into storage with a key containing an \
+                 address that doesn't exist: {}",
+                addr
+            );
+            return Err(TxRuntimeError::UnknownAddressStorageModification(
+                addr,
+            ));
         }
     }
     Ok(())
@@ -2120,18 +2101,11 @@
 
     // Then check that the corresponding VP code does indeed exist
     let code_key = Key::wasm_code(&code_hash);
-    let (result, gas) = state.write_log().read(&code_key);
-    tx_charge_gas::<MEM, D, H, CA>(env, gas)?;
-    if result.is_none() {
-        let (is_present, gas) = state
-            .db_has_key(&code_key)
-            .map_err(TxRuntimeError::StateError)?;
-        tx_charge_gas::<MEM, D, H, CA>(env, gas)?;
-        if !is_present {
-            return Err(TxRuntimeError::InvalidVpCodeHash(
-                "The corresponding VP code doesn't exist".to_string(),
-            ));
-        }
+    let is_present = state.has_key(&code_key)?;
+    if !is_present {
+        return Err(TxRuntimeError::InvalidVpCodeHash(
+            "The corresponding VP code doesn't exist".to_string(),
+        ));
     }
     Ok(())
 }

crates/namada/src/vm/wasm/run.rs

@@ -8,7 +8,6 @@
 use borsh::BorshDeserialize;
 use namada_core::validity_predicate::VpSentinel;
 use namada_gas::{GasMetering, TxGasMeter, WASM_MEMORY_PAGE_GAS};
-use namada_state::write_log::StorageModification;
 use namada_state::{DBIter, State, StateRead, StorageHasher, DB};
 use namada_tx::data::TxSentinel;
 use namada_tx::{Commitment, Section, Tx};
@@ -507,61 +506,42 @@
                 Some((module, store)) => {
                     // Gas accounting even if the compiled module is in cache
                     let key = Key::wasm_code_len(code_hash);
-                    let tx_len = match state.write_log().read(&key).0 {
-                        Some(StorageModification::Write { value }) => {
-                            u64::try_from_slice(value).map_err(|e| {
-                                Error::ConversionError(e.to_string())
-                            })
-                        }
-                        _ => match state
-                            .db_read(&key)
-                            .map_err(|e| {
-                                Error::LoadWasmCode(format!(
-                                    "Read wasm code length failed from \
-                                     storage: key {}, error {}",
-                                    key, e
-                                ))
-                            })?
-                            .0
-                        {
-                            Some(v) => u64::try_from_slice(&v).map_err(|e| {
-                                Error::ConversionError(e.to_string())
-                            }),
-                            None => Err(Error::LoadWasmCode(format!(
+                    let tx_len = state
+                        .read::<u64>(&key)
+                        .map_err(|e| {
+                            Error::LoadWasmCode(format!(
+                                "Read wasm code length failed from storage: \
+                                 key {}, error {}",
+                                key, e
+                            ))
+                        })?
+                        .ok_or_else(|| {
+                            Error::LoadWasmCode(format!(
                                 "No wasm code length in storage: key {}",
                                 key
-                            ))),
-                        },
-                    }?;
+                            ))
+                        })?;
 
                     (module, store, tx_len)
                 }
                 None => {
                     let key = Key::wasm_code(code_hash);
-                    let code = match state.write_log().read(&key).0 {
-                        Some(StorageModification::Write { value }) => {
-                            value.clone()
-                        }
-                        _ => match state
-                            .db_read(&key)
-                            .map_err(|e| {
-                                Error::LoadWasmCode(format!(
-                                    "Read wasm code failed from storage: key \
-                                     {}, error {}",
-                                    key, e
-                                ))
-                            })?
-                            .0
-                        {
-                            Some(v) => v,
-                            None => {
-                                return Err(Error::LoadWasmCode(format!(
-                                    "No wasm code in storage: key {}",
-                                    key
-                                )));
-                            }
-                        },
-                    };
+                    let code = state
+                        .read_bytes(&key)
+                        .map_err(|e| {
+                            Error::LoadWasmCode(format!(
+                                "Read wasm code failed from storage: key {}, \
+                                 error {}",
+                                key, e
+                            ))
+                        })?
+                        .ok_or_else(|| {
+                            Error::LoadWasmCode(format!(
+                                "No wasm code in storage: key {}",
+                                key
+                            ))
+                        })?;
+
                     let tx_len = u64::try_from(code.len())
                         .map_err(|e| Error::ConversionError(e.to_string()))?;
 

crates/state/src/lib.rs

@@ -197,21 +197,18 @@ macro_rules! impl_storage_read {
                 key: &storage::Key,
             ) -> namada_storage::Result<Option<Vec<u8>>> {
                 // try to read from the write log first
-                let (log_val, gas) = self.write_log().read(key);
+                let (log_val, gas) = self.write_log().read_persistent(key);
                 self.charge_gas(gas).into_storage_result()?;
                 match log_val {
-                    Some(write_log::StorageModification::Write { ref value }) => {
+                    Some(write_log::PersistentStorageModification::Write { value }) => {
                         Ok(Some(value.clone()))
                     }
-                    Some(write_log::StorageModification::Delete) => Ok(None),
-                    Some(write_log::StorageModification::InitAccount {
+                    Some(write_log::PersistentStorageModification::Delete) => Ok(None),
+                    Some(write_log::PersistentStorageModification::InitAccount {
                         ref vp_code_hash,
                     }) => Ok(Some(vp_code_hash.to_vec())),
-                    Some(write_log::StorageModification::Temp { ref value }) => {
-                        Ok(Some(value.clone()))
-                    }
                     None => {
-                        // when not found in write log, try to read from the storage
+                        // when not found in write log try to read from the storage
                         let (value, gas) = self.db_read(key).into_storage_result()?;
                         self.charge_gas(gas).into_storage_result()?;
                         Ok(value)
@@ -225,14 +222,13 @@ macro_rules! impl_storage_read {
                 self.charge_gas(gas).into_storage_result()?;
                 match log_val {
                     Some(&write_log::StorageModification::Write { .. })
-                    | Some(&write_log::StorageModification::InitAccount { .. })
-                    | Some(&write_log::StorageModification::Temp { .. }) => Ok(true),
+                    | Some(&write_log::StorageModification::InitAccount { .. }) => Ok(true),
                     Some(&write_log::StorageModification::Delete) => {
                         // the given key has been deleted
                         Ok(false)
                     }
-                    None => {
-                        // when not found in write log, try to check the storage
+                    Some(&write_log::StorageModification::Temp { .. }) | None => {
+                        // when not found in write log or only found a temporary value, 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)
@@ -551,8 +547,7 @@ where
                         self.write_log_iter.next()
                     {
                         match modification {
-                            write_log::StorageModification::Write { value }
-                            | write_log::StorageModification::Temp { value } => {
+                            write_log::StorageModification::Write { value } => {
                                 let gas = value.len() as u64;
                                 return Some((key, value, gas));
                             }
@@ -562,7 +557,8 @@ 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::Delete
+                            | write_log::StorageModification::Temp { .. } => {
                                 continue;
                             }
                         }