chore: fix inspector, cleanup loop

crates/inspector/src/handler.rs

@@ -4,8 +4,8 @@ use handler::{evm::FrameTr, EvmTr, FrameResult, Handler, ItemOrResult};
 use interpreter::{
     instructions::InstructionTable,
     interpreter_types::{Jumps, LoopControl},
-    FrameInput, Host, InitialAndFloorGas, InstructionContext, InstructionResult, Interpreter,
-    InterpreterAction, InterpreterTypes,
+    FrameInput, Host, InitialAndFloorGas, InstructionResult, Interpreter, InterpreterAction,
+    InterpreterTypes,
 };
 use state::bytecode::opcode;
 
@@ -185,32 +185,20 @@ where
 {
     loop {
         inspector.step(interpreter, context);
+        if interpreter.bytecode.is_end() {
+            break;
+        }
 
-        // SAFETY: In analysis we are doing padding of bytecode so that we are sure that last
-        // byte instruction is STOP so we are safe to just increment program_counter bcs on last instruction
-        // it will do noop and just stop execution of this contract
         let opcode = interpreter.bytecode.opcode();
-        interpreter.bytecode.relative_jump(1);
-
-        // Execute instruction.
-        let instruction_context = InstructionContext {
-            interpreter,
-            host: context,
-        };
-        instructions[opcode as usize](instruction_context);
+        interpreter.step(instructions, context);
 
         if (opcode::LOG0..=opcode::LOG4).contains(&opcode) {
             inspect_log(interpreter, context, &mut inspector);
         }
 
-        let done = interpreter.bytecode.is_end();
-        if done {
-            interpreter.bytecode.revert_to_previous_pointer();
-        }
-
         inspector.step_end(interpreter, context);
 
-        if done {
+        if interpreter.bytecode.is_end() {
             break;
         }
     }

crates/inspector/src/lib.rs

@@ -34,3 +34,50 @@ pub use inspect::{InspectCommitEvm, InspectEvm};
 pub use inspector::*;
 pub use noop::NoOpInspector;
 pub use traits::*;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use ::handler::{MainBuilder, MainContext};
+    use context::{BlockEnv, CfgEnv, Context, Journal, TxEnv};
+    use database::{BenchmarkDB, BENCH_CALLER, BENCH_TARGET};
+    use interpreter::{interpreter::EthInterpreter, InstructionResult, InterpreterTypes};
+    use primitives::TxKind;
+    use state::{bytecode::opcode, Bytecode};
+
+    struct HaltInspector;
+    impl<CTX, INTR: InterpreterTypes> Inspector<CTX, INTR> for HaltInspector {
+        fn step(&mut self, interp: &mut interpreter::Interpreter<INTR>, _context: &mut CTX) {
+            interp.halt(InstructionResult::Stop);
+        }
+    }
+
+    #[test]
+    fn test_step_halt() {
+        let bytecode = [opcode::INVALID];
+        let r = run(&bytecode, HaltInspector);
+        dbg!(&r);
+        assert!(r.is_success());
+    }
+
+    fn run(
+        bytecode: &[u8],
+        inspector: impl Inspector<
+            Context<BlockEnv, TxEnv, CfgEnv, BenchmarkDB, Journal<BenchmarkDB>, ()>,
+            EthInterpreter,
+        >,
+    ) -> context::result::ExecutionResult {
+        let bytecode = Bytecode::new_raw(bytecode.to_vec().into());
+        let ctx = Context::mainnet().with_db(BenchmarkDB::new_bytecode(bytecode));
+        let mut evm = ctx.build_mainnet_with_inspector(inspector);
+        evm.inspect_one_tx(
+            TxEnv::builder()
+                .caller(BENCH_CALLER)
+                .kind(TxKind::Call(BENCH_TARGET))
+                .gas_limit(21100)
+                .build()
+                .unwrap(),
+        )
+        .unwrap()
+    }
+}

crates/interpreter/src/interpreter.rs

@@ -193,6 +193,7 @@ impl<IW: InterpreterTypes> Interpreter<IW> {
     /// Takes the next action from the control and returns it.
     #[inline]
     pub fn take_next_action(&mut self) -> InterpreterAction {
+        self.bytecode.reset_action();
         // Return next action if it is some.
         core::mem::take(self.bytecode.action()).expect("Interpreter to set action")
     }
@@ -237,11 +238,7 @@ impl<IW: InterpreterTypes> Interpreter<IW> {
     /// This uses dummy Host.
     #[inline]
     pub fn step_dummy(&mut self, instruction_table: &InstructionTable<IW, DummyHost>) {
-        let context = InstructionContext {
-            interpreter: self,
-            host: &mut DummyHost,
-        };
-        context.step(instruction_table);
+        self.step(instruction_table, &mut DummyHost);
     }
 
     /// Executes the interpreter until it returns or stops.
@@ -252,22 +249,8 @@ impl<IW: InterpreterTypes> Interpreter<IW> {
         host: &mut H,
     ) -> InterpreterAction {
         while self.bytecode.is_not_end() {
-            // Get current opcode.
-            let opcode = self.bytecode.opcode();
-
-            // SAFETY: In analysis we are doing padding of bytecode so that we are sure that last
-            // byte instruction is STOP so we are safe to just increment program_counter bcs on last instruction
-            // it will do noop and just stop execution of this contract
-            self.bytecode.relative_jump(1);
-            let context = InstructionContext {
-                interpreter: self,
-                host,
-            };
-            // Execute instruction.
-            instruction_table[opcode as usize](context);
+            self.step(instruction_table, host);
         }
-        self.bytecode.revert_to_previous_pointer();
-
         self.take_next_action()
     }
 }

crates/interpreter/src/interpreter/ext_bytecode.rs

@@ -1,7 +1,7 @@
 use super::{Immediates, Jumps, LegacyBytecode};
 use crate::{interpreter_types::LoopControl, InterpreterAction};
 use bytecode::{utils::read_u16, Bytecode};
-use core::{ops::Deref, ptr};
+use core::ops::Deref;
 use primitives::B256;
 
 #[cfg(feature = "serde")]
@@ -14,10 +14,9 @@ pub struct ExtBytecode {
     /// Actions that the EVM should do. It contains return value of the Interpreter or inputs for `CALL` or `CREATE` instructions.
     /// For `RETURN` or `REVERT` instructions it contains the result of the instruction.
     pub action: Option<InterpreterAction>,
+    has_set_action: bool,
     /// The base bytecode.
     base: Bytecode,
-    /// The previous instruction pointer.
-    previous_pointer: Option<*const u8>,
     /// The current instruction pointer.
     instruction_pointer: *const u8,
 }
@@ -47,7 +46,7 @@ impl ExtBytecode {
             instruction_pointer,
             bytecode_hash: None,
             action: None,
-            previous_pointer: None,
+            has_set_action: false,
         }
     }
 
@@ -59,7 +58,7 @@ impl ExtBytecode {
             instruction_pointer,
             bytecode_hash: Some(hash),
             action: None,
-            previous_pointer: None,
+            has_set_action: false,
         }
     }
 
@@ -79,23 +78,18 @@ impl ExtBytecode {
 impl LoopControl for ExtBytecode {
     #[inline]
     fn is_end(&self) -> bool {
-        self.instruction_pointer.is_null()
+        self.has_set_action
     }
 
     #[inline]
-    fn revert_to_previous_pointer(&mut self) {
-        if let Some(previous_pointer) = self.previous_pointer {
-            self.instruction_pointer = previous_pointer;
-        }
+    fn reset_action(&mut self) {
+        self.has_set_action = false;
     }
 
     #[inline]
     fn set_action(&mut self, action: InterpreterAction) {
+        self.has_set_action = true;
         self.action = Some(action);
-        self.previous_pointer = Some(core::mem::replace(
-            &mut self.instruction_pointer,
-            ptr::null(),
-        ));
     }
 
     #[inline]

crates/interpreter/src/interpreter_types.rs

@@ -254,13 +254,9 @@ pub trait LoopControl {
     }
     /// Is end of the loop.
     fn is_end(&self) -> bool;
-    /// Reverts to previous instruction pointer.
-    ///
-    /// After the loop is finished, the instruction pointer is set to the previous one.
-    fn revert_to_previous_pointer(&mut self);
-    /// Set return action and set instruction pointer to null. Preserve previous pointer
-    ///
-    /// Previous pointer can be restored by calling [`LoopControl::revert_to_previous_pointer`].
+    /// Sets the `end` flag internally. Action should be taken after.
+    fn reset_action(&mut self);
+    /// Set return action.
     fn set_action(&mut self, action: InterpreterAction);
     /// Returns the current action.
     fn action(&mut self) -> &mut Option<InterpreterAction>;