Skip to content

Commit

Permalink
Fix estimate_gas result differs widely from the used_gas (polkado…
Browse files Browse the repository at this point in the history 
…t-evm#1239)

* Fix add `size_limit` to the proof_size of weight info

* Fix issues in the record dynamic opcode

* Debug test case

* Debug test case

* Fix test cases

* Fix clippy

* Sload cache item
  • Loading branch information
@boundless-forest
boundless-forest committed Nov 10, 2023
1 parent 9e9c31b commit 176cb34
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 133 deletions.
230 changes: 97 additions & 133 deletions frame/evm/src/runner/stack.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use evm::{
backend::Backend as BackendT,
executor::stack::{Accessed, StackExecutor, StackState as StackStateT, StackSubstateMetadata},
gasometer::{GasCost, StorageTarget},
ExitError, ExitReason, Opcode, Transfer,
ExitError, ExitReason, ExternalOperation, Opcode, Transfer,
};
// Substrate
use frame_support::{
Expand Down Expand Up @@ -959,47 +959,49 @@ where
.config()
.create_contract_limit
.unwrap_or_default() as u64;

let (weight_info, recorded) = self.info_mut();

if let Some(weight_info) = weight_info {
match op {
evm::ExternalOperation::AccountBasicRead => {
ExternalOperation::AccountBasicRead => {
weight_info.try_record_proof_size_or_fail(ACCOUNT_BASIC_PROOF_SIZE)?
}
evm::ExternalOperation::AddressCodeRead(address) => {
ExternalOperation::AddressCodeRead(address) => {
let maybe_record = !recorded.account_codes.contains(&address);
// Skip if the address has been already recorded this block
if maybe_record {
// First we record account emptiness check.
// Transfers to EOAs with standard 21_000 gas limit are able to
// pay for this pov size.
weight_info.try_record_proof_size_or_fail(IS_EMPTY_CHECK_PROOF_SIZE)?;

if <AccountCodes<T>>::decode_len(address).unwrap_or(0) == 0 {
return Ok(());
}
// Try to record fixed sized `AccountCodesMetadata` read
// Tentatively 16 + 20 + 40

weight_info
.try_record_proof_size_or_fail(ACCOUNT_CODES_METADATA_PROOF_SIZE)?;
if let Some(meta) = <AccountCodesMetadata<T>>::get(address) {
weight_info.try_record_proof_size_or_fail(meta.size)?;
} else {
// If it does not exist, try to record `create_contract_limit` first.
weight_info.try_record_proof_size_or_fail(size_limit)?;
let meta = Pallet::<T>::account_code_metadata(address);
let actual_size = meta.size;
// Refund if applies
weight_info.refund_proof_size(size_limit.saturating_sub(actual_size));
} else if let Some(remaining_proof_size) =
weight_info.remaining_proof_size()
{
let pre_size = remaining_proof_size.min(size_limit);
weight_info.try_record_proof_size_or_fail(pre_size)?;

let actual_size = Pallet::<T>::account_code_metadata(address).size;
if actual_size > pre_size {
return Err(ExitError::OutOfGas);
}
// Refund unused proof size
weight_info.refund_proof_size(pre_size.saturating_sub(actual_size));
}
recorded.account_codes.push(address);
}
}
evm::ExternalOperation::IsEmpty => {
ExternalOperation::IsEmpty => {
weight_info.try_record_proof_size_or_fail(IS_EMPTY_CHECK_PROOF_SIZE)?
}
evm::ExternalOperation::Write => {
ExternalOperation::Write => {
weight_info.try_record_proof_size_or_fail(WRITE_PROOF_SIZE)?
}
};
Expand All @@ -1014,7 +1016,7 @@ where
target: evm::gasometer::StorageTarget,
) -> Result<(), ExitError> {
// If account code or storage slot is in the overlay it is already accounted for and early exit
let mut accessed_storage: Option<AccessedStorage> = match target {
let accessed_storage: Option<AccessedStorage> = match target {
StorageTarget::Address(address) => {
if self.recorded().account_codes.contains(&address) {
return Ok(());
Expand Down Expand Up @@ -1042,132 +1044,95 @@ where
.config()
.create_contract_limit
.unwrap_or_default() as u64;
let (weight_info, recorded) = self.info_mut();

let (weight_info, recorded) = {
let (weight_info, recorded) = self.info_mut();
if let Some(weight_info) = weight_info {
(weight_info, recorded)
} else {
if let Some(weight_info) = weight_info {
// proof_size_limit is None indicates no need to record proof size, return directly.
if weight_info.proof_size_limit.is_none() {
return Ok(());
}
};
};

// Record ref_time first
// TODO benchmark opcodes, until this is done we do used_gas to weight conversion for ref_time
let mut record_account_codes_proof_size =
|address: H160, empty_check: bool| -> Result<(), ExitError> {
let mut base_size = ACCOUNT_CODES_METADATA_PROOF_SIZE;
if empty_check {
base_size = base_size.saturating_add(IS_EMPTY_CHECK_PROOF_SIZE);
}
weight_info.try_record_proof_size_or_fail(base_size)?;

// Record proof_size
// Return if proof size recording is disabled
let proof_size_limit = if let Some(proof_size_limit) = weight_info.proof_size_limit {
proof_size_limit
} else {
return Ok(());
};
if let Some(meta) = <AccountCodesMetadata<T>>::get(address) {
weight_info.try_record_proof_size_or_fail(meta.size)?;
} else if let Some(remaining_proof_size) = weight_info.remaining_proof_size() {
let pre_size = remaining_proof_size.min(size_limit);
weight_info.try_record_proof_size_or_fail(pre_size)?;

let mut maybe_record_and_refund = |with_empty_check: bool| -> Result<(), ExitError> {
let address = if let Some(AccessedStorage::AccountCodes(address)) = accessed_storage {
address
} else {
// This must be unreachable, a valid target must be set.
// TODO decide how do we want to gracefully handle.
return Err(ExitError::OutOfGas);
};
// First try to record fixed sized `AccountCodesMetadata` read
// Tentatively 20 + 8 + 32
let mut base_cost = ACCOUNT_CODES_METADATA_PROOF_SIZE;
if with_empty_check {
base_cost = base_cost.saturating_add(IS_EMPTY_CHECK_PROOF_SIZE);
}
weight_info.try_record_proof_size_or_fail(base_cost)?;
if let Some(meta) = <AccountCodesMetadata<T>>::get(address) {
weight_info.try_record_proof_size_or_fail(meta.size)?;
} else {
// If it does not exist, try to record `create_contract_limit` first.
weight_info.try_record_proof_size_or_fail(size_limit)?;
let meta = Pallet::<T>::account_code_metadata(address);
let actual_size = meta.size;
// Refund if applies
weight_info.refund_proof_size(size_limit.saturating_sub(actual_size));
}
recorded.account_codes.push(address);
// Already recorded, return
Ok(())
};
let actual_size = Pallet::<T>::account_code_metadata(address).size;
if actual_size > pre_size {
return Err(ExitError::OutOfGas);
}
// Refund unused proof size
weight_info.refund_proof_size(pre_size.saturating_sub(actual_size));
}

// Proof size is fixed length for writes (a 32-byte hash in a merkle trie), and
// the full key/value for reads. For read and writes over the same storage, the full value
// is included.
// For cold reads involving code (call, callcode, staticcall and delegatecall):
// - We depend on https://github.com/paritytech/frontier/pull/893
// - Try to get the cached size or compute it on the fly
// - We record the actual size after caching, refunding the difference between it and the initially deducted
// contract size limit.
let opcode_proof_size = match opcode {
// Basic account fixed length
Opcode::BALANCE => {
accessed_storage = None;
U256::from(ACCOUNT_BASIC_PROOF_SIZE)
}
Opcode::EXTCODESIZE | Opcode::EXTCODECOPY | Opcode::EXTCODEHASH => {
return maybe_record_and_refund(false)
}
Opcode::CALLCODE | Opcode::CALL | Opcode::DELEGATECALL | Opcode::STATICCALL => {
return maybe_record_and_refund(true)
}
// (H160, H256) double map blake2 128 concat key size (68) + value 32
Opcode::SLOAD => U256::from(ACCOUNT_STORAGE_PROOF_SIZE),
Opcode::SSTORE => {
let (address, index) =
Ok(())
};

// Proof size is fixed length for writes (a 32-byte hash in a merkle trie), and
// the full key/value for reads. For read and writes over the same storage, the full value
// is included.
// For cold reads involving code (call, callcode, staticcall and delegatecall):
// - We depend on https://github.com/paritytech/frontier/pull/893
// - Try to get the cached size or compute it on the fly
// - We record the actual size after caching, refunding the difference between it and the initially deducted
// contract size limit.
match opcode {
Opcode::BALANCE => {
weight_info.try_record_proof_size_or_fail(ACCOUNT_BASIC_PROOF_SIZE)?;
}
Opcode::EXTCODESIZE | Opcode::EXTCODECOPY | Opcode::EXTCODEHASH => {
if let Some(AccessedStorage::AccountCodes(address)) = accessed_storage {
record_account_codes_proof_size(address, false)?;
recorded.account_codes.push(address);
}
}
Opcode::CALLCODE | Opcode::CALL | Opcode::DELEGATECALL | Opcode::STATICCALL => {
if let Some(AccessedStorage::AccountCodes(address)) = accessed_storage {
record_account_codes_proof_size(address, true)?;
recorded.account_codes.push(address);
}
}
Opcode::SLOAD => {
if let Some(AccessedStorage::AccountStorages((address, index))) =
accessed_storage
{
(address, index)
} else {
// This must be unreachable, a valid target must be set.
// TODO decide how do we want to gracefully handle.
return Err(ExitError::OutOfGas);
};
let mut cost = WRITE_PROOF_SIZE;
let maybe_record = !recorded.account_storages.contains_key(&(address, index));
// If the slot is yet to be accessed we charge for it, as the evm reads
// it prior to the opcode execution.
// Skip if the address and index has been already recorded this block.
if maybe_record {
cost = cost.saturating_add(ACCOUNT_STORAGE_PROOF_SIZE);
weight_info.try_record_proof_size_or_fail(ACCOUNT_STORAGE_PROOF_SIZE)?;
recorded.account_storages.insert((address, index), true);
}
}
U256::from(cost)
}
// Fixed trie 32 byte hash
Opcode::CREATE | Opcode::CREATE2 => U256::from(WRITE_PROOF_SIZE),
// When calling SUICIDE a target account will receive the self destructing
// address's balance. We need to account for both:
// - Target basic account read
// - 5 bytes of `decode_len`
Opcode::SUICIDE => {
accessed_storage = None;
U256::from(IS_EMPTY_CHECK_PROOF_SIZE)
}
// Rest of dynamic opcodes that do not involve proof size recording, do nothing
_ => return Ok(()),
};

if opcode_proof_size > U256::from(u64::MAX) {
weight_info.try_record_proof_size_or_fail(proof_size_limit)?;
return Err(ExitError::OutOfGas);
}

// Cache the storage access
match accessed_storage {
Some(AccessedStorage::AccountStorages((address, index))) => {
recorded.account_storages.insert((address, index), true);
}
Some(AccessedStorage::AccountCodes(address)) => {
recorded.account_codes.push(address);
}
_ => {}
Opcode::SSTORE => {
if let Some(AccessedStorage::AccountStorages((address, index))) =
accessed_storage
{
let size = WRITE_PROOF_SIZE.saturating_add(ACCOUNT_STORAGE_PROOF_SIZE);
weight_info.try_record_proof_size_or_fail(size)?;
recorded.account_storages.insert((address, index), true);
}
}
Opcode::CREATE | Opcode::CREATE2 => {
weight_info.try_record_proof_size_or_fail(WRITE_PROOF_SIZE)?;
}
// When calling SUICIDE a target account will receive the self destructing
// address's balance. We need to account for both:
// - Target basic account read
// - 5 bytes of `decode_len`
Opcode::SUICIDE => {
weight_info.try_record_proof_size_or_fail(IS_EMPTY_CHECK_PROOF_SIZE)?;
}
// Rest of dynamic opcodes that do not involve proof size recording, do nothing
_ => return Ok(()),
};
}

// Record cost
self.record_external_cost(None, Some(opcode_proof_size.low_u64()))?;
Ok(())
}

Expand All @@ -1181,8 +1146,7 @@ where
} else {
return Ok(());
};
// Record ref_time first
// TODO benchmark opcodes, until this is done we do used_gas to weight conversion for ref_time

if let Some(amount) = ref_time {
weight_info.try_record_ref_time_or_fail(amount)?;
}
Expand Down
22 changes: 22 additions & 0 deletions primitives/evm/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,15 @@ impl WeightInfo {
_ => return Err("must provide Some valid weight limit or None"),
})
}

fn try_consume(&self, cost: u64, limit: u64, usage: u64) -> Result<u64, ExitError> {
let usage = usage.checked_add(cost).ok_or(ExitError::OutOfGas)?;
if usage > limit {
return Err(ExitError::OutOfGas);
}
Ok(usage)
}

pub fn try_record_ref_time_or_fail(&mut self, cost: u64) -> Result<(), ExitError> {
if let (Some(ref_time_usage), Some(ref_time_limit)) =
(self.ref_time_usage, self.ref_time_limit)
Expand All @@ -127,6 +129,7 @@ impl WeightInfo {
}
Ok(())
}

pub fn try_record_proof_size_or_fail(&mut self, cost: u64) -> Result<(), ExitError> {
if let (Some(proof_size_usage), Some(proof_size_limit)) =
(self.proof_size_usage, self.proof_size_limit)
Expand All @@ -139,18 +142,37 @@ impl WeightInfo {
}
Ok(())
}

pub fn refund_proof_size(&mut self, amount: u64) {
if let Some(proof_size_usage) = self.proof_size_usage {
let proof_size_usage = proof_size_usage.saturating_sub(amount);
self.proof_size_usage = Some(proof_size_usage);
}
}

pub fn refund_ref_time(&mut self, amount: u64) {
if let Some(ref_time_usage) = self.ref_time_usage {
let ref_time_usage = ref_time_usage.saturating_sub(amount);
self.ref_time_usage = Some(ref_time_usage);
}
}
pub fn remaining_proof_size(&self) -> Option<u64> {
if let (Some(proof_size_usage), Some(proof_size_limit)) =
(self.proof_size_usage, self.proof_size_limit)
{
return Some(proof_size_limit.saturating_sub(proof_size_usage));
}
None
}

pub fn remaining_ref_time(&self) -> Option<u64> {
if let (Some(ref_time_usage), Some(ref_time_limit)) =
(self.ref_time_usage, self.ref_time_limit)
{
return Some(ref_time_limit.saturating_sub(ref_time_usage));
}
None
}
}

#[derive(Clone, Eq, PartialEq, Debug, Encode, Decode, TypeInfo)]
Expand Down

0 comments on commit 176cb34

Please sign in to comment.