From ae80c2f820922e3a08195aaa75320b6f21a1738e Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Mon, 1 Apr 2024 10:39:36 -0500 Subject: [PATCH 1/2] nit: Use uint macro & fix various small things --- bins/revme/src/cmd/statetest/runner.rs | 8 +- crates/interpreter/src/gas/calc.rs | 2 +- crates/interpreter/src/instructions/i256.rs | 182 ++++++++---------- crates/interpreter/src/instructions/macros.rs | 4 +- .../src/interpreter/shared_memory.rs | 6 +- crates/primitives/src/bytecode.rs | 6 +- crates/revm/src/evm.rs | 4 +- crates/revm/src/frame.rs | 8 +- crates/revm/src/handler.rs | 12 +- 9 files changed, 103 insertions(+), 129 deletions(-) diff --git a/bins/revme/src/cmd/statetest/runner.rs b/bins/revme/src/cmd/statetest/runner.rs index d235c572fe..b62e20c306 100644 --- a/bins/revme/src/cmd/statetest/runner.rs +++ b/bins/revme/src/cmd/statetest/runner.rs @@ -48,7 +48,7 @@ pub enum TestErrorKind { got_exception: Option, }, #[error("Unexpected output: {got_output:?} but test expects:{expected_output:?}")] - UnexpecteOutput { + UnexpectedOutput { expected_output: Option, got_output: Option, }, @@ -79,7 +79,7 @@ fn skip_test(path: &Path) -> bool { | "RevertPrecompiledTouch_storage.json" | "RevertPrecompiledTouch.json" - // txbyte is of type 02 and we dont parse tx bytes for this test to fail. + // txbyte is of type 02 and we don't parse tx bytes for this test to fail. | "typeTwoBerlin.json" // Need to handle Test errors @@ -139,7 +139,7 @@ fn check_evm_execution( } }; - // if we expect exception revm should return error from execution. + // If we expect exception revm should return error from execution. // So we do not check logs and state root. // // Note that some tests that have exception and run tests from before state clear @@ -155,7 +155,7 @@ fn check_evm_execution( // check output if let Some((expected_output, output)) = expected_output.zip(result.output()) { if expected_output != output { - let kind = TestErrorKind::UnexpecteOutput { + let kind = TestErrorKind::UnexpectedOutput { expected_output: Some(expected_output.clone()), got_output: result.output().cloned(), }; diff --git a/crates/interpreter/src/gas/calc.rs b/crates/interpreter/src/gas/calc.rs index 092fff7c85..b7f91b1814 100644 --- a/crates/interpreter/src/gas/calc.rs +++ b/crates/interpreter/src/gas/calc.rs @@ -92,7 +92,7 @@ pub fn exp_cost(power: U256) -> Option { } else { // EIP-160: EXP cost increase let gas_byte = U256::from(if SPEC::enabled(SPURIOUS_DRAGON) { - 50u64 + 50 } else { 10 }); diff --git a/crates/interpreter/src/instructions/i256.rs b/crates/interpreter/src/instructions/i256.rs index eaccbab705..fe767eee3e 100644 --- a/crates/interpreter/src/instructions/i256.rs +++ b/crates/interpreter/src/instructions/i256.rs @@ -18,6 +18,13 @@ const MIN_NEGATIVE_VALUE: U256 = U256::from_limbs([ 0x8000000000000000, ]); +const MAX_POSITIVE_VALUE: U256 = U256::from_limbs([ + 0xffffffffffffffff, + 0xffffffffffffffff, + 0xffffffffffffffff, + 0x7fffffffffffffff, +]); + const FLIPH_BITMASK_U64: u64 = 0x7FFFFFFFFFFFFFFF; #[inline] @@ -126,58 +133,7 @@ pub fn i256_mod(mut first: U256, mut second: U256) -> U256 { mod tests { use super::*; use core::num::Wrapping; - - const ZERO: U256 = U256::ZERO; - const ONE: U256 = U256::from_limbs([ - 0x0000000000000001, - 0x0000000000000000, - 0x0000000000000000, - 0x0000000000000000, - ]); - const TWO: U256 = U256::from_limbs([ - 0x0000000000000002, - 0x0000000000000000, - 0x0000000000000000, - 0x0000000000000000, - ]); - const THREE: U256 = U256::from_limbs([ - 0x0000000000000003, - 0x0000000000000000, - 0x0000000000000000, - 0x0000000000000000, - ]); - const FOUR: U256 = U256::from_limbs([ - 0x0000000000000004, - 0x0000000000000000, - 0x0000000000000000, - 0x0000000000000000, - ]); - - const NEG_ONE: U256 = U256::from_limbs([ - 0xffffffffffffffff, - 0xffffffffffffffff, - 0xffffffffffffffff, - 0xffffffffffffffff, - ]); - const NEG_TWO: U256 = U256::from_limbs([ - 0xfffffffffffffffe, - 0xffffffffffffffff, - 0xffffffffffffffff, - 0xffffffffffffffff, - ]); - const NEG_THREE: U256 = U256::from_limbs([ - 0xfffffffffffffffd, - 0xffffffffffffffff, - 0xffffffffffffffff, - 0xffffffffffffffff, - ]); - - const I256_MAX: U256 = U256::from_limbs([ - 0xffffffffffffffff, - 0xffffffffffffffff, - 0xffffffffffffffff, - 0x7fffffffffffffff, - ]); + use revm_primitives::uint; #[test] fn div_i256() { @@ -186,87 +142,101 @@ mod tests { assert_eq!(Wrapping(i8::MIN) / Wrapping(-1), Wrapping(i8::MIN)); assert_eq!(i8::MAX / -1, -i8::MAX); - // Now the same calculations based on i256 - let fifty = U256::from(50); - let one_hundred = U256::from(100); - - assert_eq!(i256_div(MIN_NEGATIVE_VALUE, NEG_ONE), MIN_NEGATIVE_VALUE); - assert_eq!(i256_div(MIN_NEGATIVE_VALUE, ONE), MIN_NEGATIVE_VALUE); - assert_eq!(i256_div(I256_MAX, ONE), I256_MAX); - assert_eq!(i256_div(I256_MAX, NEG_ONE), NEG_ONE * I256_MAX); - assert_eq!(i256_div(one_hundred, NEG_ONE), NEG_ONE * one_hundred); - assert_eq!(i256_div(one_hundred, TWO), fifty); + uint! { + assert_eq!(i256_div(MIN_NEGATIVE_VALUE, -1_U256), MIN_NEGATIVE_VALUE); + assert_eq!(i256_div(MIN_NEGATIVE_VALUE, 1_U256), MIN_NEGATIVE_VALUE); + assert_eq!(i256_div(MAX_POSITIVE_VALUE, 1_U256), MAX_POSITIVE_VALUE); + assert_eq!(i256_div(MAX_POSITIVE_VALUE, -1_U256), -1_U256 * MAX_POSITIVE_VALUE); + assert_eq!(i256_div(100_U256, -1_U256), -100_U256); + assert_eq!(i256_div(100_U256, 2_U256), 50_U256); + } } #[test] fn test_i256_sign() { - assert_eq!(i256_sign(&ZERO), Sign::Zero); - assert_eq!(i256_sign(&ONE), Sign::Plus); - assert_eq!(i256_sign(&MIN_NEGATIVE_VALUE), Sign::Minus); + uint! { + assert_eq!(i256_sign(&0_U256), Sign::Zero); + assert_eq!(i256_sign(&1_U256), Sign::Plus); + assert_eq!(i256_sign(&-1_U256), Sign::Minus); + assert_eq!(i256_sign(&MIN_NEGATIVE_VALUE), Sign::Minus); + assert_eq!(i256_sign(&MAX_POSITIVE_VALUE), Sign::Plus); + } } #[test] fn test_i256_sign_compl() { - let mut zero = ZERO; - let mut positive = ONE; - let mut negative = MIN_NEGATIVE_VALUE; - assert_eq!(i256_sign_compl(&mut zero), Sign::Zero); - assert_eq!(i256_sign_compl(&mut positive), Sign::Plus); - assert_eq!(i256_sign_compl(&mut negative), Sign::Minus); + uint! { + let mut zero = 0_U256; + let mut positive = 1_U256; + let mut negative = -1_U256; + assert_eq!(i256_sign_compl(&mut zero), Sign::Zero); + assert_eq!(i256_sign_compl(&mut positive), Sign::Plus); + assert_eq!(i256_sign_compl(&mut negative), Sign::Minus); + } } #[test] fn test_two_compl() { - assert_eq!(two_compl(ZERO), ZERO); - assert_eq!(two_compl(ONE), NEG_ONE); - assert_eq!(two_compl(NEG_ONE), ONE); - assert_eq!(two_compl(TWO), NEG_TWO); - assert_eq!(two_compl(NEG_TWO), TWO); + uint! { + assert_eq!(two_compl(0_U256), 0_U256); + assert_eq!(two_compl(1_U256), -1_U256); + assert_eq!(two_compl(-1_U256), 1_U256); + assert_eq!(two_compl(2_U256), -2_U256); + assert_eq!(two_compl(-2_U256), 2_U256); - // Two's complement of the min value is itself. - assert_eq!(two_compl(MIN_NEGATIVE_VALUE), MIN_NEGATIVE_VALUE); + // Two's complement of the min value is itself. + assert_eq!(two_compl(MIN_NEGATIVE_VALUE), MIN_NEGATIVE_VALUE); + } } #[test] fn test_two_compl_mut() { - let mut value = ONE; - two_compl_mut(&mut value); - assert_eq!(value, NEG_ONE); + uint! { + let mut value = 1_U256; + two_compl_mut(&mut value); + assert_eq!(value, -1_U256); + } } #[test] fn test_i256_cmp() { - assert_eq!(i256_cmp(&ONE, &TWO), Ordering::Less); - assert_eq!(i256_cmp(&TWO, &TWO), Ordering::Equal); - assert_eq!(i256_cmp(&THREE, &TWO), Ordering::Greater); - assert_eq!(i256_cmp(&NEG_ONE, &NEG_ONE), Ordering::Equal); - assert_eq!(i256_cmp(&NEG_ONE, &NEG_TWO), Ordering::Greater); - assert_eq!(i256_cmp(&NEG_ONE, &ZERO), Ordering::Less); - assert_eq!(i256_cmp(&NEG_TWO, &TWO), Ordering::Less); + uint! { + assert_eq!(i256_cmp(&1_U256, &2_U256), Ordering::Less); + assert_eq!(i256_cmp(&2_U256, &2_U256), Ordering::Equal); + assert_eq!(i256_cmp(&3_U256, &2_U256), Ordering::Greater); + assert_eq!(i256_cmp(&-1_U256, &-1_U256), Ordering::Equal); + assert_eq!(i256_cmp(&-1_U256, &-2_U256), Ordering::Greater); + assert_eq!(i256_cmp(&-1_U256, &0_U256), Ordering::Less); + assert_eq!(i256_cmp(&-2_U256, &2_U256), Ordering::Less); + } } #[test] fn test_i256_div() { - assert_eq!(i256_div(ONE, ZERO), ZERO); - assert_eq!(i256_div(ZERO, ONE), ZERO); - assert_eq!(i256_div(ZERO, NEG_ONE), ZERO); - assert_eq!(i256_div(MIN_NEGATIVE_VALUE, ONE), MIN_NEGATIVE_VALUE); - assert_eq!(i256_div(FOUR, TWO), TWO); - assert_eq!(i256_div(MIN_NEGATIVE_VALUE, MIN_NEGATIVE_VALUE), ONE); - assert_eq!(i256_div(TWO, NEG_ONE), NEG_TWO); - assert_eq!(i256_div(NEG_TWO, NEG_ONE), TWO); + uint! { + assert_eq!(i256_div(1_U256, 0_U256), 0_U256); + assert_eq!(i256_div(0_U256, 1_U256), 0_U256); + assert_eq!(i256_div(0_U256, -1_U256), 0_U256); + assert_eq!(i256_div(MIN_NEGATIVE_VALUE, 1_U256), MIN_NEGATIVE_VALUE); + assert_eq!(i256_div(4_U256, 2_U256), 2_U256); + assert_eq!(i256_div(MIN_NEGATIVE_VALUE, MIN_NEGATIVE_VALUE), 1_U256); + assert_eq!(i256_div(2_U256, -1_U256), -2_U256); + assert_eq!(i256_div(-2_U256, -1_U256), 2_U256); + } } #[test] fn test_i256_mod() { - assert_eq!(i256_mod(ZERO, ONE), ZERO); - assert_eq!(i256_mod(ONE, ZERO), ZERO); - assert_eq!(i256_mod(FOUR, TWO), ZERO); - assert_eq!(i256_mod(THREE, TWO), ONE); - assert_eq!(i256_mod(MIN_NEGATIVE_VALUE, ONE), ZERO); - assert_eq!(i256_mod(TWO, TWO), ZERO); - assert_eq!(i256_mod(TWO, THREE), TWO); - assert_eq!(i256_mod(NEG_TWO, THREE), NEG_TWO); - assert_eq!(i256_mod(TWO, NEG_THREE), TWO); - assert_eq!(i256_mod(NEG_TWO, NEG_THREE), NEG_TWO); + uint! { + assert_eq!(i256_mod(0_U256, 1_U256), 0_U256); + assert_eq!(i256_mod(1_U256, 0_U256), 0_U256); + assert_eq!(i256_mod(4_U256, 2_U256), 0_U256); + assert_eq!(i256_mod(3_U256, 2_U256), 1_U256); + assert_eq!(i256_mod(MIN_NEGATIVE_VALUE, 1_U256), 0_U256); + assert_eq!(i256_mod(2_U256, 2_U256), 0_U256); + assert_eq!(i256_mod(2_U256, 3_U256), 2_U256); + assert_eq!(i256_mod(-2_U256, 3_U256), -2_U256); + assert_eq!(i256_mod(2_U256, -3_U256), 2_U256); + assert_eq!(i256_mod(-2_U256, -3_U256), -2_U256); + } } } diff --git a/crates/interpreter/src/instructions/macros.rs b/crates/interpreter/src/instructions/macros.rs index 861489d38f..b194f36ac9 100644 --- a/crates/interpreter/src/instructions/macros.rs +++ b/crates/interpreter/src/instructions/macros.rs @@ -1,4 +1,4 @@ -//! Utility macros to help implementementing opcode instruction functions. +//! Utility macros to help implementing opcode instruction functions. /// Fails the instruction if the current call is static. #[macro_export] @@ -78,7 +78,7 @@ macro_rules! resize_memory { return $ret; } - // Gas is calculated in evm words (256bits). + // Gas is calculated in evm words (256 bits). let words_num = rounded_size / 32; if !$interp .gas diff --git a/crates/interpreter/src/interpreter/shared_memory.rs b/crates/interpreter/src/interpreter/shared_memory.rs index c49cc319ed..433e24abb0 100644 --- a/crates/interpreter/src/interpreter/shared_memory.rs +++ b/crates/interpreter/src/interpreter/shared_memory.rs @@ -303,7 +303,8 @@ impl SharedMemory { } } -/// Rounds up `x` to the closest multiple of 32. If `x % 32 == 0` then `x` is returned. +/// Rounds up `x` to the closest multiple of 32. If `x % 32 == 0` then `x` is returned. Note, if `x` +/// is greater than `usize::MAX - 31` this will return `usize::MAX` which isn't a multiple of 32. #[inline] pub fn next_multiple_of_32(x: usize) -> usize { let r = x.bitand(31).not().wrapping_add(1).bitand(31); @@ -330,6 +331,9 @@ mod tests { let next_multiple = x + 32 - (x % 32); assert_eq!(next_multiple, next_multiple_of_32(x)); } + + // We expect large values to saturate and not overflow. + assert_eq!(usize::MAX, next_multiple_of_32(usize::MAX)); } #[test] diff --git a/crates/primitives/src/bytecode.rs b/crates/primitives/src/bytecode.rs index e13040bd43..8fc436a982 100644 --- a/crates/primitives/src/bytecode.rs +++ b/crates/primitives/src/bytecode.rs @@ -105,12 +105,12 @@ impl Bytecode { } } - /// Create new checked bytecode + /// Create new checked bytecode. /// /// # Safety /// - /// Bytecode need to end with STOP (0x00) opcode as checked bytecode assumes - /// that it is safe to iterate over bytecode without checking lengths + /// Bytecode needs to end with STOP (0x00) opcode as checked bytecode assumes + /// that it is safe to iterate over bytecode without checking lengths. pub unsafe fn new_checked(bytecode: Bytes, len: usize) -> Self { Self { bytecode, diff --git a/crates/revm/src/evm.rs b/crates/revm/src/evm.rs index d9307cccba..76bf4b633c 100644 --- a/crates/revm/src/evm.rs +++ b/crates/revm/src/evm.rs @@ -217,7 +217,7 @@ impl Evm<'_, EXT, DB> { &mut self, first_frame: Frame, ) -> Result> { - // take instruction talbe + // take instruction table let table = self .handler .take_instruction_table() @@ -265,7 +265,7 @@ impl Evm<'_, EXT, DB> { let next_action = interpreter.run(shared_memory, instruction_table, self); // take error and break the loop if there is any. - // This error is set From Interpreter when its interacting with Host. + // This error is set From Interpreter when it's interacting with Host. core::mem::replace(&mut self.context.evm.error, Ok(()))?; // take shared memory back. shared_memory = interpreter.take_memory(); diff --git a/crates/revm/src/frame.rs b/crates/revm/src/frame.rs index 3a7ade26e6..a48fd3c14c 100644 --- a/crates/revm/src/frame.rs +++ b/crates/revm/src/frame.rs @@ -12,7 +12,7 @@ use std::boxed::Box; pub struct CallFrame { /// Call frame has return memory range where output will be stored. pub return_memory_range: Range, - /// Frame data + /// Frame data. pub frame_data: FrameData, } @@ -20,15 +20,15 @@ pub struct CallFrame { pub struct CreateFrame { /// Create frame has a created address. pub created_address: Address, - /// Frame data + /// Frame data. pub frame_data: FrameData, } #[derive(Debug)] pub struct FrameData { - /// Journal checkpoint + /// Journal checkpoint. pub checkpoint: JournalCheckpoint, - /// Interpreter + /// Interpreter. pub interpreter: Interpreter, } diff --git a/crates/revm/src/handler.rs b/crates/revm/src/handler.rs index 6cd2e5b244..a6dfde551e 100644 --- a/crates/revm/src/handler.rs +++ b/crates/revm/src/handler.rs @@ -21,7 +21,7 @@ use self::register::{HandleRegister, HandleRegisterBox}; /// sections of the code. This allows nice integration of different chains or /// to disable some mainnet behavior. pub struct Handler<'a, H: Host + 'a, EXT, DB: Database> { - /// Handler config. + /// Handler configuration. pub cfg: HandlerCfg, /// Instruction table type. pub instruction_table: Option>, @@ -29,9 +29,9 @@ pub struct Handler<'a, H: Host + 'a, EXT, DB: Database> { pub registers: Vec>, /// Validity handles. pub validation: ValidationHandler<'a, EXT, DB>, - /// Pre execution handle + /// Pre execution handle. pub pre_execution: PreExecutionHandler<'a, EXT, DB>, - /// post Execution handle + /// Post Execution handle. pub post_execution: PostExecutionHandler<'a, EXT, DB>, /// Execution loop that handles frames. pub execution: ExecutionHandler<'a, EXT, DB>, @@ -40,7 +40,7 @@ pub struct Handler<'a, H: Host + 'a, EXT, DB: Database> { impl<'a, EXT, DB: Database> EvmHandler<'a, EXT, DB> { /// Created new Handler with given configuration. /// - /// Internaly it calls `mainnet_with_spec` with the given spec id. + /// Internally it calls `mainnet_with_spec` with the given spec id. /// Or `optimism_with_spec` if the optimism feature is enabled and `cfg.is_optimism` is set. pub fn new(cfg: HandlerCfg) -> Self { cfg_if::cfg_if! { @@ -85,7 +85,7 @@ impl<'a, EXT, DB: Database> EvmHandler<'a, EXT, DB> { handler } - /// Optimism with spec. Similar to [`Self::mainnet_with_spec`] + /// Optimism with spec. Similar to [`Self::mainnet_with_spec`]. #[cfg(feature = "optimism")] pub fn optimism_with_spec(spec_id: SpecId) -> Self { spec_to_generic!(spec_id, Self::optimism::()) @@ -185,7 +185,7 @@ impl<'a, EXT, DB: Database> EvmHandler<'a, EXT, DB> { let registers = core::mem::take(&mut self.registers); // register for optimism is added as a register, so we need to create mainnet handler here. let mut handler = Handler::mainnet_with_spec(spec_id); - // apply all registers to default handeler and raw mainnet instruction table. + // apply all registers to default handler and raw mainnet instruction table. for register in registers { handler.append_handler_register(register) } From d19ba5c2b32b6ca923497f63b91dd2588b1a1009 Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Mon, 1 Apr 2024 10:51:48 -0500 Subject: [PATCH 2/2] Move MAX_POSITIVE_VALUE back to tests --- crates/interpreter/src/instructions/i256.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/interpreter/src/instructions/i256.rs b/crates/interpreter/src/instructions/i256.rs index fe767eee3e..0067c6668e 100644 --- a/crates/interpreter/src/instructions/i256.rs +++ b/crates/interpreter/src/instructions/i256.rs @@ -18,13 +18,6 @@ const MIN_NEGATIVE_VALUE: U256 = U256::from_limbs([ 0x8000000000000000, ]); -const MAX_POSITIVE_VALUE: U256 = U256::from_limbs([ - 0xffffffffffffffff, - 0xffffffffffffffff, - 0xffffffffffffffff, - 0x7fffffffffffffff, -]); - const FLIPH_BITMASK_U64: u64 = 0x7FFFFFFFFFFFFFFF; #[inline] @@ -135,6 +128,13 @@ mod tests { use core::num::Wrapping; use revm_primitives::uint; + const MAX_POSITIVE_VALUE: U256 = U256::from_limbs([ + 0xffffffffffffffff, + 0xffffffffffffffff, + 0xffffffffffffffff, + 0x7fffffffffffffff, + ]); + #[test] fn div_i256() { // Sanity checks based on i8. Notice that we need to use `Wrapping` here because