From 9e27943899e4064585423b7d83d43e63f108fa03 Mon Sep 17 00:00:00 2001 From: Elias Tazartes Date: Wed, 18 Oct 2023 18:42:04 +0700 Subject: [PATCH 1/3] feat: implement block hash opcode, soon tests --- .trunk/trunk.yaml | 2 +- crates/evm/src/errors.cairo | 1 + .../src/instructions/block_information.cairo | 27 ++++++++-- crates/evm/src/stack.cairo | 17 ++++++- .../test_block_information.cairo | 51 +++++++++++++++++++ crates/utils/src/traits.cairo | 4 +- 6 files changed, 95 insertions(+), 7 deletions(-) diff --git a/.trunk/trunk.yaml b/.trunk/trunk.yaml index 9c3d2d447..f4c851519 100644 --- a/.trunk/trunk.yaml +++ b/.trunk/trunk.yaml @@ -2,7 +2,7 @@ # To learn more about the format of this file, see https://docs.trunk.io/reference/trunk-yaml version: 0.1 cli: - version: 1.17.0 + version: 1.17.1 plugins: sources: - id: trunk diff --git a/crates/evm/src/errors.cairo b/crates/evm/src/errors.cairo index 6cbf69fc3..e31cdeb0b 100644 --- a/crates/evm/src/errors.cairo +++ b/crates/evm/src/errors.cairo @@ -19,6 +19,7 @@ const WRITE_IN_STATIC_CONTEXT: felt252 = 'KKT: WriteInStaticContext'; // STARKNET_SYSCALLS const READ_SYSCALL_FAILED: felt252 = 'KKT: read syscall failed'; +const BLOCK_HASH_SYSCALL_FAILED: felt252 = 'KKT: block_hash syscall failed'; #[derive(Drop, Copy, PartialEq)] enum EVMError { diff --git a/crates/evm/src/instructions/block_information.cairo b/crates/evm/src/instructions/block_information.cairo index b1ea71a6b..4a8dcf6c4 100644 --- a/crates/evm/src/instructions/block_information.cairo +++ b/crates/evm/src/instructions/block_information.cairo @@ -1,11 +1,12 @@ //! Block Information. -use evm::errors::EVMError; +use evm::errors::{EVMError, BLOCK_HASH_SYSCALL_FAILED}; use evm::machine::{Machine, MachineCurrentContextTrait}; use evm::stack::StackTrait; // Corelib imports -use starknet::info::{get_block_number, get_block_timestamp}; +use starknet::info::{get_block_number, get_block_timestamp, get_block_info}; +use starknet::{get_block_hash_syscall}; use utils::constants::CHAIN_ID; #[generate_trait] @@ -14,7 +15,27 @@ impl BlockInformation of BlockInformationTrait { /// Get the hash of one of the 256 most recent complete blocks. /// # Specification: https://www.evm.codes/#40?fork=shanghai fn exec_blockhash(ref self: Machine) -> Result<(), EVMError> { - Result::Err(EVMError::NotImplemented) + let block_number = self.stack.pop_u64()?; + let current_block = get_block_number(); + + // If input block number is lower than current_block - 256, return 0 + // If input block number is higher than current_block - 10, return 0 + // Note: in the specs, input block number can be equal - at most - to the current block number minus one. + // In Starknet, the `get_block_hash_syscall` is capped at current block minus ten. + // TODO: monitor the changes in the `get_block_hash_syscall` syscall. + // source: https://docs.starknet.io/documentation/architecture_and_concepts/Smart_Contracts/system-calls-cairo1/#get_block_hash + if block_number + 10 > current_block || block_number + 256 < current_block { + return self.stack.push(0); + } + + let block_hash = get_block_hash_syscall(block_number); + match block_hash { + Result::Ok(block_hash) => self.stack.push(block_hash.into()), + // This syscall should not error out, as we made sure block_number =< current_block - 10 + // In case of failed syscall, we can either return 0, or revert. + // Since this situation would be highly breaking, we choose to revert. + Result::Err(_) => Result::Err(EVMError::SyscallFailed(BLOCK_HASH_SYSCALL_FAILED)), + } } /// 0x41 - COINBASE diff --git a/crates/evm/src/stack.cairo b/crates/evm/src/stack.cairo index 113669cac..9d91767d3 100644 --- a/crates/evm/src/stack.cairo +++ b/crates/evm/src/stack.cairo @@ -36,6 +36,7 @@ trait StackTrait { fn push(ref self: Stack, item: u256) -> Result<(), EVMError>; fn pop(ref self: Stack) -> Result; fn pop_usize(ref self: Stack) -> Result; + fn pop_u64(ref self: Stack) -> Result; fn pop_i256(ref self: Stack) -> Result; fn pop_eth_address(ref self: Stack) -> Result; fn pop_n(ref self: Stack, n: usize) -> Result, EVMError>; @@ -114,11 +115,25 @@ impl StackImpl of StackTrait { #[inline(always)] fn pop_usize(ref self: Stack) -> Result { let item: u256 = self.pop()?; - // item.try_into().ok_or(EVMError::TypeConversionError(TYPE_CONVERSION_ERROR)) let item: usize = item.try_into_result()?; Result::Ok(item) } + /// Calls `Stack::pop` and tries to convert it to usize + /// + /// # Errors + /// + /// Returns `EVMError::StackError` with appropriate message + /// In case: + /// - Stack is empty + /// - Type conversion failed + #[inline(always)] + fn pop_u64(ref self: Stack) -> Result { + let item: u256 = self.pop()?; + let item: u64 = item.try_into_result()?; + Result::Ok(item) + } + /// Calls `Stack::pop` and convert it to i256 /// /// # Errors diff --git a/crates/evm/src/tests/test_instructions/test_block_information.cairo b/crates/evm/src/tests/test_instructions/test_block_information.cairo index 28cbc1943..7dfbe6c50 100644 --- a/crates/evm/src/tests/test_instructions/test_block_information.cairo +++ b/crates/evm/src/tests/test_instructions/test_block_information.cairo @@ -1,9 +1,60 @@ +use debug::U256PrintImpl; use evm::instructions::BlockInformationTrait; use evm::stack::StackTrait; use evm::tests::test_utils::setup_machine; use starknet::testing::{set_block_timestamp, set_block_number}; use utils::constants::CHAIN_ID; +/// 0x40 - BLOCKHASH +#[test] +#[available_gas(20000000)] +fn test_block_hash_below_bounds() { + // Given + let mut machine = setup_machine(); + + set_block_number(500); + + // When + machine.stack.push(243).unwrap(); + machine.exec_blockhash(); + + // Then + assert(machine.stack.peek().unwrap() == 0, 'stack top should be 1692873993'); +} + +#[test] +#[available_gas(20000000)] +fn test_block_hash_above_bounds() { + // Given + let mut machine = setup_machine(); + + set_block_number(500); + + // When + machine.stack.push(491).unwrap(); + machine.exec_blockhash(); + + // Then + assert(machine.stack.peek().unwrap() == 0, 'stack top should be 1692873993'); +} + +#[test] +#[available_gas(20000000)] +fn test_block_hash_within_bounds() { + // Given + let mut machine = setup_machine(); + + set_block_number(500); + + // When + machine.stack.push(244).unwrap(); + machine.exec_blockhash(); + // Then + machine.stack.peek().unwrap().print(); + assert(machine.stack.peek().unwrap() == 1692873993, 'stack top should be 1692873993'); +} + + #[test] #[available_gas(20000000)] fn test_block_timestamp_set_to_1692873993() { diff --git a/crates/utils/src/traits.cairo b/crates/utils/src/traits.cairo index eea4afc0b..0061727ce 100644 --- a/crates/utils/src/traits.cairo +++ b/crates/utils/src/traits.cairo @@ -85,11 +85,11 @@ trait TryIntoResult { fn try_into_result(self: T) -> Result; } -impl U256TryIntoResultU32 of TryIntoResult { +impl U256TryIntoResultU32> of TryIntoResult { /// Converts a u256 into a Result /// If the u256 is larger than MAX_U32, it returns an error. /// Otherwise, it returns the casted value. - fn try_into_result(self: u256) -> Result { + fn try_into_result(self: u256) -> Result { match self.try_into() { Option::Some(value) => Result::Ok(value), Option::None => Result::Err(EVMError::TypeConversionError(TYPE_CONVERSION_ERROR)) From d54429ced42ce412e5b774417d9891522a27d17b Mon Sep 17 00:00:00 2001 From: Elias Tazartes Date: Wed, 18 Oct 2023 18:50:39 +0700 Subject: [PATCH 2/3] fix: ignore test as mock blockhash not implemented --- .../tests/test_instructions/test_block_information.cairo | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/evm/src/tests/test_instructions/test_block_information.cairo b/crates/evm/src/tests/test_instructions/test_block_information.cairo index 7dfbe6c50..ff84788fd 100644 --- a/crates/evm/src/tests/test_instructions/test_block_information.cairo +++ b/crates/evm/src/tests/test_instructions/test_block_information.cairo @@ -38,6 +38,9 @@ fn test_block_hash_above_bounds() { assert(machine.stack.peek().unwrap() == 0, 'stack top should be 1692873993'); } +// TODO: implement exec_blockhash testing for block number within bounds +// https://github.com/starkware-libs/cairo/blob/77a7e7bc36aa1c317bb8dd5f6f7a7e6eef0ab4f3/crates/cairo-lang-starknet/cairo_level_tests/interoperability.cairo#L173 +#[ignore] #[test] #[available_gas(20000000)] fn test_block_hash_within_bounds() { @@ -50,8 +53,7 @@ fn test_block_hash_within_bounds() { machine.stack.push(244).unwrap(); machine.exec_blockhash(); // Then - machine.stack.peek().unwrap().print(); - assert(machine.stack.peek().unwrap() == 1692873993, 'stack top should be 1692873993'); + assert(machine.stack.peek().unwrap() == 0xF, 'stack top should be 0xF'); } From 845f8fc1b1f9f3457004a2d436ed49b0463bacbe Mon Sep 17 00:00:00 2001 From: Elias Tazartes Date: Thu, 19 Oct 2023 09:59:35 +0700 Subject: [PATCH 3/3] fix: fix comments --- crates/evm/src/instructions/block_information.cairo | 4 ++-- crates/evm/src/stack.cairo | 2 +- .../test_instructions/test_block_information.cairo | 10 +++++----- crates/utils/src/traits.cairo | 6 +++--- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/evm/src/instructions/block_information.cairo b/crates/evm/src/instructions/block_information.cairo index 166861539..a4c720c73 100644 --- a/crates/evm/src/instructions/block_information.cairo +++ b/crates/evm/src/instructions/block_information.cairo @@ -29,8 +29,8 @@ impl BlockInformation of BlockInformationTrait { return self.stack.push(0); } - let block_hash = get_block_hash_syscall(block_number); - match block_hash { + let maybe_block_hash = get_block_hash_syscall(block_number); + match maybe_block_hash { Result::Ok(block_hash) => self.stack.push(block_hash.into()), // This syscall should not error out, as we made sure block_number =< current_block - 10 // In case of failed syscall, we can either return 0, or revert. diff --git a/crates/evm/src/stack.cairo b/crates/evm/src/stack.cairo index 1e3e48f35..6e6cf2699 100644 --- a/crates/evm/src/stack.cairo +++ b/crates/evm/src/stack.cairo @@ -118,7 +118,7 @@ impl StackImpl of StackTrait { Result::Ok(item) } - /// Calls `Stack::pop` and tries to convert it to usize + /// Calls `Stack::pop` and tries to convert it to u64 /// /// # Errors /// diff --git a/crates/evm/src/tests/test_instructions/test_block_information.cairo b/crates/evm/src/tests/test_instructions/test_block_information.cairo index 4bf48ae55..564ef3bd9 100644 --- a/crates/evm/src/tests/test_instructions/test_block_information.cairo +++ b/crates/evm/src/tests/test_instructions/test_block_information.cairo @@ -12,7 +12,7 @@ use utils::constants::CHAIN_ID; /// 0x40 - BLOCKHASH #[test] #[available_gas(20000000)] -fn test_block_hash_below_bounds() { +fn test_exec_blockhash_below_bounds() { // Given let mut machine = setup_machine(); @@ -23,12 +23,12 @@ fn test_block_hash_below_bounds() { machine.exec_blockhash(); // Then - assert(machine.stack.peek().unwrap() == 0, 'stack top should be 1692873993'); + assert(machine.stack.peek().unwrap() == 0, 'stack top should be 0'); } #[test] #[available_gas(20000000)] -fn test_block_hash_above_bounds() { +fn test_exec_blockhash_above_bounds() { // Given let mut machine = setup_machine(); @@ -39,7 +39,7 @@ fn test_block_hash_above_bounds() { machine.exec_blockhash(); // Then - assert(machine.stack.peek().unwrap() == 0, 'stack top should be 1692873993'); + assert(machine.stack.peek().unwrap() == 0, 'stack top should be 0'); } // TODO: implement exec_blockhash testing for block number within bounds @@ -47,7 +47,7 @@ fn test_block_hash_above_bounds() { #[ignore] #[test] #[available_gas(20000000)] -fn test_block_hash_within_bounds() { +fn test_exec_blockhash_within_bounds() { // Given let mut machine = setup_machine(); diff --git a/crates/utils/src/traits.cairo b/crates/utils/src/traits.cairo index 0061727ce..1f49d1596 100644 --- a/crates/utils/src/traits.cairo +++ b/crates/utils/src/traits.cairo @@ -85,9 +85,9 @@ trait TryIntoResult { fn try_into_result(self: T) -> Result; } -impl U256TryIntoResultU32> of TryIntoResult { - /// Converts a u256 into a Result - /// If the u256 is larger than MAX_U32, it returns an error. +impl U256TryIntoResult> of TryIntoResult { + /// Converts a u256 into a Result + /// If the u256 cannot be converted into U, it returns an error. /// Otherwise, it returns the casted value. fn try_into_result(self: u256) -> Result { match self.try_into() {