From d07d86bfd359c7960280c87c156e4e7405d80246 Mon Sep 17 00:00:00 2001 From: Yair <92672946+yair-starkware@users.noreply.github.com> Date: Thu, 14 Sep 2023 09:55:03 +0300 Subject: [PATCH] feat(execution): calculate transaction hashes for execution (#1140) --- Cargo.lock | 1 + crates/papyrus_execution/Cargo.toml | 1 + .../papyrus_execution/src/execution_test.rs | 11 -- crates/papyrus_execution/src/lib.rs | 111 ++++++++++++++++-- crates/papyrus_rpc/src/v0_4_0/api/api_impl.rs | 12 +- .../papyrus_rpc/src/v0_4_0/execution_test.rs | 20 ++-- 6 files changed, 114 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f2afbf3c9d..02f782f1a3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4361,6 +4361,7 @@ dependencies = [ "indexmap 1.9.3", "itertools 0.10.5", "lazy_static", + "papyrus_common", "papyrus_config", "papyrus_storage", "rand", diff --git a/crates/papyrus_execution/Cargo.toml b/crates/papyrus_execution/Cargo.toml index 6f2c8c485d..680137afad 100644 --- a/crates/papyrus_execution/Cargo.toml +++ b/crates/papyrus_execution/Cargo.toml @@ -19,6 +19,7 @@ cairo-lang-starknet.workspace = true cairo-vm.workspace = true itertools.workspace = true lazy_static.workspace = true +papyrus_common = { path = "../papyrus_common", version = "0.0.5" } papyrus_config = { path = "../papyrus_config", version = "0.0.5" } papyrus_storage = { path = "../papyrus_storage", version = "0.0.5" } rand = { workspace = true, optional = true } diff --git a/crates/papyrus_execution/src/execution_test.rs b/crates/papyrus_execution/src/execution_test.rs index 758b4de3ed..4513af1909 100644 --- a/crates/papyrus_execution/src/execution_test.rs +++ b/crates/papyrus_execution/src/execution_test.rs @@ -134,7 +134,6 @@ fn execute_call_cairo1() { // TODO(yair): Compare to the expected fee instead of asserting that it is not zero (all // estimate_fee tests). -#[ignore = "need to pass tx hashes"] #[test] fn estimate_fee_invoke() { let tx = TxsScenarioBuilder::default() @@ -147,7 +146,6 @@ fn estimate_fee_invoke() { } } -#[ignore = "need to pass tx hashes"] #[test] fn estimate_fee_declare_deprecated_class() { let tx = TxsScenarioBuilder::default().declare_deprecated_class(*ACCOUNT_ADDRESS).collect(); @@ -159,7 +157,6 @@ fn estimate_fee_declare_deprecated_class() { } } -#[ignore = "need to pass tx hashes"] #[test] fn estimate_fee_declare_class() { let tx = TxsScenarioBuilder::default().declare_class(*ACCOUNT_ADDRESS).collect(); @@ -171,7 +168,6 @@ fn estimate_fee_declare_class() { } } -#[ignore = "need to pass tx hashes"] #[test] fn estimate_fee_deploy_account() { let tx = TxsScenarioBuilder::default().deploy_account().collect(); @@ -183,7 +179,6 @@ fn estimate_fee_deploy_account() { } } -#[ignore = "need to pass tx hashes"] #[test] fn estimate_fee_combination() { let txs = TxsScenarioBuilder::default() @@ -225,7 +220,6 @@ fn serialization_precision() { assert_eq!(input, deserialized); } -#[ignore = "need to pass tx hashes"] #[test] fn simulate_invoke() { let ((storage_reader, storage_writer), _temp_dir) = get_test_storage(); @@ -303,7 +297,6 @@ fn simulate_invoke() { } } -#[ignore = "need to pass tx hashes"] #[test] fn simulate_declare_deprecated() { let ((storage_reader, storage_writer), _temp_dir) = get_test_storage(); @@ -363,7 +356,6 @@ fn simulate_declare_deprecated() { } } -#[ignore = "need to pass tx hashes"] #[test] fn simulate_declare() { let ((storage_reader, storage_writer), _temp_dir) = get_test_storage(); @@ -423,7 +415,6 @@ fn simulate_declare() { } } -#[ignore = "need to pass tx hashes"] #[test] fn simulate_deploy_account() { let ((storage_reader, storage_writer), _temp_dir) = get_test_storage(); @@ -497,7 +488,6 @@ fn simulate_deploy_account() { } } -#[ignore = "need to pass tx hashes"] #[test] fn simulate_invoke_from_new_account() { let ((storage_reader, storage_writer), _temp_dir) = get_test_storage(); @@ -534,7 +524,6 @@ fn simulate_invoke_from_new_account() { assert_matches!(invoke_trace.execute_invocation, FunctionInvocationResult::Ok(_)); } -#[ignore = "need to pass tx hashes"] #[test] fn simulate_invoke_from_new_account_validate_and_charge() { let ((storage_reader, storage_writer), _temp_dir) = get_test_storage(); diff --git a/crates/papyrus_execution/src/lib.rs b/crates/papyrus_execution/src/lib.rs index 4d4932678b..0e8824b441 100644 --- a/crates/papyrus_execution/src/lib.rs +++ b/crates/papyrus_execution/src/lib.rs @@ -12,7 +12,6 @@ pub mod testing_instances; pub mod objects; use std::collections::{BTreeMap, HashMap}; -use std::iter; use std::sync::Arc; use blockifier::block_context::{BlockContext, FeeTokenAddresses, GasPrices}; @@ -35,6 +34,7 @@ use cairo_lang_starknet::casm_contract_class::CasmContractClass; use cairo_vm::types::errors::program_errors::ProgramError; use execution_utils::get_trace_constructor; use objects::TransactionTrace; +use papyrus_common::transaction_hash::get_transaction_hash; use papyrus_storage::compiled_class::CasmStorageReader; use papyrus_storage::db::RO; use papyrus_storage::header::HeaderStorageReader; @@ -62,7 +62,9 @@ use starknet_api::transaction::{ Transaction, TransactionHash, }; +use starknet_api::StarknetApiError; use state_reader::ExecutionStateReader; +use tracing::trace; /// Result type for execution functions. pub type ExecutionResult = Result; @@ -140,6 +142,8 @@ pub enum ExecutionError { NotSynced { state_number: StateNumber, compiled_class_marker: BlockNumber }, #[error(transparent)] StateError(#[from] StateError), + #[error("Failed to calculate transaction hash.")] + TransactionHashCalculationFailed(StarknetApiError), #[error(transparent)] PreExecutionError(#[from] PreExecutionError), #[error(transparent)] @@ -273,6 +277,95 @@ pub enum ExecutableTransactionInput { L1Handler(L1HandlerTransaction, Fee), } +impl ExecutableTransactionInput { + fn calc_tx_hash(self, chain_id: &ChainId) -> ExecutionResult<(Self, TransactionHash)> { + match self.apply_on_transaction(|tx| get_transaction_hash(tx, chain_id)) { + (original_tx, Ok(tx_hash)) => Ok((original_tx, tx_hash)), + (_, Err(err)) => Err(ExecutionError::TransactionHashCalculationFailed(err)), + } + } + + /// Applies a non consuming function on the transaction as if it was of type [Transaction] of + /// StarknetAPI and returns the result without cloning the original transaction. + // TODO(yair): Refactor this. + fn apply_on_transaction(self, func: F) -> (Self, T) + where + F: Fn(&Transaction) -> T, + { + match self { + ExecutableTransactionInput::Invoke(tx) => { + let as_transaction = Transaction::Invoke(tx); + let res = func(&as_transaction); + let Transaction::Invoke(tx) = as_transaction else { + unreachable!("Should be invoke transaction.") + }; + (Self::Invoke(tx), res) + } + ExecutableTransactionInput::DeclareV0(tx, class) => { + let as_transaction = Transaction::Declare(DeclareTransaction::V0(tx)); + let res = func(&as_transaction); + let Transaction::Declare(DeclareTransaction::V0(tx)) = as_transaction else { + unreachable!("Should be declare v0 transaction.") + }; + (Self::DeclareV0(tx, class), res) + } + ExecutableTransactionInput::DeclareV1(tx, class) => { + let as_transaction = Transaction::Declare(DeclareTransaction::V1(tx)); + let res = func(&as_transaction); + let Transaction::Declare(DeclareTransaction::V1(tx)) = as_transaction else { + unreachable!("Should be declare v1 transaction.") + }; + (Self::DeclareV1(tx, class), res) + } + ExecutableTransactionInput::DeclareV2(tx, class) => { + let as_transaction = Transaction::Declare(DeclareTransaction::V2(tx)); + let res = func(&as_transaction); + let Transaction::Declare(DeclareTransaction::V2(tx)) = as_transaction else { + unreachable!("Should be declare v2 transaction.") + }; + (Self::DeclareV2(tx, class), res) + } + ExecutableTransactionInput::DeclareV3(tx, class) => { + let as_transaction = Transaction::Declare(DeclareTransaction::V3(tx)); + let res = func(&as_transaction); + let Transaction::Declare(DeclareTransaction::V3(tx)) = as_transaction else { + unreachable!("Should be declare v3 transaction.") + }; + (Self::DeclareV3(tx, class), res) + } + ExecutableTransactionInput::DeployAccount(tx) => { + let as_transaction = Transaction::DeployAccount(tx); + let res = func(&as_transaction); + let Transaction::DeployAccount(tx) = as_transaction else { + unreachable!("Should be deploy account transaction.") + }; + (Self::DeployAccount(tx), res) + } + ExecutableTransactionInput::L1Handler(tx, fee) => { + let as_transaction = Transaction::L1Handler(tx); + let res = func(&as_transaction); + let Transaction::L1Handler(tx) = as_transaction else { + unreachable!("Should be L1 handler transaction.") + }; + (Self::L1Handler(tx, fee), res) + } + } + } +} + +/// Calculates the transaction hashes for a series of transactions without cloning the transactions. +fn calc_tx_hashes( + txs: Vec, + chain_id: &ChainId, +) -> ExecutionResult<(Vec, Vec)> { + Ok(txs + .into_iter() + .map(|tx| tx.calc_tx_hash(chain_id)) + .collect::, _>>()? + .into_iter() + .unzip()) +} + /// Returns the fee estimation for a series of transactions. pub fn estimate_fee( txs: Vec, @@ -333,13 +426,17 @@ fn execute_transactions( execution_config, ); - let tx_hashes_iter: Box>> = match tx_hashes { - Some(hashes) => Box::new(hashes.into_iter().map(Some)), - None => Box::new(iter::repeat(None)), + let (txs, tx_hashes) = match tx_hashes { + Some(tx_hashes) => (txs, tx_hashes), + None => { + let tx_hashes = calc_tx_hashes(txs, chain_id)?; + trace!("Calculated tx hashes: {:?}", tx_hashes); + tx_hashes + } }; let mut res = vec![]; - for (tx, tx_hash) in txs.into_iter().zip(tx_hashes_iter) { + for (tx, tx_hash) in txs.into_iter().zip(tx_hashes.into_iter()) { let blockifier_tx = to_blockifier_tx(tx, tx_hash)?; let tx_execution_info = blockifier_tx.execute(&mut cached_state, &block_context, charge_fee, validate)?; @@ -351,10 +448,8 @@ fn execute_transactions( fn to_blockifier_tx( tx: ExecutableTransactionInput, - tx_hash: Option, + tx_hash: TransactionHash, ) -> ExecutionResult { - // TODO(yair): Remove the unwrap once the blockifier calculates the tx hash. - let tx_hash = tx_hash.unwrap_or_default(); match tx { ExecutableTransactionInput::Invoke(invoke_tx) => Ok(BlockifierTransaction::from_api( Transaction::Invoke(invoke_tx), diff --git a/crates/papyrus_rpc/src/v0_4_0/api/api_impl.rs b/crates/papyrus_rpc/src/v0_4_0/api/api_impl.rs index 569e0f9b13..43d5714d8d 100644 --- a/crates/papyrus_rpc/src/v0_4_0/api/api_impl.rs +++ b/crates/papyrus_rpc/src/v0_4_0/api/api_impl.rs @@ -628,18 +628,13 @@ impl JsonRpcV0_4Server for JsonRpcServerV0_4Impl { transactions: Vec, block_id: BlockId, ) -> RpcResult> { - // TODO(yair): remove this once the transaction hash is calculated in the blockifier. - if true { - return Err(internal_server_error("Fee estimation not supported yet.")); - } - trace!("Estimating fee of transactions: {:#?}", transactions); let executable_txns = transactions.into_iter().map(|tx| tx.try_into()).collect::>()?; let txn = self.storage_reader.begin_ro_txn().map_err(internal_server_error)?; let block_number = get_block_number(&txn, block_id)?; - let state_number = StateNumber::right_after_block(block_number); + let state_number = StateNumber::right_before_block(block_number); let block_execution_config = self.execution_config.get_execution_config_for_block(block_number).map_err(|err| { internal_server_error(format!("Failed to get execution config: {}", err)) @@ -668,11 +663,6 @@ impl JsonRpcV0_4Server for JsonRpcServerV0_4Impl { transactions: Vec, simulation_flags: Vec, ) -> RpcResult> { - // TODO(yair): remove this once the transaction hash is calculated in the blockifier. - if true { - return Err(internal_server_error("Simulate transactions not supported yet.")); - } - trace!("Simulating transactions: {:#?}", transactions); let executable_txns = transactions.into_iter().map(|tx| tx.try_into()).collect::>()?; diff --git a/crates/papyrus_rpc/src/v0_4_0/execution_test.rs b/crates/papyrus_rpc/src/v0_4_0/execution_test.rs index 2a911b18ce..b1ec6b6270 100644 --- a/crates/papyrus_rpc/src/v0_4_0/execution_test.rs +++ b/crates/papyrus_rpc/src/v0_4_0/execution_test.rs @@ -149,7 +149,6 @@ async fn execution_call() { assert_matches!(err, Error::Call(err) if err == CONTRACT_ERROR.into()); } -#[ignore = "need to pass tx hashes"] #[tokio::test] async fn call_estimate_fee() { let (module, storage_writer) = @@ -183,15 +182,14 @@ async fn call_estimate_fee() { // TODO(yair): verify this is the correct fee, got this value by printing the result of the // call. let expected_fee_estimate = vec![FeeEstimate { - gas_consumed: stark_felt!("0x19a2"), + gas_consumed: stark_felt!("0x9ba"), gas_price: *GAS_PRICE, - overall_fee: Fee(656200000000000), + overall_fee: Fee(249000000000000), }]; assert_eq!(res, expected_fee_estimate); } -#[ignore = "need to pass tx hashes"] #[tokio::test] async fn call_simulate() { let (module, storage_writer) = @@ -232,9 +230,9 @@ async fn call_simulate() { // call. // Why is it different from the estimate_fee call? let expected_fee_estimate = FeeEstimate { - gas_consumed: stark_felt!("0x19b7"), + gas_consumed: stark_felt!("0x9ba"), gas_price: *GAS_PRICE, - overall_fee: Fee(658300000000000), + overall_fee: Fee(249000000000000), }; assert_eq!(simulated_tx.fee_estimation, expected_fee_estimate); @@ -250,7 +248,6 @@ async fn call_simulate() { assert_matches!(invoke_trace.fee_transfer_invocation, Some(_)); } -#[ignore = "need to pass tx hashes"] #[tokio::test] async fn call_simulate_skip_validate() { let (module, storage_writer) = @@ -291,9 +288,9 @@ async fn call_simulate_skip_validate() { // call. // Why is it different from the estimate_fee call? let expected_fee_estimate = FeeEstimate { - gas_consumed: stark_felt!("0x19a2"), + gas_consumed: stark_felt!("0x9ba"), gas_price: *GAS_PRICE, - overall_fee: Fee(656200000000000), + overall_fee: Fee(249000000000000), }; assert_eq!(simulated_tx.fee_estimation, expected_fee_estimate); @@ -309,7 +306,6 @@ async fn call_simulate_skip_validate() { assert_matches!(invoke_trace.fee_transfer_invocation, Some(_)); } -#[ignore = "need to pass tx hashes"] #[tokio::test] async fn call_simulate_skip_fee_charge() { let (module, storage_writer) = @@ -350,9 +346,9 @@ async fn call_simulate_skip_fee_charge() { // call. // Why is it different from the estimate_fee call? let expected_fee_estimate = FeeEstimate { - gas_consumed: stark_felt!("0x19b7"), + gas_consumed: stark_felt!("9ba"), gas_price: *GAS_PRICE, - overall_fee: Fee(658300000000000), + overall_fee: Fee(249000000000000), }; assert_eq!(simulated_tx.fee_estimation, expected_fee_estimate);