diff --git a/crates/evm/src/context.cairo b/crates/evm/src/context.cairo index b20e50d7c..5becd1439 100644 --- a/crates/evm/src/context.cairo +++ b/crates/evm/src/context.cairo @@ -140,9 +140,9 @@ struct ExecutionContext { destroyed_contracts: Array, events: Array, create_addresses: Array, - return_data: Array, + // Return data of a child context. + return_data: Span, parent_ctx: Nullable, - child_return_data: Option>, } impl DefaultBoxExecutionContext of Default> { @@ -165,8 +165,7 @@ impl ExecutionContextImpl of ExecutionContextTrait { starknet_address: ContractAddress, call_ctx: CallContext, parent_ctx: Nullable, - child_return_data: Option>, - return_data: Array, + return_data: Span, ) -> ExecutionContext { ExecutionContext { id, @@ -180,7 +179,6 @@ impl ExecutionContextImpl of ExecutionContextTrait { create_addresses: Default::default(), return_data, parent_ctx, - child_return_data, } } @@ -232,12 +230,12 @@ impl ExecutionContextImpl of ExecutionContextTrait { #[inline(always)] fn return_data(self: @ExecutionContext) -> Span { - self.return_data.span() + *self.return_data } /// Stops the current execution context. #[inline(always)] - fn stop(ref self: ExecutionContext) { + fn set_stopped(ref self: ExecutionContext) { self.status = Status::Stopped; } @@ -247,9 +245,8 @@ impl ExecutionContextImpl of ExecutionContextTrait { /// (it is stopped) and contract creation and contract storage writes are /// reverted on its finalization. #[inline(always)] - fn revert(ref self: ExecutionContext, revert_reason: Span) { + fn set_reverted(ref self: ExecutionContext) { self.status = Status::Reverted; - ArrayExtensionTrait::concat(ref self.return_data, revert_reason); } // ************************************************************************* @@ -301,11 +298,6 @@ impl ExecutionContextImpl of ExecutionContextTrait { 'print debug'.print(); } - #[inline(always)] - fn set_return_data(ref self: ExecutionContext, value: Array) { - self.return_data = value; - } - #[inline(always)] fn set_pc(ref self: ExecutionContext, value: u32) { self.program_counter = value; @@ -316,10 +308,6 @@ impl ExecutionContextImpl of ExecutionContextTrait { *self.program_counter } - #[inline(always)] - fn child_return_data(self: @ExecutionContext) -> Option> { - *self.child_return_data - } #[inline(always)] fn append_event(ref self: ExecutionContext, event: Event) { diff --git a/crates/evm/src/instructions/environmental_information.cairo b/crates/evm/src/instructions/environmental_information.cairo index 066295933..0e7e0ca95 100644 --- a/crates/evm/src/instructions/environmental_information.cairo +++ b/crates/evm/src/instructions/environmental_information.cairo @@ -165,7 +165,7 @@ impl EnvironmentInformationImpl of EnvironmentInformationTrait { /// Get the size of return data. /// # Specification: https://www.evm.codes/#3d?fork=shanghai fn exec_returndatasize(ref self: Machine) -> Result<(), EVMError> { - let size: u32 = self.return_data().len(); + let size = self.return_data().len(); self.stack.push(size.into()) } diff --git a/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo b/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo index 21d3e3a89..319a00f34 100644 --- a/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo +++ b/crates/evm/src/instructions/stop_and_arithmetic_operations.cairo @@ -17,7 +17,7 @@ impl StopAndArithmeticOperations of StopAndArithmeticOperationsTrait { /// Halts the execution of the current program. /// # Specification: https://www.evm.codes/#00?fork=shanghai fn exec_stop(ref self: Machine) -> Result<(), EVMError> { - self.stop(); + self.set_stopped(); Result::Ok(()) } diff --git a/crates/evm/src/instructions/system_operations.cairo b/crates/evm/src/instructions/system_operations.cairo index e56dc8bc6..5245ff2e2 100644 --- a/crates/evm/src/instructions/system_operations.cairo +++ b/crates/evm/src/instructions/system_operations.cairo @@ -32,10 +32,10 @@ impl SystemOperations of SystemOperationsTrait { fn exec_return(ref self: Machine) -> Result<(), EVMError> { let offset = self.stack.pop_usize()?; let size = self.stack.pop_usize()?; - let mut return_data = array![]; + let mut return_data = Default::default(); self.memory.load_n(size, ref return_data, offset); - self.set_return_data(return_data); - self.stop(); + self.set_return_data(return_data.span()); + self.set_stopped(); Result::Ok(()) } diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index f4f202298..1fe53a3b0 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -2,6 +2,7 @@ /// Internal imports. use evm::context::{CallContextTrait, Status}; +use evm::context::{ExecutionContextTrait, ExecutionContext}; use evm::errors::{EVMError, PC_OUT_OF_BOUNDS}; use evm::instructions::{ duplication_operations, environmental_information, ExchangeOperationsTrait, logging_operations, @@ -24,7 +25,7 @@ trait EVMInterpreterTrait { fn run(ref self: EVMInterpreter, ref machine: Machine); /// Decodes the opcode at `pc` and executes the associated function. fn decode_and_execute(ref self: EVMInterpreter, ref machine: Machine) -> Result<(), EVMError>; - fn handle_revert(ref self: EVMInterpreter, ref machine: Machine); + fn finalize_revert(ref self: EVMInterpreter, ref machine: Machine); } @@ -49,16 +50,16 @@ impl EVMInterpreterImpl of EVMInterpreterTrait { machine.storage_journal.finalize_local(); if machine.is_root() { machine.storage_journal.finalize_global(); - } + }; }, - Status::Reverted => { self.handle_revert(ref machine); } + Status::Reverted => { self.finalize_revert(ref machine); } } }, Result::Err(error) => { // If an error occurred, revert execution machine. // Currently, revert reason is a Span. - machine.revert(u256_to_bytes_array(error.into()).span()); - self.handle_revert(ref machine); + machine.set_reverted(); + self.finalize_revert(ref machine); } } } @@ -656,9 +657,9 @@ impl EVMInterpreterImpl of EVMInterpreterTrait { return Result::Err(EVMError::UnknownOpcode(opcode)); } - /// Handles the revert of an execution context. + /// Finalizes the revert of an execution context. /// Clears all pending journal entries, not finalizing the pending state changes applied inside this context. - fn handle_revert(ref self: EVMInterpreter, ref machine: Machine) { + fn finalize_revert(ref self: EVMInterpreter, ref machine: Machine) { machine.storage_journal.clear_local(); //TODO add the rest of the revert handling. } diff --git a/crates/evm/src/machine.cairo b/crates/evm/src/machine.cairo index 79b370199..14c79d72b 100644 --- a/crates/evm/src/machine.cairo +++ b/crates/evm/src/machine.cairo @@ -6,6 +6,7 @@ use evm::{ }, stack::{Stack, StackTrait}, memory::{Memory, MemoryTrait} }; +use nullable::{match_nullable, FromNullableResult}; use starknet::{EthAddress, ContractAddress}; @@ -77,9 +78,9 @@ impl MachineCurrentContextImpl of MachineCurrentContextTrait { } #[inline(always)] - fn revert(ref self: Machine, revert_reason: Span) { + fn set_reverted(ref self: Machine) { let mut current_execution_ctx = self.current_ctx.unbox(); - current_execution_ctx.revert(revert_reason); + current_execution_ctx.set_reverted(); self.current_ctx = BoxTrait::new(current_execution_ctx); } @@ -144,14 +145,14 @@ impl MachineCurrentContextImpl of MachineCurrentContextTrait { #[inline(always)] fn return_data(ref self: Machine) -> Span { let current_execution_ctx = self.current_ctx.unbox(); - let return_data = current_execution_ctx.return_data.span(); + let return_data = current_execution_ctx.return_data; self.current_ctx = BoxTrait::new(current_execution_ctx); return_data } /// Stops the current execution context. #[inline(always)] - fn stop(ref self: Machine) { + fn set_stopped(ref self: Machine) { let mut current_execution_ctx = self.current_ctx.unbox(); current_execution_ctx.status = Status::Stopped; self.current_ctx = BoxTrait::new(current_execution_ctx); @@ -258,20 +259,27 @@ impl MachineCurrentContextImpl of MachineCurrentContextTrait { is_root } + /// Sets the `return_data` field of the appropriate execution context, + /// taking into acount EVM specs: If the current context is the root + /// context, sets the return_data field of the root context. If the current + /// context is a subcontext, sets the return_data field of the parent. + /// Should be called when returning from a context. #[inline(always)] - fn set_return_data(ref self: Machine, value: Array) { - let mut current_execution_ctx = self.current_ctx.unbox(); - current_execution_ctx.return_data = value; - self.current_ctx = BoxTrait::new(current_execution_ctx); - } - - /// Getter for the return data of a child context, accessed from its parent context - /// Enabler for RETURNDATASIZE and RETURNDATACOPY opcodes - #[inline(always)] - fn child_return_data(ref self: Machine) -> Option> { - let mut current_execution_ctx = self.current_ctx.unbox(); - let child_return_data = current_execution_ctx.child_return_data(); - self.current_ctx = BoxTrait::new(current_execution_ctx); - child_return_data + fn set_return_data(ref self: Machine, value: Span) { + let mut current_ctx = self.current_ctx.unbox(); + let maybe_parent_ctx = current_ctx.parent_ctx; + match match_nullable(maybe_parent_ctx) { + // Due to ownership mechanism, both branches need to explicitly re-bind the parent_ctx. + FromNullableResult::Null => { + current_ctx.return_data = value; + current_ctx.parent_ctx = Default::default(); + }, + FromNullableResult::NotNull(parent_ctx) => { + let mut parent_ctx = parent_ctx.unbox(); + parent_ctx.return_data = value; + current_ctx.parent_ctx = NullableTrait::new(parent_ctx); + } + } + self.current_ctx = BoxTrait::new(current_ctx); } } diff --git a/crates/evm/src/tests/test_execution_context.cairo b/crates/evm/src/tests/test_execution_context.cairo index 35422cac1..ae8926d4b 100644 --- a/crates/evm/src/tests/test_execution_context.cairo +++ b/crates/evm/src/tests/test_execution_context.cairo @@ -72,13 +72,7 @@ fn test_execution_context_new() { // When let mut execution_context = ExecutionContextTrait::new( - context_id, - evm_address, - starknet_address, - call_ctx, - parent_ctx, - Default::default(), - return_data + context_id, evm_address, starknet_address, call_ctx, parent_ctx, return_data.span() ); // Then @@ -107,7 +101,7 @@ fn test_execution_context_stop_and_revert() { let mut execution_context = setup_execution_context(); // When - execution_context.stop(); + execution_context.set_stopped(); // Then assert(execution_context.stopped() == true, 'should be stopped'); @@ -121,11 +115,10 @@ fn test_execution_context_revert() { // When let revert_reason = array![0, 1, 2, 3].span(); - execution_context.revert(revert_reason); + execution_context.set_reverted(); // Then assert(execution_context.reverted() == true, 'should be reverted'); - assert(execution_context.return_data() == revert_reason, 'wrong revert reason'); } #[test] @@ -157,20 +150,6 @@ fn test_is_root() { } -#[test] -#[available_gas(300000)] -fn test_child_return_data() { - // Given - let mut execution_context = setup_execution_context(); - - // When - let child_return_data = execution_context.child_return_data().unwrap(); - - // Then - assert(child_return_data == array![1, 2, 3].span(), 'wrong child_return_data'); -} - - #[test] #[available_gas(300000)] fn test_origin() { @@ -181,5 +160,5 @@ fn test_origin() { let origin = execution_context.origin(); // Then - assert(origin == evm_address(), 'wrong child_return_data'); + assert(origin == evm_address(), 'wrong origin'); } diff --git a/crates/evm/src/tests/test_instructions/test_environment_information.cairo b/crates/evm/src/tests/test_instructions/test_environment_information.cairo index d5f2e15b9..1090efa5f 100644 --- a/crates/evm/src/tests/test_instructions/test_environment_information.cairo +++ b/crates/evm/src/tests/test_instructions/test_environment_information.cairo @@ -6,7 +6,7 @@ use evm::memory::{InternalMemoryTrait, MemoryTrait}; use evm::stack::StackTrait; use evm::tests::test_utils::{ setup_machine, setup_machine_with_calldata, setup_machine_with_bytecode, evm_address, callvalue, - setup_machine_with_nested_execution_context, other_evm_address + setup_machine_with_nested_execution_context, other_evm_address, return_from_subcontext }; use integer::u32_overflowing_add; @@ -473,8 +473,8 @@ fn test_returndatasize() { // Given let return_data: Array = array![1, 2, 3, 4, 5]; let size = return_data.len(); - let mut machine = setup_machine(); - machine.set_return_data(return_data); + let mut machine = setup_machine_with_nested_execution_context(); + return_from_subcontext(ref machine, return_data.span()); machine.exec_returndatasize(); @@ -539,49 +539,49 @@ fn test_returndata_copy_with_multiple_words() { fn test_returndata_copy(dest_offset: u32, offset: u32, mut size: u32) { // Given - let mut machine = setup_machine(); - machine - .set_return_data( - array![ - 1, - 2, - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36 - ] - ); - + let mut machine = setup_machine_with_nested_execution_context(); + // Set the return data of the current context + + let return_data = array![ + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34, + 35, + 36 + ]; + + return_from_subcontext(ref machine, return_data.span()); let return_data: Span = machine.return_data(); if (size == 0) { diff --git a/crates/evm/src/tests/test_instructions/test_system_operations.cairo b/crates/evm/src/tests/test_instructions/test_system_operations.cairo index 5d8167615..3614911b3 100644 --- a/crates/evm/src/tests/test_instructions/test_system_operations.cairo +++ b/crates/evm/src/tests/test_instructions/test_system_operations.cairo @@ -3,14 +3,16 @@ use evm::instructions::MemoryOperationTrait; use evm::instructions::SystemOperationsTrait; use evm::machine::{Machine, MachineCurrentContextTrait}; use evm::stack::StackTrait; -use evm::tests::test_utils::setup_machine; +use evm::tests::test_utils::{ + setup_machine_with_nested_execution_context, setup_machine, parent_ctx_return_data +}; use utils::helpers::load_word; #[test] #[available_gas(20000000)] fn test_exec_return() { // Given - let mut machine = setup_machine(); + let mut machine = setup_machine_with_nested_execution_context(); // When machine.stack.push(1000); machine.stack.push(0); @@ -21,14 +23,15 @@ fn test_exec_return() { assert(machine.exec_return().is_ok(), 'Exec return failed'); // Then - assert(1000 == load_word(32, machine.return_data()), 'Wrong return_data'); + assert(1000 == load_word(32, parent_ctx_return_data(ref machine)), 'Wrong return_data'); + assert(machine.stopped(), 'machine should be stopped') } #[test] #[available_gas(20000000)] fn test_exec_return_with_offset() { // Given - let mut machine = setup_machine(); + let mut machine = setup_machine_with_nested_execution_context(); // When machine.stack.push(1); machine.stack.push(0); @@ -39,5 +42,6 @@ fn test_exec_return_with_offset() { assert(machine.exec_return().is_ok(), 'Exec return failed'); // Then - assert(256 == load_word(32, machine.return_data()), 'Wrong return_data'); + assert(256 == load_word(32, parent_ctx_return_data(ref machine)), 'Wrong return_data'); + assert(machine.stopped(), 'machine should be stopped') } diff --git a/crates/evm/src/tests/test_machine.cairo b/crates/evm/src/tests/test_machine.cairo index 64a234bae..e3497bd33 100644 --- a/crates/evm/src/tests/test_machine.cairo +++ b/crates/evm/src/tests/test_machine.cairo @@ -2,7 +2,7 @@ use evm::context::CallContextTrait; use evm::machine::{Machine, MachineCurrentContextTrait}; use evm::tests::test_utils::{ evm_address, setup_machine_with_bytecode, setup_machine, starknet_address, - setup_execution_context + setup_execution_context, setup_machine_with_nested_execution_context, parent_ctx_return_data }; @@ -55,7 +55,7 @@ fn test_set_pc() { fn test_revert() { let mut machine = Default::default(); - machine.revert(array![0xde, 0xbf].span()); + machine.set_reverted(); assert(machine.reverted(), 'ctx should be reverted'); assert(machine.stopped(), 'ctx should be stopped'); @@ -63,10 +63,10 @@ fn test_revert() { #[test] #[available_gas(20000000)] -fn test_stop() { +fn test_set_stopped() { let mut machine = Default::default(); - machine.stop(); + machine.set_stopped(); assert(!machine.reverted(), 'ctx should not be reverted'); assert(machine.stopped(), 'ctx should be stopped'); @@ -153,21 +153,30 @@ fn test_create_addresses() { assert(create_addresses.len() == 0, 'wrong length'); } + #[test] #[available_gas(20000000)] -fn test_return_data() { +fn test_set_return_data_root() { let mut machine: Machine = Default::default(); - + machine.set_return_data(array![0x01, 0x02, 0x03].span()); let return_data = machine.return_data(); - assert(return_data.len() == 0, 'wrong length'); + assert(return_data == array![0x01, 0x02, 0x03].span(), 'wrong return data'); } +#[test] +#[available_gas(20000000)] +fn test_set_return_data_subctx() { + let mut machine: Machine = setup_machine_with_nested_execution_context(); + machine.set_return_data(array![0x01, 0x02, 0x03].span()); + let return_data = parent_ctx_return_data(ref machine); + assert(return_data == array![0x01, 0x02, 0x03].span(), 'wrong return data'); +} #[test] #[available_gas(20000000)] -fn test_child_return_data() { - let mut machine: Machine = setup_machine(); +fn test_return_data() { + let mut machine: Machine = Default::default(); - let return_data = machine.child_return_data().unwrap(); - assert(return_data == array![1, 2, 3].span(), 'wrong child return data'); + let return_data = machine.return_data(); + assert(return_data.len() == 0, 'wrong length'); } diff --git a/crates/evm/src/tests/test_utils.cairo b/crates/evm/src/tests/test_utils.cairo index fd32f0fc3..944272cb3 100644 --- a/crates/evm/src/tests/test_utils.cairo +++ b/crates/evm/src/tests/test_utils.cairo @@ -4,7 +4,8 @@ use evm::context::{ }; use evm::kakarot_core::{IExtendedKakarotCoreDispatcher, KakarotCore}; -use evm::machine::Machine; +use evm::machine::{Machine, MachineCurrentContextTrait}; +use nullable::{match_nullable, FromNullableResult}; use starknet::{ StorageBaseAddress, storage_base_address_from_felt252, contract_address_try_from_felt252, ContractAddress, EthAddress, deploy_syscall, get_contract_address, contract_address_const @@ -78,17 +79,10 @@ fn setup_execution_context() -> ExecutionContext { let call_ctx = setup_call_context(); let starknet_address: ContractAddress = starknet_address(); let evm_address: EthAddress = evm_address(); - let return_data = Default::default(); - let child_return_data = Option::Some(array![1, 2, 3].span()); + let return_data = array![1, 2, 3].span(); ExecutionContextTrait::new( - context_id, - evm_address, - starknet_address, - call_ctx, - Default::default(), - child_return_data, - return_data, + context_id, evm_address, starknet_address, call_ctx, Default::default(), return_data, ) } @@ -123,16 +117,10 @@ fn setup_execution_context_with_bytecode(bytecode: Span) -> ExecutionContext let call_ctx = setup_call_context_with_bytecode(bytecode); let starknet_address: ContractAddress = starknet_address(); let evm_address: EthAddress = evm_address(); - let return_data = Default::default(); + let return_data = Default::default().span(); ExecutionContextTrait::new( - context_id, - evm_address, - starknet_address, - call_ctx, - Default::default(), - Default::default(), - return_data, + context_id, evm_address, starknet_address, call_ctx, Default::default(), return_data, ) } @@ -153,16 +141,10 @@ fn setup_execution_context_with_calldata(calldata: Span) -> ExecutionContext let call_ctx = setup_call_context_with_calldata(calldata); let starknet_address: ContractAddress = starknet_address(); let evm_address: EthAddress = evm_address(); - let return_data = Default::default(); + let return_data = Default::default().span(); ExecutionContextTrait::new( - context_id, - evm_address, - starknet_address, - call_ctx, - Default::default(), - Default::default(), - return_data, + context_id, evm_address, starknet_address, call_ctx, Default::default(), return_data, ) } @@ -244,3 +226,35 @@ fn deploy_kakarot_core() -> IExtendedKakarotCoreDispatcher { IExtendedKakarotCoreDispatcher { contract_address } } + + +// Simulate return of subcontext where +/// 1. Set `return_data` field of parent context +/// 2. make `parent_ctx` of `current_ctx` the current ctx +fn return_from_subcontext(ref self: Machine, return_data: Span) { + self.set_return_data(return_data); + let current_ctx = self.current_ctx.unbox(); + let parent_ctx = current_ctx.parent_ctx.deref(); + self.current_ctx = BoxTrait::new(parent_ctx); +} + +/// Returns the `return_data` field of the parent_ctx of the current_ctx. +fn parent_ctx_return_data(ref self: Machine) -> Span { + let mut current_ctx = self.current_ctx.unbox(); + let maybe_parent_ctx = current_ctx.parent_ctx; + let value = match match_nullable(maybe_parent_ctx) { + // Due to ownership mechanism, both branches need to explicitly re-bind the parent_ctx. + FromNullableResult::Null => { + current_ctx.parent_ctx = Default::default(); + array![].span() + }, + FromNullableResult::NotNull(parent_ctx) => { + let mut parent_ctx = parent_ctx.unbox(); + let value = parent_ctx.return_data(); + current_ctx.parent_ctx = NullableTrait::new(parent_ctx); + value + } + }; + self.current_ctx = BoxTrait::new(current_ctx); + value +} diff --git a/docs/general/execution_context.md b/docs/general/execution_context.md index 13dd1a63c..48dbe8124 100644 --- a/docs/general/execution_context.md +++ b/docs/general/execution_context.md @@ -25,9 +25,8 @@ classDiagram destroyed_contracts: Array~EthAddress~, events: Array~Event~, create_addresses: Array~EthAddress~, - return_data: Array~u32~, + return_data: Span~u8~, parent_ctx: Nullable~ExecutionContext~, - child_return_data: Option~Span~u8~~ } class CallContext{ diff --git a/docs/general/machine.md b/docs/general/machine.md index 71678c0cc..c6873e96a 100644 --- a/docs/general/machine.md +++ b/docs/general/machine.md @@ -28,18 +28,18 @@ To overcome the problem stated above, we have come up with the following design: it. - Each execution context has a `parent_ctx` field, which value is either a pointer to its parent execution context or `null`. -- Each execution context has a `child_return_data` field, which value is either +- Each execution context has a `return_data` field, whose value is either nothing or the return data from the child context. This is meant to enable opcodes `RETURNDATASIZE` and `RETURNDATACOPY`. These two opcodes are the only ones enabling a current context to access its child context's return data. - The execution context tree is a directed acyclic graph, where each execution - context has at most one parent, and at most one child. + context has at most one parent. - A specific execution context is accessible by traversing the execution context tree, starting from the root execution context, and following the execution context tree until the desired execution context is reached. The machine also stores a pointer to the current execution context. - The execution context tree is initialized with a single root execution - context, which has no parent and no child. It has `id` field equal to 0. + context, which has no parent and no child. It has an `id` field equal to 0. The following diagram describes the model of the Kakarot Machine.