From 0629883f5a40e913a5d9498fa37886348c858c70 Mon Sep 17 00:00:00 2001 From: Luca Provini Date: Wed, 17 Jan 2024 13:55:32 +0100 Subject: [PATCH] refactor(Inspector): Add CreateOutcome in create/create_end return (#980) * rename main Evm structs, introduce wip external type * tests * Split evm and external context * continue previous commit * wip inspector handle register * add few more handlers for frame and host * Add instruction handle * add instruction handler registration and Inspector wrap * Rm Spec generic, more handlers, start factory * move towards the builder, allow EVM modify * wip on EvmBuilder and modify functionality * EvmBuilder with stages wip * Add wip stages for builer * make handle register simple function, add raw instruction table, split external data from registers * wip on simple builder functions and handler registry * Examples and cleanup * fix lifetime and fmt * Add more handlers, deduct caller, validate tx agains state * All handlers counted, started on docs, some cleanup * renaming and docs * Support all Inspector functionality with Handler * Handler restructured. Documentation added * more docs on registers * integrate builder, fmt, move optimism l1block * add utility builder stage functions * add precompiles, fix bugs with journal spec * spec to generic, optimism build * fix optimism test * fuck macros * clippy and fmt * fix trace block example * ci fixes * Flatten builder stages to generic and handler stage * EvmBuilder doc and refactor fn access * ignore rust code in book * make revme compile, will refactor this in future * Rename handles to Pre/Post Execution and ExecutionLoop * fix optimism clippy * small rename * FrameData and docs * check links mdbook * comments and cleanup * comment * Add initialize interepreter to first frame * clippy * clippy2 * feat: create outcome * fix: createOutcome properties to pub and removed wrapper functions * review: fixed some review comments and added some improvement over instruction results * fix: removed unused is_revert method * review: adjusted comments, moved create_outcome to its own file * fix: no std check --------- Co-authored-by: rakita --- crates/interpreter/src/create_outcome.rs | 68 +++++++++++++++++++ crates/interpreter/src/interpreter.rs | 53 +++++++++++---- crates/interpreter/src/lib.rs | 2 + .../src/handler/mainnet/execution_loop.rs | 8 ++- crates/revm/src/inspector.rs | 8 +-- crates/revm/src/inspector/customprinter.rs | 5 +- crates/revm/src/inspector/eip3155.rs | 5 +- crates/revm/src/inspector/gas.rs | 12 ++-- crates/revm/src/inspector/handler_register.rs | 26 ++++--- 9 files changed, 149 insertions(+), 38 deletions(-) create mode 100644 crates/interpreter/src/create_outcome.rs diff --git a/crates/interpreter/src/create_outcome.rs b/crates/interpreter/src/create_outcome.rs new file mode 100644 index 0000000000..41c456b173 --- /dev/null +++ b/crates/interpreter/src/create_outcome.rs @@ -0,0 +1,68 @@ +use crate::{Gas, InstructionResult, InterpreterResult}; +use revm_primitives::{Address, Bytes}; + +/// Represents the outcome of a create operation in an interpreter. +/// +/// This struct holds the result of the operation along with an optional address. +/// It provides methods to determine the next action based on the result of the operation. +pub struct CreateOutcome { + // The result of the interpreter operation. + pub result: InterpreterResult, + // An optional address associated with the create operation. + pub address: Option
, +} + +impl CreateOutcome { + /// Constructs a new `CreateOutcome`. + /// + /// # Arguments + /// + /// * `result` - An `InterpreterResult` representing the result of the interpreter operation. + /// * `address` - An optional `Address` associated with the create operation. + /// + /// # Returns + /// + /// A new `CreateOutcome` instance. + pub fn new(result: InterpreterResult, address: Option
) -> Self { + Self { result, address } + } + + /// Retrieves a reference to the `InstructionResult` from the `InterpreterResult`. + /// + /// This method provides access to the `InstructionResult` which represents the + /// outcome of the instruction execution. It encapsulates the result information + /// such as whether the instruction was executed successfully, resulted in a revert, + /// or encountered a fatal error. + /// + /// # Returns + /// + /// A reference to the `InstructionResult`. + pub fn instruction_result(&self) -> &InstructionResult { + &self.result.result + } + + /// Retrieves a reference to the output bytes from the `InterpreterResult`. + /// + /// This method returns the output of the interpreted operation. The output is + /// typically used when the operation successfully completes and returns data. + /// + /// # Returns + /// + /// A reference to the output `Bytes`. + pub fn output(&self) -> &Bytes { + &self.result.output + } + + /// Retrieves a reference to the `Gas` details from the `InterpreterResult`. + /// + /// This method provides access to the gas details of the operation, which includes + /// information about gas used, remaining, and refunded. It is essential for + /// understanding the gas consumption of the operation. + /// + /// # Returns + /// + /// A reference to the `Gas` details. + pub fn gas(&self) -> &Gas { + &self.result.gas + } +} diff --git a/crates/interpreter/src/interpreter.rs b/crates/interpreter/src/interpreter.rs index 1caed8a239..da6433b325 100644 --- a/crates/interpreter/src/interpreter.rs +++ b/crates/interpreter/src/interpreter.rs @@ -8,14 +8,15 @@ pub use contract::Contract; pub use shared_memory::{next_multiple_of_32, SharedMemory}; pub use stack::{Stack, STACK_LIMIT}; +use crate::alloc::borrow::ToOwned; use crate::{ - primitives::Bytes, push, push_b256, return_ok, return_revert, CallInputs, CreateInputs, Gas, - Host, InstructionResult, + primitives::Bytes, push, push_b256, return_ok, return_revert, CallInputs, CreateInputs, + CreateOutcome, Gas, Host, InstructionResult, }; use alloc::boxed::Box; use core::cmp::min; use core::ops::Range; -use revm_primitives::{Address, U256}; +use revm_primitives::U256; pub use self::shared_memory::EMPTY_SHARED_MEMORY; @@ -93,24 +94,52 @@ impl Interpreter { } } - /// When sub create call returns we can insert output of that call into this interpreter. - pub fn insert_create_output(&mut self, result: InterpreterResult, address: Option
) { - self.return_data_buffer = match result.result { + /// Inserts the output of a `create` call into the interpreter. + /// + /// This function is used after a `create` call has been executed. It processes the outcome + /// of that call and updates the state of the interpreter accordingly. + /// + /// # Arguments + /// + /// * `create_outcome` - A `CreateOutcome` struct containing the results of the `create` call. + /// + /// # Behavior + /// + /// The function updates the `return_data_buffer` with the data from `create_outcome`. + /// Depending on the `InstructionResult` indicated by `create_outcome`, it performs one of the following: + /// + /// - `Ok`: Pushes the address from `create_outcome` to the stack, updates gas costs, and records any gas refunds. + /// - `Revert`: Pushes `U256::ZERO` to the stack and updates gas costs. + /// - `FatalExternalError`: Sets the `instruction_result` to `InstructionResult::FatalExternalError`. + /// - `Default`: Pushes `U256::ZERO` to the stack. + /// + /// # Side Effects + /// + /// - Updates `return_data_buffer` with the data from `create_outcome`. + /// - Modifies the stack by pushing values depending on the `InstructionResult`. + /// - Updates gas costs and records refunds in the interpreter's `gas` field. + /// - May alter `instruction_result` in case of external errors. + pub fn insert_create_outcome(&mut self, create_outcome: CreateOutcome) { + let instruction_result = create_outcome.instruction_result(); + + self.return_data_buffer = if instruction_result.is_revert() { // Save data to return data buffer if the create reverted - return_revert!() => result.output, + create_outcome.output().to_owned() + } else { // Otherwise clear it - _ => Bytes::new(), + Bytes::new() }; - match result.result { + match instruction_result { return_ok!() => { + let address = create_outcome.address; push_b256!(self, address.unwrap_or_default().into_word()); - self.gas.erase_cost(result.gas.remaining()); - self.gas.record_refund(result.gas.refunded()); + self.gas.erase_cost(create_outcome.gas().remaining()); + self.gas.record_refund(create_outcome.gas().refunded()); } return_revert!() => { push!(self, U256::ZERO); - self.gas.erase_cost(result.gas.remaining()); + self.gas.erase_cost(create_outcome.gas().remaining()); } InstructionResult::FatalExternalError => { self.instruction_result = InstructionResult::FatalExternalError; diff --git a/crates/interpreter/src/lib.rs b/crates/interpreter/src/lib.rs index eac96926bc..e63cbb3a1b 100644 --- a/crates/interpreter/src/lib.rs +++ b/crates/interpreter/src/lib.rs @@ -12,6 +12,7 @@ extern crate alloc; #[macro_use] mod macros; +mod create_outcome; pub mod gas; mod host; mod inner_models; @@ -20,6 +21,7 @@ pub mod instructions; mod interpreter; // Reexport primary types. +pub use create_outcome::CreateOutcome; pub use gas::Gas; pub use host::{DummyHost, Host}; pub use inner_models::*; diff --git a/crates/revm/src/handler/mainnet/execution_loop.rs b/crates/revm/src/handler/mainnet/execution_loop.rs index 9de9d2e37d..b9a1c4c87f 100644 --- a/crates/revm/src/handler/mainnet/execution_loop.rs +++ b/crates/revm/src/handler/mainnet/execution_loop.rs @@ -1,7 +1,7 @@ use crate::{ db::Database, interpreter::{ - return_ok, return_revert, CallInputs, CreateInputs, Gas, InstructionResult, + return_ok, return_revert, CallInputs, CreateInputs, CreateOutcome, Gas, InstructionResult, InterpreterResult, SharedMemory, }, primitives::{Env, Spec, TransactTo}, @@ -92,9 +92,10 @@ pub fn frame_return( let Some(parent_stack_frame) = parent_stack_frame else { return Some(result); }; + let create_outcome = CreateOutcome::new(result, Some(created_address)); parent_stack_frame .interpreter - .insert_create_output(result, Some(created_address)) + .insert_create_outcome(create_outcome) } FrameData::Call { return_memory_range, @@ -151,10 +152,11 @@ pub fn sub_create( match context.evm.make_create_frame(SPEC::SPEC_ID, &inputs) { FrameOrResult::Frame(new_frame) => Some(new_frame), FrameOrResult::Result(result) => { + let create_outcome = CreateOutcome::new(result, None); // insert result of the failed creation of create CallStackFrame. curent_stack_frame .interpreter - .insert_create_output(result, None); + .insert_create_outcome(create_outcome); None } } diff --git a/crates/revm/src/inspector.rs b/crates/revm/src/inspector.rs index 60ce286ce6..ea4c87243a 100644 --- a/crates/revm/src/inspector.rs +++ b/crates/revm/src/inspector.rs @@ -18,7 +18,7 @@ mod noop; // Exports. pub use handler_register::{inspector_handle_register, inspector_instruction, GetInspector}; -use revm_interpreter::InterpreterResult; +use revm_interpreter::{CreateOutcome, InterpreterResult}; /// [Inspector] implementations. pub mod inspectors { #[cfg(feature = "std")] @@ -109,7 +109,7 @@ pub trait Inspector { &mut self, context: &mut EvmContext, inputs: &mut CreateInputs, - ) -> Option<(InterpreterResult, Option
)> { + ) -> Option { let _ = context; let _ = inputs; None @@ -125,9 +125,9 @@ pub trait Inspector { context: &mut EvmContext, result: InterpreterResult, address: Option
, - ) -> (InterpreterResult, Option
) { + ) -> CreateOutcome { let _ = context; - (result, address) + CreateOutcome::new(result, address) } /// Called when a contract has been self-destructed with funds transferred to target. diff --git a/crates/revm/src/inspector/customprinter.rs b/crates/revm/src/inspector/customprinter.rs index fc65544b56..0ad14995c7 100644 --- a/crates/revm/src/inspector/customprinter.rs +++ b/crates/revm/src/inspector/customprinter.rs @@ -2,6 +2,7 @@ //! It is a great tool if some debugging is needed. use core::ops::Range; +use revm_interpreter::CreateOutcome; use crate::{ inspectors::GasInspector, @@ -73,7 +74,7 @@ impl Inspector for CustomPrintTracer { context: &mut EvmContext, result: InterpreterResult, address: Option
, - ) -> (InterpreterResult, Option
) { + ) -> CreateOutcome { self.gas_inspector.create_end(context, result, address) } @@ -97,7 +98,7 @@ impl Inspector for CustomPrintTracer { &mut self, _context: &mut EvmContext, inputs: &mut CreateInputs, - ) -> Option<(InterpreterResult, Option
)> { + ) -> Option { println!( "CREATE CALL: caller:{:?}, scheme:{:?}, value:{:?}, init_code:{:?}, gas:{:?}", inputs.caller, inputs.scheme, inputs.value, inputs.init_code, inputs.gas_limit diff --git a/crates/revm/src/inspector/eip3155.rs b/crates/revm/src/inspector/eip3155.rs index 22269082a3..7ec60d51ea 100644 --- a/crates/revm/src/inspector/eip3155.rs +++ b/crates/revm/src/inspector/eip3155.rs @@ -5,6 +5,7 @@ use crate::{ EvmContext, GetInspector, Inspector, }; use core::ops::Range; +use revm_interpreter::CreateOutcome; use serde_json::json; use std::io::Write; @@ -118,7 +119,7 @@ impl Inspector for TracerEip3155 { &mut self, _context: &mut EvmContext, _inputs: &mut CreateInputs, - ) -> Option<(InterpreterResult, Option
)> { + ) -> Option { None } @@ -127,7 +128,7 @@ impl Inspector for TracerEip3155 { context: &mut EvmContext, result: InterpreterResult, address: Option
, - ) -> (InterpreterResult, Option
) { + ) -> CreateOutcome { self.gas_inspector.create_end(context, result, address) } } diff --git a/crates/revm/src/inspector/gas.rs b/crates/revm/src/inspector/gas.rs index f1266177e5..1a83afae35 100644 --- a/crates/revm/src/inspector/gas.rs +++ b/crates/revm/src/inspector/gas.rs @@ -1,5 +1,7 @@ //! GasIspector. Helper Inspector to calculate gas for others. +use revm_interpreter::CreateOutcome; + use crate::{ interpreter::InterpreterResult, primitives::{db::Database, Address}, @@ -59,13 +61,15 @@ impl Inspector for GasInspector { _context: &mut EvmContext, result: InterpreterResult, address: Option
, - ) -> (InterpreterResult, Option
) { - (result, address) + ) -> CreateOutcome { + CreateOutcome::new(result, address) } } #[cfg(test)] mod tests { + use revm_interpreter::CreateOutcome; + use crate::{ inspector::GetInspector, inspectors::GasInspector, @@ -128,7 +132,7 @@ mod tests { &mut self, context: &mut EvmContext, call: &mut CreateInputs, - ) -> Option<(InterpreterResult, Option
)> { + ) -> Option { self.gas_inspector.create(context, call); None } @@ -138,7 +142,7 @@ mod tests { context: &mut EvmContext, result: InterpreterResult, address: Option
, - ) -> (InterpreterResult, Option
) { + ) -> CreateOutcome { self.gas_inspector.create_end(context, result, address) } } diff --git a/crates/revm/src/inspector/handler_register.rs b/crates/revm/src/inspector/handler_register.rs index d46d52a8fe..ea66a88c94 100644 --- a/crates/revm/src/inspector/handler_register.rs +++ b/crates/revm/src/inspector/handler_register.rs @@ -119,7 +119,7 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector<'a, DB>>( .get_inspector() .create(&mut context.evm, &mut create_inputs) { - return FrameOrResult::Result(output.0); + return FrameOrResult::Result(output.result); }; context.evm.make_create_frame(spec_id, &create_inputs) } @@ -177,8 +177,8 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector<'a, DB>>( handler.execution_loop.sub_create = Arc::new( move |context, frame, mut inputs| -> Option> { let inspector = context.external.get_inspector(); - if let Some((result, address)) = inspector.create(&mut context.evm, &mut inputs) { - frame.interpreter.insert_create_output(result, address); + if let Some(create_outcome) = inspector.create(&mut context.evm, &mut inputs) { + frame.interpreter.insert_create_outcome(create_outcome); return None; } @@ -188,10 +188,10 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector<'a, DB>>( Some(new_frame) } FrameOrResult::Result(result) => { - let (result, address) = + let create_outcome = inspector.create_end(&mut context.evm, result, frame.created_address()); // insert result of the failed creation of create CallStackFrame. - frame.interpreter.insert_create_output(result, address); + frame.interpreter.insert_create_outcome(create_outcome); None } } @@ -234,9 +234,12 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector<'a, DB>>( let inspector = &mut context.external.get_inspector(); result = match &mut child.frame_data { FrameData::Create { created_address } => { - let (result, address) = - inspector.create_end(&mut context.evm, result, Some(*created_address)); - if let Some(address) = address { + let create_outcome = inspector.create_end( + &mut context.evm, + result.clone(), + Some(*created_address), + ); + if let Some(address) = create_outcome.address { *created_address = address; } result @@ -297,6 +300,7 @@ mod tests { Database, Evm, EvmContext, Inspector, }; use core::ops::Range; + use revm_interpreter::CreateOutcome; #[test] fn test_make_boxed_instruction_table() { @@ -369,7 +373,7 @@ mod tests { &mut self, _context: &mut EvmContext, _call: &mut CreateInputs, - ) -> Option<(InterpreterResult, Option
)> { + ) -> Option { None } @@ -378,8 +382,8 @@ mod tests { _context: &mut EvmContext, result: InterpreterResult, address: Option
, - ) -> (InterpreterResult, Option
) { - (result, address) + ) -> CreateOutcome { + CreateOutcome::new(result, address) } }