From 27159b9ece306752c1e58c37e412d9c81180b57c Mon Sep 17 00:00:00 2001 From: Ivan Litteri <67517699+ilitteri@users.noreply.github.com> Date: Thu, 31 Oct 2024 12:54:25 -0300 Subject: [PATCH 1/3] fix(l1): add temporary fix to transaction filtering & handle tx filtering unwrap edge case (#1018) **Motivation** To avoid panicking when the transaction filter fails to filter a transaction because we do not yet filter transactions with invalid tip. **Description** - Filter transactions whose effective gas tip calculation underflowed. - Handle the unwrap of the `effective_gas_tip` calculation. Even though this is an edge case that shouldn't happen, it is better to have a descriptive error if it happens someday than to panic. --------- Co-authored-by: fmoletta <99273364+fmoletta@users.noreply.github.com> --- crates/blockchain/error.rs | 2 ++ crates/blockchain/mempool.rs | 7 +++++++ crates/blockchain/payload.rs | 34 +++++++++++++++++++++++----------- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/crates/blockchain/error.rs b/crates/blockchain/error.rs index 230b0fff3..e4632ca6f 100644 --- a/crates/blockchain/error.rs +++ b/crates/blockchain/error.rs @@ -32,6 +32,8 @@ pub enum InvalidBlockError { GasUsedMismatch, #[error("Blob gas used doesn't match value in header")] BlobGasUsedMismatch, + #[error("Invalid transaction: {0}")] + InvalidTransaction(String), } #[derive(Debug, thiserror::Error)] diff --git a/crates/blockchain/mempool.rs b/crates/blockchain/mempool.rs index f32ed10bc..3b7fe0a0e 100644 --- a/crates/blockchain/mempool.rs +++ b/crates/blockchain/mempool.rs @@ -79,6 +79,7 @@ pub fn filter_transactions( if filter.only_plain_txs && is_blob_tx || filter.only_blob_txs && !is_blob_tx { return false; } + // Filter by tip & base_fee if let Some(min_tip) = filter.min_tip { if !tx @@ -87,7 +88,13 @@ pub fn filter_transactions( { return false; } + // This is a temporary fix to avoid invalid transactions to be included. + // This should be removed once https://github.com/lambdaclass/ethereum_rust/issues/680 + // is addressed. + } else if tx.effective_gas_tip(filter.base_fee).is_none() { + return false; } + // Filter by blob gas fee if let (true, Some(blob_fee)) = (is_blob_tx, filter.blob_fee) { if !tx.max_fee_per_blob_gas().is_some_and(|fee| fee >= blob_fee) { diff --git a/crates/blockchain/payload.rs b/crates/blockchain/payload.rs index 885cac812..e2fd6a624 100644 --- a/crates/blockchain/payload.rs +++ b/crates/blockchain/payload.rs @@ -25,7 +25,7 @@ use crate::{ GAS_LIMIT_BOUND_DIVISOR, GAS_PER_BLOB, MAX_BLOB_GAS_PER_BLOCK, MIN_GAS_LIMIT, TARGET_BLOB_GAS_PER_BLOCK, TX_GAS_COST, }, - error::ChainError, + error::{ChainError, InvalidBlockError}, mempool::{self, PendingTxFilter}, }; @@ -222,7 +222,7 @@ pub fn apply_withdrawals(context: &mut PayloadBuildContext) -> Result<(), EvmErr /// Returns two transaction queues, one for plain and one for blob txs fn fetch_mempool_transactions( context: &mut PayloadBuildContext, -) -> Result<(TransactionQueue, TransactionQueue), StoreError> { +) -> Result<(TransactionQueue, TransactionQueue), ChainError> { let tx_filter = PendingTxFilter { /*TODO(https://github.com/lambdaclass/ethereum_rust/issues/680): add tip filter */ base_fee: context.base_fee_per_gas(), @@ -245,12 +245,12 @@ fn fetch_mempool_transactions( TransactionQueue::new( mempool::filter_transactions(&plain_tx_filter, store)?, context.base_fee_per_gas(), - ), + )?, // Blob txs TransactionQueue::new( mempool::filter_transactions(&blob_tx_filter, store)?, context.base_fee_per_gas(), - ), + )?, )) } @@ -320,7 +320,7 @@ pub fn fill_transactions(context: &mut PayloadBuildContext) -> Result<(), ChainE // Execute tx let receipt = match apply_transaction(&head_tx, context) { Ok(receipt) => { - txs.shift(); + txs.shift()?; // Pull transaction from the mempool mempool::remove_transaction( tx_hash, @@ -464,7 +464,10 @@ impl From for Transaction { impl TransactionQueue { /// Creates a new TransactionQueue from a set of transactions grouped by sender and sorted by nonce - fn new(mut txs: HashMap>, base_fee: Option) -> Self { + fn new( + mut txs: HashMap>, + base_fee: Option, + ) -> Result { let mut heads = Vec::new(); for (address, txs) in txs.iter_mut() { // Pull the first tx from each list and add it to the heads list @@ -472,18 +475,22 @@ impl TransactionQueue { let head_tx = txs.remove(0); heads.push(HeadTransaction { // We already ran this method when filtering the transactions from the mempool so it shouldn't fail - tip: head_tx.effective_gas_tip(base_fee).unwrap(), + tip: head_tx + .effective_gas_tip(base_fee) + .ok_or(ChainError::InvalidBlock( + InvalidBlockError::InvalidTransaction("Attempted to add an invalid transaction to the block. The transaction filter must have failed.".to_owned()), + ))?, tx: head_tx, sender: *address, }); } // Sort heads by higest tip (and lowest timestamp if tip is equal) heads.sort(); - TransactionQueue { + Ok(TransactionQueue { heads, txs, base_fee, - } + }) } /// Remove all transactions from the queue @@ -513,7 +520,7 @@ impl TransactionQueue { /// Remove the top transaction /// Add a tx from the same sender to the head transactions - fn shift(&mut self) { + fn shift(&mut self) -> Result<(), ChainError> { let tx = self.heads.remove(0); if let Some(txs) = self.txs.get_mut(&tx.sender) { // Fetch next head @@ -521,7 +528,11 @@ impl TransactionQueue { let head_tx = txs.remove(0); let head = HeadTransaction { // We already ran this method when filtering the transactions from the mempool so it shouldn't fail - tip: head_tx.effective_gas_tip(self.base_fee).unwrap(), + tip: head_tx.effective_gas_tip(self.base_fee).ok_or( + ChainError::InvalidBlock( + InvalidBlockError::InvalidTransaction("Attempted to add an invalid transaction to the block. The transaction filter must have failed.".to_owned()), + ), + )?, tx: head_tx, sender: tx.sender, }; @@ -533,6 +544,7 @@ impl TransactionQueue { self.heads.insert(index, head); } } + Ok(()) } } From acad2ee0dfcb5c5b387e62831d46f2e846759279 Mon Sep 17 00:00:00 2001 From: Ivan Litteri <67517699+ilitteri@users.noreply.github.com> Date: Thu, 31 Oct 2024 13:16:59 -0300 Subject: [PATCH 2/3] refactor(l2): add `-w` flag to cmds that send txs (#1036) **Motivation** Nowadays, all the commands that send transactions do not wait for transaction receipts. If you run the same command multiple times the same transaction with the same nonce is going to be sent to the node; another problem is that the users have to perform additional steps to make sure that their transaction was finalized or not. As this is not an implementation problem but a misuse of the CLI, it'd be good for users to also have the option to wait for their transactions to be finalized. **Description** Adds a `-w` flag to the cmds `deploy`, `transfer`, `send`, `deposit`, `withdraw`, and `claim-withdraw` which when set, waits for the transaction sent to be finalized (a.k.a. wait for its receipt). --- cmd/ethereum_rust_l2/src/commands/wallet.rs | 58 ++++++++++++++++++++- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/cmd/ethereum_rust_l2/src/commands/wallet.rs b/cmd/ethereum_rust_l2/src/commands/wallet.rs index b8ccbfbb4..8f4c57020 100644 --- a/cmd/ethereum_rust_l2/src/commands/wallet.rs +++ b/cmd/ethereum_rust_l2/src/commands/wallet.rs @@ -42,11 +42,17 @@ pub(crate) enum Command { help = "Specify the wallet in which you want to deposit your funds." )] to: Option
, + #[clap(short = 'w', required = false)] + wait_for_receipt: bool, #[clap(long, short = 'e', required = false)] explorer_url: bool, }, #[clap(about = "Finalize a pending withdrawal.")] - ClaimWithdraw { l2_withdrawal_tx_hash: H256 }, + ClaimWithdraw { + l2_withdrawal_tx_hash: H256, + #[clap(short = 'w', required = false)] + wait_for_receipt: bool, + }, #[clap(about = "Transfer funds to another wallet.")] Transfer { // TODO: Parse ether instead. @@ -56,6 +62,10 @@ pub(crate) enum Command { token_address: Option
, #[clap(long = "to")] to: Address, + #[clap(long = "nonce")] + nonce: Option, + #[clap(short = 'w', required = false)] + wait_for_receipt: bool, #[clap( long = "l1", required = false, @@ -79,6 +89,8 @@ pub(crate) enum Command { help = "Specify the token address, the base token is used as default." )] token_address: Option
, + #[clap(short = 'w', required = false)] + wait_for_receipt: bool, #[clap(long, short = 'e', required = false)] explorer_url: bool, }, @@ -121,6 +133,8 @@ pub(crate) enum Command { gas_price: Option, #[clap(long = "priority-gas-price", required = false)] priority_gas_price: Option, + #[clap(short = 'w', required = false)] + wait_for_receipt: bool, }, #[clap(about = "Make a call to a contract")] Call { @@ -177,6 +191,8 @@ pub(crate) enum Command { gas_price: Option, #[clap(long = "priority-gas-price", required = false)] priority_gas_price: Option, + #[clap(short = 'w', required = false)] + wait_for_receipt: bool, }, } @@ -258,6 +274,7 @@ impl Command { amount, token_address, to, + wait_for_receipt, explorer_url: _, } => { if to.is_some() { @@ -275,7 +292,9 @@ impl Command { amount, token_address: None, to: cfg.contracts.common_bridge, + wait_for_receipt, l1: true, + nonce: None, explorer_url: false, } .run(cfg) @@ -285,6 +304,7 @@ impl Command { } Command::ClaimWithdraw { l2_withdrawal_tx_hash, + wait_for_receipt, } => { let (withdrawal_l2_block_number, claimed_amount) = match rollup_client .get_transaction_by_hash(l2_withdrawal_tx_hash) @@ -329,11 +349,17 @@ impl Command { .await?; println!("Withdrawal claim sent: {tx_hash:#x}"); + + if wait_for_receipt { + wait_for_transaction_receipt(ð_client, tx_hash).await?; + } } Command::Transfer { amount, token_address, to, + nonce, + wait_for_receipt, l1, explorer_url: _, } => { @@ -351,7 +377,7 @@ impl Command { } else { cfg.network.l2_chain_id }, - nonce: client.get_nonce(from).await?, + nonce: nonce.unwrap_or(client.get_nonce(from).await?), max_fee_per_gas: client.get_gas_price().await?.as_u64() * 100, gas_limit: 21000 * 100, ..Default::default() @@ -369,12 +395,17 @@ impl Command { "[{}] Transfer sent: {tx_hash:#x}", if l1 { "L1" } else { "L2" } ); + + if wait_for_receipt { + wait_for_transaction_receipt(&client, tx_hash).await?; + } } Command::Withdraw { amount, to, nonce, token_address: _, + wait_for_receipt, explorer_url: _, } => { let withdraw_transaction = PrivilegedL2Transaction { @@ -393,6 +424,10 @@ impl Command { .await?; println!("Withdrawal sent: {tx_hash:#x}"); + + if wait_for_receipt { + wait_for_transaction_receipt(&rollup_client, tx_hash).await?; + } } Command::WithdrawalProof { tx_hash } => { let (_index, path) = get_withdraw_merkle_proof(&rollup_client, tx_hash).await?; @@ -414,6 +449,7 @@ impl Command { gas_limit, gas_price, priority_gas_price, + wait_for_receipt, } => { let client = match l1 { true => eth_client, @@ -442,6 +478,10 @@ impl Command { "[{}] Transaction sent: {tx_hash:#x}", if l1 { "L1" } else { "L2" } ); + + if wait_for_receipt { + wait_for_transaction_receipt(&client, tx_hash).await?; + } } Command::Call { to, @@ -482,6 +522,7 @@ impl Command { gas_limit, gas_price, priority_gas_price, + wait_for_receipt, } => { let client = match l1 { true => eth_client, @@ -507,8 +548,21 @@ impl Command { println!("Contract deployed in tx: {deployment_tx_hash:#x}"); println!("Contract address: {deployed_contract_address:#x}"); + + if wait_for_receipt { + wait_for_transaction_receipt(&client, deployment_tx_hash).await?; + } } }; Ok(()) } } + +pub async fn wait_for_transaction_receipt(client: &EthClient, tx_hash: H256) -> eyre::Result<()> { + println!("Waiting for transaction receipt..."); + while client.get_transaction_receipt(tx_hash).await?.is_none() { + tokio::time::sleep(std::time::Duration::from_secs(1)).await; + } + println!("Transaction confirmed"); + Ok(()) +} From acd036579d90de89632e3df2f5ce0870615fee77 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Thu, 31 Oct 2024 14:41:22 -0300 Subject: [PATCH 3/3] fix(l1): made the tx-spammer work with our node (#1027) **Motivation** Have the tx spammer running instead of immediatly shutting down when we are the first node in the devnet setup. **Description** This PR tackled a couple of issues until we were able to process an initial transaction, (they are small changes): - We were returning an error on `eth_getTransactionCount` when we either didn't have a `latest` block before genesis or at any point when we don't have `pending` blocks. The solution in this case was returning `0x0` in those cases. - When requesting `eth_getTransactionCount` on `pending` after some transaction went through we were defaulting to `0x0`, it appears that the idea is to default to the `latest` in those case which make sense. - There were a missing filter that made the node panic when building payloads for transactions with fees lower than the base_fee_per_gas, this generated that those kind of transactions stopped the node's ability to build blocks when they were in the mempool, we made a quick workaround in this PR, but will remove it in favor of #1018 Closes #1026 --- crates/networking/rpc/eth/account.rs | 5 ++--- crates/networking/rpc/types/block_identifier.rs | 10 +++++++++- test_data/el-stability-check.yml | 2 +- test_data/network_params.yaml | 4 +++- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/crates/networking/rpc/eth/account.rs b/crates/networking/rpc/eth/account.rs index 283d83b3f..953999daa 100644 --- a/crates/networking/rpc/eth/account.rs +++ b/crates/networking/rpc/eth/account.rs @@ -156,9 +156,8 @@ impl RpcHandler for GetTransactionCountRequest { ); let Some(block_number) = self.block.resolve_block_number(&storage)? else { - return Err(RpcErr::Internal( - "Could not resolve block number".to_owned(), - )); // Should we return Null here? + return serde_json::to_value("0x0") + .map_err(|error| RpcErr::Internal(error.to_string())); }; let nonce = storage diff --git a/crates/networking/rpc/types/block_identifier.rs b/crates/networking/rpc/types/block_identifier.rs index a995b35d2..5b596ccad 100644 --- a/crates/networking/rpc/types/block_identifier.rs +++ b/crates/networking/rpc/types/block_identifier.rs @@ -39,7 +39,15 @@ impl BlockIdentifier { BlockTag::Finalized => storage.get_finalized_block_number(), BlockTag::Safe => storage.get_safe_block_number(), BlockTag::Latest => storage.get_latest_block_number(), - BlockTag::Pending => storage.get_pending_block_number(), + BlockTag::Pending => { + storage + .get_pending_block_number() + // If there are no pending blocks, we return the latest block number + .and_then(|pending_block_number| match pending_block_number { + Some(block_number) => Ok(Some(block_number)), + None => storage.get_latest_block_number(), + }) + } }, } } diff --git a/test_data/el-stability-check.yml b/test_data/el-stability-check.yml index 6b4a38a64..d28765a06 100644 --- a/test_data/el-stability-check.yml +++ b/test_data/el-stability-check.yml @@ -24,7 +24,7 @@ tasks: - name: run_task_matrix title: "Check block proposals from all client pairs" - timeout: 2m + timeout: 3m configVars: matrixValues: "validatorPairNames" config: diff --git a/test_data/network_params.yaml b/test_data/network_params.yaml index cbb4ac117..d2c4fc7ec 100644 --- a/test_data/network_params.yaml +++ b/test_data/network_params.yaml @@ -20,4 +20,6 @@ assertoor_params: run_block_proposal_check: false run_blob_transaction_test: true tests: - - 'https://raw.githubusercontent.com/lambdaclass/lambda_ethereum_rust/refs/heads/main/test_data/el-stability-check.yml' \ No newline at end of file + - 'https://raw.githubusercontent.com/lambdaclass/lambda_ethereum_rust/refs/heads/main/test_data/el-stability-check.yml' +tx_spammer_params: + tx_spammer_extra_args: ["--accounts=10", --txcount=10]