Skip to content

Commit

Permalink
fix(levm): memory size calculations (#1445)
Browse files Browse the repository at this point in the history
**Motivation**

<!-- Why does this pull request exist? What are its goals? -->
- Currently we have some errors in gas calculations related to memory
when "size" is set to 0. Memory shouldn't ever expand on that scenario.
- Tidy code for calculating memory size.

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->
- Create function `calculate_memory_size` in memory module
- Use function in every opcode that needs it

<!-- Link to issues: Resolves #111, Resolves #222 -->

**Results**
- 7 more tests pass when comparing directly to last main changes.

Closes #1442
  • Loading branch information
JereSalo authored Dec 9, 2024
1 parent f438285 commit dd680ee
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 96 deletions.
11 changes: 11 additions & 0 deletions crates/vm/levm/src/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,14 @@ fn cost(memory_size: usize) -> Result<usize, VMError> {
)
.ok_or(OutOfGasError::MemoryExpansionCostOverflow)?)
}

pub fn calculate_memory_size(offset: usize, size: usize) -> Result<usize, VMError> {
if size == 0 {
return Ok(0);
}

offset
.checked_add(size)
.and_then(|sum| sum.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE))
.ok_or(VMError::OutOfOffset)
}
48 changes: 14 additions & 34 deletions crates/vm/levm/src/opcode_handlers/environment.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use crate::{
call_frame::CallFrame,
constants::WORD_SIZE_IN_BYTES_USIZE,
errors::{InternalError, OpcodeSuccess, VMError},
gas_cost, memory,
gas_cost,
memory::{self, calculate_memory_size},
vm::{word_to_address, VM},
};
use ethrex_core::U256;
Expand Down Expand Up @@ -150,17 +150,11 @@ impl VM {
.try_into()
.map_err(|_err| VMError::VeryLargeNumber)?;

let new_memory_size = calculate_memory_size(dest_offset, size)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::calldatacopy(
dest_offset
.checked_add(size)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
current_call_frame.memory.len(),
size,
)?,
gas_cost::calldatacopy(new_memory_size, current_call_frame.memory.len(), size)?,
)?;

if size == 0 {
Expand Down Expand Up @@ -229,17 +223,11 @@ impl VM {
.try_into()
.map_err(|_| VMError::VeryLargeNumber)?;

let new_memory_size = calculate_memory_size(destination_offset, size)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::codecopy(
destination_offset
.checked_add(size)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
current_call_frame.memory.len(),
size,
)?,
gas_cost::codecopy(new_memory_size, current_call_frame.memory.len(), size)?,
)?;

if size == 0 {
Expand Down Expand Up @@ -318,14 +306,12 @@ impl VM {

let (account_info, address_was_cold) = self.access_account(address);

let new_memory_size = calculate_memory_size(dest_offset, size)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::extcodecopy(
dest_offset
.checked_add(size)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
new_memory_size,
current_call_frame.memory.len(),
address_was_cold,
)?,
Expand Down Expand Up @@ -388,17 +374,11 @@ impl VM {
.try_into()
.map_err(|_| VMError::VeryLargeNumber)?;

let new_memory_size = calculate_memory_size(dest_offset, size)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::returndatacopy(
dest_offset
.checked_add(size)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
current_call_frame.memory.len(),
size,
)?,
gas_cost::returndatacopy(new_memory_size, current_call_frame.memory.len(), size)?,
)?;

if size == 0 {
Expand Down
16 changes: 5 additions & 11 deletions crates/vm/levm/src/opcode_handlers/keccak.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use crate::{
call_frame::CallFrame,
constants::WORD_SIZE_IN_BYTES_USIZE,
errors::{OpcodeSuccess, VMError},
gas_cost, memory,
gas_cost,
memory::{self, calculate_memory_size},
vm::VM,
};
use ethrex_core::U256;
Expand All @@ -27,17 +27,11 @@ impl VM {
.try_into()
.map_err(|_| VMError::VeryLargeNumber)?;

let new_memory_size = calculate_memory_size(offset, size)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::keccak256(
offset
.checked_add(size)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
current_call_frame.memory.len(),
size,
)?,
gas_cost::keccak256(new_memory_size, current_call_frame.memory.len(), size)?,
)?;

let mut hasher = Keccak256::new();
Expand Down
12 changes: 5 additions & 7 deletions crates/vm/levm/src/opcode_handlers/logging.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use crate::{
call_frame::CallFrame,
constants::WORD_SIZE_IN_BYTES_USIZE,
errors::{OpcodeSuccess, VMError},
gas_cost, memory,
gas_cost,
memory::{self, calculate_memory_size},
vm::VM,
};
use bytes::Bytes;
Expand Down Expand Up @@ -40,14 +40,12 @@ impl VM {
topics.push(H256::from_slice(&topic_bytes));
}

let new_memory_size = calculate_memory_size(offset, size)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::log(
offset
.checked_add(size)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
new_memory_size,
current_call_frame.memory.len(),
size,
number_of_topics,
Expand Down
50 changes: 14 additions & 36 deletions crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use crate::{
call_frame::CallFrame,
constants::{WORD_SIZE, WORD_SIZE_IN_BYTES_USIZE},
errors::{OpcodeSuccess, OutOfGasError, VMError},
gas_cost, memory,
gas_cost,
memory::{self, calculate_memory_size},
opcodes::Opcode,
vm::VM,
};
Expand Down Expand Up @@ -64,16 +65,11 @@ impl VM {
.try_into()
.map_err(|_| VMError::VeryLargeNumber)?;

let new_memory_size = calculate_memory_size(offset, WORD_SIZE_IN_BYTES_USIZE)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::mload(
offset
.checked_add(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
current_call_frame.memory.len(),
)?,
gas_cost::mload(new_memory_size, current_call_frame.memory.len())?,
)?;

current_call_frame
Expand All @@ -94,16 +90,11 @@ impl VM {
.try_into()
.map_err(|_err| VMError::VeryLargeNumber)?;

let new_memory_size = calculate_memory_size(offset, WORD_SIZE_IN_BYTES_USIZE)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::mstore(
offset
.checked_add(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
current_call_frame.memory.len(),
)?,
gas_cost::mstore(new_memory_size, current_call_frame.memory.len())?,
)?;

let value = current_call_frame.stack.pop()?;
Expand All @@ -127,16 +118,11 @@ impl VM {
.try_into()
.map_err(|_| VMError::VeryLargeNumber)?;

let new_memory_size = calculate_memory_size(offset, 1)?;

self.increase_consumed_gas(
current_call_frame,
gas_cost::mstore8(
offset
.checked_add(1)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?,
current_call_frame.memory.len(),
)?,
gas_cost::mstore8(new_memory_size, current_call_frame.memory.len())?,
)?;

let value = current_call_frame.stack.pop()?;
Expand Down Expand Up @@ -295,17 +281,9 @@ impl VM {
.try_into()
.map_err(|_| VMError::VeryLargeNumber)?;

let new_memory_size_for_dest = dest_offset
.checked_add(size)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?;

let new_memory_size_for_src = src_offset
.checked_add(size)
.ok_or(VMError::OutOfOffset)?
.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)
.ok_or(VMError::OutOfOffset)?;
let new_memory_size_for_dest = calculate_memory_size(dest_offset, size)?;

let new_memory_size_for_src = calculate_memory_size(src_offset, size)?;

self.increase_consumed_gas(
current_call_frame,
Expand Down
13 changes: 5 additions & 8 deletions crates/vm/levm/src/opcode_handlers/system.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use crate::{
call_frame::CallFrame,
constants::WORD_SIZE_IN_BYTES_USIZE,
errors::{InternalError, OpcodeSuccess, OutOfGasError, ResultReason, VMError},
gas_cost, memory,
errors::{InternalError, OpcodeSuccess, ResultReason, VMError},
gas_cost,
memory::{self, calculate_memory_size},
vm::{word_to_address, VM},
};
use ethrex_core::{types::TxKind, Address, U256};
Expand Down Expand Up @@ -193,9 +194,7 @@ impl VM {
.try_into()
.map_err(|_| VMError::VeryLargeNumber)?;

let new_memory_size = offset
.checked_add(size)
.ok_or(VMError::OutOfGas(OutOfGasError::GasCostOverflow))?;
let new_memory_size = calculate_memory_size(offset, size)?;
self.increase_consumed_gas(
current_call_frame,
memory::expansion_cost(new_memory_size, current_call_frame.memory.len())?.into(),
Expand Down Expand Up @@ -461,9 +460,7 @@ impl VM {
.try_into()
.map_err(|_err| VMError::VeryLargeNumber)?;

let new_memory_size = offset
.checked_add(size)
.ok_or(VMError::OutOfGas(OutOfGasError::GasCostOverflow))?;
let new_memory_size = calculate_memory_size(offset, size)?;
self.increase_consumed_gas(
current_call_frame,
memory::expansion_cost(new_memory_size, current_call_frame.memory.len())?.into(),
Expand Down
10 changes: 10 additions & 0 deletions crates/vm/levm/tests/edge_case_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,3 +288,13 @@ fn test_non_compliance_codecopy_memory_resize() {
&U256::from(14400)
);
}

#[test]
fn test_non_compliance_log_gas_cost() {
let mut vm = new_vm_with_bytecode(Bytes::copy_from_slice(&[56, 68, 68, 68, 131, 163])).unwrap();
vm.env.gas_price = U256::zero();
vm.env.gas_limit = U256::from(100_000_000);
vm.env.block_gas_limit = U256::from(100_000_001);
let res = vm.transact().unwrap();
assert_eq!(res.gas_used, 22511);
}

0 comments on commit dd680ee

Please sign in to comment.