diff --git a/CHANGELOG.md b/CHANGELOG.md index e445a4c75f..6dd98781d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Next release +- fix: txn hash calculation and refactor - chore: remove all da/settlement related code - fix: re-execute txs instead of simulating for txn receipts - chore: rebase on latest blockifier diff --git a/Cargo.lock b/Cargo.lock index a57e71977d..8e4c7f4859 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6241,6 +6241,7 @@ dependencies = [ "cairo-vm", "flate2", "indexmap 2.2.5", + "log", "mp-felt", "mp-hashers", "num-bigint", diff --git a/crates/client/rpc/src/lib.rs b/crates/client/rpc/src/lib.rs index 9a5b0577fa..2704c54159 100644 --- a/crates/client/rpc/src/lib.rs +++ b/crates/client/rpc/src/lib.rs @@ -142,7 +142,7 @@ where C: HeaderBackend + 'static, { pub fn current_spec_version(&self) -> RpcResult { - Ok("0.4.0".to_string()) + Ok("0.7.0".to_string()) } } diff --git a/crates/pallets/starknet/src/lib.rs b/crates/pallets/starknet/src/lib.rs index a11018ba8d..40ccf71a61 100644 --- a/crates/pallets/starknet/src/lib.rs +++ b/crates/pallets/starknet/src/lib.rs @@ -518,7 +518,10 @@ pub mod pallet { } _ => run_revertible_transaction(&transaction, &mut state, &block_context, true, charge_fee), } - .map_err(|_| Error::::TransactionExecutionFailed)?; + .map_err(|e| { + log!(error, "Invoke transaction execution failed: {:?}", e); + Error::::TransactionExecutionFailed + })?; Self::emit_and_store_tx_and_fees_events( transaction.tx_hash, @@ -569,7 +572,10 @@ pub mod pallet { // Execute let tx_execution_infos = run_non_revertible_transaction(&transaction, &mut state, &Self::get_block_context(), true, charge_fee) - .map_err(|_| Error::::TransactionExecutionFailed)?; + .map_err(|e| { + log!(error, "Declare transaction execution failed: {:?}", e); + Error::::TransactionExecutionFailed + })?; Self::emit_and_store_tx_and_fees_events( transaction.tx_hash(), @@ -616,7 +622,10 @@ pub mod pallet { // Execute let tx_execution_infos = run_non_revertible_transaction(&transaction, &mut state, &Self::get_block_context(), true, charge_fee) - .map_err(|_| Error::::TransactionExecutionFailed)?; + .map_err(|e| { + log!(error, "Deploy account transaction execution failed: {:?}", e); + Error::::TransactionExecutionFailed + })?; Self::emit_and_store_tx_and_fees_events( transaction.tx_hash, @@ -666,8 +675,10 @@ pub mod pallet { // Execute let tx_execution_infos = - execute_l1_handler_transaction(&transaction, &mut state, &Self::get_block_context()) - .map_err(|_| Error::::TransactionExecutionFailed)?; + execute_l1_handler_transaction(&transaction, &mut state, &Self::get_block_context()).map_err(|e| { + log!(error, "L1 Handler transaction execution failed: {:?}", e); + Error::::TransactionExecutionFailed + })?; Self::emit_and_store_tx_and_fees_events( transaction.tx_hash, diff --git a/crates/pallets/starknet/src/simulations.rs b/crates/pallets/starknet/src/simulations.rs index e8852e63f7..a0db97c90d 100644 --- a/crates/pallets/starknet/src/simulations.rs +++ b/crates/pallets/starknet/src/simulations.rs @@ -17,7 +17,7 @@ use sp_runtime::DispatchError; use starknet_api::transaction::TransactionVersion; use crate::blockifier_state_adapter::BlockifierStateAdapter; -use crate::{Config, Error, Pallet}; +use crate::{log, Config, Error, Pallet}; type ReExecutionResult = Result< Vec<(TransactionExecutionInfo, Option)>, @@ -49,8 +49,22 @@ impl Pallet { let fee_res_iterator = transactions .into_iter() .map(|tx| match Self::execute_account_transaction(&tx, &mut state, &block_context, simulation_flags) { - Ok(execution_info) if !execution_info.is_reverted() => Ok(execution_info), - Err(_) | Ok(_) => Err(Error::::TransactionExecutionFailed), + Ok(execution_info) => { + if !execution_info.is_reverted() { + Ok(execution_info) + } else { + log!( + debug, + "Transaction execution reverted during fee estimation: {:?}", + execution_info.revert_error + ); + Err(Error::::TransactionExecutionFailed) + } + } + Err(e) => { + log!(debug, "Transaction execution failed during fee estimation: {:?}", e); + Err(Error::::TransactionExecutionFailed) + } }) .map(|exec_info_res| { exec_info_res.map(|exec_info| { @@ -102,7 +116,7 @@ impl Pallet { )?; let result = res.0.map_err(|e| { - log::error!("Transaction execution failed during simulation: {e}"); + log!(debug, "Transaction execution failed during simulation: {:?}", e); PlaceHolderErrorTypeForFailedStarknetExecution }); @@ -135,7 +149,7 @@ impl Pallet { let mut state = BlockifierStateAdapter::::default(); let tx_execution_result = Self::execute_message(&message, &mut state, &block_context).map_err(|e| { - log::error!("Transaction execution failed during simulation: {e}"); + log!(debug, "Transaction execution failed during simulation: {:?}", e); PlaceHolderErrorTypeForFailedStarknetExecution }); @@ -157,14 +171,17 @@ impl Pallet { let tx_execution_infos = match message.execute(&mut cached_state, &Self::get_block_context(), true, true) { Ok(execution_info) if !execution_info.is_reverted() => Ok(execution_info), Err(e) => { - log::error!( - "Transaction execution failed during fee estimation: {e} {:?}", + log!( + debug, + "Transaction execution failed during fee estimation: {:?} {:?}", + e, std::error::Error::source(&e) ); Err(Error::::TransactionExecutionFailed) } Ok(execution_info) => { - log::error!( + log!( + debug, "Transaction execution reverted during fee estimation: {}", // Safe due to the `match` branch order execution_info.revert_error.unwrap() @@ -205,7 +222,7 @@ impl Pallet { transactions_before.iter().try_for_each(|tx| { Self::execute_transaction(tx, &mut state, &block_context, &SimulationFlags::default()).map_err(|e| { - log::error!("Failed to reexecute a tx: {}", e); + log!(debug, "Failed to reexecute a tx: {}", e); PlaceHolderErrorTypeForFailedStarknetExecution })?; @@ -224,14 +241,14 @@ impl Pallet { &SimulationFlags::default(), ) .map_err(|e| { - log::error!("Failed to reexecute a tx: {}", e); + log!(debug, "Failed to reexecute a tx: {}", e); PlaceHolderErrorTypeForFailedStarknetExecution }); let res = res .map(|r| if with_state_diff { (r, Some(transactional_state.to_state_diff())) } else { (r, None) }); commit_transactional_state(transactional_state).map_err(|e| { - log::error!("Failed to commit state changes: {:?}", e); + log!(debug, "Failed to commit state changes: {:?}", e); PlaceHolderErrorTypeForFailedStarknetExecution })?; @@ -316,7 +333,7 @@ impl Pallet { // Once the state diff of this tx is generated, we apply those changes on the original state // so that next txs being simulated are ontop of this one (avoid nonce error) commit_transactional_state(transactional_state).map_err(|e| { - log::error!("Failed to commit state changes: {:?}", e); + log!(error, "Failed to commit state changes: {:?}", e); PlaceHolderErrorTypeForFailedStarknetExecution })?; diff --git a/crates/pallets/starknet/src/transaction_validation.rs b/crates/pallets/starknet/src/transaction_validation.rs index 60b62e19d7..acdd35619c 100644 --- a/crates/pallets/starknet/src/transaction_validation.rs +++ b/crates/pallets/starknet/src/transaction_validation.rs @@ -80,7 +80,10 @@ impl Pallet { } } // TODO: have more granular error mapping - .map_err(|_| InvalidTransaction::BadProof) + .map_err(|e| { + log!(debug, "Error in pre_validate_unsigned_tx: {:?}", e); + InvalidTransaction::BadProof + }) } Transaction::L1HandlerTransaction(transaction) => { Self::ensure_l1_message_not_executed(&transaction.tx.nonce) @@ -141,7 +144,10 @@ impl Pallet { Ok(None) } } - .map_err(|_| InvalidTransaction::BadProof)?; + .map_err(|e| { + log!(debug, "Error in validate_unsigned_tx: {:?}", e); + InvalidTransaction::BadProof + })?; Ok(()) } diff --git a/crates/primitives/simulations/src/lib.rs b/crates/primitives/simulations/src/lib.rs index e669a95be3..57773b2afe 100644 --- a/crates/primitives/simulations/src/lib.rs +++ b/crates/primitives/simulations/src/lib.rs @@ -51,6 +51,9 @@ impl From> for SimulationFlags { } } + // estimate_fee does not charge fees or do any balance checks + flags_out.charge_fee = false; + flags_out } } diff --git a/crates/primitives/transactions/Cargo.toml b/crates/primitives/transactions/Cargo.toml index 0f15be614b..b320be9003 100644 --- a/crates/primitives/transactions/Cargo.toml +++ b/crates/primitives/transactions/Cargo.toml @@ -32,6 +32,7 @@ serde_json = { workspace = true, optional = true } thiserror = { workspace = true, optional = true } # Other optional +log = "0.4.20" parity-scale-codec = { workspace = true, features = [ "derive", ], optional = true } diff --git a/crates/primitives/transactions/src/compute_hash.rs b/crates/primitives/transactions/src/compute_hash.rs index a0651c9a98..aaa549e4ad 100644 --- a/crates/primitives/transactions/src/compute_hash.rs +++ b/crates/primitives/transactions/src/compute_hash.rs @@ -7,8 +7,8 @@ use starknet_api::data_availability::DataAvailabilityMode; use starknet_api::transaction::{ Calldata, DeclareTransaction, DeclareTransactionV0V1, DeclareTransactionV2, DeclareTransactionV3, DeployAccountTransaction, DeployAccountTransactionV1, DeployAccountTransactionV3, InvokeTransaction, - InvokeTransactionV0, InvokeTransactionV1, InvokeTransactionV3, L1HandlerTransaction, Resource, - ResourceBoundsMapping, TransactionHash, + InvokeTransactionV0, InvokeTransactionV1, InvokeTransactionV3, L1HandlerTransaction, PaymasterData, Resource, + ResourceBoundsMapping, Tip, TransactionHash, }; use starknet_core::crypto::compute_hash_on_elements; use starknet_crypto::FieldElement; @@ -50,8 +50,8 @@ fn prepare_data_availability_modes( fee_data_availability_mode: DataAvailabilityMode, ) -> FieldElement { let mut buffer = [0u8; 32]; - buffer[8..12].copy_from_slice(&(nonce_data_availability_mode as u32).to_be_bytes()); - buffer[12..].copy_from_slice(&(fee_data_availability_mode as u32).to_be_bytes()); + buffer[24..28].copy_from_slice(&(nonce_data_availability_mode as u32).to_be_bytes()); + buffer[28..].copy_from_slice(&(fee_data_availability_mode as u32).to_be_bytes()); // Safe to unwrap because we left most significant bit of the buffer empty FieldElement::from_bytes_be(&buffer).unwrap() @@ -109,39 +109,27 @@ impl ComputeTransactionHash for InvokeTransactionV3 { let version = if offset_version { SIMULATE_TX_VERSION_OFFSET + FieldElement::THREE } else { FieldElement::THREE }; let sender_address = Felt252Wrapper::from(self.sender_address).into(); - let gas_hash = compute_hash_on_elements(&[ - FieldElement::from(self.tip.0), - prepare_resource_bound_value(&self.resource_bounds, Resource::L1Gas), - prepare_resource_bound_value(&self.resource_bounds, Resource::L2Gas), - ]); - let paymaster_hash = compute_hash_on_elements( - &self.paymaster_data.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), - ); let nonce = Felt252Wrapper::from(self.nonce.0).into(); - let data_availability_modes = - prepare_data_availability_modes(self.nonce_data_availability_mode, self.fee_data_availability_mode); - let data_hash = { - let account_deployment_data_hash = compute_hash_on_elements( - &self.account_deployment_data.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), - ); - let calldata_hash = compute_hash_on_elements( - &self.calldata.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), - ); - compute_hash_on_elements(&[account_deployment_data_hash, calldata_hash]) - }; + let account_deployment_data_hash = PoseidonHasher::compute_hash_on_elements( + &self.account_deployment_data.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), + ); + let calldata_hash = PoseidonHasher::compute_hash_on_elements( + &self.calldata.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), + ); - Felt252Wrapper(PoseidonHasher::compute_hash_on_elements(&[ + compute_transaction_hash_common_v3( prefix, version, sender_address, - gas_hash, - paymaster_hash, chain_id.into(), nonce, - data_availability_modes, - data_hash, - ])) - .into() + self.tip, + &self.paymaster_data, + self.nonce_data_availability_mode, + self.fee_data_availability_mode, + &self.resource_bounds, + vec![account_deployment_data_hash, calldata_hash], + ) } } @@ -223,35 +211,28 @@ impl ComputeTransactionHash for DeclareTransactionV3 { let version = if offset_version { SIMULATE_TX_VERSION_OFFSET + FieldElement::THREE } else { FieldElement::THREE }; let sender_address = Felt252Wrapper::from(self.sender_address).into(); - let gas_hash = compute_hash_on_elements(&[ - FieldElement::from(self.tip.0), - prepare_resource_bound_value(&self.resource_bounds, Resource::L1Gas), - prepare_resource_bound_value(&self.resource_bounds, Resource::L2Gas), - ]); - let paymaster_hash = compute_hash_on_elements( - &self.paymaster_data.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), - ); let nonce = Felt252Wrapper::from(self.nonce.0).into(); - let data_availability_modes = - prepare_data_availability_modes(self.nonce_data_availability_mode, self.fee_data_availability_mode); - let account_deployment_data_hash = compute_hash_on_elements( + let account_deployment_data_hash = PoseidonHasher::compute_hash_on_elements( &self.account_deployment_data.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), ); - Felt252Wrapper(PoseidonHasher::compute_hash_on_elements(&[ + compute_transaction_hash_common_v3( prefix, version, sender_address, - gas_hash, - paymaster_hash, chain_id.into(), nonce, - data_availability_modes, - account_deployment_data_hash, - Felt252Wrapper::from(self.class_hash).into(), - Felt252Wrapper::from(self.compiled_class_hash).into(), - ])) - .into() + self.tip, + &self.paymaster_data, + self.nonce_data_availability_mode, + self.fee_data_availability_mode, + &self.resource_bounds, + vec![ + account_deployment_data_hash, + Felt252Wrapper::from(self.class_hash).into(), + Felt252Wrapper::from(self.compiled_class_hash).into(), + ], + ) } } impl ComputeTransactionHash for DeclareTransaction { @@ -329,33 +310,26 @@ impl ComputeTransactionHash for DeployAccountTransactionV3 { .unwrap(), ) .into(); - let gas_hash = compute_hash_on_elements(&[ - FieldElement::from(self.tip.0), - prepare_resource_bound_value(&self.resource_bounds, Resource::L1Gas), - prepare_resource_bound_value(&self.resource_bounds, Resource::L2Gas), - ]); - let paymaster_hash = compute_hash_on_elements( - &self.paymaster_data.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), - ); let nonce = Felt252Wrapper::from(self.nonce.0).into(); - let data_availability_modes = - prepare_data_availability_modes(self.nonce_data_availability_mode, self.fee_data_availability_mode); - let constructor_calldata_hash = compute_hash_on_elements(&constructor_calldata); + let constructor_calldata_hash = PoseidonHasher::compute_hash_on_elements(&constructor_calldata); - Felt252Wrapper(PoseidonHasher::compute_hash_on_elements(&[ + compute_transaction_hash_common_v3( prefix, version, contract_address, - gas_hash, - paymaster_hash, chain_id.into(), nonce, - data_availability_modes, - constructor_calldata_hash, - Felt252Wrapper::from(self.class_hash).into(), - Felt252Wrapper::from(self.contract_address_salt).into(), - ])) - .into() + self.tip, + &self.paymaster_data, + self.nonce_data_availability_mode, + self.fee_data_availability_mode, + &self.resource_bounds, + vec![ + constructor_calldata_hash, + Felt252Wrapper::from(self.class_hash).into(), + Felt252Wrapper::from(self.contract_address_salt).into(), + ], + ) } } @@ -381,6 +355,44 @@ impl ComputeTransactionHash for L1HandlerTransaction { } } +#[allow(clippy::too_many_arguments)] +fn compute_transaction_hash_common_v3( + tx_hash_prefix: FieldElement, + version: FieldElement, + sender_address: FieldElement, + chain_id: FieldElement, + nonce: FieldElement, + tip: Tip, + paymaster_data: &PaymasterData, + nonce_data_availability_mode: DataAvailabilityMode, + fee_data_availability_mode: DataAvailabilityMode, + resource_bounds: &ResourceBoundsMapping, + additional_data: Vec, +) -> TransactionHash { + let gas_hash = PoseidonHasher::compute_hash_on_elements(&[ + FieldElement::from(tip.0), + prepare_resource_bound_value(resource_bounds, Resource::L1Gas), + prepare_resource_bound_value(resource_bounds, Resource::L2Gas), + ]); + let paymaster_hash = PoseidonHasher::compute_hash_on_elements( + &paymaster_data.0.iter().map(|f| Felt252Wrapper::from(*f).into()).collect::>(), + ); + let data_availability_modes = + prepare_data_availability_modes(nonce_data_availability_mode, fee_data_availability_mode); + let mut data_to_hash = vec![ + tx_hash_prefix, + version, + sender_address, + gas_hash, + paymaster_hash, + chain_id, + nonce, + data_availability_modes, + ]; + data_to_hash.extend(additional_data); + Felt252Wrapper(PoseidonHasher::compute_hash_on_elements(data_to_hash.as_slice())).into() +} + #[cfg(test)] #[path = "compute_hash_tests.rs"] mod compute_hash_tests; diff --git a/crates/primitives/transactions/src/execution.rs b/crates/primitives/transactions/src/execution.rs index 702fd34bb1..d1530ea738 100644 --- a/crates/primitives/transactions/src/execution.rs +++ b/crates/primitives/transactions/src/execution.rs @@ -408,7 +408,7 @@ impl< Self::handle_nonce(state, &tx_context.tx_info, strict_nonce_checking)?; // Check if user has funds to pay the worst case scenario fees - if charge_fee && tx_context.tx_info.enforce_fee()? { + if charge_fee { self.check_fee_bounds(&tx_context)?; blockifier::fee::fee_utils::verify_can_pay_committed_bounds(state, &tx_context)?;