Skip to content

Commit

Permalink
Reviews gas multipliers
Browse files Browse the repository at this point in the history
  • Loading branch information
grarco committed Jul 21, 2023
1 parent 2261975 commit a6c478a
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 64 deletions.
18 changes: 10 additions & 8 deletions core/src/ledger/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> = std::result::Result<T, Error>;
Expand Down Expand Up @@ -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)?,
)
}
Expand Down
61 changes: 39 additions & 22 deletions core/src/ledger/storage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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)),
}
}

Expand All @@ -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 _)),
}
}
}
Expand All @@ -602,7 +611,7 @@ where
) -> (<D as DBIter<'_>>::PrefixIter, u64) {
(
self.db.iter_optional_prefix(Some(prefix)),
prefix.len() as _,
prefix.len() as u64 * STORAGE_ACCESS_GAS_PER_BYTE,
)
}

Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -860,16 +876,17 @@ where
) -> Result<(Option<Header>, 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)),
}
}

Expand Down
4 changes: 2 additions & 2 deletions core/src/ledger/storage/wl_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ where
storage_iter,
write_log_iter,
},
gas::MIN_STORAGE_GAS,
gas::STORAGE_ACCESS_GAS_PER_BYTE,
)
}

Expand All @@ -308,7 +308,7 @@ where
storage_iter,
write_log_iter,
},
gas::MIN_STORAGE_GAS,
gas::STORAGE_ACCESS_GAS_PER_BYTE,
)
}

Expand Down
43 changes: 28 additions & 15 deletions core/src/ledger/storage/write_log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
}
}

Expand All @@ -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),
}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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();
Expand Down
8 changes: 4 additions & 4 deletions shared/src/ledger/vp_host_fns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
}

Expand All @@ -318,7 +318,7 @@ pub fn get_tx_index(
gas_meter: &mut VpGasMeter,
tx_index: &TxIndex,
) -> EnvResult<TxIndex> {
add_gas(gas_meter, MIN_STORAGE_GAS)?;
add_gas(gas_meter, STORAGE_ACCESS_GAS_PER_BYTE)?;
Ok(*tx_index)
}

Expand All @@ -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())
}

Expand Down
Loading

0 comments on commit a6c478a

Please sign in to comment.