skip nonce check on fee estimation (#2)

Co-authored-by: Ammar Arif <[email protected]>
cartridge-gg · Aug 16, 2024 · 2610f0c · 2610f0c
1 parent df4cc97
commit 2610f0c
Show file tree

Hide file tree

Showing 9 changed files with 198 additions and 26 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -53,7 +53,7 @@ axum = "0.7.5"
 base64 = "0.13.1"
 bincode = "2.0.0-rc.3"
 bitvec = "1.0.1"
-blockifier = "=0.8.0-rc.1"
+blockifier = { git = "https://github.com/cartridge-gg/blockifier", branch = "nonce-check-flag" }
 bloomfilter = "1.0.12"
 bytes = "1.4.0"
 cached = "0.44.0"

diff --git a/crates/executor/src/estimate.rs b/crates/executor/src/estimate.rs
@@ -9,6 +9,7 @@ pub fn estimate(
     execution_state: ExecutionState<'_>,
     transactions: Vec<Transaction>,
     skip_validate: bool,
+    skip_nonce_check: bool,
 ) -> Result<Vec<FeeEstimate>, TransactionExecutionError> {
     let block_number = execution_state.header.number;
 
@@ -32,7 +33,13 @@ pub fn estimate(
         let tx_info: Result<
             blockifier::transaction::objects::TransactionExecutionInfo,
             blockifier::transaction::errors::TransactionExecutionError,
-        > = transaction.execute(&mut state, &block_context, false, !skip_validate);
+        > = transaction.execute(
+            &mut state,
+            &block_context,
+            false,
+            !skip_validate,
+            !skip_nonce_check,
+        );
 
         match tx_info {
             Ok(tx_info) => {

diff --git a/crates/executor/src/simulate.rs b/crates/executor/src/simulate.rs
@@ -108,6 +108,7 @@ pub fn simulate(
             &block_context,
             !skip_fee_charge,
             !skip_validate,
+            true,
         );
         let state_diff = to_state_diff(&mut tx_state, transaction_declared_deprecated_class_hash)?;
         tx_state.commit();
@@ -185,7 +186,7 @@ pub fn trace(
 
         let mut tx_state = CachedState::<_>::create_transactional(&mut state);
         let tx_info = tx
-            .execute(&mut tx_state, &block_context, true, true)
+            .execute(&mut tx_state, &block_context, true, true, true)
             .map_err(|e| {
                 // Update the cache with the error. Lock the cache before sending to avoid
                 // race conditions between senders and receivers.

diff --git a/crates/rpc/src/v05/method/estimate_fee.rs b/crates/rpc/src/v05/method/estimate_fee.rs
@@ -142,7 +142,13 @@ pub async fn estimate_fee(
             .map(|tx| crate::executor::map_broadcasted_transaction(&tx, context.chain_id))
             .collect::<Result<Vec<_>, _>>()?;
 
-        let result = pathfinder_executor::estimate(state, transactions, false)?;
+        let result = pathfinder_executor::estimate(
+            state,
+            transactions,
+            false,
+            // skip nonce check because it is not necessary for fee estimation
+            true,
+        )?;
 
         Ok::<_, EstimateFeeError>(result)
     })

diff --git a/crates/rpc/src/v06/method/estimate_fee.rs b/crates/rpc/src/v06/method/estimate_fee.rs
@@ -192,7 +192,13 @@ pub async fn estimate_fee_impl(
             .map(|tx| crate::executor::map_broadcasted_transaction(&tx, context.chain_id))
             .collect::<Result<Vec<_>, _>>()?;
 
-        let result = pathfinder_executor::estimate(state, transactions, skip_validate)?;
+        let result = pathfinder_executor::estimate(
+            state,
+            transactions,
+            skip_validate,
+            // skip nonce check because it is not necessary for fee estimation
+            true,
+        )?;
 
         Ok::<_, EstimateFeeError>(result)
     })

diff --git a/crates/rpc/src/v06/method/estimate_message_fee.rs b/crates/rpc/src/v06/method/estimate_message_fee.rs
@@ -152,7 +152,13 @@ pub(crate) async fn estimate_message_fee_impl(
 
         let transaction = create_executor_transaction(input, context.chain_id)?;
 
-        let result = pathfinder_executor::estimate(state, vec![transaction], false)?;
+        let result = pathfinder_executor::estimate(
+            state,
+            vec![transaction],
+            false,
+            // skip nonce check because it is not necessary for fee estimation
+            true,
+        )?;
 
         Ok::<_, EstimateMessageFeeError>(result)
     })

diff --git a/crates/rpc/src/v06/method/simulate_transactions.rs b/crates/rpc/src/v06/method/simulate_transactions.rs
@@ -902,6 +902,39 @@ pub(crate) mod tests {
         pretty_assertions_sorted::assert_eq!(result.0, expected);
     }
 
+    #[tokio::test]
+    async fn test_simulate_transaction_with_invalid_nonce_will_fail() {
+        let (context, _, _, _) = crate::test_setup::test_context().await;
+
+        // create transaction with invalid nonce
+        let input_json = serde_json::json!({
+            "block_id": {"block_number": 1},
+            "transactions": [
+                {
+                    "contract_address_salt": "0x46c0d4abf0192a788aca261e58d7031576f7d8ea5229f452b0f23e691dd5971",
+                    "max_fee": "0x0",
+                    "signature": [],
+                    "class_hash": DUMMY_ACCOUNT_CLASS_HASH,
+                    "nonce": "0x1337",
+                    "version": TransactionVersion::ONE_WITH_QUERY_VERSION,
+                    "constructor_calldata": [],
+                    "type": "DEPLOY_ACCOUNT"
+                }
+            ],
+            "simulation_flags": ["SKIP_FEE_CHARGE"]
+        });
+
+        // assert that the simulation returns an error due to invalid nonce
+        let input = SimulateTransactionInput::deserialize(&input_json).unwrap();
+        let err = simulate_transactions(context, input).await.unwrap_err();
+
+        if let SimulateTransactionError::TransactionExecutionError { error, .. } = err {
+            assert!(error.contains("Invalid transaction nonce"))
+        } else {
+            panic!("Unexpected error")
+        }
+    }
+
     #[tokio::test]
     async fn declare_cairo_v0_class() {
         pub const CAIRO0_DEFINITION: &[u8] =

diff --git a/crates/rpc/src/v07/method/estimate_fee.rs b/crates/rpc/src/v07/method/estimate_fee.rs
@@ -38,7 +38,10 @@ mod tests {
     use crate::v06::method::estimate_fee::*;
     use crate::v06::types::PriceUnit;
 
-    fn declare_transaction(account_contract_address: ContractAddress) -> BroadcastedTransaction {
+    fn declare_transaction(
+        account_contract_address: ContractAddress,
+        nonce: TransactionNonce,
+    ) -> BroadcastedTransaction {
         let sierra_definition = include_bytes!("../../../fixtures/contracts/storage_access.json");
         let sierra_hash =
             class_hash!("0544b92d358447cb9e50b65092b7169f931d29e05c1404a2cd08c6fd7e32ba90");
@@ -60,7 +63,7 @@ mod tests {
                 version: TransactionVersion::TWO,
                 max_fee,
                 signature: vec![],
-                nonce: TransactionNonce(Default::default()),
+                nonce,
                 contract_class,
                 sender_address: account_contract_address,
                 compiled_class_hash: casm_hash,
@@ -71,14 +74,15 @@ mod tests {
     fn deploy_transaction(
         account_contract_address: ContractAddress,
         universal_deployer_address: ContractAddress,
+        nonce: TransactionNonce,
     ) -> BroadcastedTransaction {
         let max_fee = Fee::default();
         let sierra_hash =
             class_hash!("0544b92d358447cb9e50b65092b7169f931d29e05c1404a2cd08c6fd7e32ba90");
 
         BroadcastedTransaction::Invoke(BroadcastedInvokeTransaction::V1(
             BroadcastedInvokeTransactionV1 {
-                nonce: transaction_nonce!("0x1"),
+                nonce,
                 version: TransactionVersion::ONE,
                 max_fee,
                 signature: vec![],
@@ -104,12 +108,15 @@ mod tests {
         ))
     }
 
-    fn invoke_transaction(account_contract_address: ContractAddress) -> BroadcastedTransaction {
+    fn invoke_transaction(
+        account_contract_address: ContractAddress,
+        nonce: TransactionNonce,
+    ) -> BroadcastedTransaction {
         let max_fee = Fee::default();
 
         BroadcastedTransaction::Invoke(BroadcastedInvokeTransaction::V1(
             BroadcastedInvokeTransactionV1 {
-                nonce: transaction_nonce!("0x2"),
+                nonce,
                 version: TransactionVersion::ONE,
                 max_fee,
                 signature: vec![],
@@ -147,7 +154,10 @@ mod tests {
         ))
     }
 
-    fn invoke_v3_transaction(account_contract_address: ContractAddress) -> BroadcastedTransaction {
+    fn invoke_v3_transaction(
+        account_contract_address: ContractAddress,
+        nonce: TransactionNonce,
+    ) -> BroadcastedTransaction {
         BroadcastedTransaction::Invoke(BroadcastedInvokeTransaction::V3(
             BroadcastedInvokeTransactionV3 {
                 version: TransactionVersion::THREE,
@@ -165,7 +175,7 @@ mod tests {
                     // AccountCallArray::data_len
                     call_param!("0"),
                 ],
-                nonce: transaction_nonce!("0x3"),
+                nonce,
                 resource_bounds: ResourceBounds::default(),
                 tip: Tip(0),
                 paymaster_data: vec![],
@@ -185,16 +195,22 @@ mod tests {
             .await;
 
         // declare test class
-        let declare_transaction = declare_transaction(account_contract_address);
+        let declare_transaction =
+            declare_transaction(account_contract_address, TransactionNonce::default());
         // deploy with unversal deployer contract
-        let deploy_transaction =
-            deploy_transaction(account_contract_address, universal_deployer_address);
+        let deploy_transaction = deploy_transaction(
+            account_contract_address,
+            universal_deployer_address,
+            transaction_nonce!("0x1"),
+        );
         // invoke deployed contract
-        let invoke_transaction = invoke_transaction(account_contract_address);
+        let invoke_transaction =
+            invoke_transaction(account_contract_address, transaction_nonce!("0x2"));
         // do the same invoke with a v0 transaction
         let invoke_v0_transaction = invoke_v0_transaction();
         // do the same invoke with a v3 transaction
-        let invoke_v3_transaction = invoke_v3_transaction(account_contract_address);
+        let invoke_v3_transaction =
+            invoke_v3_transaction(account_contract_address, transaction_nonce!("0x3"));
 
         let input = EstimateFeeInput {
             request: vec![
@@ -262,24 +278,121 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn declare_deploy_and_invoke_sierra_class_starknet_0_13_1_1() {
+    async fn declare_deploy_and_invoke_sierra_class_starknet_0_13_1_with_invalid_nonce_will_pass() {
+        let (context, last_block_header, account_contract_address, universal_deployer_address) =
+            crate::test_setup::test_context_with_starknet_version(StarknetVersion::new(
+                0, 13, 1, 1,
+            ))
+            .await;
+
+        // declare test class
+        let declare_transaction =
+            declare_transaction(account_contract_address, transaction_nonce!("0x1"));
+        // deploy with unversal deployer contract
+        let deploy_transaction = deploy_transaction(
+            account_contract_address,
+            universal_deployer_address,
+            transaction_nonce!("0x2"),
+        );
+        // invoke deployed contract
+        let invoke_transaction =
+            invoke_transaction(account_contract_address, transaction_nonce!("0x3"));
+        // do the same invoke with a v0 transaction
+        let invoke_v0_transaction = invoke_v0_transaction();
+        // do the same invoke with a v3 transaction
+        let invoke_v3_transaction =
+            invoke_v3_transaction(account_contract_address, transaction_nonce!("0x4"));
+
+        let input = EstimateFeeInput {
+            request: vec![
+                declare_transaction,
+                deploy_transaction,
+                invoke_transaction,
+                invoke_v0_transaction,
+                invoke_v3_transaction,
+            ],
+            simulation_flags: SimulationFlags(vec![]),
+            block_id: BlockId::Number(last_block_header.number),
+        };
+        let result = super::estimate_fee(context, input).await.unwrap();
+        let declare_expected = FeeEstimate {
+            gas_consumed: 878.into(),
+            gas_price: 1.into(),
+            overall_fee: 1262.into(),
+            unit: PriceUnit::Wei,
+            data_gas_consumed: Some(192.into()),
+            data_gas_price: Some(2.into()),
+        };
+        let deploy_expected = FeeEstimate {
+            gas_consumed: 16.into(),
+            gas_price: 1.into(),
+            overall_fee: 464.into(),
+            unit: PriceUnit::Wei,
+            data_gas_consumed: Some(224.into()),
+            data_gas_price: Some(2.into()),
+        };
+        let invoke_expected = FeeEstimate {
+            gas_consumed: 12.into(),
+            gas_price: 1.into(),
+            overall_fee: 268.into(),
+            unit: PriceUnit::Wei,
+            data_gas_consumed: Some(128.into()),
+            data_gas_price: Some(2.into()),
+        };
+        let invoke_v0_expected = FeeEstimate {
+            gas_consumed: 10.into(),
+            gas_price: 1.into(),
+            overall_fee: 266.into(),
+            unit: PriceUnit::Wei,
+            data_gas_consumed: Some(128.into()),
+            data_gas_price: Some(2.into()),
+        };
+        let invoke_v3_expected = FeeEstimate {
+            gas_consumed: 12.into(),
+            // STRK gas price is 2
+            gas_price: 2.into(),
+            overall_fee: 280.into(),
+            unit: PriceUnit::Fri,
+            data_gas_consumed: Some(128.into()),
+            data_gas_price: Some(2.into()),
+        };
+        assert_eq!(
+            result,
+            vec![
+                declare_expected,
+                deploy_expected,
+                invoke_expected,
+                invoke_v0_expected,
+                invoke_v3_expected,
+            ]
+        );
+    }
+
+    #[tokio::test]
+    async fn declare_deploy_and_invoke_sierra_class_starknet_0_13_1_1_with_arbitrary_nonce() {
         let (context, last_block_header, account_contract_address, universal_deployer_address) =
             crate::test_setup::test_context_with_starknet_version(StarknetVersion::new(
                 0, 13, 1, 1,
             ))
             .await;
 
         // declare test class
-        let declare_transaction = declare_transaction(account_contract_address);
+        let declare_transaction =
+            declare_transaction(account_contract_address, transaction_nonce!("0x1000"));
         // deploy with unversal deployer contract
-        let deploy_transaction =
-            deploy_transaction(account_contract_address, universal_deployer_address);
+        let deploy_transaction = deploy_transaction(
+            account_contract_address,
+            universal_deployer_address,
+            transaction_nonce!("0x123"),
+        );
         // invoke deployed contract
-        let invoke_transaction = invoke_transaction(account_contract_address);
+        let invoke_transaction =
+            invoke_transaction(account_contract_address, transaction_nonce!("0x31"));
         // do the same invoke with a v0 transaction
         let invoke_v0_transaction = invoke_v0_transaction();
         // do the same invoke with a v3 transaction
-        let invoke_v3_transaction = invoke_v3_transaction(account_contract_address);
+        let invoke_v3_transaction =
+            invoke_v3_transaction(account_contract_address, transaction_nonce!("0x4"));
 
         let input = EstimateFeeInput {
             request: vec![