From fabe1b699f959ef05aa59905694a87fcb6a044a1 Mon Sep 17 00:00:00 2001 From: Arni Hod Date: Sun, 12 Jan 2025 16:52:03 +0200 Subject: [PATCH] feat(starknet_gateway): use has tx from address to skip validate --- crates/starknet_gateway/src/errors.rs | 13 +++-- crates/starknet_gateway/src/gateway.rs | 12 +++-- .../src/stateful_transaction_validator.rs | 43 +++++++++++------ .../stateful_transaction_validator_test.rs | 47 +++++++++++++++---- 4 files changed, 83 insertions(+), 32 deletions(-) diff --git a/crates/starknet_gateway/src/errors.rs b/crates/starknet_gateway/src/errors.rs index 23e59c9b14..8b62ef3e12 100644 --- a/crates/starknet_gateway/src/errors.rs +++ b/crates/starknet_gateway/src/errors.rs @@ -80,11 +80,14 @@ impl From for GatewaySpecError { /// Converts a mempool client result to a gateway result. Some errors variants are unreachable in /// Gateway context, and some are not considered errors from the gateway's perspective. -pub fn mempool_client_result_to_gw_spec_result( - value: MempoolClientResult<()>, -) -> GatewayResult<()> { +/// If the result is a mempool client error that is not considered an error from the gateway's +/// perspective, the function returns `value_on_repressed_error`. +pub fn mempool_client_result_to_gw_spec_result( + value: MempoolClientResult, + value_on_repressed_error: T, +) -> GatewayResult { let err = match value { - Ok(()) => return Ok(()), + Ok(value) => return Ok(value), Err(err) => err, }; match err { @@ -104,7 +107,7 @@ pub fn mempool_client_result_to_gw_spec_result( MempoolError::P2pPropagatorClientError { .. } => { // Not an error from the gateway's perspective. warn!("P2p propagator client error: {}", mempool_error); - Ok(()) + Ok(value_on_repressed_error) } MempoolError::TransactionNotFound { .. } => { // This error is not expected to happen within the gateway, only from other diff --git a/crates/starknet_gateway/src/gateway.rs b/crates/starknet_gateway/src/gateway.rs index afc71e8dc9..d442ed4064 100644 --- a/crates/starknet_gateway/src/gateway.rs +++ b/crates/starknet_gateway/src/gateway.rs @@ -67,12 +67,12 @@ impl Gateway { ) -> GatewayResult { info!("Processing tx"); let process_tx_param = ProcessTxParams::new(self, tx); - let add_tx_args = process_tx_param.process_tx()?; + let add_tx_args = process_tx_param.process_tx().await?; let tx_hash = add_tx_args.tx.tx_hash(); let add_tx_args = AddTransactionArgsWrapper { args: add_tx_args, p2p_message_metadata }; - mempool_client_result_to_gw_spec_result(self.mempool_client.add_tx(add_tx_args).await)?; + mempool_client_result_to_gw_spec_result(self.mempool_client.add_tx(add_tx_args).await, ())?; // TODO: Also return `ContractAddress` for deploy and `ClassHash` for Declare. Ok(tx_hash) } @@ -85,6 +85,7 @@ struct ProcessTxParams { stateful_tx_validator: Arc, state_reader_factory: Arc, gateway_compiler: Arc, + mempool_client: SharedMempoolClient, chain_info: ChainInfo, tx: RpcTransaction, } @@ -96,12 +97,13 @@ impl ProcessTxParams { stateful_tx_validator: gateway.stateful_tx_validator.clone(), state_reader_factory: gateway.state_reader_factory.clone(), gateway_compiler: gateway.gateway_compiler.clone(), + mempool_client: gateway.mempool_client.clone(), chain_info: gateway.chain_info.clone(), tx, } } - fn process_tx(self) -> GatewayResult { + async fn process_tx(self) -> GatewayResult { // TODO(Arni, 1/5/2024): Perform congestion control. // Perform stateless validations. @@ -129,7 +131,9 @@ impl ProcessTxParams { GatewaySpecError::UnexpectedError { data: "Internal server error.".to_owned() } })?; - self.stateful_tx_validator.run_validate(&executable_tx, nonce, validator)?; + self.stateful_tx_validator + .run_validate(&executable_tx, nonce, self.mempool_client, validator) + .await?; // TODO(Arni): Add the Sierra and the Casm to the mempool input. Ok(AddTransactionArgs { tx: executable_tx, account_state: AccountState { address, nonce } }) diff --git a/crates/starknet_gateway/src/stateful_transaction_validator.rs b/crates/starknet_gateway/src/stateful_transaction_validator.rs index c79224ee03..974748c87c 100644 --- a/crates/starknet_gateway/src/stateful_transaction_validator.rs +++ b/crates/starknet_gateway/src/stateful_transaction_validator.rs @@ -12,16 +12,14 @@ use blockifier::versioned_constants::VersionedConstants; use mockall::automock; use starknet_api::block::BlockInfo; use starknet_api::core::Nonce; -use starknet_api::executable_transaction::{ - AccountTransaction as ExecutableTransaction, - InvokeTransaction as ExecutableInvokeTransaction, -}; +use starknet_api::executable_transaction::AccountTransaction as ExecutableTransaction; use starknet_gateway_types::errors::GatewaySpecError; +use starknet_mempool_types::communication::SharedMempoolClient; use starknet_types_core::felt::Felt; use tracing::error; use crate::config::StatefulTransactionValidatorConfig; -use crate::errors::StatefulTransactionValidatorResult; +use crate::errors::{mempool_client_result_to_gw_spec_result, StatefulTransactionValidatorResult}; use crate::state_reader::{MempoolStateReader, StateReaderFactory}; #[cfg(test)] @@ -55,13 +53,15 @@ impl StatefulTransactionValidatorTrait for BlockifierStatefulValidator { } impl StatefulTransactionValidator { - pub fn run_validate( + pub async fn run_validate( &self, executable_tx: &ExecutableTransaction, account_nonce: Nonce, + mempool_client: SharedMempoolClient, mut validator: V, ) -> StatefulTransactionValidatorResult<()> { - let skip_validate = skip_stateful_validations(executable_tx, account_nonce); + let skip_validate = + skip_stateful_validations(executable_tx, account_nonce, mempool_client).await?; let only_query = false; let charge_fee = enforce_fee(executable_tx, only_query); let execution_flags = ExecutionFlags { only_query, charge_fee, validate: !skip_validate }; @@ -104,16 +104,29 @@ impl StatefulTransactionValidator { // Check if validation of an invoke transaction should be skipped due to deploy_account not being // processed yet. This feature is used to improve UX for users sending deploy_account + invoke at // once. -fn skip_stateful_validations(tx: &ExecutableTransaction, account_nonce: Nonce) -> bool { - match 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) +async fn skip_stateful_validations( + tx: &ExecutableTransaction, + account_nonce: Nonce, + mempool_client: SharedMempoolClient, +) -> StatefulTransactionValidatorResult { + if let ExecutableTransaction::Invoke(tx) = 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. + if tx.nonce() == Nonce(Felt::ONE) && account_nonce == Nonce(Felt::ZERO) { + // We verify that a deploy_account transaction exists for this account. It is sufficient + // to check if the account exists in the mempool since it means that either it has a + // deploy_account transaction or transactions with future nonces that passed + // validations. + return mempool_client_result_to_gw_spec_result( + mempool_client.has_tx_from_address(tx.sender_address()).await, + // If the mempool returns a `P2pPropagatorClientError`, we do not skip the + // validation. + false, + ); } - ExecutableTransaction::DeployAccount(_) | ExecutableTransaction::Declare(_) => false, } + + Ok(false) } pub fn get_latest_block_info( diff --git a/crates/starknet_gateway/src/stateful_transaction_validator_test.rs b/crates/starknet_gateway/src/stateful_transaction_validator_test.rs index 366ccd4431..d8a872662b 100644 --- a/crates/starknet_gateway/src/stateful_transaction_validator_test.rs +++ b/crates/starknet_gateway/src/stateful_transaction_validator_test.rs @@ -1,3 +1,5 @@ +use std::sync::Arc; + use blockifier::blockifier::stateful_validator::{ StatefulValidatorError as BlockifierStatefulValidatorError, StatefulValidatorResult as BlockifierStatefulValidatorResult, @@ -25,6 +27,7 @@ use starknet_api::test_utils::NonceManager; use starknet_api::transaction::fields::Resource; use starknet_api::{deploy_account_tx_args, invoke_tx_args, nonce}; use starknet_gateway_types::errors::GatewaySpecError; +use starknet_mempool_types::communication::MockMempoolClient; use crate::config::StatefulTransactionValidatorConfig; use crate::state_reader::{MockStateReaderFactory, StateReaderFactory}; @@ -61,7 +64,8 @@ fn stateful_validator() -> StatefulTransactionValidator { create_executable_invoke_tx(CairoVersion::Cairo1(RunnableCairo1::Casm)), Err(STATEFUL_VALIDATOR_FEE_ERROR) )] -fn test_stateful_tx_validator( +#[tokio::test] +async fn test_stateful_tx_validator( #[case] executable_tx: AccountTransaction, #[case] expected_result: BlockifierStatefulValidatorResult<()>, stateful_validator: StatefulTransactionValidator, @@ -77,7 +81,16 @@ fn test_stateful_tx_validator( mock_validator.expect_validate().return_once(|_, _| expected_result.map(|_| ())); let account_nonce = nonce!(0); - let result = stateful_validator.run_validate(&executable_tx, account_nonce, mock_validator); + let mut mock_mempool_client = MockMempoolClient::new(); + mock_mempool_client.expect_has_tx_from_address().returning(|_| { + // The mempool does not have any transactions from the sender. + Ok(false) + }); + let mempool_client = Arc::new(mock_mempool_client); + + let result = stateful_validator + .run_validate(&executable_tx, account_nonce, mempool_client, mock_validator) + .await; assert_eq!(result, expected_result_as_stateful_transaction_result); } @@ -112,19 +125,22 @@ fn test_instantiate_validator(stateful_validator: StatefulTransactionValidator) #[case::should_skip_validation( executable_invoke_tx(invoke_tx_args!(nonce: nonce!(1))), nonce!(0), + true, true )] #[case::should_not_skip_validation_nonce_over_max_nonce_for_skip( executable_invoke_tx(invoke_tx_args!(nonce: nonce!(0))), nonce!(0), + true, false )] #[case::should_not_skip_validation_non_invoke( - executable_deploy_account_tx(deploy_account_tx_args!(), &mut NonceManager::default()) - , + executable_deploy_account_tx(deploy_account_tx_args!(), &mut NonceManager::default()), nonce!(0), - false) -] + true, + false + +)] #[case::should_not_skip_validation_account_nonce_1( executable_invoke_tx( invoke_tx_args!( @@ -133,11 +149,20 @@ fn test_instantiate_validator(stateful_validator: StatefulTransactionValidator) ) ), nonce!(1), + true, false )] -fn test_skip_stateful_validation( +#[case::should_not_skip_validation_no_tx_in_mempool( + executable_invoke_tx(invoke_tx_args!(nonce: nonce!(1))), + nonce!(0), + false, + false +)] +#[tokio::test] +async fn test_skip_stateful_validation( #[case] executable_tx: AccountTransaction, #[case] sender_nonce: Nonce, + #[case] has_tx: bool, #[case] should_skip_validate: bool, stateful_validator: StatefulTransactionValidator, ) { @@ -146,5 +171,11 @@ fn test_skip_stateful_validation( .expect_validate() .withf(move |_, skip_validate| *skip_validate == should_skip_validate) .returning(|_, _| Ok(())); - let _ = stateful_validator.run_validate(&executable_tx, sender_nonce, mock_validator); + let mut mock_mempool_client = MockMempoolClient::new(); + mock_mempool_client.expect_has_tx_from_address().returning(move |_| Ok(has_tx)); + let mempool_client = Arc::new(mock_mempool_client); + + let _ = stateful_validator + .run_validate(&executable_tx, sender_nonce, mempool_client, mock_validator) + .await; }