From 8280d0b2390d87b3fe2c7c98fd5ef3a19f790008 Mon Sep 17 00:00:00 2001 From: Arni Hod Date: Thu, 15 Aug 2024 17:11:35 +0300 Subject: [PATCH 1/2] refactor: refactor the stateful tx validator to get executable tx as input --- crates/gateway/src/config.rs | 2 + crates/gateway/src/gateway.rs | 13 +- crates/gateway/src/gateway_test.rs | 141 +++++++++++++++++- .../src/stateful_transaction_validator.rs | 31 ++-- .../stateful_transaction_validator_test.rs | 50 ++++--- crates/gateway/src/utils.rs | 126 +--------------- .../src/starknet_api_test_utils.rs | 23 +++ 7 files changed, 217 insertions(+), 169 deletions(-) diff --git a/crates/gateway/src/config.rs b/crates/gateway/src/config.rs index 4fcb38ce68..d2fb8267d5 100644 --- a/crates/gateway/src/config.rs +++ b/crates/gateway/src/config.rs @@ -139,6 +139,8 @@ pub struct StatefulTransactionValidatorConfig { pub max_nonce_for_validation_skip: Nonce, pub validate_max_n_steps: u32, pub max_recursion_depth: usize, + // TODO(Arni): Move this member out of the stateful transaction validator config. Move it into + // the gateway config. This is used during the transalation from external_tx to executable_tx. pub chain_info: ChainInfo, } diff --git a/crates/gateway/src/gateway.rs b/crates/gateway/src/gateway.rs index 2a1beca567..42302962f0 100644 --- a/crates/gateway/src/gateway.rs +++ b/crates/gateway/src/gateway.rs @@ -128,21 +128,10 @@ fn process_tx( } } - let optional_class_info = match executable_tx { - starknet_api::executable_transaction::Transaction::Declare(tx) => { - Some(tx.class_info.try_into().map_err(|e| { - error!("Failed to convert Starknet API ClassInfo to Blockifier ClassInfo: {:?}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error.".to_owned() } - })?) - } - _ => None, - }; - let validator = stateful_tx_validator.instantiate_validator(state_reader_factory)?; // TODO(Yael 31/7/24): refactor after IntrnalTransaction is ready, delete validate_info and // compute all the info outside of run_validate. - let validate_info = - stateful_tx_validator.run_validate(©_of_rpc_tx, optional_class_info, validator)?; + let validate_info = stateful_tx_validator.run_validate(&executable_tx, validator)?; // TODO(Arni): Add the Sierra and the Casm to the mempool input. Ok(MempoolInput { diff --git a/crates/gateway/src/gateway_test.rs b/crates/gateway/src/gateway_test.rs index 83a0838c03..657f7b4e69 100644 --- a/crates/gateway/src/gateway_test.rs +++ b/crates/gateway/src/gateway_test.rs @@ -2,24 +2,52 @@ use std::sync::Arc; use assert_matches::assert_matches; use blockifier::context::ChainInfo; +use blockifier::execution::contract_class::ClassInfo; use blockifier::test_utils::CairoVersion; +use blockifier::transaction::account_transaction::AccountTransaction; +use blockifier::transaction::transactions::{ + DeclareTransaction as BlockifierDeclareTransaction, + DeployAccountTransaction as BlockifierDeployAccountTransaction, + InvokeTransaction as BlockifierInvokeTransaction, +}; use mempool_test_utils::starknet_api_test_utils::{create_executable_tx, declare_tx, invoke_tx}; use mockall::predicate::eq; -use starknet_api::core::{CompiledClassHash, ContractAddress}; -use starknet_api::rpc_transaction::{RpcDeclareTransaction, RpcTransaction}; -use starknet_api::transaction::{TransactionHash, ValidResourceBounds}; +use starknet_api::core::{ + calculate_contract_address, + ChainId, + ClassHash, + CompiledClassHash, + ContractAddress, +}; +use starknet_api::rpc_transaction::{ + RpcDeclareTransaction, + RpcDeployAccountTransaction, + RpcInvokeTransaction, + RpcTransaction, +}; +use starknet_api::transaction::{ + DeclareTransaction, + DeclareTransactionV3, + DeployAccountTransaction, + DeployAccountTransactionV3, + InvokeTransactionV3, + TransactionHash, + TransactionHasher, + ValidResourceBounds, +}; use starknet_gateway_types::errors::GatewaySpecError; use starknet_mempool_types::communication::{MempoolWrapperInput, MockMempoolClient}; use starknet_mempool_types::mempool_types::{Account, AccountState, MempoolInput}; use starknet_sierra_compile::config::SierraToCasmCompilationConfig; +use tracing::error; use crate::compilation::GatewayCompiler; use crate::config::{StatefulTransactionValidatorConfig, StatelessTransactionValidatorConfig}; +use crate::errors::StatefulTransactionValidatorResult; use crate::gateway::{internal_add_tx, AppState, SharedMempoolClient}; use crate::state_reader_test_utils::{local_test_state_reader_factory, TestStateReaderFactory}; use crate::stateful_transaction_validator::StatefulTransactionValidator; use crate::stateless_transaction_validator::StatelessTransactionValidator; -use crate::utils::rpc_tx_to_account_tx; pub fn app_state( mempool_client: SharedMempoolClient, @@ -122,3 +150,108 @@ fn calculate_hash(rpc_tx: &RpcTransaction) -> TransactionHash { .unwrap(); account_tx.tx_hash() } + +// TODO(Arni): Remove this function. Replace with a function that take ownership of RpcTransaction. +fn rpc_tx_to_account_tx( + rpc_tx: &RpcTransaction, + // FIXME(yael 15/4/24): calculate class_info inside the function once compilation code is ready + optional_class_info: Option, + chain_id: &ChainId, +) -> StatefulTransactionValidatorResult { + match rpc_tx { + RpcTransaction::Declare(RpcDeclareTransaction::V3(tx)) => { + let declare_tx = DeclareTransaction::V3(DeclareTransactionV3 { + class_hash: ClassHash::default(), /* FIXME(yael 15/4/24): call the starknet-api + * function once ready */ + resource_bounds: ValidResourceBounds::AllResources( + rpc_tx.resource_bounds().clone(), + ), + tip: tx.tip, + signature: tx.signature.clone(), + nonce: tx.nonce, + compiled_class_hash: tx.compiled_class_hash, + sender_address: tx.sender_address, + nonce_data_availability_mode: tx.nonce_data_availability_mode, + fee_data_availability_mode: tx.fee_data_availability_mode, + paymaster_data: tx.paymaster_data.clone(), + account_deployment_data: tx.account_deployment_data.clone(), + }); + let tx_hash = declare_tx + .calculate_transaction_hash(chain_id, &declare_tx.version()) + .map_err(|e| { + error!("Failed to calculate tx hash: {}", e); + GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } + })?; + let class_info = + optional_class_info.expect("declare transaction should contain class info"); + let declare_tx = BlockifierDeclareTransaction::new(declare_tx, tx_hash, class_info) + .map_err(|e| { + error!("Failed to convert declare tx hash to blockifier tx type: {}", e); + GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } + })?; + Ok(AccountTransaction::Declare(declare_tx)) + } + RpcTransaction::DeployAccount(RpcDeployAccountTransaction::V3(tx)) => { + let deploy_account_tx = DeployAccountTransaction::V3(DeployAccountTransactionV3 { + resource_bounds: ValidResourceBounds::AllResources( + rpc_tx.resource_bounds().clone(), + ), + tip: tx.tip, + signature: tx.signature.clone(), + nonce: tx.nonce, + class_hash: tx.class_hash, + contract_address_salt: tx.contract_address_salt, + constructor_calldata: tx.constructor_calldata.clone(), + nonce_data_availability_mode: tx.nonce_data_availability_mode, + fee_data_availability_mode: tx.fee_data_availability_mode, + paymaster_data: tx.paymaster_data.clone(), + }); + let contract_address = calculate_contract_address( + deploy_account_tx.contract_address_salt(), + deploy_account_tx.class_hash(), + &deploy_account_tx.constructor_calldata(), + ContractAddress::default(), + ) + .map_err(|e| { + error!("Failed to calculate contract address: {}", e); + GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } + })?; + let tx_hash = deploy_account_tx + .calculate_transaction_hash(chain_id, &deploy_account_tx.version()) + .map_err(|e| { + error!("Failed to calculate tx hash: {}", e); + GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } + })?; + let deploy_account_tx = BlockifierDeployAccountTransaction::new( + deploy_account_tx, + tx_hash, + contract_address, + ); + Ok(AccountTransaction::DeployAccount(deploy_account_tx)) + } + RpcTransaction::Invoke(RpcInvokeTransaction::V3(tx)) => { + let invoke_tx = starknet_api::transaction::InvokeTransaction::V3(InvokeTransactionV3 { + resource_bounds: ValidResourceBounds::AllResources( + rpc_tx.resource_bounds().clone(), + ), + tip: tx.tip, + signature: tx.signature.clone(), + nonce: tx.nonce, + sender_address: tx.sender_address, + calldata: tx.calldata.clone(), + nonce_data_availability_mode: tx.nonce_data_availability_mode, + fee_data_availability_mode: tx.fee_data_availability_mode, + paymaster_data: tx.paymaster_data.clone(), + account_deployment_data: tx.account_deployment_data.clone(), + }); + let tx_hash = invoke_tx + .calculate_transaction_hash(chain_id, &invoke_tx.version()) + .map_err(|e| { + error!("Failed to calculate tx hash: {}", e); + GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } + })?; + let invoke_tx = BlockifierInvokeTransaction::new(invoke_tx, tx_hash); + Ok(AccountTransaction::Invoke(invoke_tx)) + } + } +} diff --git a/crates/gateway/src/stateful_transaction_validator.rs b/crates/gateway/src/stateful_transaction_validator.rs index c59be1065c..8e2fd18f63 100644 --- a/crates/gateway/src/stateful_transaction_validator.rs +++ b/crates/gateway/src/stateful_transaction_validator.rs @@ -5,14 +5,16 @@ use blockifier::blockifier::stateful_validator::{ }; use blockifier::bouncer::BouncerConfig; use blockifier::context::BlockContext; -use blockifier::execution::contract_class::ClassInfo; use blockifier::state::cached_state::CachedState; use blockifier::transaction::account_transaction::AccountTransaction; use blockifier::versioned_constants::VersionedConstants; #[cfg(test)] use mockall::automock; use starknet_api::core::{ContractAddress, Nonce}; -use starknet_api::rpc_transaction::{RpcInvokeTransaction, RpcTransaction}; +use starknet_api::executable_transaction::{ + InvokeTransaction as ExecutableInvokeTransaction, + Transaction as ExecutableTransaction, +}; use starknet_api::transaction::TransactionHash; use starknet_gateway_types::errors::GatewaySpecError; use starknet_types_core::felt::Felt; @@ -21,7 +23,7 @@ use tracing::error; use crate::config::StatefulTransactionValidatorConfig; use crate::errors::StatefulTransactionValidatorResult; use crate::state_reader::{MempoolStateReader, StateReaderFactory}; -use crate::utils::{get_sender_address, rpc_tx_to_account_tx}; +use crate::utils::get_sender_address; #[cfg(test)] #[path = "stateful_transaction_validator_test.rs"] @@ -70,19 +72,24 @@ impl StatefulTransactionValidator { // conversion is also relevant for the Mempool. pub fn run_validate( &self, - rpc_tx: &RpcTransaction, - optional_class_info: Option, + executable_tx: &ExecutableTransaction, mut validator: V, ) -> StatefulTransactionValidatorResult { - let account_tx = - rpc_tx_to_account_tx(rpc_tx, optional_class_info, &self.config.chain_info.chain_id)?; + let account_tx = AccountTransaction::try_from( + // TODO(Arni): create a try_from for &ExecutableTransaction. + executable_tx.clone(), + ) + .map_err(|error| { + error!("Failed to convert executable transaction into account transaction: {}", error); + GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } + })?; let tx_hash = account_tx.tx_hash(); let sender_address = get_sender_address(&account_tx); let account_nonce = validator.get_nonce(sender_address).map_err(|e| { error!("Failed to get nonce for sender address {}: {}", sender_address, e); GatewaySpecError::UnexpectedError { data: "Internal server error.".to_owned() } })?; - let skip_validate = skip_stateful_validations(rpc_tx, account_nonce); + let skip_validate = skip_stateful_validations(executable_tx, account_nonce); validator .validate(account_tx, skip_validate) .map_err(|err| GatewaySpecError::ValidationFailure { data: err.to_string() })?; @@ -120,15 +127,15 @@ impl StatefulTransactionValidator { // Check if validation of an invoke transaction should be skipped due to deploy_account not being // proccessed yet. This feature is used to improve UX for users sending deploy_account + invoke at // once. -fn skip_stateful_validations(tx: &RpcTransaction, account_nonce: Nonce) -> bool { +fn skip_stateful_validations(tx: &ExecutableTransaction, account_nonce: Nonce) -> bool { match tx { - RpcTransaction::Invoke(RpcInvokeTransaction::V3(tx)) => { + ExecutableTransaction::Invoke(ExecutableInvokeTransaction { tx, .. }) => { // check if the transaction nonce is 1, meaning it is post deploy_account, and the // account nonce is zero, meaning the account was not deployed yet. The mempool also // verifies that the deploy_account transaction exists. - tx.nonce == Nonce(Felt::ONE) && account_nonce == Nonce(Felt::ZERO) + tx.nonce() == Nonce(Felt::ONE) && account_nonce == Nonce(Felt::ZERO) } - RpcTransaction::DeployAccount(_) | RpcTransaction::Declare(_) => false, + ExecutableTransaction::DeployAccount(_) | ExecutableTransaction::Declare(_) => false, } } diff --git a/crates/gateway/src/stateful_transaction_validator_test.rs b/crates/gateway/src/stateful_transaction_validator_test.rs index b664c84622..fbe493bd9f 100644 --- a/crates/gateway/src/stateful_transaction_validator_test.rs +++ b/crates/gateway/src/stateful_transaction_validator_test.rs @@ -5,11 +5,8 @@ use blockifier::blockifier::stateful_validator::{ use blockifier::context::BlockContext; use blockifier::test_utils::CairoVersion; use blockifier::transaction::errors::{TransactionFeeError, TransactionPreValidationError}; -use mempool_test_utils::invoke_tx_args; use mempool_test_utils::starknet_api_test_utils::{ - deploy_account_tx, - invoke_tx, - rpc_invoke_tx, + executable_invoke_tx, TEST_SENDER_ADDRESS, VALID_L1_GAS_MAX_AMOUNT, VALID_L1_GAS_MAX_PRICE_PER_UNIT, @@ -19,7 +16,7 @@ use num_bigint::BigUint; use pretty_assertions::assert_eq; use rstest::{fixture, rstest}; use starknet_api::core::{ContractAddress, Nonce, PatriciaKey}; -use starknet_api::rpc_transaction::RpcTransaction; +use starknet_api::executable_transaction::Transaction; use starknet_api::transaction::{Resource, TransactionHash}; use starknet_api::{contract_address, felt, patricia_key}; use starknet_gateway_types::errors::GatewaySpecError; @@ -66,18 +63,16 @@ fn stateful_validator(block_context: BlockContext) -> StatefulTransactionValidat // TODO(Arni): consider testing declare and deploy account. #[rstest] #[case::valid_tx( - invoke_tx(CairoVersion::Cairo1), + executable_invoke_tx(CairoVersion::Cairo1), Ok(ValidateInfo{ - tx_hash: TransactionHash(felt!( - "0x3c6546364f70395ad7c8d1ce64b990b698be21e786bee2efff57a694f37648e" - )), + tx_hash: TransactionHash::default(), sender_address: contract_address!("0xc0020000"), account_nonce: Nonce::default() }) )] -#[case::invalid_tx(invoke_tx(CairoVersion::Cairo1), Err(STATEFUL_VALIDATOR_FEE_ERROR))] +#[case::invalid_tx(executable_invoke_tx(CairoVersion::Cairo1), Err(STATEFUL_VALIDATOR_FEE_ERROR))] fn test_stateful_tx_validator( - #[case] rpc_tx: RpcTransaction, + #[case] executable_tx: Transaction, #[case] expected_result: BlockifierStatefulValidatorResult, stateful_validator: StatefulTransactionValidator, ) { @@ -90,7 +85,7 @@ fn test_stateful_tx_validator( mock_validator.expect_validate().return_once(|_, _| expected_result.map(|_| ())); mock_validator.expect_get_nonce().returning(|_| Ok(Nonce(Felt::ZERO))); - let result = stateful_validator.run_validate(&rpc_tx, None, mock_validator); + let result = stateful_validator.run_validate(&executable_tx, mock_validator); assert_eq!(result, expected_result_as_stateful_transaction_result); } @@ -130,28 +125,45 @@ fn test_instantiate_validator() { #[rstest] #[case::should_skip_validation( - rpc_invoke_tx(invoke_tx_args!{nonce: Nonce(Felt::ONE)}), + Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx( + starknet_api::invoke_tx_args!(nonce: Nonce(Felt::ONE)) + )), Nonce::default(), true )] #[case::should_not_skip_validation_nonce_over_max_nonce_for_skip( - rpc_invoke_tx(invoke_tx_args!{nonce: Nonce(Felt::TWO)}), + Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx( + starknet_api::invoke_tx_args!(nonce: Nonce(Felt::ZERO)) + )), Nonce::default(), false )] -#[case::should_not_skip_validation_non_invoke(deploy_account_tx(), Nonce::default(), false)] +#[case::should_not_skip_validation_non_invoke( + Transaction::DeployAccount( + starknet_api::test_utils::deploy_account::executable_deploy_account_tx( + starknet_api::deploy_account_tx_args!(), Nonce::default() + ) + ), + Nonce::default(), + false)] #[case::should_not_skip_validation_account_nonce_1( - rpc_invoke_tx(invoke_tx_args!{sender_address: ContractAddress::from(TEST_SENDER_ADDRESS), nonce: Nonce(Felt::ONE)}), + Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx( + starknet_api::invoke_tx_args!( + nonce: Nonce(Felt::ONE), + sender_address: TEST_SENDER_ADDRESS.into() + ) + )), Nonce(Felt::ONE), false )] fn test_skip_stateful_validation( - #[case] rpc_tx: RpcTransaction, + #[case] executable_tx: Transaction, #[case] sender_nonce: Nonce, #[case] should_skip_validate: bool, stateful_validator: StatefulTransactionValidator, ) { - let sender_address = rpc_tx.calculate_sender_address().unwrap(); + let sender_address = executable_tx.contract_address(); + let mut mock_validator = MockStatefulTransactionValidatorTrait::new(); mock_validator .expect_get_nonce() @@ -161,5 +173,5 @@ fn test_skip_stateful_validation( .expect_validate() .withf(move |_, skip_validate| *skip_validate == should_skip_validate) .returning(|_, _| Ok(())); - let _ = stateful_validator.run_validate(&rpc_tx, None, mock_validator); + let _ = stateful_validator.run_validate(&executable_tx, mock_validator); } diff --git a/crates/gateway/src/utils.rs b/crates/gateway/src/utils.rs index d7b23b9d18..a96ac69a8a 100644 --- a/crates/gateway/src/utils.rs +++ b/crates/gateway/src/utils.rs @@ -1,37 +1,18 @@ -use blockifier::execution::contract_class::ClassInfo; use blockifier::transaction::account_transaction::AccountTransaction; -use blockifier::transaction::transactions::{ - DeclareTransaction as BlockifierDeclareTransaction, - DeployAccountTransaction as BlockifierDeployAccountTransaction, - InvokeTransaction as BlockifierInvokeTransaction, -}; -use starknet_api::core::{calculate_contract_address, ChainId, ClassHash, ContractAddress}; +use starknet_api::core::{ChainId, ContractAddress}; use starknet_api::executable_transaction::{ DeclareTransaction as ExecutableDeclareTransaction, DeployAccountTransaction as ExecutableDeployAccountTransaction, InvokeTransaction as ExecutableInvokeTransaction, Transaction as ExecutableTransaction, }; -use starknet_api::rpc_transaction::{ - RpcDeclareTransaction, - RpcDeployAccountTransaction, - RpcInvokeTransaction, - RpcTransaction, -}; -use starknet_api::transaction::{ - DeclareTransaction, - DeclareTransactionV3, - DeployAccountTransaction, - DeployAccountTransactionV3, - InvokeTransactionV3, - TransactionHasher, - ValidResourceBounds, -}; +use starknet_api::rpc_transaction::{RpcDeclareTransaction, RpcTransaction}; +use starknet_api::transaction::DeclareTransaction; use starknet_gateway_types::errors::GatewaySpecError; use tracing::{debug, error}; use crate::compilation::GatewayCompiler; -use crate::errors::{GatewayResult, StatefulTransactionValidatorResult}; +use crate::errors::GatewayResult; /// Converts an RPC transaction to an executable transaction. /// Note, for declare transaction this step is heavy, as it requires compilation of Sierra to @@ -98,105 +79,6 @@ fn compile_contract_and_build_executable_declare_tx( Ok(executable_declare_tx) } -// TODO(Arni): Remove this function. -pub fn rpc_tx_to_account_tx( - rpc_tx: &RpcTransaction, - // FIXME(yael 15/4/24): calculate class_info inside the function once compilation code is ready - optional_class_info: Option, - chain_id: &ChainId, -) -> StatefulTransactionValidatorResult { - match rpc_tx { - RpcTransaction::Declare(RpcDeclareTransaction::V3(tx)) => { - let declare_tx = DeclareTransaction::V3(DeclareTransactionV3 { - class_hash: ClassHash::default(), /* FIXME(yael 15/4/24): call the starknet-api - * function once ready */ - resource_bounds: ValidResourceBounds::AllResources(tx.resource_bounds), - tip: tx.tip, - signature: tx.signature.clone(), - nonce: tx.nonce, - compiled_class_hash: tx.compiled_class_hash, - sender_address: tx.sender_address, - nonce_data_availability_mode: tx.nonce_data_availability_mode, - fee_data_availability_mode: tx.fee_data_availability_mode, - paymaster_data: tx.paymaster_data.clone(), - account_deployment_data: tx.account_deployment_data.clone(), - }); - let tx_hash = declare_tx - .calculate_transaction_hash(chain_id, &declare_tx.version()) - .map_err(|e| { - error!("Failed to calculate tx hash: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - let class_info = - optional_class_info.expect("declare transaction should contain class info"); - let declare_tx = BlockifierDeclareTransaction::new(declare_tx, tx_hash, class_info) - .map_err(|e| { - error!("Failed to convert declare tx hash to blockifier tx type: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - Ok(AccountTransaction::Declare(declare_tx)) - } - RpcTransaction::DeployAccount(RpcDeployAccountTransaction::V3(tx)) => { - let deploy_account_tx = DeployAccountTransaction::V3(DeployAccountTransactionV3 { - resource_bounds: ValidResourceBounds::AllResources(tx.resource_bounds), - tip: tx.tip, - signature: tx.signature.clone(), - nonce: tx.nonce, - class_hash: tx.class_hash, - contract_address_salt: tx.contract_address_salt, - constructor_calldata: tx.constructor_calldata.clone(), - nonce_data_availability_mode: tx.nonce_data_availability_mode, - fee_data_availability_mode: tx.fee_data_availability_mode, - paymaster_data: tx.paymaster_data.clone(), - }); - let contract_address = calculate_contract_address( - deploy_account_tx.contract_address_salt(), - deploy_account_tx.class_hash(), - &deploy_account_tx.constructor_calldata(), - ContractAddress::default(), - ) - .map_err(|e| { - error!("Failed to calculate contract address: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - let tx_hash = deploy_account_tx - .calculate_transaction_hash(chain_id, &deploy_account_tx.version()) - .map_err(|e| { - error!("Failed to calculate tx hash: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - let deploy_account_tx = BlockifierDeployAccountTransaction::new( - deploy_account_tx, - tx_hash, - contract_address, - ); - Ok(AccountTransaction::DeployAccount(deploy_account_tx)) - } - RpcTransaction::Invoke(RpcInvokeTransaction::V3(tx)) => { - let invoke_tx = starknet_api::transaction::InvokeTransaction::V3(InvokeTransactionV3 { - resource_bounds: ValidResourceBounds::AllResources(tx.resource_bounds), - tip: tx.tip, - signature: tx.signature.clone(), - nonce: tx.nonce, - sender_address: tx.sender_address, - calldata: tx.calldata.clone(), - nonce_data_availability_mode: tx.nonce_data_availability_mode, - fee_data_availability_mode: tx.fee_data_availability_mode, - paymaster_data: tx.paymaster_data.clone(), - account_deployment_data: tx.account_deployment_data.clone(), - }); - let tx_hash = invoke_tx - .calculate_transaction_hash(chain_id, &invoke_tx.version()) - .map_err(|e| { - error!("Failed to calculate tx hash: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - let invoke_tx = BlockifierInvokeTransaction::new(invoke_tx, tx_hash); - Ok(AccountTransaction::Invoke(invoke_tx)) - } - } -} - // TODO(yael 9/5/54): Should be implemented as part of InternalTransaction in starknet-api pub fn get_sender_address(tx: &AccountTransaction) -> ContractAddress { match tx { diff --git a/crates/mempool_test_utils/src/starknet_api_test_utils.rs b/crates/mempool_test_utils/src/starknet_api_test_utils.rs index 608d3525c6..eef992fbaa 100644 --- a/crates/mempool_test_utils/src/starknet_api_test_utils.rs +++ b/crates/mempool_test_utils/src/starknet_api_test_utils.rs @@ -133,6 +133,10 @@ pub fn test_resource_bounds_mapping() -> AllResourceBounds { ) } +pub fn test_valid_resource_bounds() -> ValidResourceBounds { + ValidResourceBounds::AllResources(test_resource_bounds_mapping()) +} + /// Get the contract class used for testing. pub fn contract_class() -> ContractClass { env::set_current_dir(get_absolute_path(TEST_FILES_FOLDER)).expect("Couldn't set working dir."); @@ -176,6 +180,14 @@ pub fn invoke_tx(cairo_version: CairoVersion) -> RpcTransaction { .generate_default_invoke() } +pub fn executable_invoke_tx(cairo_version: CairoVersion) -> Transaction { + let default_account = FeatureContract::AccountWithoutValidations(cairo_version); + + MultiAccountTransactionGenerator::new_for_account_contracts([default_account]) + .account_with_id(0) + .generate_default_executable_invoke() +} + // TODO(Yael 18/6/2024): Get a final decision from product whether to support Cairo0. pub fn deploy_account_tx() -> RpcTransaction { let default_account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo1); @@ -284,6 +296,17 @@ impl AccountTransactionGenerator { rpc_invoke_tx(invoke_args) } + pub fn generate_default_executable_invoke(&mut self) -> Transaction { + let invoke_args = starknet_api::invoke_tx_args!( + sender_address: self.sender_address(), + resource_bounds: test_valid_resource_bounds(), + nonce: self.next_nonce(), + calldata: create_trivial_calldata(self.test_contract_address()), + ); + + Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx(invoke_args)) + } + pub fn generate_default_deploy_account(&mut self) -> RpcTransaction { let nonce = self.next_nonce(); assert_eq!(nonce, Nonce(Felt::ZERO)); From ae9880e9c9319b00de79711499c8c635bb71f917 Mon Sep 17 00:00:00 2001 From: Arni Hod Date: Mon, 9 Sep 2024 14:31:24 +0300 Subject: [PATCH 2/2] refactor: use the proper executable tx as mempool input in gateway --- crates/gateway/src/gateway.rs | 8 +- crates/gateway/src/gateway_test.rs | 184 ++---------------- .../src/stateful_transaction_validator.rs | 7 +- .../stateful_transaction_validator_test.rs | 29 ++- crates/gateway/src/utils.rs | 19 +- 5 files changed, 35 insertions(+), 212 deletions(-) diff --git a/crates/gateway/src/gateway.rs b/crates/gateway/src/gateway.rs index 42302962f0..38b8a54d72 100644 --- a/crates/gateway/src/gateway.rs +++ b/crates/gateway/src/gateway.rs @@ -113,8 +113,6 @@ fn process_tx( // Perform stateless validations. stateless_tx_validator.validate(&tx)?; - // TODO(Arni): remove copy_of_rpc_tx and use executable_tx directly as the mempool input. - let copy_of_rpc_tx = tx.clone(); let executable_tx = compile_contract_and_build_executable_tx( tx, &gateway_compiler, @@ -135,11 +133,7 @@ fn process_tx( // TODO(Arni): Add the Sierra and the Casm to the mempool input. Ok(MempoolInput { - tx: Transaction::new_from_rpc_tx( - copy_of_rpc_tx, - validate_info.tx_hash, - validate_info.sender_address, - ), + tx: executable_tx, account: Account { sender_address: validate_info.sender_address, state: AccountState { nonce: validate_info.account_nonce }, diff --git a/crates/gateway/src/gateway_test.rs b/crates/gateway/src/gateway_test.rs index 657f7b4e69..5228b48294 100644 --- a/crates/gateway/src/gateway_test.rs +++ b/crates/gateway/src/gateway_test.rs @@ -1,49 +1,19 @@ use std::sync::Arc; use assert_matches::assert_matches; -use blockifier::context::ChainInfo; -use blockifier::execution::contract_class::ClassInfo; use blockifier::test_utils::CairoVersion; -use blockifier::transaction::account_transaction::AccountTransaction; -use blockifier::transaction::transactions::{ - DeclareTransaction as BlockifierDeclareTransaction, - DeployAccountTransaction as BlockifierDeployAccountTransaction, - InvokeTransaction as BlockifierInvokeTransaction, -}; -use mempool_test_utils::starknet_api_test_utils::{create_executable_tx, declare_tx, invoke_tx}; +use mempool_test_utils::starknet_api_test_utils::{declare_tx, invoke_tx}; use mockall::predicate::eq; -use starknet_api::core::{ - calculate_contract_address, - ChainId, - ClassHash, - CompiledClassHash, - ContractAddress, -}; -use starknet_api::rpc_transaction::{ - RpcDeclareTransaction, - RpcDeployAccountTransaction, - RpcInvokeTransaction, - RpcTransaction, -}; -use starknet_api::transaction::{ - DeclareTransaction, - DeclareTransactionV3, - DeployAccountTransaction, - DeployAccountTransactionV3, - InvokeTransactionV3, - TransactionHash, - TransactionHasher, - ValidResourceBounds, -}; +use starknet_api::core::{ChainId, CompiledClassHash, ContractAddress}; +use starknet_api::executable_transaction::{InvokeTransaction, Transaction}; +use starknet_api::rpc_transaction::{RpcDeclareTransaction, RpcTransaction}; use starknet_gateway_types::errors::GatewaySpecError; use starknet_mempool_types::communication::{MempoolWrapperInput, MockMempoolClient}; use starknet_mempool_types::mempool_types::{Account, AccountState, MempoolInput}; use starknet_sierra_compile::config::SierraToCasmCompilationConfig; -use tracing::error; use crate::compilation::GatewayCompiler; use crate::config::{StatefulTransactionValidatorConfig, StatelessTransactionValidatorConfig}; -use crate::errors::StatefulTransactionValidatorResult; use crate::gateway::{internal_add_tx, AppState, SharedMempoolClient}; use crate::state_reader_test_utils::{local_test_state_reader_factory, TestStateReaderFactory}; use crate::stateful_transaction_validator::StatefulTransactionValidator; @@ -84,25 +54,23 @@ fn create_tx() -> (RpcTransaction, SenderAddress) { // TODO: add test with Some broadcasted message metadata #[tokio::test] async fn test_add_tx() { - let (tx, sender_address) = create_tx(); - let tx_hash = calculate_hash(&tx); + let (rpc_tx, sender_address) = create_tx(); + let rpc_invoke_tx = + assert_matches!(rpc_tx.clone(), RpcTransaction::Invoke(rpc_invoke_tx) => rpc_invoke_tx); + let executable_tx = Transaction::Invoke( + InvokeTransaction::from_rpc_tx(rpc_invoke_tx, &ChainId::create_for_testing()).unwrap(), + ); + + let tx_hash = executable_tx.tx_hash(); let mut mock_mempool_client = MockMempoolClient::new(); mock_mempool_client .expect_add_tx() .once() .with(eq(MempoolWrapperInput { - // TODO(Arni): Use external_to_executable_tx instead of `create_executable_tx`. Consider - // creating a `convertor for testing` that does not do the compilation. mempool_input: MempoolInput { - tx: create_executable_tx( - sender_address, - tx_hash, - *tx.tip(), - *tx.nonce(), - ValidResourceBounds::AllResources(*tx.resource_bounds()), - ), - account: Account { sender_address, state: AccountState { nonce: *tx.nonce() } }, + tx: executable_tx, + account: Account { sender_address, state: AccountState { nonce: *rpc_tx.nonce() } }, }, message_metadata: None, })) @@ -110,7 +78,7 @@ async fn test_add_tx() { let state_reader_factory = local_test_state_reader_factory(CairoVersion::Cairo1, false); let app_state = app_state(Arc::new(mock_mempool_client), state_reader_factory); - let response_tx_hash = internal_add_tx(app_state, tx).await.unwrap(); + let response_tx_hash = internal_add_tx(app_state, rpc_tx).await.unwrap(); assert_eq!(tx_hash, response_tx_hash); } @@ -133,125 +101,3 @@ async fn test_compiled_class_hash_mismatch() { let err = internal_add_tx(app_state, tx).await.unwrap_err(); assert_matches!(err, GatewaySpecError::CompiledClassHashMismatch); } - -fn calculate_hash(rpc_tx: &RpcTransaction) -> TransactionHash { - let optional_class_info = match &rpc_tx { - RpcTransaction::Declare(_declare_tx) => { - panic!("Declare transactions are not supported in this test") - } - _ => None, - }; - - let account_tx = rpc_tx_to_account_tx( - rpc_tx, - optional_class_info, - &ChainInfo::create_for_testing().chain_id, - ) - .unwrap(); - account_tx.tx_hash() -} - -// TODO(Arni): Remove this function. Replace with a function that take ownership of RpcTransaction. -fn rpc_tx_to_account_tx( - rpc_tx: &RpcTransaction, - // FIXME(yael 15/4/24): calculate class_info inside the function once compilation code is ready - optional_class_info: Option, - chain_id: &ChainId, -) -> StatefulTransactionValidatorResult { - match rpc_tx { - RpcTransaction::Declare(RpcDeclareTransaction::V3(tx)) => { - let declare_tx = DeclareTransaction::V3(DeclareTransactionV3 { - class_hash: ClassHash::default(), /* FIXME(yael 15/4/24): call the starknet-api - * function once ready */ - resource_bounds: ValidResourceBounds::AllResources( - rpc_tx.resource_bounds().clone(), - ), - tip: tx.tip, - signature: tx.signature.clone(), - nonce: tx.nonce, - compiled_class_hash: tx.compiled_class_hash, - sender_address: tx.sender_address, - nonce_data_availability_mode: tx.nonce_data_availability_mode, - fee_data_availability_mode: tx.fee_data_availability_mode, - paymaster_data: tx.paymaster_data.clone(), - account_deployment_data: tx.account_deployment_data.clone(), - }); - let tx_hash = declare_tx - .calculate_transaction_hash(chain_id, &declare_tx.version()) - .map_err(|e| { - error!("Failed to calculate tx hash: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - let class_info = - optional_class_info.expect("declare transaction should contain class info"); - let declare_tx = BlockifierDeclareTransaction::new(declare_tx, tx_hash, class_info) - .map_err(|e| { - error!("Failed to convert declare tx hash to blockifier tx type: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - Ok(AccountTransaction::Declare(declare_tx)) - } - RpcTransaction::DeployAccount(RpcDeployAccountTransaction::V3(tx)) => { - let deploy_account_tx = DeployAccountTransaction::V3(DeployAccountTransactionV3 { - resource_bounds: ValidResourceBounds::AllResources( - rpc_tx.resource_bounds().clone(), - ), - tip: tx.tip, - signature: tx.signature.clone(), - nonce: tx.nonce, - class_hash: tx.class_hash, - contract_address_salt: tx.contract_address_salt, - constructor_calldata: tx.constructor_calldata.clone(), - nonce_data_availability_mode: tx.nonce_data_availability_mode, - fee_data_availability_mode: tx.fee_data_availability_mode, - paymaster_data: tx.paymaster_data.clone(), - }); - let contract_address = calculate_contract_address( - deploy_account_tx.contract_address_salt(), - deploy_account_tx.class_hash(), - &deploy_account_tx.constructor_calldata(), - ContractAddress::default(), - ) - .map_err(|e| { - error!("Failed to calculate contract address: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - let tx_hash = deploy_account_tx - .calculate_transaction_hash(chain_id, &deploy_account_tx.version()) - .map_err(|e| { - error!("Failed to calculate tx hash: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - let deploy_account_tx = BlockifierDeployAccountTransaction::new( - deploy_account_tx, - tx_hash, - contract_address, - ); - Ok(AccountTransaction::DeployAccount(deploy_account_tx)) - } - RpcTransaction::Invoke(RpcInvokeTransaction::V3(tx)) => { - let invoke_tx = starknet_api::transaction::InvokeTransaction::V3(InvokeTransactionV3 { - resource_bounds: ValidResourceBounds::AllResources( - rpc_tx.resource_bounds().clone(), - ), - tip: tx.tip, - signature: tx.signature.clone(), - nonce: tx.nonce, - sender_address: tx.sender_address, - calldata: tx.calldata.clone(), - nonce_data_availability_mode: tx.nonce_data_availability_mode, - fee_data_availability_mode: tx.fee_data_availability_mode, - paymaster_data: tx.paymaster_data.clone(), - account_deployment_data: tx.account_deployment_data.clone(), - }); - let tx_hash = invoke_tx - .calculate_transaction_hash(chain_id, &invoke_tx.version()) - .map_err(|e| { - error!("Failed to calculate tx hash: {}", e); - GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } - })?; - let invoke_tx = BlockifierInvokeTransaction::new(invoke_tx, tx_hash); - Ok(AccountTransaction::Invoke(invoke_tx)) - } - } -} diff --git a/crates/gateway/src/stateful_transaction_validator.rs b/crates/gateway/src/stateful_transaction_validator.rs index 8e2fd18f63..f064acf130 100644 --- a/crates/gateway/src/stateful_transaction_validator.rs +++ b/crates/gateway/src/stateful_transaction_validator.rs @@ -23,7 +23,6 @@ use tracing::error; use crate::config::StatefulTransactionValidatorConfig; use crate::errors::StatefulTransactionValidatorResult; use crate::state_reader::{MempoolStateReader, StateReaderFactory}; -use crate::utils::get_sender_address; #[cfg(test)] #[path = "stateful_transaction_validator_test.rs"] @@ -75,6 +74,9 @@ impl StatefulTransactionValidator { executable_tx: &ExecutableTransaction, mut validator: V, ) -> StatefulTransactionValidatorResult { + let tx_hash = executable_tx.tx_hash(); + let sender_address = executable_tx.contract_address(); + let account_tx = AccountTransaction::try_from( // TODO(Arni): create a try_from for &ExecutableTransaction. executable_tx.clone(), @@ -83,8 +85,6 @@ impl StatefulTransactionValidator { error!("Failed to convert executable transaction into account transaction: {}", error); GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() } })?; - let tx_hash = account_tx.tx_hash(); - let sender_address = get_sender_address(&account_tx); let account_nonce = validator.get_nonce(sender_address).map_err(|e| { error!("Failed to get nonce for sender address {}: {}", sender_address, e); GatewaySpecError::UnexpectedError { data: "Internal server error.".to_owned() } @@ -149,6 +149,7 @@ pub fn get_latest_block_info( }) } +// TODO(Arni): Remove irrelevant fields. /// Holds members created by the stateful transaction validator, needed for /// [`MempoolInput`](starknet_mempool_types::mempool_types::MempoolInput). #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/crates/gateway/src/stateful_transaction_validator_test.rs b/crates/gateway/src/stateful_transaction_validator_test.rs index fbe493bd9f..3e41c3c524 100644 --- a/crates/gateway/src/stateful_transaction_validator_test.rs +++ b/crates/gateway/src/stateful_transaction_validator_test.rs @@ -6,7 +6,7 @@ use blockifier::context::BlockContext; use blockifier::test_utils::CairoVersion; use blockifier::transaction::errors::{TransactionFeeError, TransactionPreValidationError}; use mempool_test_utils::starknet_api_test_utils::{ - executable_invoke_tx, + executable_invoke_tx as create_executable_invoke_tx, TEST_SENDER_ADDRESS, VALID_L1_GAS_MAX_AMOUNT, VALID_L1_GAS_MAX_PRICE_PER_UNIT, @@ -17,8 +17,10 @@ use pretty_assertions::assert_eq; use rstest::{fixture, rstest}; use starknet_api::core::{ContractAddress, Nonce, PatriciaKey}; use starknet_api::executable_transaction::Transaction; +use starknet_api::test_utils::deploy_account::executable_deploy_account_tx; +use starknet_api::test_utils::invoke::executable_invoke_tx; use starknet_api::transaction::{Resource, TransactionHash}; -use starknet_api::{contract_address, felt, patricia_key}; +use starknet_api::{contract_address, deploy_account_tx_args, felt, invoke_tx_args, patricia_key}; use starknet_gateway_types::errors::GatewaySpecError; use starknet_types_core::felt::Felt; @@ -63,14 +65,17 @@ fn stateful_validator(block_context: BlockContext) -> StatefulTransactionValidat // TODO(Arni): consider testing declare and deploy account. #[rstest] #[case::valid_tx( - executable_invoke_tx(CairoVersion::Cairo1), + create_executable_invoke_tx(CairoVersion::Cairo1), Ok(ValidateInfo{ tx_hash: TransactionHash::default(), sender_address: contract_address!("0xc0020000"), account_nonce: Nonce::default() }) )] -#[case::invalid_tx(executable_invoke_tx(CairoVersion::Cairo1), Err(STATEFUL_VALIDATOR_FEE_ERROR))] +#[case::invalid_tx( + create_executable_invoke_tx(CairoVersion::Cairo1), + Err(STATEFUL_VALIDATOR_FEE_ERROR) +)] fn test_stateful_tx_validator( #[case] executable_tx: Transaction, #[case] expected_result: BlockifierStatefulValidatorResult, @@ -125,30 +130,24 @@ fn test_instantiate_validator() { #[rstest] #[case::should_skip_validation( - Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx( - starknet_api::invoke_tx_args!(nonce: Nonce(Felt::ONE)) - )), + Transaction::Invoke(executable_invoke_tx(invoke_tx_args!(nonce: Nonce(Felt::ONE)))), Nonce::default(), true )] #[case::should_not_skip_validation_nonce_over_max_nonce_for_skip( - Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx( - starknet_api::invoke_tx_args!(nonce: Nonce(Felt::ZERO)) - )), + Transaction::Invoke(executable_invoke_tx(invoke_tx_args!(nonce: Nonce(Felt::ZERO)))), Nonce::default(), false )] #[case::should_not_skip_validation_non_invoke( Transaction::DeployAccount( - starknet_api::test_utils::deploy_account::executable_deploy_account_tx( - starknet_api::deploy_account_tx_args!(), Nonce::default() - ) + executable_deploy_account_tx(deploy_account_tx_args!(), Nonce::default()) ), Nonce::default(), false)] #[case::should_not_skip_validation_account_nonce_1( - Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx( - starknet_api::invoke_tx_args!( + Transaction::Invoke(executable_invoke_tx( + invoke_tx_args!( nonce: Nonce(Felt::ONE), sender_address: TEST_SENDER_ADDRESS.into() ) diff --git a/crates/gateway/src/utils.rs b/crates/gateway/src/utils.rs index a96ac69a8a..5c0cfe0a95 100644 --- a/crates/gateway/src/utils.rs +++ b/crates/gateway/src/utils.rs @@ -1,5 +1,4 @@ -use blockifier::transaction::account_transaction::AccountTransaction; -use starknet_api::core::{ChainId, ContractAddress}; +use starknet_api::core::ChainId; use starknet_api::executable_transaction::{ DeclareTransaction as ExecutableDeclareTransaction, DeployAccountTransaction as ExecutableDeployAccountTransaction, @@ -7,7 +6,6 @@ use starknet_api::executable_transaction::{ Transaction as ExecutableTransaction, }; use starknet_api::rpc_transaction::{RpcDeclareTransaction, RpcTransaction}; -use starknet_api::transaction::DeclareTransaction; use starknet_gateway_types::errors::GatewaySpecError; use tracing::{debug, error}; @@ -78,18 +76,3 @@ fn compile_contract_and_build_executable_declare_tx( Ok(executable_declare_tx) } - -// TODO(yael 9/5/54): Should be implemented as part of InternalTransaction in starknet-api -pub fn get_sender_address(tx: &AccountTransaction) -> ContractAddress { - match tx { - AccountTransaction::Declare(tx) => match &tx.tx { - DeclareTransaction::V3(tx) => tx.sender_address, - _ => panic!("Unsupported transaction version"), - }, - AccountTransaction::DeployAccount(tx) => tx.contract_address(), - AccountTransaction::Invoke(tx) => match &tx.tx() { - starknet_api::transaction::InvokeTransaction::V3(tx) => tx.sender_address, - _ => panic!("Unsupported transaction version"), - }, - } -}