[revm] pop_top and unsafe comments (#51)

bins/revm-test/src/main.rs

@@ -37,4 +37,5 @@ pub fn simple_example() {
 fn main() {
     println!("Hello, world!");
     simple_example();
+    println!("end!");
 }

bins/revme/src/debugger/ctrl/ctrl.rs

@@ -103,6 +103,7 @@ impl Controller {
         }
     }
 }
+
 ///
 impl<DB: Database> Inspector<DB> for Controller {
     fn step(

bins/revme/src/statetest/trace.rs

@@ -39,6 +39,8 @@ impl<DB: Database> Inspector<DB> for CustomPrintTracer {
         data: &mut EVMData<'_, DB>,
         _is_static: bool,
     ) -> Return {
+        // Safety: casting. In analazis we are making this clame true that program counter will always
+        // point to bytecode of the contract.
         let opcode = unsafe { *machine.program_counter };
         let opcode_str = opcode::OPCODE_JUMPMAP[opcode as usize];
 

crates/revm/src/db/web3db.rs

@@ -39,7 +39,7 @@ impl Web3DB {
     }
 
     /// internal utility function to call tokio feature and wait for output
-    fn block_on<F: std::future::Future>(&self, f: F) -> F::Output {
+    fn block_on<F: core::future::Future>(&self, f: F) -> F::Output {
         match &self.runtime {
             Some(runtime) => runtime.block_on(f),
             None => futures::executor::block_on(f),

crates/revm/src/evm.rs

@@ -5,7 +5,7 @@ use crate::{
     BerlinSpec, ByzantineSpec, Env, Inspector, IstanbulSpec, LatestSpec, Log, LondonSpec,
     NoOpInspector, Return, Spec, SpecId, TransactOut,
 };
-use alloc::boxed::Box;
+use alloc::{boxed::Box, vec::Vec};
 use revm_precompiles::Precompiles;
 /// Struct that takes Database and enabled transact to update state dirrectly to database.
 /// additionaly it allows user to set all environment parameters.
@@ -23,6 +23,8 @@ use revm_precompiles::Precompiles;
 /// want to update anything on it. It enabled `transact_ref` and `inspect_ref` functions
 /// * Database+DatabaseCommit allow's dirrectly commiting changes of transaction. it enabled `transact_commit`
 /// and `inspect_commit`
+
+#[derive(Clone)]
 pub struct EVM<DB> {
     pub env: Env,
     pub db: Option<DB>,

crates/revm/src/instructions/i256.rs

@@ -29,7 +29,7 @@ pub struct I256(pub Sign, pub U256);
 
 #[inline(always)]
 pub fn i256_sign<const DO_TWO_COMPL: bool>(val: &mut U256) -> Sign {
-    if unsafe { val.0.get_unchecked(3) } & SIGN_BITMASK_U64 == 0 {
+    if val.0[3] & SIGN_BITMASK_U64 == 0 {
         if val.is_zero() {
             Sign::Zero
         } else {
@@ -45,9 +45,7 @@ pub fn i256_sign<const DO_TWO_COMPL: bool>(val: &mut U256) -> Sign {
 
 #[inline(always)]
 fn u256_remove_sign(val: &mut U256) {
-    unsafe {
-        *val.0.get_unchecked_mut(3) = val.0.get_unchecked(3) & FLIPH_BITMASK_U64;
-    }
+    val.0[3] &= FLIPH_BITMASK_U64;
 }
 
 #[inline(always)]
@@ -438,11 +436,11 @@ mod tests {
         let mut f = U256([1, 100, 1, 1]);
         let mut s = U256([0, 0, 10, 0]);
 
-        let time = std::time::Instant::now();
+        //let time = std::time::Instant::now();
         for i in 0..1_000_000 {
             f.0[1] = i;
             s.0[3] = div_u256::div_mod(f, s).0 .0[3];
         }
-        println!("TIME:{:?}", time.elapsed());
+        //println!("TIME:{:?}", time.elapsed());
     }
 }

crates/revm/src/instructions/macros.rs

@@ -77,6 +77,7 @@ macro_rules! pop_address {
             return Return::StackUnderflow;
         }
         let mut temp = H256::zero();
+        // Safety: Length is checked above.
         let $x1: H160 = {
             unsafe {
                 $machine
@@ -93,6 +94,7 @@ macro_rules! pop_address {
         }
         let mut temp = H256::zero();
         $x1: H160 = {
+            // Safety: Length is checked above.
             unsafe {
                 $machine
                     .stack
@@ -103,6 +105,7 @@ macro_rules! pop_address {
         };
         $x2: H160 = {
             temp = H256::zero();
+            // Safety: Length is checked above.
             unsafe {
                 $machine
                     .stack
@@ -119,29 +122,57 @@ macro_rules! pop {
         if $machine.stack.len() < 1 {
             return Return::StackUnderflow;
         }
+        // Safety: Length is checked above.
         let $x1 = unsafe { $machine.stack.pop_unsafe() };
     };
     ( $machine:expr, $x1:ident, $x2:ident) => {
         if $machine.stack.len() < 2 {
             return Return::StackUnderflow;
         }
+        // Safety: Length is checked above.
         let ($x1, $x2) = unsafe { $machine.stack.pop2_unsafe() };
     };
     ( $machine:expr, $x1:ident, $x2:ident, $x3:ident) => {
         if $machine.stack.len() < 3 {
             return Return::StackUnderflow;
         }
+        // Safety: Length is checked above.
         let ($x1, $x2, $x3) = unsafe { $machine.stack.pop3_unsafe() };
     };
 
     ( $machine:expr, $x1:ident, $x2:ident, $x3:ident, $x4:ident) => {
         if $machine.stack.len() < 4 {
             return Return::StackUnderflow;
         }
+        // Safety: Length is checked above.
         let ($x1, $x2, $x3, $x4) = unsafe { $machine.stack.pop4_unsafe() };
     };
 }
 
+macro_rules! pop_top {
+    ( $machine:expr, $x1:ident) => {
+        if $machine.stack.len() < 1 {
+            return Return::StackUnderflow;
+        }
+        // Safety: Length is checked above.
+        let $x1 = unsafe { $machine.stack.top_unsafe() };
+    };
+    ( $machine:expr, $x1:ident, $x2:ident) => {
+        if $machine.stack.len() < 2 {
+            return Return::StackUnderflow;
+        }
+        // Safety: Length is checked above.
+        let ($x1, $x2) = unsafe { $machine.stack.pop_top_unsafe() };
+    };
+    ( $machine:expr, $x1:ident, $x2:ident, $x3:ident) => {
+        if $machine.stack.len() < 3 {
+            return Return::StackUnderflow;
+        }
+        // Safety: Length is checked above.
+        let ($x1, $x2, $x3) = unsafe { $machine.stack.pop2_top_unsafe() };
+    };
+}
+
 macro_rules! push_h256 {
 	( $machine:expr, $( $x:expr ),* ) => (
 		$(
@@ -167,9 +198,8 @@ macro_rules! push {
 macro_rules! op1_u256_fn {
     ( $machine:expr, $op:path ) => {{
         //gas!($machine, $gas);
-        pop!($machine, op1);
-        let ret = $op(op1);
-        push!($machine, ret);
+        pop_top!($machine, op1);
+        *op1 = $op(*op1);
 
         Return::Continue
     }};
@@ -178,9 +208,9 @@ macro_rules! op1_u256_fn {
 macro_rules! op2_u256_bool_ref {
     ( $machine:expr, $op:ident) => {{
         //gas!($machine, $gas);
-        pop!($machine, op1, op2);
+        pop_top!($machine, op1, op2);
         let ret = op1.$op(&op2);
-        push!($machine, if ret { U256::one() } else { U256::zero() });
+        *op2 = if ret { U256::one() } else { U256::zero() };
 
         Return::Continue
     }};
@@ -189,10 +219,8 @@ macro_rules! op2_u256_bool_ref {
 macro_rules! op2_u256 {
     ( $machine:expr, $op:ident) => {{
         //gas!($machine, $gas);
-        pop!($machine, op1, op2);
-        let ret = op1.$op(op2);
-        push!($machine, ret);
-
+        pop_top!($machine, op1, op2);
+        *op2 = op1.$op(*op2);
         Return::Continue
     }};
 }
@@ -201,16 +229,16 @@ macro_rules! op2_u256_tuple {
     ( $machine:expr, $op:ident) => {{
         //gas!($machine, $gas);
 
-        pop!($machine, op1, op2);
-        let (ret, ..) = op1.$op(op2);
-        push!($machine, ret);
+        pop_top!($machine, op1, op2);
+        let (ret, ..) = op1.$op(*op2);
+        *op2 = ret;
 
         Return::Continue
     }};
     ( $machine:expr, $op:ident ) => {{
-        pop!($machine, op1, op2);
+        pop_top!($machine, op1, op2);
         let (ret, ..) = op1.$op(op2);
-        push!($machine, ret);
+        *op2 = ret;
 
         Return::Continue
     }};
@@ -220,9 +248,8 @@ macro_rules! op2_u256_fn {
     ( $machine:expr, $op:path ) => {{
         //gas!($machine, $gas);
 
-        pop!($machine, op1, op2);
-        let ret = $op(op1, op2);
-        push!($machine, ret);
+        pop_top!($machine, op1, op2);
+        *op2 = $op(op1, *op2);
 
         Return::Continue
     }};
@@ -236,9 +263,8 @@ macro_rules! op3_u256_fn {
     ( $machine:expr, $op:path) => {{
         //gas!($machine, $gas);
 
-        pop!($machine, op1, op2, op3);
-        let ret = $op(op1, op2, op3);
-        push!($machine, ret);
+        pop_top!($machine, op1, op2, op3);
+        *op3 = $op(op1, op2, *op3);
 
         Return::Continue
     }};
@@ -250,40 +276,28 @@ macro_rules! op3_u256_fn {
 
 macro_rules! as_usize_saturated {
     ( $v:expr ) => {{
-        if unsafe {
-            *$v.0.get_unchecked(1) != 0
-                || *$v.0.get_unchecked(2) != 0
-                || *$v.0.get_unchecked(3) != 0
-        } {
+        if $v.0[1] != 0 || $v.0[2] != 0 || $v.0[3] != 0 {
             usize::MAX
         } else {
-            unsafe { *$v.0.get_unchecked(0) as usize }
+            $v.0[0] as usize
         }
     }};
 }
 
 macro_rules! as_usize_or_fail {
     ( $v:expr ) => {{
-        if unsafe {
-            *$v.0.get_unchecked(1) != 0
-                || *$v.0.get_unchecked(2) != 0
-                || *$v.0.get_unchecked(3) != 0
-        } {
+        if $v.0[1] != 0 || $v.0[2] != 0 || $v.0[3] != 0 {
             return Return::OutOfGas;
         }
 
-        unsafe { *$v.0.get_unchecked(0) as usize }
+        $v.0[0] as usize
     }};
 
     ( $v:expr, $reason:expr ) => {{
-        if unsafe {
-            *$v.0.get_unchecked(1) != 0
-                || *$v.0.get_unchecked(2) != 0
-                || *$v.0.get_unchecked(3) != 0
-        } {
+        if $v.0[1] != 0 || $v.0[2] != 0 || $v.0[3] != 0 {
             return $reason;
         }
 
-        unsafe { *$v.0.get_unchecked(0) as usize }
+        $v.0[0] as usize
     }};
 }

crates/revm/src/instructions/misc.rs

@@ -20,6 +20,7 @@ pub fn codecopy(machine: &mut Machine) -> Return {
     let code_offset = as_usize_saturated!(code_offset);
     memory_resize!(machine, memory_offset, len);
 
+    // Safety: set_data is unsafe function and memory_resize ensures us that it is safe to call it
     machine
         .memory
         .set_data(memory_offset, code_offset, len, &machine.contract.code);
@@ -68,6 +69,7 @@ pub fn calldatacopy(machine: &mut Machine) -> Return {
     let data_offset = as_usize_saturated!(data_offset);
     memory_resize!(machine, memory_offset, len);
 
+    // Safety: set_data is unsafe function and memory_resize ensures us that it is safe to call it
     machine
         .memory
         .set_data(memory_offset, data_offset, len, &machine.contract.input);
@@ -111,7 +113,7 @@ pub fn mstore8(machine: &mut Machine) -> Return {
     let index = as_usize_or_fail!(index, Return::OutOfGas);
     memory_resize!(machine, index, 1);
     let value = (value.low_u32() & 0xff) as u8;
-    // SAFETY: we resized our memory two lines above.
+    // Safety: we resized our memory two lines above.
     unsafe { machine.memory.set_byte(index, value) }
     Return::Continue
 }
@@ -123,6 +125,8 @@ pub fn jump(machine: &mut Machine) -> Return {
     let dest = as_usize_or_fail!(dest, Return::InvalidJump);
 
     if machine.contract.is_valid_jump(dest) {
+        // Safety: In analazis we are checking create our jump table and we do check above to be
+        // sure that jump is safe to execute.
         machine.program_counter = unsafe { machine.contract.code.as_ptr().add(dest) };
         Return::Continue
     } else {
@@ -138,6 +142,8 @@ pub fn jumpi(machine: &mut Machine) -> Return {
     if !value.is_zero() {
         let dest = as_usize_or_fail!(dest, Return::InvalidJump);
         if machine.contract.is_valid_jump(dest) {
+            // Safety: In analazis we are checking if jump is valid destination and this if.
+            // make this unsafe block safe.
             machine.program_counter = unsafe { machine.contract.code.as_ptr().add(dest) };
             Return::Continue
         } else {
@@ -172,6 +178,8 @@ pub fn push<const N: usize>(machine: &mut Machine) -> Return {
     //gas!(machine, gas::VERYLOW);
 
     let start = machine.program_counter;
+    // Safety: In Analazis we appended needed bytes for bytecode so that we are safe to just add without
+    // checking if it is out of bound. This makes both of our unsafes block safe to do.
     let ret = machine
         .stack
         .push_slice::<N>(unsafe { core::slice::from_raw_parts(start, N) });

crates/revm/src/instructions/system.rs

@@ -162,6 +162,7 @@ pub fn extcodecopy<H: Host, SPEC: Spec>(machine: &mut Machine, host: &mut H) ->
     let code_offset = min(as_usize_saturated!(code_offset), code.len());
     memory_resize!(machine, memory_offset, len);
 
+    // Safety: set_data is unsafe function and memory_resize ensures us that it is safe to call it
     machine
         .memory
         .set_data(memory_offset, code_offset, len, &code);
@@ -289,8 +290,8 @@ pub fn log<H: Host, SPEC: Spec>(machine: &mut Machine, n: u8, host: &mut H) -> R
 
     let mut topics = Vec::with_capacity(n);
     for _ in 0..(n) {
-        /*** SAFETY stack bounds already checked few lines above */
         let mut t = H256::zero();
+        // Sefety: stack bounds already checked few lines above
         unsafe { machine.stack.pop_unsafe().to_big_endian(t.as_bytes_mut()) };
         topics.push(t);
     }

crates/revm/src/lib.rs

@@ -1,6 +1,5 @@
 #![allow(dead_code)]
-//#![forbid(unsafe_code, unused_variables, unused_imports)]
-//#![no_std] only blocker in auto_impl check: https://github.com/bluealloy/revm/issues/4
+//#![no_std]
 
 pub mod db;
 mod evm;

crates/revm/src/machine/machine.rs

@@ -163,6 +163,7 @@ impl Machine {
 
     /// Return a reference of the program counter.
     pub fn program_counter(&self) -> usize {
+        // Sefety: this is just substraction of pointers, it is safe to do.
         unsafe {
             self.program_counter
                 .offset_from(self.contract.code.as_ptr()) as usize
@@ -186,6 +187,9 @@ impl Machine {
                 }
             }
             let opcode = unsafe { *self.program_counter };
+            // Safety: In analazis 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.program_counter = unsafe { self.program_counter.offset(1) };
             ret = eval::<H, SPEC>(opcode, self, host);