From abc96f5eec52b2a78a0534834c7983cfc21f5f4a Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 5 Mar 2024 23:33:46 +0100 Subject: [PATCH 1/2] feat: use `impl` instead of `dyn` in `GetInspector` --- crates/interpreter/src/instructions/i256.rs | 2 - crates/interpreter/src/interpreter.rs | 4 +- crates/primitives/src/result.rs | 37 +++++++++------ crates/revm/src/builder.rs | 5 +- crates/revm/src/db/in_memory_db.rs | 12 +++-- crates/revm/src/frame.rs | 15 ++++-- crates/revm/src/handler/mainnet/execution.rs | 5 +- .../src/handler/mainnet/post_execution.rs | 7 +-- crates/revm/src/inspector/handler_register.rs | 46 ++++++++++--------- 9 files changed, 76 insertions(+), 57 deletions(-) diff --git a/crates/interpreter/src/instructions/i256.rs b/crates/interpreter/src/instructions/i256.rs index e20209ff48..a26ae69ad4 100644 --- a/crates/interpreter/src/instructions/i256.rs +++ b/crates/interpreter/src/instructions/i256.rs @@ -136,10 +136,8 @@ mod tests { let one = U256::from(1); let one_hundred = U256::from(100); let fifty = U256::from(50); - let _fifty_sign = Sign::Plus; let two = U256::from(2); let neg_one_hundred = U256::from(100); - let _neg_one_hundred_sign = Sign::Minus; let minus_one = U256::from(1); let max_value = U256::from(2).pow(U256::from(255)) - U256::from(1); let neg_max_value = U256::from(2).pow(U256::from(255)) - U256::from(1); diff --git a/crates/interpreter/src/interpreter.rs b/crates/interpreter/src/interpreter.rs index e9377098f4..79620b9085 100644 --- a/crates/interpreter/src/interpreter.rs +++ b/crates/interpreter/src/interpreter.rs @@ -5,7 +5,7 @@ mod stack; pub use analysis::BytecodeLocked; pub use contract::Contract; -pub use shared_memory::{next_multiple_of_32, SharedMemory}; +pub use shared_memory::{next_multiple_of_32, SharedMemory, EMPTY_SHARED_MEMORY}; pub use stack::{Stack, STACK_LIMIT}; use crate::{ @@ -17,8 +17,6 @@ use revm_primitives::U256; use std::borrow::ToOwned; use std::boxed::Box; -pub use self::shared_memory::EMPTY_SHARED_MEMORY; - #[derive(Debug)] pub struct Interpreter { /// Contract information and invoking data diff --git a/crates/primitives/src/result.rs b/crates/primitives/src/result.rs index 2b7c37d401..70c5b44aea 100644 --- a/crates/primitives/src/result.rs +++ b/crates/primitives/src/result.rs @@ -52,14 +52,6 @@ impl ExecutionResult { matches!(self, Self::Halt { .. }) } - /// Return logs, if execution is not successful, function will return empty vec. - pub fn logs(&self) -> Vec { - match self { - Self::Success { logs, .. } => logs.clone(), - _ => Vec::new(), - } - } - /// Returns the output data of the execution. /// /// Returns `None` if the execution was halted. @@ -82,7 +74,15 @@ impl ExecutionResult { } } - /// Consumes the type and returns logs, if execution is not successful, function will return empty vec. + /// Returns the logs if execution is successful, or an empty list otherwise. + pub fn logs(&self) -> &[Log] { + match self { + Self::Success { logs, .. } => logs, + _ => &[], + } + } + + /// Consumes `self` and returns the logs if execution is successful, or an empty list otherwise. pub fn into_logs(self) -> Vec { match self { Self::Success { logs, .. } => logs, @@ -90,12 +90,13 @@ impl ExecutionResult { } } + /// Returns the gas used. pub fn gas_used(&self) -> u64 { - let (Self::Success { gas_used, .. } - | Self::Revert { gas_used, .. } - | Self::Halt { gas_used, .. }) = self; - - *gas_used + match *self { + Self::Success { gas_used, .. } + | Self::Revert { gas_used, .. } + | Self::Halt { gas_used, .. } => gas_used, + } } } @@ -123,6 +124,14 @@ impl Output { Output::Create(data, _) => data, } } + + /// Returns the created address, if any. + pub fn address(&self) -> Option<&Address> { + match self { + Output::Call(_) => None, + Output::Create(_, address) => address.as_ref(), + } + } } /// Main EVM error. diff --git a/crates/revm/src/builder.rs b/crates/revm/src/builder.rs index 5e0864ce3a..b0d2735eda 100644 --- a/crates/revm/src/builder.rs +++ b/crates/revm/src/builder.rs @@ -497,10 +497,9 @@ mod test { .build(); let Context { - external, - evm: EvmContext { db, .. }, + external: _, + evm: EvmContext { db: _, .. }, } = evm.into_context(); - let _ = (external, db); } #[test] diff --git a/crates/revm/src/db/in_memory_db.rs b/crates/revm/src/db/in_memory_db.rs index bca142e3ae..7ecf5236c0 100644 --- a/crates/revm/src/db/in_memory_db.rs +++ b/crates/revm/src/db/in_memory_db.rs @@ -428,7 +428,9 @@ mod tests { let (key, value) = (U256::from(123), U256::from(456)); let mut new_state = CacheDB::new(init_state); - let _ = new_state.insert_account_storage(account, key, value); + new_state + .insert_account_storage(account, key, value) + .unwrap(); assert_eq!(new_state.basic(account).unwrap().unwrap().nonce, nonce); assert_eq!(new_state.storage(account, key), Ok(value)); @@ -449,10 +451,14 @@ mod tests { let (key0, value0) = (U256::from(123), U256::from(456)); let (key1, value1) = (U256::from(789), U256::from(999)); - let _ = init_state.insert_account_storage(account, key0, value0); + init_state + .insert_account_storage(account, key0, value0) + .unwrap(); let mut new_state = CacheDB::new(init_state); - let _ = new_state.replace_account_storage(account, [(key1, value1)].into()); + new_state + .replace_account_storage(account, [(key1, value1)].into()) + .unwrap(); assert_eq!(new_state.basic(account).unwrap().unwrap().nonce, nonce); assert_eq!(new_state.storage(account, key0), Ok(U256::ZERO)); diff --git a/crates/revm/src/frame.rs b/crates/revm/src/frame.rs index ad5eada494..3a7ade26e6 100644 --- a/crates/revm/src/frame.rs +++ b/crates/revm/src/frame.rs @@ -110,10 +110,9 @@ impl FrameResult { /// Contains either a frame or a result. pub enum FrameOrResult { - /// Boxed call frame, + /// Boxed call or create frame. Frame(Frame), - /// Boxed create frame - /// Interpreter result + /// Call or create result. Result(FrameResult), } @@ -187,6 +186,16 @@ impl Frame { Self::Create(create_frame) => &mut create_frame.frame_data, } } + + /// Returns a reference to the interpreter. + pub fn interpreter(&self) -> &Interpreter { + &self.frame_data().interpreter + } + + /// Returns a mutable reference to the interpreter. + pub fn interpreter_mut(&mut self) -> &mut Interpreter { + &mut self.frame_data_mut().interpreter + } } impl FrameOrResult { diff --git a/crates/revm/src/handler/mainnet/execution.rs b/crates/revm/src/handler/mainnet/execution.rs index 7ecf8228c6..941d3b8fd4 100644 --- a/crates/revm/src/handler/mainnet/execution.rs +++ b/crates/revm/src/handler/mainnet/execution.rs @@ -37,14 +37,15 @@ pub fn frame_return_with_refund_flag( } _ => {} } + // Calculate gas refund for transaction. // If config is set to disable gas refund, it will return 0. // If spec is set to london, it will decrease the maximum refund amount to 5th part of // gas spend. (Before london it was 2th part of gas spend) if refund_enabled { // EIP-3529: Reduction in refunds - gas.set_final_refund::() - }; + gas.set_final_refund::(); + } } /// Handle output of the transaction diff --git a/crates/revm/src/handler/mainnet/post_execution.rs b/crates/revm/src/handler/mainnet/post_execution.rs index 1e1494163c..2c62f1e587 100644 --- a/crates/revm/src/handler/mainnet/post_execution.rs +++ b/crates/revm/src/handler/mainnet/post_execution.rs @@ -1,7 +1,7 @@ use crate::{ interpreter::{Gas, SuccessOrHalt}, primitives::{ - db::Database, EVMError, ExecutionResult, Output, ResultAndState, Spec, SpecId::LONDON, U256, + db::Database, EVMError, ExecutionResult, ResultAndState, Spec, SpecId::LONDON, U256, }, Context, FrameResult, }; @@ -94,10 +94,7 @@ pub fn output( }, SuccessOrHalt::Revert => ExecutionResult::Revert { gas_used: final_gas_used, - output: match output { - Output::Call(return_value) => return_value, - Output::Create(return_value, _) => return_value, - }, + output: output.into_data(), }, SuccessOrHalt::Halt(reason) => ExecutionResult::Halt { reason, diff --git a/crates/revm/src/inspector/handler_register.rs b/crates/revm/src/inspector/handler_register.rs index d0c8b721cf..0598c4f829 100644 --- a/crates/revm/src/inspector/handler_register.rs +++ b/crates/revm/src/inspector/handler_register.rs @@ -11,11 +11,13 @@ use std::{boxed::Box, rc::Rc, sync::Arc, vec::Vec}; /// Provides access to an `Inspector` instance. pub trait GetInspector { - fn get_inspector(&mut self) -> &mut dyn Inspector; + /// Returns the associated `Inspector`. + fn get_inspector(&mut self) -> &mut impl Inspector; } impl> GetInspector for INSP { - fn get_inspector(&mut self) -> &mut dyn Inspector { + #[inline(always)] + fn get_inspector(&mut self) -> &mut impl Inspector { self } } @@ -37,8 +39,7 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector>( ) { // Every instruction inside flat table that is going to be wrapped by inspector calls. let table = handler - .instruction_table - .take() + .take_instruction_table() .expect("Handler must have instruction table"); let mut table = match table { InstructionTables::Plain(table) => table @@ -123,7 +124,7 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector>( } // cast vector to array. - handler.instruction_table = Some(InstructionTables::Boxed( + handler.set_instruction_table(InstructionTables::Boxed( table.try_into().unwrap_or_else(|_| unreachable!()), )); @@ -146,10 +147,10 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector>( create_input_stack_inner.borrow_mut().push(inputs.clone()); let mut frame_or_result = old_handle(ctx, inputs); - - let inspector = ctx.external.get_inspector(); if let Ok(FrameOrResult::Frame(frame)) = &mut frame_or_result { - inspector.initialize_interp(&mut frame.frame_data_mut().interpreter, &mut ctx.evm) + ctx.external + .get_inspector() + .initialize_interp(frame.interpreter_mut(), &mut ctx.evm) } frame_or_result }, @@ -160,20 +161,18 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector>( let old_handle = handler.execution.call.clone(); handler.execution.call = Arc::new( move |ctx, mut inputs| -> Result> { - let inspector = ctx.external.get_inspector(); - let _mems = inputs.return_memory_offset.clone(); - // call inspector callto change input or return outcome. - if let Some(outcome) = inspector.call(&mut ctx.evm, &mut inputs) { - call_input_stack_inner.borrow_mut().push(inputs.clone()); + // Call inspector to change input or return outcome. + let outcome = ctx.external.get_inspector().call(&mut ctx.evm, &mut inputs); + call_input_stack_inner.borrow_mut().push(inputs.clone()); + if let Some(outcome) = outcome { return Ok(FrameOrResult::Result(FrameResult::Call(outcome))); } - call_input_stack_inner.borrow_mut().push(inputs.clone()); let mut frame_or_result = old_handle(ctx, inputs); - - let inspector = ctx.external.get_inspector(); if let Ok(FrameOrResult::Frame(frame)) = &mut frame_or_result { - inspector.initialize_interp(&mut frame.frame_data_mut().interpreter, &mut ctx.evm) + ctx.external + .get_inspector() + .initialize_interp(frame.interpreter_mut(), &mut ctx.evm) } frame_or_result }, @@ -184,9 +183,11 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector>( let old_handle = handler.execution.insert_call_outcome.clone(); handler.execution.insert_call_outcome = Arc::new(move |ctx, frame, shared_memory, mut outcome| { - let inspector = ctx.external.get_inspector(); let call_inputs = call_input_stack_inner.borrow_mut().pop().unwrap(); - outcome = inspector.call_end(&mut ctx.evm, &call_inputs, outcome); + outcome = ctx + .external + .get_inspector() + .call_end(&mut ctx.evm, &call_inputs, outcome); old_handle(ctx, frame, shared_memory, outcome) }); @@ -194,9 +195,11 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector>( let create_input_stack_inner = create_input_stack.clone(); let old_handle = handler.execution.insert_create_outcome.clone(); handler.execution.insert_create_outcome = Arc::new(move |ctx, frame, mut outcome| { - let inspector = ctx.external.get_inspector(); let create_inputs = create_input_stack_inner.borrow_mut().pop().unwrap(); - outcome = inspector.create_end(&mut ctx.evm, &create_inputs, outcome); + outcome = ctx + .external + .get_inspector() + .create_end(&mut ctx.evm, &create_inputs, outcome); old_handle(ctx, frame, outcome) }); @@ -214,7 +217,6 @@ pub fn inspector_handle_register<'a, DB: Database, EXT: GetInspector>( *outcome = inspector.create_end(&mut ctx.evm, &create_inputs, outcome.clone()); } } - //inspector.last_frame_return(ctx, frame_result); old_handle(ctx, frame_result) }); } From 4a756bc19f1bf04a7d91b4e7ef3cf09080cf3ed2 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Wed, 6 Mar 2024 00:14:25 +0100 Subject: [PATCH 2/2] chore: clippy --- bins/revme/src/cmd/statetest/runner.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bins/revme/src/cmd/statetest/runner.rs b/bins/revme/src/cmd/statetest/runner.rs index 73c8d71d69..12b5b7c186 100644 --- a/bins/revme/src/cmd/statetest/runner.rs +++ b/bins/revme/src/cmd/statetest/runner.rs @@ -115,7 +115,7 @@ fn check_evm_execution( evm: &Evm<'_, EXT, &mut State>, print_json_outcome: bool, ) -> Result<(), TestError> { - let logs_root = log_rlp_hash(&exec_result.as_ref().map(|r| r.logs()).unwrap_or_default()); + let logs_root = log_rlp_hash(exec_result.as_ref().map(|r| r.logs()).unwrap_or_default()); let state_root = state_merkle_trie_root(evm.context.evm.db.cache.trie_account()); let print_json_output = |error: Option| {