From 0429775fa213705c9599a7992ac5b698cafd45f0 Mon Sep 17 00:00:00 2001 From: enitrat Date: Mon, 16 Sep 2024 18:15:07 +0200 Subject: [PATCH 1/3] fix: returndata buffer emptied on stop --- crates/evm/src/call_helpers.cairo | 4 +- .../src/instructions/push_operations.cairo | 243 +++++++++--------- .../stop_and_arithmetic_operations.cairo | 17 +- .../src/instructions/system_operations.cairo | 207 ++++----------- crates/evm/src/interpreter.cairo | 11 +- crates/evm/src/model/vm.cairo | 24 +- crates/utils/src/helpers.cairo | 74 +++++- 7 files changed, 285 insertions(+), 295 deletions(-) diff --git a/crates/evm/src/call_helpers.cairo b/crates/evm/src/call_helpers.cairo index ae1b47a32..df0d2a24a 100644 --- a/crates/evm/src/call_helpers.cairo +++ b/crates/evm/src/call_helpers.cairo @@ -107,9 +107,11 @@ pub impl CallHelpersImpl of CallHelpers { self.stack.push(0)?; }, ExecutionResultStatus::Exception => { - // If the call has halted exceptionnaly, the return_data is emptied. + // If the call has halted exceptionnaly, + // the return_data is emptied, and nothing is stored in memory self.return_data = [].span(); self.stack.push(0)?; + return Result::Ok(()); }, } diff --git a/crates/evm/src/instructions/push_operations.cairo b/crates/evm/src/instructions/push_operations.cairo index abe7306a9..de3dbade2 100644 --- a/crates/evm/src/instructions/push_operations.cairo +++ b/crates/evm/src/instructions/push_operations.cairo @@ -4,7 +4,8 @@ use evm::errors::EVMError; use evm::gas; use evm::model::vm::{VM, VMTrait}; use evm::stack::StackTrait; -use utils::helpers::load_word; +use utils::helpers::FromBytes; +use utils::helpers::U8SpanExTrait; /// Place i bytes items on stack. #[inline(always)] @@ -18,7 +19,7 @@ fn exec_push_i(ref self: VM, i: u8) -> Result<(), EVMError> { if data.len() == 0 { self.stack.push(0) } else { - self.stack.push(load_word(i, data)) + self.stack.push(data.pad_right_with_zeroes(i).from_be_bytes_partial().unwrap()) } } @@ -277,9 +278,55 @@ pub impl PushOperations of PushOperationsTrait { #[cfg(test)] mod tests { + use evm::gas; use evm::instructions::PushOperationsTrait; + use evm::model::vm::VMTrait; use evm::stack::StackTrait; use evm::test_utils::{VMBuilderTrait}; + use super::exec_push_i; + + #[test] + fn test_exec_push_i() { + // Test cases: (bytes_to_push, bytecode_length, expected_value) + let test_cases: Span<(u8, u8, u256)> = [ + (1, 1, 0xFF), // Push 1 byte + (16, 16, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF), // Push 16 bytes + ( + 32, 32, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF + ), // Push 32 bytes (max) + ( + 32, 16, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00000000000000000000000000000000 + ), // Push 32 bytes, but only 16 available + (1, 0, 0), // Push 1 byte, but no bytes available + ].span(); + + for elem in test_cases { + let (bytes_to_push, bytecode_length, expected_value) = *elem; + + let mut vm = VMBuilderTrait::new_with_presets() + .with_bytecode(get_n_0xFF(bytecode_length)) + .build(); + let initial_pc = vm.pc(); + let initial_gas = vm.gas_left(); + + // Execute exec_push_i + let result = exec_push_i(ref vm, bytes_to_push); + + assert!(result.is_ok()); + + // Check stack + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), expected_value); + + // Check PC increment + let pc_increment = vm.pc() - initial_pc; + assert_eq!(pc_increment, bytes_to_push.into()); + + // Check gas consumption + let gas_consumed = initial_gas - vm.gas_left(); + assert_eq!(gas_consumed, gas::VERYLOW); + } + } fn get_n_0xFF(mut n: u8) -> Span { let mut array: Array = ArrayTrait::new(); @@ -298,8 +345,8 @@ mod tests { // When vm.exec_push0().expect('exec_push0 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0); } #[test] @@ -310,8 +357,8 @@ mod tests { // When vm.exec_push1().expect('exec_push1 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFF); } #[test] @@ -322,8 +369,8 @@ mod tests { // When vm.exec_push2().expect('exec_push2 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFF); } #[test] @@ -334,8 +381,8 @@ mod tests { // When vm.exec_push3().expect('exec_push3 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFF); } #[test] @@ -346,8 +393,8 @@ mod tests { // When vm.exec_push4().expect('exec_push4 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFF); } #[test] @@ -358,8 +405,8 @@ mod tests { // When vm.exec_push5().expect('exec_push5 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFF); } #[test] @@ -370,8 +417,8 @@ mod tests { // When vm.exec_push6().expect('exec_push6 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFF); } #[test] @@ -382,8 +429,8 @@ mod tests { // When vm.exec_push7().expect('exec_push7 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFF); } @@ -395,8 +442,8 @@ mod tests { // When vm.exec_push8().expect('exec_push8 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFF); } #[test] @@ -407,8 +454,8 @@ mod tests { // When vm.exec_push9().expect('exec_push9 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFF); } #[test] @@ -419,8 +466,8 @@ mod tests { // When vm.exec_push10().expect('exec_push10 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -431,8 +478,8 @@ mod tests { // When vm.exec_push11().expect('exec_push11 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -443,8 +490,8 @@ mod tests { // When vm.exec_push12().expect('exec_push12 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -455,8 +502,8 @@ mod tests { // When vm.exec_push13().expect('exec_push13 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -467,8 +514,8 @@ mod tests { // When vm.exec_push14().expect('exec_push14 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -479,8 +526,8 @@ mod tests { // When vm.exec_push15().expect('exec_push15 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -491,8 +538,8 @@ mod tests { // When vm.exec_push16().expect('exec_push16 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert(vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, 'invalid stack top'); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -503,10 +550,8 @@ mod tests { // When vm.exec_push17().expect('exec_push17 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -517,10 +562,8 @@ mod tests { // When vm.exec_push18().expect('exec_push18 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] fn test_push19() { @@ -530,11 +573,8 @@ mod tests { // When vm.exec_push19().expect('exec_push19 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -545,11 +585,8 @@ mod tests { // When vm.exec_push20().expect('exec_push20 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -560,11 +597,8 @@ mod tests { // When vm.exec_push21().expect('exec_push21 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -575,11 +609,8 @@ mod tests { // When vm.exec_push22().expect('exec_push22 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] fn test_push23() { @@ -589,11 +620,8 @@ mod tests { // When vm.exec_push23().expect('exec_push23 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] fn test_push24() { @@ -603,11 +631,8 @@ mod tests { // When vm.exec_push24().expect('exec_push24 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -618,11 +643,8 @@ mod tests { // When vm.exec_push25().expect('exec_push25 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' - ); + assert_eq!(vm.stack.len(), 1); + assert_eq!(vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF); } #[test] @@ -633,10 +655,9 @@ mod tests { // When vm.exec_push26().expect('exec_push26 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' + assert_eq!(vm.stack.len(), 1); + assert_eq!( + vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF ); } @@ -648,10 +669,9 @@ mod tests { // When vm.exec_push27().expect('exec_push27 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' + assert_eq!(vm.stack.len(), 1); + assert_eq!( + vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF ); } @@ -663,10 +683,9 @@ mod tests { // When vm.exec_push28().expect('exec_push28 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm.stack.peek().unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' + assert_eq!(vm.stack.len(), 1); + assert_eq!( + vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF ); } @@ -678,13 +697,9 @@ mod tests { // When vm.exec_push29().expect('exec_push29 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm - .stack - .peek() - .unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' + assert_eq!(vm.stack.len(), 1); + assert_eq!( + vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF ); } @@ -696,13 +711,9 @@ mod tests { // When vm.exec_push30().expect('exec_push30 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm - .stack - .peek() - .unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' + assert_eq!(vm.stack.len(), 1); + assert_eq!( + vm.stack.peek().unwrap(), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF ); } @@ -714,13 +725,10 @@ mod tests { // When vm.exec_push31().expect('exec_push31 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm - .stack - .peek() - .unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' + assert_eq!(vm.stack.len(), 1); + assert_eq!( + vm.stack.peek().unwrap(), + 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF ); } @@ -732,13 +740,10 @@ mod tests { // When vm.exec_push32().expect('exec_push32 failed'); // Then - assert(vm.stack.len() == 1, 'stack should have one element'); - assert( - vm - .stack - .peek() - .unwrap() == 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, - 'invalid stack top' + assert_eq!(vm.stack.len(), 1); + assert_eq!( + vm.stack.peek().unwrap(), + 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF ); } } diff --git a/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo b/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo index c27227971..cbad783f9 100644 --- a/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo +++ b/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo @@ -14,9 +14,13 @@ pub impl StopAndArithmeticOperations of StopAndArithmeticOperationsTrait { /// 0x00 - STOP /// Halts the execution of the current program. /// # Specification: https://www.evm.codes/#00?fork=shanghai - fn exec_stop(ref self: VM) -> Result<(), EVMError> { + fn exec_stop(ref self: VM) { + // return_data stored the return_data for the last executed sub context + // see CALLs opcodes. When we run the STOP opcode, we stop the current + // execution context with *no* return data (unlike RETURN and REVERT). + // hence we just clear the return_data and stop. + self.return_data = [].span(); self.stop(); - Result::Ok(()) } /// 0x01 - ADD @@ -268,15 +272,16 @@ mod tests { #[test] - fn test_exec_stop() { + fn test_exec_stop_should_stop_and_empty_return_data() { // Given let mut vm = VMBuilderTrait::new_with_presets().build(); - + vm.return_data = [1, 2, 3].span(); // When - vm.exec_stop().expect('exec_stop failed'); + vm.exec_stop(); // Then - assert(!vm.is_running(), 'ctx not stopped'); + assert!(!vm.is_running()); + assert_eq!(vm.return_data, [].span()); } #[test] diff --git a/crates/evm/src/instructions/system_operations.cairo b/crates/evm/src/instructions/system_operations.cairo index 525f71f7e..50436dc5c 100644 --- a/crates/evm/src/instructions/system_operations.cairo +++ b/crates/evm/src/instructions/system_operations.cairo @@ -451,6 +451,7 @@ mod tests { #[test] fn test_exec_call() { // Given + // Set vm bytecode // (call 0xffffff 0xabfa740ccd 0 0 0 0 1) let bytecode = [ @@ -478,99 +479,47 @@ mod tests { 0xf1, 0x00 ].span(); + let code_hash = bytecode.compute_keccak256_hash(); let mut vm = VMBuilderTrait::new_with_presets().with_bytecode(bytecode).build(); - let eoa_account = Account { + let caller_account = Account { address: vm.message().target, balance: 0, - code: [].span(), - code_hash: EMPTY_KECCAK, + code: bytecode, + code_hash: code_hash, nonce: 0, is_created: false, selfdestruct: false, }; - vm.env.state.set_account(eoa_account); + vm.env.state.set_account(caller_account); // Deploy bytecode at 0xabfa740ccd - // ret (+ 0x1 0x1) + // (+ 0x1 0x1) let eth_address: EthAddress = 0xabfa740ccd_u256.into(); let starknet_address = compute_starknet_address( test_address(), eth_address, 0.try_into().unwrap() ); + // Deploy bytecode at 0x1234 + // SSTORE 0x42 at 0x42 let deployed_bytecode = [ - 0x60, 0x01, 0x60, 0x01, 0x01, 0x60, 0x00, 0x53, 0x60, 0x20, 0x60, 0x00, 0xf3 - ].span(); - let code_hash = deployed_bytecode.compute_keccak256_hash(); - let contract_account = Account { - address: Address { evm: eth_address, starknet: starknet_address, }, - balance: 0, - code: deployed_bytecode, - code_hash: code_hash, - nonce: 1, - is_created: false, - selfdestruct: false, - }; - vm.env.state.set_account(contract_account); - - // When - EVMTrait::execute_code(ref vm); - - // Then - assert(!vm.is_running(), 'run should be success'); - assert(2 == load_word(1, vm.return_data()), 'Wrong return_data'); - assert(!vm.is_running(), 'vm should be stopped'); - } - - #[test] - fn test_exec_call_no_return() { - // Given - - // Set vm bytecode - // (call 0xffffff 0xabfa740ccd 0 0 0 0 1) - let bytecode = [ 0x60, 0x01, 0x60, - 0x00, + 0x01, + 0x01, 0x60, 0x00, + 0x53, 0x60, - 0x00, + 0x42, + 0x60, + 0x42, + 0x55, + 0x60, + 0x20, 0x60, 0x00, - 0x64, - 0xab, - 0xfa, - 0x74, - 0x0c, - 0xcd, - 0x62, - 0xff, - 0xff, - 0xff, - // CALL - 0xf1, - 0x00 + 0xf3 ].span(); - let code_hash = bytecode.compute_keccak256_hash(); - let mut vm = VMBuilderTrait::new_with_presets().with_bytecode(bytecode).build(); - let caller_account = Account { - address: vm.message().target, - balance: 0, - code: bytecode, - code_hash: code_hash, - nonce: 0, - is_created: false, - selfdestruct: false, - }; - vm.env.state.set_account(caller_account); - - // Deploy bytecode at 0xabfa740ccd - // (+ 0x1 0x1) - let eth_address: EthAddress = 0xabfa740ccd_u256.into(); - let starknet_address = compute_starknet_address( - test_address(), eth_address, 0.try_into().unwrap() - ); - let deployed_bytecode = [0x60, 0x01, 0x60, 0x01, 0x01, 0x60, 0x00, 0x53, 0x00].span(); let code_hash = deployed_bytecode.compute_keccak256_hash(); let contract_account = Account { address: Address { evm: eth_address, starknet: starknet_address, }, @@ -587,16 +536,18 @@ mod tests { EVMTrait::execute_code(ref vm); // Then - assert(!vm.is_running(), 'run should be success'); - assert(vm.return_data().is_empty(), 'Wrong return_data len'); - assert(!vm.is_running(), 'vm should be stopped'); + assert!(!vm.is_error()); + assert!(!vm.is_running()); + let storage_val = vm.env.state.read_state(contract_account.address.evm, 0x42); + assert_eq!(storage_val, 0x42); } #[test] fn test_exec_staticcall() { // Given + // Set vm bytecode - // (staticcall 0xffffff 0xabfa740ccd 0 0 0 0 1) + // (call 0xffffff 0xabfa740ccd 0 0 0 0 1) let bytecode = [ 0x60, 0x01, @@ -606,6 +557,8 @@ mod tests { 0x00, 0x60, 0x00, + 0x60, + 0x00, 0x64, 0xab, 0xfa, @@ -627,90 +580,38 @@ mod tests { balance: 0, code: bytecode, code_hash: code_hash, - nonce: 1, + nonce: 0, is_created: false, selfdestruct: false, }; vm.env.state.set_account(caller_account); - // Deploy bytecode at 0xabfa740ccd - // ret (+ 0x1 0x1) + let eth_address: EthAddress = 0xabfa740ccd_u256.into(); let starknet_address = compute_starknet_address( test_address(), eth_address, 0.try_into().unwrap() ); + // Deploy bytecode at 0x1234 + // SSTORE 0x42 at 0x42 let deployed_bytecode = [ - 0x60, 0x01, 0x60, 0x01, 0x01, 0x60, 0x00, 0x53, 0x60, 0x20, 0x60, 0x00, 0xf3 - ].span(); - let code_hash = deployed_bytecode.compute_keccak256_hash(); - let contract_account = Account { - address: Address { evm: eth_address, starknet: starknet_address, }, - balance: 0, - code: deployed_bytecode, - code_hash: code_hash, - nonce: 1, - is_created: false, - selfdestruct: false, - }; - vm.env.state.set_account(contract_account); - - // When - EVMTrait::execute_code(ref vm); - - // Then - assert_eq!(2, load_word(1, vm.return_data())); - assert(!vm.is_running(), 'vm should be stopped'); - } - - #[test] - fn test_exec_staticcall_no_return() { - // Given - // Set vm bytecode - // (call 0xffffff 0xabfa740ccd 0 0 0 0 1) - let bytecode = [ 0x60, 0x01, 0x60, - 0x00, + 0x01, + 0x01, 0x60, 0x00, + 0x53, 0x60, - 0x00, + 0x42, + 0x60, + 0x42, + 0x55, + 0x60, + 0x20, 0x60, 0x00, - 0x64, - 0xab, - 0xfa, - 0x74, - 0x0c, - 0xcd, - 0x62, - 0xff, - 0xff, - 0xff, - // STATICCALL - 0xfa, - 0x00 + 0xf3 ].span(); - let code_hash = bytecode.compute_keccak256_hash(); - let mut vm = VMBuilderTrait::new_with_presets().with_bytecode(bytecode).build(); - let caller_account = Account { - address: vm.message().target, - balance: 0, - code: bytecode, - code_hash: code_hash, - nonce: 1, - is_created: false, - selfdestruct: false, - }; - vm.env.state.set_account(caller_account); - - // Deploy bytecode at 0xabfa740ccd - // (+ 0x1 0x1) - let eth_address: EthAddress = 0xabfa740ccd_u256.into(); - let starknet_address = compute_starknet_address( - test_address(), eth_address, 0.try_into().unwrap() - ); - let deployed_bytecode = [0x60, 0x01, 0x60, 0x01, 0x01, 0x60, 0x00, 0x53, 0x00].span(); let code_hash = deployed_bytecode.compute_keccak256_hash(); let contract_account = Account { address: Address { evm: eth_address, starknet: starknet_address, }, @@ -727,11 +628,12 @@ mod tests { EVMTrait::execute_code(ref vm); // Then - assert(!vm.is_running(), 'run should be success'); - assert(vm.return_data().is_empty(), 'Wrong return_data len'); - assert(!vm.is_running(), 'vm should be stopped') + assert!(!vm.is_error()); + assert!(!vm.is_running()); + assert_eq!(vm.stack.peek().unwrap(), 0); // STATICCALL should fail because of SSTORE } + #[test] fn test_exec_call_code() { // Given @@ -775,7 +677,7 @@ mod tests { vm.env.state.set_account(eoa_account); // Deploy bytecode at 0x1234 - // ret (+ 0x1 0x1) + // SSTORE 0x42 at 0x42 let deployed_bytecode = [ 0x60, 0x01, @@ -816,13 +718,12 @@ mod tests { EVMTrait::execute_code(ref vm); // Then - assert(!vm.is_running(), 'run should be success'); - assert(2 == load_word(1, vm.return_data()), 'Wrong return_data'); - assert(!vm.is_running(), 'vm should be stopped'); + assert!(!vm.is_error()); + assert!(!vm.is_running()); let storage_val = vm.env.state.read_state(vm.message.target.evm, 0x42); - assert(storage_val == 0x42, 'storage value is not 0x42'); + assert_eq!(storage_val, 0x42); } #[test] @@ -866,8 +767,7 @@ mod tests { }; vm.env.state.set_account(eoa_account); - // Deploy bytecode at 0x1234 - // ret (+ 0x1 0x1) + // SSTORE 0x42 at 0x42 let deployed_bytecode = [ 0x60, 0x01, @@ -908,13 +808,12 @@ mod tests { EVMTrait::execute_code(ref vm); // Then - assert(!vm.is_running(), 'run should be success'); - assert(2 == load_word(1, vm.return_data()), 'Wrong return_data'); - assert(!vm.is_running(), 'vm should be stopped'); + assert!(!vm.is_error()); + assert!(!vm.is_running()); let storage_val = vm.env.state.read_state(vm.message.target.evm, 0x42); - assert(storage_val == 0x42, 'storage value is not 0x42'); + assert_eq!(storage_val, 0x42); } //! In the exec_create tests, we query the balance of the contract being created by doing a diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index e42c38e95..fb74149be 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -304,8 +304,13 @@ pub impl EVMImpl of EVMTrait { let pc = vm.pc(); let bytecode = vm.message().code; - // Check if PC is not out of bounds. - if pc >= bytecode.len() || vm.is_running() == false { + // If PC is out of bounds, stop the VM + // Also empties the returndata - akin to executing the STOP opcode. + if pc >= bytecode.len() { + vm.exec_stop(); + } + + if !vm.is_running() { // REVERT opcode case if vm.is_error() { return ExecutionResult { @@ -372,7 +377,7 @@ pub impl EVMImpl of EVMTrait { // Call the appropriate function based on the opcode. if opcode == 0x00 { // STOP - return self.exec_stop(); + return Result::Ok(self.exec_stop()); } if opcode == 0x01 { // ADD diff --git a/crates/evm/src/model/vm.cairo b/crates/evm/src/model/vm.cairo index daea45824..5cbd14775 100644 --- a/crates/evm/src/model/vm.cairo +++ b/crates/evm/src/model/vm.cairo @@ -1,5 +1,6 @@ +use core::cmp::min; use core::dict::{Felt252Dict, Felt252DictTrait}; -use core::num::traits::CheckedSub; +use core::num::traits::{SaturatingSub, CheckedSub}; use core::starknet::EthAddress; use evm::errors::EVMError; use evm::memory::Memory; @@ -139,20 +140,23 @@ pub impl VMImpl of VMTrait { /// # Returns /// /// * A `Span` containing the requested bytecode slice. - /// * If the requested slice is out of bounds, returns an empty slice. + /// * If the requested slice extends beyond the code length, returns remaining bytes. #[inline(always)] fn read_code(self: @VM, len: usize) -> Span { let pc = self.pc(); let code_len = self.message().code.len(); - // Check if the requested slice is within bounds - if pc + len > code_len { - // If out of bounds, return an empty slice - [].span() - } else { - // If within bounds, return the requested slice - self.message().code.slice(pc, len) + // If pc is out of bounds, return an empty span + if pc >= code_len { + return [].span(); } + + // Calculate the actual length to read + let remaining = code_len.saturating_sub(pc); + let actual_len = min(len, remaining); + + // Return the slice with the actual length + self.message().code.slice(pc, actual_len) } #[inline(always)] @@ -249,7 +253,7 @@ mod tests { let read_code = vm.read_code(2); - assert_eq!(read_code, [].span()); + assert_eq!(read_code, [0x03].span()); assert_eq!(vm.pc(), 2); } diff --git a/crates/utils/src/helpers.cairo b/crates/utils/src/helpers.cairo index 4a29f6dd8..8db8e0253 100644 --- a/crates/utils/src/helpers.cairo +++ b/crates/utils/src/helpers.cairo @@ -316,8 +316,6 @@ pub impl U8SpanExImpl of U8SpanExTrait { /// than the span length, returns an empty span of length `len` if offset is grearter than the /// span length fn slice_right_padded(self: Span, offset: usize, len: usize) -> Span { - let mut arr = array![]; - let start = if offset <= self.len() { offset } else { @@ -333,6 +331,7 @@ pub impl U8SpanExImpl of U8SpanExTrait { } // Copy the span + let mut arr = array![]; arr.append_span(slice); while arr.len() != len { @@ -342,6 +341,50 @@ pub impl U8SpanExImpl of U8SpanExTrait { arr.span() } + /// Clones and pads the given span with 0s to the right to the given length, if data is more + /// than the given length, it is truncated from the right side + /// # Examples + /// ``` + /// let span = [0x01, 0x02, 0x03, 0x04, 0x05].span(); + /// let expected = [0x01, 0x02, 0x03, 0x04, 0x05, 0x0, 0x0, 0x0, 0x0, 0x0].span(); + /// let result = span.pad_right_with_zeroes(10); + /// + /// assert_eq!(result, expected); + /// + /// // Truncates the data if it is more than the given length + /// let span = [0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a].span(); + /// let expected = [0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09].span(); + /// let result = span.pad_right_with_zeroes(9); + /// + /// assert_eq!(result, expected); + /// ``` + /// # Arguments + /// * `len` - The length of the padded span + /// + /// # Returns + /// * A span of length `len` right padded with 0s if the span length is less than `len`, returns + /// a span of length `len` if the span length is greater than `len` then the data is truncated + /// from the right side + fn pad_right_with_zeroes(self: Span, len: usize) -> Span { + if self.len() >= len { + return self.slice(0, len); + } + + // Create a new array with the original data + let mut arr = array![]; + for i in self { + arr.append(*i); + }; + + // Pad with zeroes + while arr.len() != len { + arr.append(0); + }; + + arr.span() + } + + /// Clones and pads the given span with 0s to the given length, if data is more than the given /// length, it is truncated from the right side # Examples /// ``` @@ -2258,6 +2301,33 @@ mod tests { assert_eq!(result, expected); } + + #[test] + fn test_pad_right_with_zeroes_len_10() { + let span = [0x01, 0x02, 0x03, 0x04, 0x05].span(); + let expected = [0x01, 0x02, 0x03, 0x04, 0x05, 0x0, 0x0, 0x0, 0x0, 0x0].span(); + let result = span.pad_right_with_zeroes(10); + + assert_eq!(result, expected); + } + + #[test] + fn test_pad_right_with_zeroes_truncate() { + let span = [0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a].span(); + let expected = [0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09].span(); + let result = span.pad_right_with_zeroes(9); + + assert_eq!(result, expected); + } + + #[test] + fn test_pad_right_with_zeroes_same_length() { + let span = [0x01, 0x02, 0x03, 0x04, 0x05].span(); + let expected = [0x01, 0x02, 0x03, 0x04, 0x05].span(); + let result = span.pad_right_with_zeroes(5); + + assert_eq!(result, expected); + } } From 8a78323e8d5d5547509cb3565c5940d5305b58a6 Mon Sep 17 00:00:00 2001 From: enitrat Date: Mon, 16 Sep 2024 18:19:01 +0200 Subject: [PATCH 2/3] fmt --- crates/evm/src/backend/validation.cairo | 3 +-- crates/evm/src/instructions/system_operations.cairo | 10 ++++------ crates/utils/src/eth_transaction.cairo | 2 +- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/evm/src/backend/validation.cairo b/crates/evm/src/backend/validation.cairo index 0c7a4ab3d..4329363f1 100644 --- a/crates/evm/src/backend/validation.cairo +++ b/crates/evm/src/backend/validation.cairo @@ -1,4 +1,3 @@ -use contracts::IKakarotCore; use contracts::account_contract::{IAccountDispatcher, IAccountDispatcherTrait}; use contracts::kakarot_core::KakarotCore; use core::ops::SnapshotDeref; @@ -78,7 +77,7 @@ mod tests { use utils::constants::BLOCK_GAS_LIMIT; use utils::eth_transaction::common::TxKind; use utils::eth_transaction::eip1559::TxEip1559; - use utils::eth_transaction::transaction::{Transaction, TransactionTrait}; + use utils::eth_transaction::transaction::Transaction; fn set_up() -> KakarotCore::ContractState { // Define the addresses used in the tests, whose calls will be mocked diff --git a/crates/evm/src/instructions/system_operations.cairo b/crates/evm/src/instructions/system_operations.cairo index 50436dc5c..9b734faac 100644 --- a/crates/evm/src/instructions/system_operations.cairo +++ b/crates/evm/src/instructions/system_operations.cairo @@ -493,13 +493,11 @@ mod tests { vm.env.state.set_account(caller_account); // Deploy bytecode at 0xabfa740ccd - // (+ 0x1 0x1) + // SSTORE 0x42 at 0x42 let eth_address: EthAddress = 0xabfa740ccd_u256.into(); let starknet_address = compute_starknet_address( test_address(), eth_address, 0.try_into().unwrap() ); - // Deploy bytecode at 0x1234 - // SSTORE 0x42 at 0x42 let deployed_bytecode = [ 0x60, 0x01, @@ -547,7 +545,7 @@ mod tests { // Given // Set vm bytecode - // (call 0xffffff 0xabfa740ccd 0 0 0 0 1) + // (staticcall 0xffffff 0xabfa740ccd 0 0 0 0 1) let bytecode = [ 0x60, 0x01, @@ -586,12 +584,12 @@ mod tests { }; vm.env.state.set_account(caller_account); + // Deploy bytecode at 0xabfa740ccd + // SSTORE 0x42 at 0x42 let eth_address: EthAddress = 0xabfa740ccd_u256.into(); let starknet_address = compute_starknet_address( test_address(), eth_address, 0.try_into().unwrap() ); - // Deploy bytecode at 0x1234 - // SSTORE 0x42 at 0x42 let deployed_bytecode = [ 0x60, 0x01, diff --git a/crates/utils/src/eth_transaction.cairo b/crates/utils/src/eth_transaction.cairo index 585264db5..1f5d7a7d0 100644 --- a/crates/utils/src/eth_transaction.cairo +++ b/crates/utils/src/eth_transaction.cairo @@ -5,7 +5,7 @@ pub mod legacy; pub mod transaction; pub mod tx_type; use core::cmp::min; -use core::num::traits::{CheckedAdd, Zero}; +use core::num::traits::CheckedAdd; use core::option::OptionTrait; use core::starknet::{EthAddress, secp256_trait::Signature,}; use utils::errors::{EthTransactionError, RLPErrorImpl}; From a7ced20e30366d5c15f4793d4fe13e9093d0ac12 Mon Sep 17 00:00:00 2001 From: Mathieu <60658558+enitrat@users.noreply.github.com> Date: Tue, 17 Sep 2024 10:09:10 +0200 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Oba --- .../src/instructions/stop_and_arithmetic_operations.cairo | 6 +++--- crates/evm/src/instructions/system_operations.cairo | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo b/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo index cbad783f9..08f91b7cc 100644 --- a/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo +++ b/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo @@ -15,10 +15,10 @@ pub impl StopAndArithmeticOperations of StopAndArithmeticOperationsTrait { /// Halts the execution of the current program. /// # Specification: https://www.evm.codes/#00?fork=shanghai fn exec_stop(ref self: VM) { - // return_data stored the return_data for the last executed sub context - // see CALLs opcodes. When we run the STOP opcode, we stop the current + // return_data store the return_data for the last executed sub context + // see CALLs opcodes. When it runs the STOP opcode, it stops the current // execution context with *no* return data (unlike RETURN and REVERT). - // hence we just clear the return_data and stop. + // hence it just clear the return_data and stop. self.return_data = [].span(); self.stop(); } diff --git a/crates/evm/src/instructions/system_operations.cairo b/crates/evm/src/instructions/system_operations.cairo index 9b734faac..37799f2ee 100644 --- a/crates/evm/src/instructions/system_operations.cairo +++ b/crates/evm/src/instructions/system_operations.cairo @@ -541,7 +541,7 @@ mod tests { } #[test] - fn test_exec_staticcall() { + fn test_should_fail_exec_staticcall() { // Given // Set vm bytecode