From f448178a91b312cf92441493fe94fa9392217db5 Mon Sep 17 00:00:00 2001 From: enitrat Date: Tue, 17 Sep 2024 18:27:41 +0200 Subject: [PATCH 1/6] fix: increment pc post-opcode execution --- crates/evm/src/instructions/push_operations.cairo | 3 ++- crates/evm/src/interpreter.cairo | 8 ++++++-- crates/evm/src/model/vm.cairo | 11 +++++------ 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/evm/src/instructions/push_operations.cairo b/crates/evm/src/instructions/push_operations.cairo index de3dbade2..7175e5071 100644 --- a/crates/evm/src/instructions/push_operations.cairo +++ b/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); diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index 88eaf4704..bd1adaa85 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -334,11 +334,15 @@ 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); } diff --git a/crates/evm/src/model/vm.cairo b/crates/evm/src/model/vm.cairo index 5cbd14775..a584a71a2 100644 --- a/crates/evm/src/model/vm.cairo +++ b/crates/evm/src/model/vm.cairo @@ -130,7 +130,7 @@ 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 /// @@ -142,8 +142,7 @@ pub impl VMImpl of VMTrait { /// * A `Span` 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 { - let pc = self.pc(); + fn read_code(self: @VM, pc: usize, len: usize) -> Span { let code_len = self.message().code.len(); // If pc is out of bounds, return an empty span @@ -239,7 +238,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 +250,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 +262,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); From 44038799c66ade149bafde52270fa96e1a10e0f7 Mon Sep 17 00:00:00 2001 From: enitrat Date: Tue, 17 Sep 2024 18:29:57 +0200 Subject: [PATCH 2/6] fmt --- crates/evm/src/interpreter.cairo | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index bd1adaa85..5775b5fcd 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -337,7 +337,6 @@ pub impl EVMImpl of EVMTrait { 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); From 38eae4b43d93b57a8d0dfae6085edd16d5ffdec7 Mon Sep 17 00:00:00 2001 From: enitrat Date: Tue, 17 Sep 2024 18:54:10 +0200 Subject: [PATCH 3/6] fix: increment JUMPi by hand if no-jump case --- crates/evm/src/instructions/memory_operations.cairo | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/evm/src/instructions/memory_operations.cairo b/crates/evm/src/instructions/memory_operations.cairo index 3af514dfd..e6e36321a 100644 --- a/crates/evm/src/instructions/memory_operations.cairo +++ b/crates/evm/src/instructions/memory_operations.cairo @@ -203,6 +203,10 @@ pub impl MemoryOperation of MemoryOperationTrait { // 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(()) From b2909749ce69e97ca11f218fdc0dfadc3ddd59ee Mon Sep 17 00:00:00 2001 From: enitrat Date: Tue, 17 Sep 2024 21:09:32 +0200 Subject: [PATCH 4/6] adapt test --- crates/evm/src/instructions/memory_operations.cairo | 12 ++++-------- crates/evm/src/instructions/push_operations.cairo | 3 +++ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/crates/evm/src/instructions/memory_operations.cairo b/crates/evm/src/instructions/memory_operations.cairo index e6e36321a..206713be4 100644 --- a/crates/evm/src/instructions/memory_operations.cairo +++ b/crates/evm/src/instructions/memory_operations.cairo @@ -713,10 +713,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] @@ -758,10 +756,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] diff --git a/crates/evm/src/instructions/push_operations.cairo b/crates/evm/src/instructions/push_operations.cairo index 7175e5071..f5fa6b9bb 100644 --- a/crates/evm/src/instructions/push_operations.cairo +++ b/crates/evm/src/instructions/push_operations.cairo @@ -329,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 { let mut array: Array = ArrayTrait::new(); + array.append(0x00); while n != 0 { array.append(0xFF); n -= 1; From 12c560c20b35e2631973ee0b29aacc1e28802a5b Mon Sep 17 00:00:00 2001 From: enitrat Date: Tue, 17 Sep 2024 21:46:47 +0200 Subject: [PATCH 5/6] fix doc --- crates/evm/src/model/vm.cairo | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/evm/src/model/vm.cairo b/crates/evm/src/model/vm.cairo index a584a71a2..3067f3efb 100644 --- a/crates/evm/src/model/vm.cairo +++ b/crates/evm/src/model/vm.cairo @@ -135,6 +135,7 @@ pub impl VMImpl of VMTrait { /// # 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 From 044de309578292a0b53d391e2a0078af8d99ebc4 Mon Sep 17 00:00:00 2001 From: enitrat Date: Wed, 18 Sep 2024 10:29:18 +0200 Subject: [PATCH 6/6] comments in jumpi opcode --- .../evm/src/instructions/memory_operations.cairo | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/crates/evm/src/instructions/memory_operations.cairo b/crates/evm/src/instructions/memory_operations.cairo index 206713be4..3597803d0 100644 --- a/crates/evm/src/instructions/memory_operations.cairo +++ b/crates/evm/src/instructions/memory_operations.cairo @@ -188,22 +188,14 @@ 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);