From 70ab8b6f3ab3df01c6168f5cf22b724ddeb21547 Mon Sep 17 00:00:00 2001 From: Luca Provini Date: Thu, 18 Jan 2024 19:48:30 +0100 Subject: [PATCH] refactor(Inspector): add CallOutcome to call/call_end (#985) * 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: add callOutcome and added to inspector.call * added meaningful rust docs --- crates/interpreter/src/call_outcome.rs | 89 +++++++++++++++++++ crates/interpreter/src/interpreter.rs | 49 ++++++---- crates/interpreter/src/lib.rs | 2 + .../src/handler/mainnet/execution_loop.rs | 19 ++-- crates/revm/src/inspector.rs | 7 +- crates/revm/src/inspector/customprinter.rs | 4 +- crates/revm/src/inspector/eip3155.rs | 5 +- crates/revm/src/inspector/gas.rs | 4 +- crates/revm/src/inspector/handler_register.rs | 17 ++-- 9 files changed, 152 insertions(+), 44 deletions(-) create mode 100644 crates/interpreter/src/call_outcome.rs diff --git a/crates/interpreter/src/call_outcome.rs b/crates/interpreter/src/call_outcome.rs new file mode 100644 index 0000000000..3c6ac62a62 --- /dev/null +++ b/crates/interpreter/src/call_outcome.rs @@ -0,0 +1,89 @@ +use crate::{Gas, InstructionResult, InterpreterResult}; +use core::ops::Range; +use revm_primitives::Bytes; + +/// Represents the outcome of a call operation in a virtual machine. +/// +/// This struct encapsulates the result of executing an instruction by an interpreter, including +/// the result itself, gas usage information, and the memory offset where output data is stored. +/// +/// # Fields +/// +/// * `interpreter_result` - The result of the interpreter's execution, including output data and gas usage. +/// * `memory_offset` - The range in memory where the output data is located. +pub struct CallOutcome { + pub interpreter_result: InterpreterResult, + pub memory_offset: Range, +} + +impl CallOutcome { + /// Constructs a new `CallOutcome`. + /// + /// Creates an instance of `CallOutcome` with the given interpreter result and memory offset. + /// + /// # Arguments + /// + /// * `interpreter_result` - The result of the interpreter's execution. + /// * `memory_offset` - The range in memory indicating where the output data is stored. + pub fn new(interpreter_result: InterpreterResult, memory_offset: Range) -> Self { + Self { + interpreter_result, + memory_offset, + } + } + + /// Returns a reference to the instruction result. + /// + /// Provides access to the result of the executed instruction. + /// + /// # Returns + /// + /// A reference to the `InstructionResult`. + pub fn instruction_result(&self) -> &InstructionResult { + &self.interpreter_result.result + } + + /// Returns the gas usage information. + /// + /// Provides access to the gas usage details of the executed instruction. + /// + /// # Returns + /// + /// An instance of `Gas` representing the gas usage. + pub fn gas(&self) -> Gas { + self.interpreter_result.gas + } + + /// Returns a reference to the output data. + /// + /// Provides access to the output data generated by the executed instruction. + /// + /// # Returns + /// + /// A reference to the output data as `Bytes`. + pub fn output(&self) -> &Bytes { + &self.interpreter_result.output + } + + /// Returns the start position of the memory offset. + /// + /// Provides the starting index of the memory range where the output data is stored. + /// + /// # Returns + /// + /// The starting index of the memory offset as `usize`. + pub fn memory_start(&self) -> usize { + self.memory_offset.start + } + + /// Returns the length of the memory range. + /// + /// Provides the length of the memory range where the output data is stored. + /// + /// # Returns + /// + /// The length of the memory range as `usize`. + pub fn memory_length(&self) -> usize { + self.memory_offset.len() + } +} diff --git a/crates/interpreter/src/interpreter.rs b/crates/interpreter/src/interpreter.rs index da6433b325..2a575ba7bc 100644 --- a/crates/interpreter/src/interpreter.rs +++ b/crates/interpreter/src/interpreter.rs @@ -10,8 +10,8 @@ pub use stack::{Stack, STACK_LIMIT}; use crate::alloc::borrow::ToOwned; use crate::{ - primitives::Bytes, push, push_b256, return_ok, return_revert, CallInputs, CreateInputs, - CreateOutcome, Gas, Host, InstructionResult, + primitives::Bytes, push, push_b256, return_ok, return_revert, CallInputs, CallOutcome, + CreateInputs, CreateOutcome, Gas, Host, InstructionResult, }; use alloc::boxed::Box; use core::cmp::min; @@ -150,32 +150,51 @@ impl Interpreter { } } - /// When sub call returns we can insert output of that call into this interpreter. + /// Inserts the outcome of a call into the virtual machine's state. /// - /// Note that shared memory is required as a input field. - /// As SharedMemory inside Interpreter is taken and replaced with empty (not valid) memory. - pub fn insert_call_output( + /// This function takes the result of a call, represented by `CallOutcome`, + /// and updates the virtual machine's state accordingly. It involves updating + /// the return data buffer, handling gas accounting, and setting the memory + /// in shared storage based on the outcome of the call. + /// + /// # Arguments + /// + /// * `shared_memory` - A mutable reference to the shared memory used by the virtual machine. + /// * `call_outcome` - The outcome of the call to be processed, containing details such as + /// instruction result, gas information, and output data. + /// + /// # Behavior + /// + /// The function first copies the output data from the call outcome to the virtual machine's + /// return data buffer. It then checks the instruction result from the call outcome: + /// + /// - `return_ok!()`: Processes successful execution, refunds gas, and updates shared memory. + /// - `return_revert!()`: Handles a revert by only updating the gas usage and shared memory. + /// - `InstructionResult::FatalExternalError`: Sets the instruction result to a fatal external error. + /// - Any other result: No specific action is taken. + pub fn insert_call_outcome( &mut self, shared_memory: &mut SharedMemory, - result: InterpreterResult, - memory_return_offset: Range, + call_outcome: CallOutcome, ) { - let out_offset = memory_return_offset.start; - let out_len = memory_return_offset.len(); + let out_offset = call_outcome.memory_start(); + let out_len = call_outcome.memory_length(); - self.return_data_buffer = result.output; + self.return_data_buffer = call_outcome.output().to_owned(); let target_len = min(out_len, self.return_data_buffer.len()); - match result.result { + match call_outcome.instruction_result() { return_ok!() => { // return unspend gas. - self.gas.erase_cost(result.gas.remaining()); - self.gas.record_refund(result.gas.refunded()); + let remaining = call_outcome.gas().remaining(); + let refunded = call_outcome.gas().refunded(); + self.gas.erase_cost(remaining); + self.gas.record_refund(refunded); shared_memory.set(out_offset, &self.return_data_buffer[..target_len]); push!(self, U256::from(1)); } return_revert!() => { - self.gas.erase_cost(result.gas.remaining()); + self.gas.erase_cost(call_outcome.gas().remaining()); shared_memory.set(out_offset, &self.return_data_buffer[..target_len]); push!(self, U256::ZERO); } diff --git a/crates/interpreter/src/lib.rs b/crates/interpreter/src/lib.rs index e63cbb3a1b..5ee07a3a52 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 call_outcome; mod create_outcome; pub mod gas; mod host; @@ -21,6 +22,7 @@ pub mod instructions; mod interpreter; // Reexport primary types. +pub use call_outcome::CallOutcome; pub use create_outcome::CreateOutcome; pub use gas::Gas; pub use host::{DummyHost, Host}; diff --git a/crates/revm/src/handler/mainnet/execution_loop.rs b/crates/revm/src/handler/mainnet/execution_loop.rs index b9a1c4c87f..0d6fc314e1 100644 --- a/crates/revm/src/handler/mainnet/execution_loop.rs +++ b/crates/revm/src/handler/mainnet/execution_loop.rs @@ -9,6 +9,7 @@ use crate::{ }; use alloc::boxed::Box; use core::ops::Range; +use revm_interpreter::CallOutcome; /// Creates first frame. #[inline] @@ -106,12 +107,11 @@ pub fn frame_return( let Some(parent_stack_frame) = parent_stack_frame else { return Some(result); }; + let call_outcome = CallOutcome::new(result, return_memory_range); - parent_stack_frame.interpreter.insert_call_output( - shared_memory, - result, - return_memory_range, - ) + parent_stack_frame + .interpreter + .insert_call_outcome(shared_memory, call_outcome) } } None @@ -132,11 +132,10 @@ pub fn sub_call( { FrameOrResult::Frame(new_frame) => Some(new_frame), FrameOrResult::Result(result) => { - curent_stack_frame.interpreter.insert_call_output( - shared_memory, - result, - return_memory_offset, - ); + let call_outcome = CallOutcome::new(result, return_memory_offset); + curent_stack_frame + .interpreter + .insert_call_outcome(shared_memory, call_outcome); None } } diff --git a/crates/revm/src/inspector.rs b/crates/revm/src/inspector.rs index ea4c87243a..b7b0a165b8 100644 --- a/crates/revm/src/inspector.rs +++ b/crates/revm/src/inspector.rs @@ -1,5 +1,3 @@ -use core::ops::Range; - use crate::{ interpreter::{CallInputs, CreateInputs, Interpreter}, primitives::{db::Database, Address, Log, U256}, @@ -18,7 +16,8 @@ mod noop; // Exports. pub use handler_register::{inspector_handle_register, inspector_instruction, GetInspector}; -use revm_interpreter::{CreateOutcome, InterpreterResult}; +use revm_interpreter::{CallOutcome, CreateOutcome, InterpreterResult}; + /// [Inspector] implementations. pub mod inspectors { #[cfg(feature = "std")] @@ -81,7 +80,7 @@ pub trait Inspector { &mut self, context: &mut EvmContext, inputs: &mut CallInputs, - ) -> Option<(InterpreterResult, Range)> { + ) -> Option { let _ = context; let _ = inputs; None diff --git a/crates/revm/src/inspector/customprinter.rs b/crates/revm/src/inspector/customprinter.rs index 0ad14995c7..4446065a4a 100644 --- a/crates/revm/src/inspector/customprinter.rs +++ b/crates/revm/src/inspector/customprinter.rs @@ -1,7 +1,7 @@ //! Custom print inspector, it has step level information of execution. //! It is a great tool if some debugging is needed. -use core::ops::Range; +use revm_interpreter::CallOutcome; use revm_interpreter::CreateOutcome; use crate::{ @@ -82,7 +82,7 @@ impl Inspector for CustomPrintTracer { &mut self, _context: &mut EvmContext, inputs: &mut CallInputs, - ) -> Option<(InterpreterResult, Range)> { + ) -> Option { println!( "SM CALL: {:?}, context:{:?}, is_static:{:?}, transfer:{:?}, input_size:{:?}", inputs.contract, diff --git a/crates/revm/src/inspector/eip3155.rs b/crates/revm/src/inspector/eip3155.rs index 7ec60d51ea..4d86a344d4 100644 --- a/crates/revm/src/inspector/eip3155.rs +++ b/crates/revm/src/inspector/eip3155.rs @@ -4,7 +4,8 @@ use crate::{ primitives::{db::Database, hex, Address, U256}, EvmContext, GetInspector, Inspector, }; -use core::ops::Range; + +use revm_interpreter::CallOutcome; use revm_interpreter::CreateOutcome; use serde_json::json; use std::io::Write; @@ -90,7 +91,7 @@ impl Inspector for TracerEip3155 { &mut self, _context: &mut EvmContext, _inputs: &mut CallInputs, - ) -> Option<(InterpreterResult, Range)> { + ) -> Option { None } diff --git a/crates/revm/src/inspector/gas.rs b/crates/revm/src/inspector/gas.rs index 1a83afae35..69d68862e3 100644 --- a/crates/revm/src/inspector/gas.rs +++ b/crates/revm/src/inspector/gas.rs @@ -68,6 +68,7 @@ impl Inspector for GasInspector { #[cfg(test)] mod tests { + use revm_interpreter::CallOutcome; use revm_interpreter::CreateOutcome; use crate::{ @@ -77,7 +78,6 @@ mod tests { primitives::{Address, Log}, Database, EvmContext, Inspector, }; - use core::ops::Range; #[derive(Default, Debug)] struct StackInspector { @@ -116,7 +116,7 @@ mod tests { &mut self, context: &mut EvmContext, call: &mut CallInputs, - ) -> Option<(InterpreterResult, Range)> { + ) -> Option { self.gas_inspector.call(context, call) } diff --git a/crates/revm/src/inspector/handler_register.rs b/crates/revm/src/inspector/handler_register.rs index ea66a88c94..e76cc9ccd6 100644 --- a/crates/revm/src/inspector/handler_register.rs +++ b/crates/revm/src/inspector/handler_register.rs @@ -9,7 +9,7 @@ use crate::{ CallStackFrame, Evm, FrameData, FrameOrResult, Inspector, JournalEntry, }; use alloc::{boxed::Box, sync::Arc, vec::Vec}; -use revm_interpreter::CreateInputs; +use revm_interpreter::{CallOutcome, CreateInputs}; pub trait GetInspector<'a, DB: Database> { fn get_inspector(&mut self) -> &mut dyn Inspector; @@ -106,7 +106,7 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector<'a, DB>>( .get_inspector() .call(&mut context.evm, &mut call_inputs) { - return FrameOrResult::Result(output.0); + return FrameOrResult::Result(output.interpreter_result); } // first call frame does not have return range. context.evm.make_call_frame(&call_inputs, 0..0) @@ -203,8 +203,8 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector<'a, DB>>( move |context, mut inputs, frame, memory, return_memory_offset| -> Option> { // inspector handle let inspector = context.external.get_inspector(); - if let Some((result, range)) = inspector.call(&mut context.evm, &mut inputs) { - frame.interpreter.insert_call_output(memory, result, range); + if let Some(call_outcome) = inspector.call(&mut context.evm, &mut inputs) { + frame.interpreter.insert_call_outcome(memory, call_outcome); return None; } match context @@ -218,9 +218,8 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector<'a, DB>>( FrameOrResult::Result(result) => { // inspector handle let result = inspector.call_end(&mut context.evm, result); - frame - .interpreter - .insert_call_output(memory, result, return_memory_offset); + let call_outcome = CallOutcome::new(result, return_memory_offset); + frame.interpreter.insert_call_outcome(memory, call_outcome); None } } @@ -299,7 +298,7 @@ mod tests { primitives::{Address, BerlinSpec}, Database, Evm, EvmContext, Inspector, }; - use core::ops::Range; + use revm_interpreter::CreateOutcome; #[test] @@ -349,7 +348,7 @@ mod tests { &mut self, _context: &mut EvmContext, _call: &mut CallInputs, - ) -> Option<(InterpreterResult, Range)> { + ) -> Option { if self.call { unreachable!("call should not be called twice") }