From 2953f93336813ab68b6fe4102df2861e75b96a69 Mon Sep 17 00:00:00 2001 From: Thomas Coratger <60488569+tcoratger@users.noreply.github.com> Date: Thu, 6 Jun 2024 19:32:29 +0200 Subject: [PATCH] add retry in calldata (#1151) * add retry in signature * add retries to the entrypoint value * remove extra signature length * answer comments --------- Co-authored-by: Gregory Edison --- .trunk/trunk.yaml | 8 +-- lib/kakarot | 2 +- src/bin/hive_chain.rs | 2 +- src/eth_provider/constant.rs | 2 +- .../database/types/transaction.rs | 6 +-- src/eth_provider/provider.rs | 41 ++++++--------- src/eth_provider/starknet/kakarot_core.rs | 18 +++++-- src/tracing/mod.rs | 2 +- tests/tests/eth_provider.rs | 50 +++++++++++++------ 9 files changed, 74 insertions(+), 57 deletions(-) diff --git a/.trunk/trunk.yaml b/.trunk/trunk.yaml index 3f1387148..7ca9f9026 100644 --- a/.trunk/trunk.yaml +++ b/.trunk/trunk.yaml @@ -22,16 +22,16 @@ lint: - git-diff-check - hadolint@2.12.0 - markdownlint@0.41.0 - - osv-scanner@1.7.3 + - osv-scanner@1.7.4 - oxipng@9.1.1 - - prettier@3.2.5 + - prettier@3.3.0 - rustfmt@1.65.0 - shellcheck@0.10.0 - shfmt@3.6.0 - taplo@0.8.1 - terrascan@1.19.1 - - trivy@0.51.4 - - trufflehog@3.77.0 + - trivy@0.52.0 + - trufflehog@3.78.0 - yamllint@1.35.1 ignore: - linters: [ALL] diff --git a/lib/kakarot b/lib/kakarot index 6156c2b80..0cf51c295 160000 --- a/lib/kakarot +++ b/lib/kakarot @@ -1 +1 @@ -Subproject commit 6156c2b800ddac89c789e0f4a9d52625b994606b +Subproject commit 0cf51c29516978d5edb03d92f581de411e83ac41 diff --git a/src/bin/hive_chain.rs b/src/bin/hive_chain.rs index b47478093..8894c1ea0 100644 --- a/src/bin/hive_chain.rs +++ b/src/bin/hive_chain.rs @@ -62,7 +62,7 @@ async fn main() -> eyre::Result<()> { for transaction in body.transactions { let signer = transaction.recover_signer().ok_or_eyre("failed to recover signer")?; let chain_id = transaction.chain_id().ok_or_eyre("failed to recover chain id")?; - let starknet_tx = to_starknet_transaction(&transaction, Some(chain_id), signer, u64::MAX)?; + let starknet_tx = to_starknet_transaction(&transaction, Some(chain_id), signer, u64::MAX, 0)?; let nonce = match &starknet_tx { BroadcastedInvokeTransaction::V1(starknet_tx) => starknet_tx.nonce, diff --git a/src/eth_provider/constant.rs b/src/eth_provider/constant.rs index 6d3f876c2..7b23316aa 100644 --- a/src/eth_provider/constant.rs +++ b/src/eth_provider/constant.rs @@ -20,7 +20,7 @@ pub const ADDRESS_HEX_STRING_LEN: usize = 40; /// Starknet Modulus: 0x800000000000011000000000000000000000000000000000000000000000001 pub const STARKNET_MODULUS: U256 = U256::from_limbs([0x1, 0, 0, 0x0800_0000_0000_0011]); /// Maximum number of times a transaction can be retried -pub const TRANSACTION_MAX_RETRIES: u64 = 10; +pub const TRANSACTION_MAX_RETRIES: u8 = 10; #[cfg(feature = "hive")] use { diff --git a/src/eth_provider/database/types/transaction.rs b/src/eth_provider/database/types/transaction.rs index ac5669574..7d2f582c7 100644 --- a/src/eth_provider/database/types/transaction.rs +++ b/src/eth_provider/database/types/transaction.rs @@ -68,11 +68,11 @@ pub struct StoredPendingTransaction { #[serde(deserialize_with = "crate::eth_provider::database::types::serde::deserialize_intermediate")] pub tx: Transaction, /// Number of retries - pub retries: u64, + pub retries: u8, } impl StoredPendingTransaction { - pub const fn new(tx: Transaction, retries: u64) -> Self { + pub const fn new(tx: Transaction, retries: u8) -> Self { Self { tx, retries } } } @@ -80,7 +80,7 @@ impl StoredPendingTransaction { #[cfg(any(test, feature = "arbitrary", feature = "testing"))] impl<'a> StoredPendingTransaction { pub fn arbitrary_with_optional_fields(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { - Ok(Self { tx: StoredTransaction::arbitrary_with_optional_fields(u)?.into(), retries: u64::arbitrary(u)? }) + Ok(Self { tx: StoredTransaction::arbitrary_with_optional_fields(u)?.into(), retries: u8::arbitrary(u)? }) } } diff --git a/src/eth_provider/provider.rs b/src/eth_provider/provider.rs index b92f5cb0f..f3e32efb9 100644 --- a/src/eth_provider/provider.rs +++ b/src/eth_provider/provider.rs @@ -552,31 +552,8 @@ where let filter = into_filter("tx.hash", &transaction_signed.hash, HASH_HEX_STRING_LEN); let pending_transaction = self.database.get_one::(filter.clone(), None).await?; - // Determine the maximum fee - let max_fee = if cfg!(feature = "hive") { - u64::MAX - } else { - // TODO(Kakarot Fee Mechanism): When we no longer need to use the Starknet fees, remove this line. - // We need to get the balance (in Kakarot/Starknet native Token) of the signer to compute the Starknet maximum `max_fee`. - // We used to set max_fee = u64::MAX, but it'll fail if the signer doesn't have enough balance to pay the fees. - let eth_fees_per_gas = - transaction_signed.effective_gas_price(Some(transaction_signed.max_fee_per_gas() as u64)) as u64; - let eth_fees = eth_fees_per_gas.saturating_mul(transaction_signed.gas_limit()); - let balance = self.balance(signer, None).await?; - let max_fee: u64 = balance.try_into().unwrap_or(u64::MAX); - let max_fee = (u128::from(max_fee) * 80 / 100) as u64; - - // We add the retry count to the max fee in order to bypass the - // DuplicateTx error in Starknet, which rejects incoming transactions - // with the same hash. Incrementing the max fee causes the Starknet - // hash to change, allowing the transaction to pass. - let retries = pending_transaction.as_ref().map(|tx| tx.retries + 1).unwrap_or_default(); - max_fee.saturating_sub(eth_fees).saturating_add(retries) - }; - - // Deploy EVM transaction signer if Hive feature is enabled - #[cfg(feature = "hive")] - self.deploy_evm_transaction_signer(signer).await?; + // Number of retries for the transaction (0 if it's a new transaction) + let retries = pending_transaction.as_ref().map(|tx| tx.retries).unwrap_or_default(); // Serialize transaction document let transaction = @@ -601,12 +578,22 @@ where self.database.update_one::(transaction.into(), filter, true).await?; } + // The max fee is always set to 0. This means that no fee is perceived by the + // Starknet sequencer, which is the intended behavior has fee perception is + // handled by the Kakarot execution layer through EVM gas accounting. + let max_fee = 0; + // Convert the transaction to a Starknet transaction - let starnet_transaction = to_starknet_transaction(&transaction_signed, maybe_chain_id, signer, max_fee)?; + let starknet_transaction = + to_starknet_transaction(&transaction_signed, maybe_chain_id, signer, max_fee, retries + 1)?; + + // Deploy EVM transaction signer if Hive feature is enabled + #[cfg(feature = "hive")] + self.deploy_evm_transaction_signer(signer).await?; // Add the transaction to the Starknet provider let res = - self.starknet_provider.add_invoke_transaction(starnet_transaction).await.map_err(KakarotError::from)?; + self.starknet_provider.add_invoke_transaction(starknet_transaction).await.map_err(KakarotError::from)?; // Return transaction hash if testing feature is enabled, otherwise log and return Ethereum hash if cfg!(feature = "testing") { diff --git a/src/eth_provider/starknet/kakarot_core.rs b/src/eth_provider/starknet/kakarot_core.rs index 4bc7b33cb..568019f32 100644 --- a/src/eth_provider/starknet/kakarot_core.rs +++ b/src/eth_provider/starknet/kakarot_core.rs @@ -99,6 +99,7 @@ pub fn to_starknet_transaction( chain_id: Option, signer: Address, max_fee: u64, + retries: u8, ) -> EthProviderResult { let sender_address = starknet_address(signer); @@ -141,10 +142,20 @@ pub fn to_starknet_transaction( } let mut calldata = Vec::with_capacity(capacity); + + // assert that the selector < FieldElement::MAX - retries + assert!(*ETH_SEND_TRANSACTION < FieldElement::MAX - retries.into()); + let selector = *ETH_SEND_TRANSACTION + retries.into(); + + // Retries are used to alter the transaction hash in order to avoid the + // DuplicateTx error from the Starknet gateway, encountered whenever + // a transaction with the same hash is sent multiple times. + // We add the retries to the selector in the calldata, since the selector + // is not used in by the EOA contract during the transaction execution. calldata.append(&mut vec![ FieldElement::ONE, // call array length *KAKAROT_ADDRESS, // contract address - *ETH_SEND_TRANSACTION, // selector + selector, // selector + retries FieldElement::ZERO, // data offset signed_data.len().into(), // data length signed_data.len().into(), // calldata length @@ -179,6 +190,7 @@ mod tests { Some(1_802_203_764), Address::from_str("0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266").unwrap(), 0, + 0, ) .unwrap() { @@ -190,7 +202,7 @@ mod tests { FieldElement::from(131_015_958_324_370_192_097_986_834_655_742_602_650_u128), FieldElement::from(263_740_705_169_547_910_390_939_684_488_449_712_973_u128), FieldElement::from(164_374_192_806_466_935_713_473_791_294_001_132_486_u128), - FieldElement::ONE + FieldElement::ONE, ] ); @@ -252,6 +264,6 @@ mod tests { transaction.transaction.set_input(vec![0; 30000].into()); // Attempt to convert the transaction into a Starknet transaction - to_starknet_transaction(&transaction, Some(1), transaction.recover_signer().unwrap(), 100_000_000).unwrap(); + to_starknet_transaction(&transaction, Some(1), transaction.recover_signer().unwrap(), 100_000_000, 0).unwrap(); } } diff --git a/src/tracing/mod.rs b/src/tracing/mod.rs index 51662ae52..06f1b2643 100644 --- a/src/tracing/mod.rs +++ b/src/tracing/mod.rs @@ -288,7 +288,7 @@ mod tests { use url::Url; #[tokio::test(flavor = "multi_thread")] - #[ignore = "This test is used for debugging purposes only"] + #[ignore = "this test is used for debugging purposes only"] async fn test_debug_tracing() { // Set the env vars std::env::set_var("KAKAROT_ADDRESS", "CHECK THE KAKAROT ADDRESS FOR THE BLOCK YOU ARE DEBUGGING"); diff --git a/tests/tests/eth_provider.rs b/tests/tests/eth_provider.rs index 9faac94a2..762cafa8d 100644 --- a/tests/tests/eth_provider.rs +++ b/tests/tests/eth_provider.rs @@ -837,25 +837,43 @@ async fn test_retry_transactions(#[future] katana: Katana, _setup: ()) { .await .expect("Failed to insert transaction in mined collection"); - // Retrieve the retried transactions. - let retried_transactions = eth_provider.retry_transactions().await.expect("Failed to retry transactions"); + let mut pending_tx_hashes: Vec = Vec::new(); - // Assert that there is only one retried transaction. - assert_eq!(retried_transactions.len(), 1); + for i in 0..TRANSACTION_MAX_RETRIES + 2 { + // Retrieve the retried transactions. + let retried_transactions = eth_provider.retry_transactions().await.expect("Failed to retry transactions"); - // Retrieve the pending transactions. - let pending_transactions = eth_provider - .database() - .get::(None, None) - .await - .expect("Failed get pending transactions"); + // Assert that there is only one retried transaction before reaching retry limit. + assert_eq!(retried_transactions.len(), usize::from(i < TRANSACTION_MAX_RETRIES)); + + // Retrieve the pending transactions. + let pending_transactions = eth_provider + .database() + .get::(None, None) + .await + .expect("Failed get pending transactions"); + + if i < TRANSACTION_MAX_RETRIES { + // Ensure that the spurious transactions are dropped from the pending transactions collection + assert_eq!(pending_transactions.len(), 1); - // Ensure that the spurious transactions are dropped from the pending transactions collection - assert_eq!(pending_transactions.len(), 1); + // Ensure that the retry is incremented for the first transaction + assert_eq!(pending_transactions.first().unwrap().retries, i + 1); - // Ensure that the retry is incremented for the first transaction - assert_eq!(pending_transactions.first().unwrap().retries, 1); + // Ensure that the transaction1 is still in the pending transactions collection + assert_eq!(pending_transactions.first().unwrap().tx, transaction1); - // Ensure that the transaction1 is still in the pending transactions collection - assert_eq!(pending_transactions.first().unwrap().tx, transaction1); + // Get the pending transaction hash + let pending_tx_hash = retried_transactions.first().unwrap(); + + // Ensure that the pending transaction hash is not already in the list + // Transaction hashes should be unique + assert!(!pending_tx_hashes.contains(pending_tx_hash)); + + // Add the pending transaction hash to the list + pending_tx_hashes.push(*pending_tx_hash); + } else { + assert_eq!(pending_transactions.len(), 0); + } + } }