From a6c478acebde6447418807fbfc03513d10713e1a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 21 Jul 2023 12:53:13 +0200 Subject: [PATCH] Reviews gas multipliers --- core/src/ledger/gas.rs | 18 ++++---- core/src/ledger/storage/mod.rs | 61 +++++++++++++++++---------- core/src/ledger/storage/wl_storage.rs | 4 +- core/src/ledger/storage/write_log.rs | 43 ++++++++++++------- shared/src/ledger/vp_host_fns.rs | 8 ++-- shared/src/vm/host_env.rs | 15 +++---- shared/src/vm/wasm/memory.rs | 11 ++--- 7 files changed, 96 insertions(+), 64 deletions(-) diff --git a/core/src/ledger/gas.rs b/core/src/ledger/gas.rs index 38007b28cab..381ad56c511 100644 --- a/core/src/ledger/gas.rs +++ b/core/src/ledger/gas.rs @@ -21,14 +21,16 @@ const TX_SIZE_GAS_PER_BYTE: u64 = 10; const COMPILE_GAS_PER_BYTE: u64 = 1; const PARALLEL_GAS_DIVIDER: u64 = 10; -/// The minimum gas cost for accessing the storage -pub const MIN_STORAGE_GAS: u64 = 1; -/// The gas cost for verifying the signature of a transaction +/// The cost of accessing the storage, per byte +pub const STORAGE_ACCESS_GAS_PER_BYTE: u64 = 1; +/// The cost of writing to storage, per byte +pub const STORAGE_WRITE_GAS_PER_BYTE: u64 = 100; +/// The cost of verifying the signature of a transaction pub const VERIFY_TX_SIG_GAS_COST: u64 = 10; -/// The gas cost for validating wasm vp code +/// The cost of validating wasm vp code pub const WASM_VALIDATION_GAS_PER_BYTE: u64 = 1; -/// The cost for writing a byte to storage -pub const STORAGE_WRITE_GAS_PER_BYTE: u64 = 100; +/// The cost of accessing the WASM memory, per byte +pub const VM_MEMORY_ACCESS_GAS_PER_BYTE: u64 = 1; /// Gas module result for functions that may fail pub type Result = std::result::Result; @@ -57,11 +59,11 @@ pub trait GasMetering { fn add_wasm_load_from_storage_gas(&mut self, bytes_len: u64) -> Result<()> { tracing::error!( "Adding load from storage cost: {}", - bytes_len * MIN_STORAGE_GAS + bytes_len * STORAGE_ACCESS_GAS_PER_BYTE ); //FIXME: remove self.consume( bytes_len - .checked_mul(MIN_STORAGE_GAS) + .checked_mul(STORAGE_ACCESS_GAS_PER_BYTE) .ok_or(Error::GasOverflow)?, ) } diff --git a/core/src/ledger/storage/mod.rs b/core/src/ledger/storage/mod.rs index 409125d59c0..4ddaebba7fd 100644 --- a/core/src/ledger/storage/mod.rs +++ b/core/src/ledger/storage/mod.rs @@ -27,7 +27,9 @@ pub use wl_storage::{ #[cfg(feature = "wasm-runtime")] pub use self::masp_conversions::update_allowed_conversions; pub use self::masp_conversions::{encode_asset_type, ConversionState}; -use crate::ledger::gas::MIN_STORAGE_GAS; +use crate::ledger::gas::{ + STORAGE_ACCESS_GAS_PER_BYTE, STORAGE_WRITE_GAS_PER_BYTE, +}; use crate::ledger::parameters::{self, EpochDuration, Parameters}; use crate::ledger::storage::merkle_tree::{ Error as MerkleTreeError, MerkleRoot, @@ -547,7 +549,10 @@ where /// Check if the given key is present in storage. Returns the result and the /// gas cost. pub fn has_key(&self, key: &Key) -> Result<(bool, u64)> { - Ok((self.block.tree.has_key(key)?, key.len() as _)) + Ok(( + self.block.tree.has_key(key)?, + key.len() as u64 * STORAGE_ACCESS_GAS_PER_BYTE, + )) } /// Returns a value from the specified subspace and the gas cost @@ -560,10 +565,11 @@ where match self.db.read_subspace_val(key)? { Some(v) => { - let gas = key.len() + v.len(); - Ok((Some(v), gas as _)) + let gas = + (key.len() + v.len()) as u64 * STORAGE_ACCESS_GAS_PER_BYTE; + Ok((Some(v), gas)) } - None => Ok((None, key.len() as _)), + None => Ok((None, key.len() as u64 * STORAGE_ACCESS_GAS_PER_BYTE)), } } @@ -583,10 +589,13 @@ where self.get_last_block_height(), )? { Some(v) => { - let gas = key.len() + v.len(); - Ok((Some(v), gas as _)) + let gas = (key.len() + v.len()) as u64 + * STORAGE_ACCESS_GAS_PER_BYTE; + Ok((Some(v), gas)) + } + None => { + Ok((None, key.len() as u64 * STORAGE_ACCESS_GAS_PER_BYTE)) } - None => Ok((None, key.len() as _)), } } } @@ -602,7 +611,7 @@ where ) -> (>::PrefixIter, u64) { ( self.db.iter_optional_prefix(Some(prefix)), - prefix.len() as _, + prefix.len() as u64 * STORAGE_ACCESS_GAS_PER_BYTE, ) } @@ -625,10 +634,10 @@ where self.block.tree.update(key, value)?; let len = value.len(); - let gas = key.len() + len; + let gas = (key.len() + len) as u64 * STORAGE_WRITE_GAS_PER_BYTE; let size_diff = self.db.write_subspace_val(self.block.height, key, value)?; - Ok((gas as _, size_diff)) + Ok((gas, size_diff)) } /// Delete the specified subspace and returns the gas cost and the size @@ -642,8 +651,9 @@ where deleted_bytes_len = self.db.delete_subspace_val(self.block.height, key)?; } - let gas = key.len() + deleted_bytes_len as usize; - Ok((gas as _, deleted_bytes_len)) + let gas = (key.len() + deleted_bytes_len as usize) as u64 + * STORAGE_WRITE_GAS_PER_BYTE; + Ok((gas, deleted_bytes_len)) } /// Set the block header. @@ -696,17 +706,23 @@ where /// Get the chain ID as a raw string pub fn get_chain_id(&self) -> (String, u64) { - (self.chain_id.to_string(), CHAIN_ID_LENGTH as _) + ( + self.chain_id.to_string(), + CHAIN_ID_LENGTH as u64 * STORAGE_ACCESS_GAS_PER_BYTE, + ) } /// Get the block height pub fn get_block_height(&self) -> (BlockHeight, u64) { - (self.block.height, MIN_STORAGE_GAS) + (self.block.height, STORAGE_ACCESS_GAS_PER_BYTE) } /// Get the block hash pub fn get_block_hash(&self) -> (BlockHash, u64) { - (self.block.hash.clone(), BLOCK_HASH_LENGTH as _) + ( + self.block.hash.clone(), + BLOCK_HASH_LENGTH as u64 * STORAGE_ACCESS_GAS_PER_BYTE, + ) } /// Get the Merkle tree with stores and diffs in the DB @@ -829,12 +845,12 @@ where /// Get the current (yet to be committed) block epoch pub fn get_current_epoch(&self) -> (Epoch, u64) { - (self.block.epoch, MIN_STORAGE_GAS) + (self.block.epoch, STORAGE_ACCESS_GAS_PER_BYTE) } /// Get the epoch of the last committed block pub fn get_last_epoch(&self) -> (Epoch, u64) { - (self.last_epoch, MIN_STORAGE_GAS) + (self.last_epoch, STORAGE_ACCESS_GAS_PER_BYTE) } /// Initialize the first epoch. The first epoch begins at genesis time. @@ -860,16 +876,17 @@ where ) -> Result<(Option
, u64)> { match height { Some(h) if h == self.get_block_height().0 => { - Ok((self.header.clone(), MIN_STORAGE_GAS)) + Ok((self.header.clone(), STORAGE_ACCESS_GAS_PER_BYTE)) } Some(h) => match self.db.read_block_header(h)? { Some(header) => { - let gas = header.encoded_len() as u64; + let gas = header.encoded_len() as u64 + * STORAGE_ACCESS_GAS_PER_BYTE; Ok((Some(header), gas)) } - None => Ok((None, MIN_STORAGE_GAS)), + None => Ok((None, STORAGE_ACCESS_GAS_PER_BYTE)), }, - None => Ok((self.header.clone(), MIN_STORAGE_GAS)), + None => Ok((self.header.clone(), STORAGE_ACCESS_GAS_PER_BYTE)), } } diff --git a/core/src/ledger/storage/wl_storage.rs b/core/src/ledger/storage/wl_storage.rs index f9f60335595..99ab366812e 100644 --- a/core/src/ledger/storage/wl_storage.rs +++ b/core/src/ledger/storage/wl_storage.rs @@ -283,7 +283,7 @@ where storage_iter, write_log_iter, }, - gas::MIN_STORAGE_GAS, + gas::STORAGE_ACCESS_GAS_PER_BYTE, ) } @@ -308,7 +308,7 @@ where storage_iter, write_log_iter, }, - gas::MIN_STORAGE_GAS, + gas::STORAGE_ACCESS_GAS_PER_BYTE, ) } diff --git a/core/src/ledger/storage/write_log.rs b/core/src/ledger/storage/write_log.rs index 250ac226228..9963be7cf1b 100644 --- a/core/src/ledger/storage/write_log.rs +++ b/core/src/ledger/storage/write_log.rs @@ -7,7 +7,9 @@ use itertools::Itertools; use thiserror::Error; use crate::ledger; -use crate::ledger::gas::STORAGE_WRITE_GAS_PER_BYTE; +use crate::ledger::gas::{ + STORAGE_ACCESS_GAS_PER_BYTE, STORAGE_WRITE_GAS_PER_BYTE, +}; use crate::ledger::storage::{Storage, StorageHasher}; use crate::types::address::{Address, EstablishedAddressGen}; use crate::types::hash::Hash; @@ -136,9 +138,9 @@ impl WriteLog { key.len() + value.len() } }; - (Some(v), gas as _) + (Some(v), gas as u64 * STORAGE_ACCESS_GAS_PER_BYTE) } - None => (None, key.len() as _), + None => (None, key.len() as u64 * STORAGE_ACCESS_GAS_PER_BYTE), } } @@ -164,9 +166,9 @@ impl WriteLog { key.len() + value.len() } }; - (Some(v), gas as _) + (Some(v), gas as u64 * STORAGE_ACCESS_GAS_PER_BYTE) } - None => (None, key.len() as _), + None => (None, key.len() as u64 * STORAGE_ACCESS_GAS_PER_BYTE), } } @@ -267,7 +269,8 @@ impl WriteLog { // the previous value exists on the storage None => len as i64, }; - Ok((gas as _, size_diff)) + // Temp writes are not propagated to db so just charge the cost of accessing storage + Ok((gas as u64 * STORAGE_ACCESS_GAS_PER_BYTE, size_diff)) } /// Delete a key and its value, and return the gas cost and the size @@ -295,7 +298,7 @@ impl WriteLog { None => 0, }; let gas = key.len() + size_diff as usize; - Ok((gas as _, -size_diff)) + Ok((gas as u64 * STORAGE_WRITE_GAS_PER_BYTE, -size_diff)) } /// Delete a key and its value. @@ -334,7 +337,8 @@ impl WriteLog { let addr = address_gen.generate_address("TODO more randomness".as_bytes()); let key = storage::Key::validity_predicate(&addr); - let gas = (key.len() + vp_code_hash.len()) as _; + let gas = (key.len() + vp_code_hash.len()) as u64 + * STORAGE_WRITE_GAS_PER_BYTE; self.tx_write_log .insert(key, StorageModification::InitAccount { vp_code_hash }); (addr, gas) @@ -347,7 +351,7 @@ impl WriteLog { .iter() .fold(0, |acc, (k, v)| acc + k.len() + v.len()); self.ibc_events.insert(event); - len as _ + len as u64 * STORAGE_ACCESS_GAS_PER_BYTE } /// Get the storage keys changed and accounts keys initialized in the @@ -584,7 +588,7 @@ mod tests { // delete a non-existing key let (gas, diff) = write_log.delete(&key).unwrap(); - assert_eq!(gas, key.len() as u64); + assert_eq!(gas, key.len() as u64 * STORAGE_WRITE_GAS_PER_BYTE); assert_eq!(diff, 0); // insert a value @@ -617,12 +621,15 @@ mod tests { // delete the key let (gas, diff) = write_log.delete(&key).unwrap(); - assert_eq!(gas, (key.len() + updated.len()) as u64); + assert_eq!( + gas, + (key.len() + updated.len()) as u64 * STORAGE_WRITE_GAS_PER_BYTE + ); assert_eq!(diff, -(updated.len() as i64)); // delete the deleted key again let (gas, diff) = write_log.delete(&key).unwrap(); - assert_eq!(gas, key.len() as u64); + assert_eq!(gas, key.len() as u64 * STORAGE_WRITE_GAS_PER_BYTE); assert_eq!(diff, 0); // read the deleted key @@ -631,7 +638,7 @@ mod tests { StorageModification::Delete => {} _ => panic!("unexpected result"), } - assert_eq!(gas, key.len() as u64); + assert_eq!(gas, key.len() as u64 * STORAGE_ACCESS_GAS_PER_BYTE); // insert again let reinserted = "reinserted".as_bytes().to_vec(); @@ -653,7 +660,10 @@ mod tests { let vp_hash = Hash::sha256(init_vp); let (addr, gas) = write_log.init_account(&address_gen, vp_hash); let vp_key = storage::Key::validity_predicate(&addr); - assert_eq!(gas, (vp_key.len() + vp_hash.len()) as u64); + assert_eq!( + gas, + (vp_key.len() + vp_hash.len()) as u64 * STORAGE_WRITE_GAS_PER_BYTE + ); // read let (value, gas) = write_log.read(&vp_key); @@ -663,7 +673,10 @@ mod tests { } _ => panic!("unexpected result"), } - assert_eq!(gas, (vp_key.len() + vp_hash.len()) as u64); + assert_eq!( + gas, + (vp_key.len() + vp_hash.len()) as u64 * STORAGE_ACCESS_GAS_PER_BYTE + ); // get all let (_changed_keys, init_accounts) = write_log.get_partitioned_keys(); diff --git a/shared/src/ledger/vp_host_fns.rs b/shared/src/ledger/vp_host_fns.rs index cf26ca9c1e3..71072930a82 100644 --- a/shared/src/ledger/vp_host_fns.rs +++ b/shared/src/ledger/vp_host_fns.rs @@ -11,7 +11,7 @@ use namada_core::types::storage::{ }; use thiserror::Error; -use super::gas::MIN_STORAGE_GAS; +use super::gas::STORAGE_ACCESS_GAS_PER_BYTE; use crate::ledger::gas; use crate::ledger::gas::{GasMetering, VpGasMeter}; use crate::ledger::storage::write_log::WriteLog; @@ -293,7 +293,7 @@ pub fn get_tx_code_hash( .get_section(tx.code_sechash()) .and_then(Section::code_sec) .map(|x| x.code.hash()); - add_gas(gas_meter, MIN_STORAGE_GAS)?; + add_gas(gas_meter, STORAGE_ACCESS_GAS_PER_BYTE)?; Ok(hash) } @@ -318,7 +318,7 @@ pub fn get_tx_index( gas_meter: &mut VpGasMeter, tx_index: &TxIndex, ) -> EnvResult { - add_gas(gas_meter, MIN_STORAGE_GAS)?; + add_gas(gas_meter, STORAGE_ACCESS_GAS_PER_BYTE)?; Ok(*tx_index) } @@ -331,7 +331,7 @@ where DB: storage::DB + for<'iter> storage::DBIter<'iter>, H: StorageHasher, { - add_gas(gas_meter, MIN_STORAGE_GAS)?; + add_gas(gas_meter, STORAGE_ACCESS_GAS_PER_BYTE)?; Ok(storage.native_token.clone()) } diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index cebc3411288..b97ff78817d 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -2,6 +2,7 @@ //! within a virtual machine. use std::collections::BTreeSet; use std::convert::TryInto; +use std::f64::MIN; use std::num::TryFromIntError; use borsh::{BorshDeserialize, BorshSerialize}; @@ -15,7 +16,7 @@ use super::wasm::TxCache; use super::wasm::VpCache; use super::WasmCacheAccess; use crate::ledger::gas::{ - self, VpGasMeter, MIN_STORAGE_GAS, WASM_VALIDATION_GAS_PER_BYTE, + self, VpGasMeter, STORAGE_ACCESS_GAS_PER_BYTE, WASM_VALIDATION_GAS_PER_BYTE, }; use crate::ledger::storage::write_log::{self, WriteLog}; use crate::ledger::storage::{self, Storage, StorageHasher}; @@ -851,7 +852,6 @@ where .write(&key, value) .map_err(TxRuntimeError::StorageModificationError)?; tx_add_gas(env, gas) - // TODO: charge the size diff } /// Temporary storage write function exposed to the wasm VM Tx environment. The @@ -892,7 +892,6 @@ where .write_temp(&key, value) .map_err(TxRuntimeError::StorageModificationError)?; tx_add_gas(env, gas) - // TODO: charge the size diff } fn check_address_existence( @@ -968,7 +967,6 @@ where .delete(&key) .map_err(TxRuntimeError::StorageModificationError)?; tx_add_gas(env, gas) - // TODO: charge the size diff } /// Emitting an IBC event function exposed to the wasm VM Tx environment. @@ -1368,7 +1366,8 @@ where let verifiers = unsafe { env.ctx.verifiers.get() }; verifiers.insert(addr); - tx_add_gas(env, addr_len) + // This is not a storage write, use the same multiplier used for a storage read + tx_add_gas(env, addr_len * STORAGE_ACCESS_GAS_PER_BYTE) } /// Update a validity predicate function exposed to the wasm VM Tx environment @@ -1408,7 +1407,6 @@ where .write(&key, code_hash) .map_err(TxRuntimeError::StorageModificationError)?; tx_add_gas(env, gas) - // TODO: charge the size diff } /// Initialize a new account established address. @@ -1501,7 +1499,7 @@ where CA: WasmCacheAccess, { let tx_index = unsafe { env.ctx.tx_index.get() }; - tx_add_gas(env, crate::vm::host_env::gas::MIN_STORAGE_GAS)?; + tx_add_gas(env, crate::vm::host_env::gas::STORAGE_ACCESS_GAS_PER_BYTE)?; Ok(tx_index.0) } @@ -1576,7 +1574,7 @@ where CA: WasmCacheAccess, { let storage = unsafe { env.ctx.storage.get() }; - tx_add_gas(env, MIN_STORAGE_GAS)?; + tx_add_gas(env, STORAGE_ACCESS_GAS_PER_BYTE)?; let native_token = storage.native_token.clone(); let native_token_string = native_token.encode(); let gas = env @@ -1792,6 +1790,7 @@ where .map_err(vp_host_fns::RuntimeError::EncodingError)?; Ok( + // TODO: once the runtime gas meter is implemented we need to benchmark this funcion and charge the gas here. For the moment, the cost of this is included in the benchmark of the masp vp HostEnvResult::from(crate::ledger::masp::verify_shielded_tx(&shielded)) .to_i64(), ) diff --git a/shared/src/vm/wasm/memory.rs b/shared/src/vm/wasm/memory.rs index 7e79ab1be03..3e0c7975c9f 100644 --- a/shared/src/vm/wasm/memory.rs +++ b/shared/src/vm/wasm/memory.rs @@ -6,6 +6,7 @@ use std::str::Utf8Error; use std::sync::Arc; use borsh::BorshSerialize; +use namada_core::ledger::gas::VM_MEMORY_ACCESS_GAS_PER_BYTE; use thiserror::Error; use wasmer::{ vm, BaseTunables, HostEnvInitError, LazyInit, Memory, MemoryError, @@ -257,16 +258,16 @@ impl VmMemory for WasmMemory { fn read_bytes(&self, offset: u64, len: usize) -> Result<(Vec, u64)> { let memory = self.inner.get_ref().ok_or(Error::UninitializedMemory)?; let bytes = read_memory_bytes(memory, offset, len)?; - let gas = bytes.len(); - Ok((bytes, gas as _)) + let gas = bytes.len() as u64 * VM_MEMORY_ACCESS_GAS_PER_BYTE; + Ok((bytes, gas)) } /// Write bytes into memory at the given offset and return the gas cost fn write_bytes(&self, offset: u64, bytes: impl AsRef<[u8]>) -> Result { - let gas = bytes.as_ref().len(); + let gas = bytes.as_ref().len() as u64 * VM_MEMORY_ACCESS_GAS_PER_BYTE; let memory = self.inner.get_ref().ok_or(Error::UninitializedMemory)?; write_memory_bytes(memory, offset, bytes)?; - Ok(gas as _) + Ok(gas) } /// Read string from memory at the given offset and bytes length, and return @@ -276,7 +277,7 @@ impl VmMemory for WasmMemory { let string = std::str::from_utf8(&bytes) .map_err(Error::InvalidUtf8String)? .to_string(); - Ok((string, gas as _)) + Ok((string, gas)) } /// Write string into memory at the given offset and return the gas cost