From 97b679be7f64711ce5b9f636ce7754083a2ddb98 Mon Sep 17 00:00:00 2001 From: Ammar Arif Date: Mon, 21 Oct 2024 11:08:59 -0400 Subject: [PATCH] feat(katana-pool): invalidate declare tx if the class already exists (#2564) --- crates/katana/pool/src/validation/error.rs | 5 ++ crates/katana/pool/src/validation/mod.rs | 11 +++++ crates/katana/pool/src/validation/stateful.rs | 16 +++++- .../rpc/rpc-types/src/error/starknet.rs | 1 + crates/katana/rpc/rpc/tests/starknet.rs | 49 ++++++++++++++++--- 5 files changed, 74 insertions(+), 8 deletions(-) diff --git a/crates/katana/pool/src/validation/error.rs b/crates/katana/pool/src/validation/error.rs index 1dc68d5c14..6eed85c0b2 100644 --- a/crates/katana/pool/src/validation/error.rs +++ b/crates/katana/pool/src/validation/error.rs @@ -63,4 +63,9 @@ pub enum InvalidTransactionError { /// The nonce that the tx is using. tx_nonce: Nonce, }, + + /// Error when a Declare transaction is trying to declare a class that has already been + /// declared. + #[error("Class with hash {class_hash:#x} has already been declared.")] + ClassAlreadyDeclared { class_hash: ClassHash }, } diff --git a/crates/katana/pool/src/validation/mod.rs b/crates/katana/pool/src/validation/mod.rs index 7544d853ea..72aeb9fca8 100644 --- a/crates/katana/pool/src/validation/mod.rs +++ b/crates/katana/pool/src/validation/mod.rs @@ -16,6 +16,12 @@ pub struct Error { pub error: Box, } +impl Error { + pub fn new(hash: TxHash, error: Box) -> Self { + Self { hash, error } + } +} + pub type ValidationResult = Result, Error>; /// A trait for validating transactions before they are added to the transaction pool. @@ -23,6 +29,11 @@ pub trait Validator { type Transaction: PoolTransaction; /// Validate a transaction. + /// + /// The `Err` variant of the returned `Result` should only be used to represent unexpected + /// errors that occurred during the validation process ie, provider + /// [error](katana_provider::error::ProviderError), and not for indicating that the + /// transaction is invalid. For that purpose, use the [`ValidationOutcome::Invalid`] enum. fn validate(&self, tx: Self::Transaction) -> ValidationResult; /// Validate a batch of transactions. diff --git a/crates/katana/pool/src/validation/stateful.rs b/crates/katana/pool/src/validation/stateful.rs index d803c17f1d..6e6dbfe1b4 100644 --- a/crates/katana/pool/src/validation/stateful.rs +++ b/crates/katana/pool/src/validation/stateful.rs @@ -102,6 +102,19 @@ impl Validator for TxValidator { let tx_nonce = tx.nonce(); let address = tx.sender(); + // For declare transactions, perform a static check if there's already an existing class + // with the same hash. + if let ExecutableTx::Declare(ref declare_tx) = tx.transaction { + let class_hash = declare_tx.class_hash(); + let class = this.state.class(class_hash).map_err(|e| Error::new(tx.hash, e.into()))?; + + // Return an error if the class already exists. + if class.is_some() { + let error = InvalidTransactionError::ClassAlreadyDeclared { class_hash }; + return Ok(ValidationOutcome::Invalid { tx, error }); + } + } + // Get the current nonce of the account from the pool or the state let current_nonce = if let Some(nonce) = this.pool_nonces.get(&address) { *nonce @@ -125,7 +138,8 @@ impl Validator for TxValidator { _ => tx.nonce() == Nonce::ONE && current_nonce == Nonce::ZERO, }; - // prepare a stateful validator and validate the transaction + // prepare a stateful validator and run the account validation logic (ie __validate__ + // entrypoint) let result = validate( this.prepare(), tx, diff --git a/crates/katana/rpc/rpc-types/src/error/starknet.rs b/crates/katana/rpc/rpc-types/src/error/starknet.rs index 9fdddba0cf..614184d681 100644 --- a/crates/katana/rpc/rpc-types/src/error/starknet.rs +++ b/crates/katana/rpc/rpc-types/src/error/starknet.rs @@ -182,6 +182,7 @@ impl From> for StarknetApiError { fn from(error: Box) -> Self { match error.as_ref() { InvalidTransactionError::InsufficientFunds { .. } => Self::InsufficientAccountBalance, + InvalidTransactionError::ClassAlreadyDeclared { .. } => Self::ClassAlreadyDeclared, InvalidTransactionError::IntrinsicFeeTooLow { .. } => Self::InsufficientMaxFee, InvalidTransactionError::NonAccount { .. } => Self::NonAccount, InvalidTransactionError::InvalidNonce { .. } => { diff --git a/crates/katana/rpc/rpc/tests/starknet.rs b/crates/katana/rpc/rpc/tests/starknet.rs index f71868bd19..ac4c6b655c 100644 --- a/crates/katana/rpc/rpc/tests/starknet.rs +++ b/crates/katana/rpc/rpc/tests/starknet.rs @@ -34,6 +34,13 @@ use tokio::sync::Mutex; mod common; +/// Macro used to assert that the given error is a Starknet error. +macro_rules! assert_starknet_err { + ($err:expr, $api_err:pat) => { + assert_matches!($err, AccountError::Provider(ProviderError::StarknetError($api_err))) + }; +} + #[tokio::test] async fn declare_and_deploy_contract() -> Result<()> { let sequencer = @@ -143,6 +150,41 @@ async fn declare_and_deploy_legacy_contract() -> Result<()> { Ok(()) } +#[tokio::test] +async fn declaring_already_existing_class() -> Result<()> { + let config = get_default_test_config(SequencingConfig::default()); + let sequencer = TestSequencer::start(config).await; + + let account = sequencer.account(); + let provider = sequencer.provider(); + + let path = PathBuf::from("tests/test_data/cairo1_contract.json"); + let (contract, compiled_hash) = common::prepare_contract_declaration_params(&path)?; + let class_hash = contract.class_hash(); + + // Declare the class for the first time. + let res = account.declare_v2(contract.clone().into(), compiled_hash).send().await?; + + // check that the tx is executed successfully and return the correct receipt + let _ = dojo_utils::TransactionWaiter::new(res.transaction_hash, &provider).await?; + // check that the class is actually declared + assert!(provider.get_class(BlockId::Tag(BlockTag::Pending), class_hash).await.is_ok()); + + // ----------------------------------------------------------------------- + // Declaring the same class again should fail with a ClassAlreadyDeclared error + + // We set max fee manually to avoid perfoming fee estimation as we just want to test that the + // pool validation will reject the tx. + // + // The value of the max fee is also irrelevant here, as the validator will only perform static + // checks and will not run the account's validation. + + let result = account.declare_v2(contract.into(), compiled_hash).max_fee(Felt::ONE).send().await; + assert_starknet_err!(result.unwrap_err(), StarknetError::ClassAlreadyDeclared); + + Ok(()) +} + #[rstest::rstest] #[tokio::test] async fn deploy_account( @@ -343,13 +385,6 @@ async fn concurrent_transactions_submissions( Ok(()) } -/// Macro used to assert that the given error is a Starknet error. -macro_rules! assert_starknet_err { - ($err:expr, $api_err:pat) => { - assert_matches!($err, AccountError::Provider(ProviderError::StarknetError($api_err))) - }; -} - #[rstest::rstest] #[tokio::test] async fn ensure_validator_have_valid_state(