fix: increment pc post-opcode execution (#955)

* fix: increment pc post-opcode execution

* fmt

* fix: increment JUMPi by hand if no-jump case

* adapt test

* fix doc

* comments in jumpi opcode

crates/evm/src/instructions/memory_operations.cairo

@@ -188,21 +188,17 @@ pub impl MemoryOperation of MemoryOperationTrait {
     /// The new pc target has to be a JUMPDEST opcode.
     /// # Specification: https://www.evm.codes/#57?fork=shanghai
     fn exec_jumpi(ref self: VM) -> Result<(), EVMError> {
-        self.charge_gas(gas::HIGH)?;
+        let index = self.stack.pop_usize()?;
+        let b = self.stack.pop()?;
 
-        // Peek the value so we don't need to push it back again in case we want to call `exec_jump`
-        let b = self.stack.peek_at(1)?;
+        self.charge_gas(gas::HIGH)?;
 
         if b != 0x0 {
-            let index = self.stack.pop_usize()?;
             jump(ref self, index)?;
-            // counter would have been already popped by `exec_jump`
-            // so we just remove `b`
-            self.stack.pop()?;
         } else {
-            // remove both `value` and `b`
-            self.stack.pop()?;
-            self.stack.pop()?;
+            // Return with a PC incremented by one - as JUMP and JUMPi increments
+            // are skipped in the main `execute_code` loop
+            self.set_pc(self.pc() + 1);
         }
 
         Result::Ok(())
@@ -709,10 +705,8 @@ mod tests {
 
         // Then
         let pc = vm.pc();
-        // ideally we should assert that it incremented, but incrementing is done by
-        // `decode_and_execute`
-        // so we can assume that will be done
-        assert(pc == old_pc, 'PC should be same');
+        // If the jump is not taken, the PC should be incremented by 1
+        assert_eq!(pc, old_pc + 1);
     }
 
     #[test]
@@ -754,10 +748,8 @@ mod tests {
 
         // Then
         let pc = vm.pc();
-        // ideally we should assert that it incremented, but incrementing is done by
-        // `decode_and_execute`
-        // so we can assume that will be done
-        assert(pc == old_pc, 'PC should be same');
+        // If the jump is not taken, the PC should be incremented by 1
+        assert_eq!(pc, old_pc + 1);
     }
 
     #[test]

crates/evm/src/instructions/push_operations.cairo

@@ -12,7 +12,8 @@ use utils::helpers::U8SpanExTrait;
 fn exec_push_i(ref self: VM, i: u8) -> Result<(), EVMError> {
     self.charge_gas(gas::VERYLOW)?;
     let i = i.into();
-    let data = self.read_code(i);
+    let args_pc = self.pc() + 1; // PUSH_N Arguments begin at pc + 1
+    let data = self.read_code(args_pc, i);
 
     self.set_pc(self.pc() + i);
 
@@ -328,8 +329,11 @@ mod tests {
         }
     }
 
+    /// Get a span of 0xFF bytes of length n preceded by a 0x00 byte
+    /// to account for the argument offset in push operations
     fn get_n_0xFF(mut n: u8) -> Span<u8> {
         let mut array: Array<u8> = ArrayTrait::new();
+        array.append(0x00);
         while n != 0 {
             array.append(0xFF);
             n -= 1;

crates/evm/src/interpreter.cairo

@@ -334,11 +334,14 @@ pub impl EVMImpl of EVMTrait {
         }
 
         let opcode: u8 = *bytecode.at(pc);
-        // Increment pc
-        vm.set_pc(pc + 1);
 
         match Self::execute_opcode(ref vm, opcode) {
             Result::Ok(_) => {
+                if opcode != 0x56 && opcode != 0x57 {
+                    // Increment pc if not a JUMP family opcode
+                    vm.set_pc(vm.pc() + 1);
+                }
+
                 if vm.is_running() {
                     return Self::execute_code(ref vm);
                 }

crates/evm/src/model/vm.cairo

@@ -130,20 +130,20 @@ pub impl VMImpl of VMTrait {
         self.accessed_storage_keys.spanset()
     }
 
-    /// Reads and returns data from bytecode starting at the current program counter.
+    /// Reads and returns data from bytecode starting at the provided pc.
     ///
     /// # Arguments
     ///
     /// * `self` - The `VM` instance to read the data from.
+    /// * `pc` - The starting position in the bytecode to read from.
     /// * `len` - The length of the data to read from the bytecode.
     ///
     /// # Returns
     ///
     /// * A `Span<u8>` containing the requested bytecode slice.
     /// * If the requested slice extends beyond the code length, returns remaining bytes.
     #[inline(always)]
-    fn read_code(self: @VM, len: usize) -> Span<u8> {
-        let pc = self.pc();
+    fn read_code(self: @VM, pc: usize, len: usize) -> Span<u8> {
         let code_len = self.message().code.len();
 
         // If pc is out of bounds, return an empty span
@@ -239,7 +239,7 @@ mod tests {
         let mut vm = VMBuilderTrait::new_with_presets().with_bytecode(bytecode).build();
         vm.set_pc(1);
 
-        let read_code = vm.read_code(3);
+        let read_code = vm.read_code(vm.pc(), 3);
 
         assert_eq!(read_code, [0x02, 0x03, 0x04].span());
         assert_eq!(vm.pc(), 1);
@@ -251,7 +251,7 @@ mod tests {
         let mut vm = VMBuilderTrait::new_with_presets().with_bytecode(bytecode).build();
         vm.set_pc(2);
 
-        let read_code = vm.read_code(2);
+        let read_code = vm.read_code(vm.pc(), 3);
 
         assert_eq!(read_code, [0x03].span());
         assert_eq!(vm.pc(), 2);
@@ -263,7 +263,7 @@ mod tests {
         let mut vm = VMBuilderTrait::new_with_presets().with_bytecode(bytecode).build();
         vm.set_pc(3);
 
-        let read_code = vm.read_code(2);
+        let read_code = vm.read_code(vm.pc(), 3);
 
         assert_eq!(read_code, [0x04, 0x05].span());
         assert_eq!(vm.pc(), 3);