From 9981422e4d22e7d06708944da4ecc1d2d7dd3cad Mon Sep 17 00:00:00 2001 From: Maximo Palopoli <96491141+maximopalopoli@users.noreply.github.com> Date: Fri, 29 Nov 2024 19:33:09 -0300 Subject: [PATCH] fix(levm): fixing vm arithmetic tests changes (#1333) **Motivation** The goal is to make pass the tests in the arithmetic subfolder of the VMTests folder. **Description** These changes are mostly local, so don't change much in the passed tests count. --------- Co-authored-by: ilitteri Co-authored-by: Ivan Litteri <67517699+ilitteri@users.noreply.github.com> --- .../vm/levm/src/opcode_handlers/arithmetic.rs | 28 ++++++----- .../src/opcode_handlers/bitwise_comparison.rs | 46 +++++++++---------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/crates/vm/levm/src/opcode_handlers/arithmetic.rs b/crates/vm/levm/src/opcode_handlers/arithmetic.rs index 4aaecec32..0ca9a331f 100644 --- a/crates/vm/levm/src/opcode_handlers/arithmetic.rs +++ b/crates/vm/levm/src/opcode_handlers/arithmetic.rs @@ -237,19 +237,14 @@ impl VM { ) -> Result { self.increase_consumed_gas(current_call_frame, gas_cost::SIGNEXTEND)?; - let byte_size: usize = current_call_frame - .stack - .pop()? - .try_into() - .map_err(|_| VMError::VeryLargeNumber)?; - + let byte_size = current_call_frame.stack.pop()?; let value_to_extend = current_call_frame.stack.pop()?; - let bits_per_byte: usize = 8; - let sign_bit_position_on_byte = 7; + let bits_per_byte = U256::from(8); + let sign_bit_position_on_byte = U256::from(7); - let max_byte_size: usize = 31; - let byte_size: usize = byte_size.min(max_byte_size); + let max_byte_size = U256::from(31); + let byte_size = byte_size.min(max_byte_size); let total_bits = bits_per_byte .checked_mul(byte_size) .ok_or(VMError::Internal( @@ -261,9 +256,20 @@ impl VM { .ok_or(VMError::Internal( InternalError::ArithmeticOperationOverflow, ))?; + + let sign_bit_index: usize = match sign_bit_index.try_into() { + Ok(val) => val, + Err(_) => { + // this means the value_to_extend was too big to extend, so remains the same. + // Maybe this verification could be done before in this function + current_call_frame.stack.push(value_to_extend)?; + return Ok(OpcodeSuccess::Continue); + } + }; + let is_negative = value_to_extend.bit(sign_bit_index); - let sign_bit_mask = checked_shift_left(U256::one(), sign_bit_index)? + let sign_bit_mask = checked_shift_left(U256::one(), U256::from(sign_bit_index))? .checked_sub(U256::one()) .ok_or(VMError::Internal( InternalError::ArithmeticOperationUnderflow, diff --git a/crates/vm/levm/src/opcode_handlers/bitwise_comparison.rs b/crates/vm/levm/src/opcode_handlers/bitwise_comparison.rs index 265432a7f..399dd8a4f 100644 --- a/crates/vm/levm/src/opcode_handlers/bitwise_comparison.rs +++ b/crates/vm/levm/src/opcode_handlers/bitwise_comparison.rs @@ -202,12 +202,10 @@ impl VM { let shift = current_call_frame.stack.pop()?; let value = current_call_frame.stack.pop()?; - let shift_usize: usize = shift.try_into().map_err(|_| VMError::VeryLargeNumber)?; // we know its not bigger than 256 - if shift < U256::from(256) { current_call_frame .stack - .push(checked_shift_left(value, shift_usize)?)?; + .push(checked_shift_left(value, shift)?)?; } else { current_call_frame.stack.push(U256::zero())?; } @@ -221,12 +219,10 @@ impl VM { let shift = current_call_frame.stack.pop()?; let value = current_call_frame.stack.pop()?; - let shift_usize: usize = shift.try_into().map_err(|_| VMError::VeryLargeNumber)?; // we know its not bigger than 256 - if shift < U256::from(256) { current_call_frame .stack - .push(checked_shift_right(value, shift_usize)?)?; + .push(checked_shift_right(value, shift)?)?; } else { current_call_frame.stack.push(U256::zero())?; } @@ -253,32 +249,32 @@ impl VM { } pub fn arithmetic_shift_right(value: U256, shift: U256) -> Result { - let shift_usize: usize = shift.try_into().map_err(|_| VMError::VeryLargeNumber)?; // we know its not bigger than 256 - if value.bit(255) { // if negative fill with 1s - let shifted = checked_shift_right(value, shift_usize)?; + let shifted = checked_shift_right(value, shift)?; let mask = checked_shift_left( U256::MAX, - 256_usize.checked_sub(shift_usize).ok_or(VMError::Internal( - InternalError::ArithmeticOperationUnderflow, - ))?, // Note that this is already checked in op_sar + (U256::from(256)) + .checked_sub(shift) + .ok_or(VMError::Internal( + InternalError::ArithmeticOperationUnderflow, + ))?, // Note that this is already checked in op_sar )?; Ok(shifted | mask) } else { - Ok(checked_shift_right(value, shift_usize)?) + Ok(checked_shift_right(value, shift)?) } } /// Instead of using unsafe <<, uses checked_mul n times, replicating n shifts. /// Note: These (checked_shift_left and checked_shift_right) are done because /// are not available in U256 -pub fn checked_shift_left(value: U256, shift: usize) -> Result { +pub fn checked_shift_left(value: U256, shift: U256) -> Result { let mut result = value; let mut shifts_left = shift; - while shifts_left > 0 { + while !shifts_left.is_zero() { result = match result.checked_mul(U256::from(2)) { Some(num) => num, None => { @@ -297,26 +293,30 @@ pub fn checked_shift_left(value: U256, shift: usize) -> Result { ))? } }; - shifts_left = shifts_left.checked_sub(1).ok_or(VMError::Internal( - InternalError::ArithmeticOperationUnderflow, - ))?; // Should not reach negative values + shifts_left = shifts_left + .checked_sub(U256::one()) + .ok_or(VMError::Internal( + InternalError::ArithmeticOperationUnderflow, + ))?; // Should not reach negative values } Ok(result) } // Instead of using unsafe >>, uses checked_div n times, replicating n shifts -fn checked_shift_right(value: U256, shift: usize) -> Result { +fn checked_shift_right(value: U256, shift: U256) -> Result { let mut result = value; let mut shifts_left = shift; - while shifts_left > 0 { + while !shifts_left.is_zero() { result = result.checked_div(U256::from(2)).ok_or(VMError::Internal( InternalError::ArithmeticOperationDividedByZero, ))?; // '2' will never be zero - shifts_left = shifts_left.checked_sub(1).ok_or(VMError::Internal( - InternalError::ArithmeticOperationUnderflow, - ))?; // Should not reach negative values + shifts_left = shifts_left + .checked_sub(U256::one()) + .ok_or(VMError::Internal( + InternalError::ArithmeticOperationUnderflow, + ))?; // Should not reach negative values } Ok(result)