From 6b98ba22e01aa0dba321bf18f5419ed34ba53028 Mon Sep 17 00:00:00 2001 From: Dylan Socolobsky Date: Wed, 27 Nov 2024 16:56:54 -0300 Subject: [PATCH] chore(l2): Remove index slicing usage --- cmd/ethrex_l2/src/commands/wallet.rs | 6 +++ crates/l2/proposer/errors.rs | 5 ++ crates/l2/proposer/l1_committer.rs | 27 ++++++---- crates/l2/proposer/l1_watcher.rs | 80 ++++++++++++++++++---------- crates/l2/sdk/src/sdk.rs | 10 +++- crates/l2/tests/tests.rs | 6 ++- crates/l2/utils/eth_client/mod.rs | 39 ++++++++++---- crates/l2/utils/merkle_tree.rs | 40 +++++++++----- crates/l2/utils/mod.rs | 16 ++++-- 9 files changed, 162 insertions(+), 67 deletions(-) diff --git a/cmd/ethrex_l2/src/commands/wallet.rs b/cmd/ethrex_l2/src/commands/wallet.rs index e9ea8de70e..e5487e18c5 100644 --- a/cmd/ethrex_l2/src/commands/wallet.rs +++ b/cmd/ethrex_l2/src/commands/wallet.rs @@ -245,6 +245,12 @@ async fn get_withdraw_merkle_proof( .collect(), tx_withdrawal_hash, ) + .map_err(|err| { + eyre::eyre!( + "Failed to generate merkle proof in get_withdraw_merkle_proof: {:?}", + err + ) + })? .ok_or_eyre("Transaction's WithdrawalData is not in block's WithdrawalDataMerkleRoot")?; Ok((index, path)) diff --git a/crates/l2/proposer/errors.rs b/crates/l2/proposer/errors.rs index d33cbe7319..bce616717a 100644 --- a/crates/l2/proposer/errors.rs +++ b/crates/l2/proposer/errors.rs @@ -1,5 +1,6 @@ use std::sync::mpsc::SendError; +use crate::utils::merkle_tree::MerkleError; use crate::utils::{config::errors::ConfigError, eth_client::errors::EthClientError}; use ethereum_types::FromStrRadixErr; use ethrex_core::types::BlobsBundleError; @@ -92,6 +93,10 @@ pub enum CommitterError { FailedToReExecuteBlock(#[from] EvmError), #[error("Committer failed to send transaction: {0}")] FailedToSendCommitment(String), + #[error("Commiter failed to decode deposit hash")] + FailedToDecodeDepositHash, + #[error("Commiter failed to merkelize: {0}")] + FailedToMerkelize(#[from] MerkleError), #[error("Withdrawal transaction was invalid")] InvalidWithdrawalTransaction, #[error("Blob estimation failed: {0}")] diff --git a/crates/l2/proposer/l1_committer.rs b/crates/l2/proposer/l1_committer.rs index 3633d15cf5..5605fed8db 100644 --- a/crates/l2/proposer/l1_committer.rs +++ b/crates/l2/proposer/l1_committer.rs @@ -117,13 +117,13 @@ impl Committer { } let withdrawal_logs_merkle_root = - self.get_withdrawals_merkle_root(withdrawal_hashes); + self.get_withdrawals_merkle_root(withdrawal_hashes)?; let deposit_logs_hash = self.get_deposit_hash( deposits .iter() .filter_map(|tx| tx.get_deposit_hash()) .collect(), - ); + )?; let state_diff = self.prepare_state_diff( &block_to_commit, @@ -180,11 +180,14 @@ impl Committer { Ok(withdrawals) } - pub fn get_withdrawals_merkle_root(&self, withdrawals_hashes: Vec) -> H256 { + pub fn get_withdrawals_merkle_root( + &self, + withdrawals_hashes: Vec, + ) -> Result { if !withdrawals_hashes.is_empty() { - merkelize(withdrawals_hashes) + merkelize(withdrawals_hashes).map_err(CommitterError::FailedToMerkelize) } else { - H256::zero() + Ok(H256::zero()) } } @@ -206,25 +209,27 @@ impl Committer { deposits } - pub fn get_deposit_hash(&self, deposit_hashes: Vec) -> H256 { + pub fn get_deposit_hash(&self, deposit_hashes: Vec) -> Result { if !deposit_hashes.is_empty() { - H256::from_slice( + Ok(H256::from_slice( [ &(deposit_hashes.len() as u16).to_be_bytes(), - &keccak( + keccak( deposit_hashes .iter() .map(H256::as_bytes) .collect::>() .concat(), ) - .as_bytes()[2..32], + .as_bytes() + .get(2..32) + .ok_or(CommitterError::FailedToDecodeDepositHash)?, ] .concat() .as_slice(), - ) + )) } else { - H256::zero() + Ok(H256::zero()) } } /// Prepare the state diff for the block. diff --git a/crates/l2/proposer/l1_watcher.rs b/crates/l2/proposer/l1_watcher.rs index a29e188eb4..7963fad060 100644 --- a/crates/l2/proposer/l1_watcher.rs +++ b/crates/l2/proposer/l1_watcher.rs @@ -79,14 +79,17 @@ impl L1Watcher { pub async fn get_pending_deposit_logs(&self) -> Result, L1WatcherError> { Ok(hex::decode( - &self - .eth_client + self.eth_client .call( self.address, Bytes::copy_from_slice(&[0x35, 0x6d, 0xa2, 0x49]), Overrides::default(), ) - .await?[2..], + .await? + .get(2..) + .ok_or(L1WatcherError::FailedToDeserializeLog( + "Not a valid hex string".to_string(), + ))?, ) .map_err(|_| L1WatcherError::FailedToDeserializeLog("Not a valid hex string".to_string()))? .chunks(32) @@ -112,22 +115,28 @@ impl L1Watcher { self.last_block_fetched, new_last_block ); - let logs = match self - .eth_client - .get_logs( - self.last_block_fetched + 1, - new_last_block, - self.address, - self.topics[0], - ) - .await - { - Ok(logs) => logs, - Err(error) => { - warn!("Error when getting logs from L1: {}", error); - vec![] - } - }; + let logs; + if let Some(first_topic) = self.topics.first() { + logs = match self + .eth_client + .get_logs( + self.last_block_fetched + 1, + new_last_block, + self.address, + *first_topic, + ) + .await + { + Ok(logs) => logs, + Err(error) => { + warn!("Error when getting logs from L1: {}", error); + vec![] + } + }; + } else { + warn!("Error when getting logs from L1: topics vector is empty"); + logs = vec![]; + } debug!("Logs: {:#?}", logs); @@ -158,14 +167,31 @@ impl L1Watcher { .unwrap_or_default(); for log in logs { - let mint_value = format!("{:#x}", log.log.topics[1]) - .parse::() - .map_err(|e| { - L1WatcherError::FailedToDeserializeLog(format!( - "Failed to parse mint value from log: {e:#?}" - )) - })?; - let beneficiary = format!("{:#x}", log.log.topics[2].into_uint()) + let mint_value = format!( + "{:#x}", + log.log + .topics + .get(1) + .ok_or(L1WatcherError::FailedToDeserializeLog( + "Failed to parse mint value from log: log.topics[1] out of bounds" + .to_owned() + ))? + ) + .parse::() + .map_err(|e| { + L1WatcherError::FailedToDeserializeLog(format!( + "Failed to parse mint value from log: {e:#?}" + )) + })?; + let beneficiary_uint = log + .log + .topics + .get(2) + .ok_or(L1WatcherError::FailedToDeserializeLog( + "Failed to parse beneficiary from log: log.topics[2] out of bounds".to_owned(), + ))? + .into_uint(); + let beneficiary = format!("{:#x}", beneficiary_uint) .parse::
() .map_err(|e| { L1WatcherError::FailedToDeserializeLog(format!( diff --git a/crates/l2/sdk/src/sdk.rs b/crates/l2/sdk/src/sdk.rs index 1113b82760..b121df6166 100644 --- a/crates/l2/sdk/src/sdk.rs +++ b/crates/l2/sdk/src/sdk.rs @@ -163,7 +163,14 @@ pub async fn claim_withdraw( let mut calldata = Vec::new(); // Function selector - calldata.extend_from_slice(&keccak(CLAIM_WITHDRAWAL_SIGNATURE).as_bytes()[..4]); + calldata.extend_from_slice( + keccak(CLAIM_WITHDRAWAL_SIGNATURE) + .as_bytes() + .get(..4) + .ok_or(EthClientError::Custom( + "failed to slice into the claim withdrawal signature".to_owned(), + ))?, + ); // bytes32 l2WithdrawalTxHash calldata.extend_from_slice(l2_withdrawal_tx_hash.as_fixed_bytes()); @@ -271,6 +278,7 @@ pub async fn get_withdraw_merkle_proof( .collect(), tx_withdrawal_hash, ) + .map_err(|err| EthClientError::Custom(format!("Failed to generate merkle proof: {err}")))? .ok_or(EthClientError::Custom( "Failed to generate merkle proof, element is not on the tree".to_string(), ))?; diff --git a/crates/l2/tests/tests.rs b/crates/l2/tests/tests.rs index 97b15823c6..6ba684cafa 100644 --- a/crates/l2/tests/tests.rs +++ b/crates/l2/tests/tests.rs @@ -226,7 +226,7 @@ async fn testito() { println!("Claiming funds on L1"); while u64::from_str_radix( - ð_client + eth_client .call( Address::from_str( &std::env::var("ON_CHAIN_PROPOSER_ADDRESS") @@ -238,7 +238,9 @@ async fn testito() { Overrides::default(), ) .await - .unwrap()[2..], + .unwrap() + .get(2..) + .unwrap(), 16, ) .unwrap() diff --git a/crates/l2/utils/eth_client/mod.rs b/crates/l2/utils/eth_client/mod.rs index e327a98138..97dc582272 100644 --- a/crates/l2/utils/eth_client/mod.rs +++ b/crates/l2/utils/eth_client/mod.rs @@ -29,7 +29,7 @@ use keccak_hash::keccak; use reqwest::Client; use secp256k1::SecretKey; use serde::{Deserialize, Serialize}; -use serde_json::json; +use serde_json::{json, Value}; use std::ops::Div; use tokio::time::{sleep, Instant}; use tracing::warn; @@ -365,7 +365,9 @@ impl EthClient { // Add the nonce just if present, otherwise the RPC will use the latest nonce if let Some(nonce) = transaction.nonce { - data["nonce"] = json!(format!("{:#x}", nonce)); + if let Value::Object(ref mut map) = data { + map.insert("nonce".to_owned(), json!(format!("{:#x}", nonce))); + } } let request = RpcRequest { @@ -377,8 +379,12 @@ impl EthClient { match self.send_request(request).await { Ok(RpcResponse::Success(result)) => u64::from_str_radix( - &serde_json::from_value::(result.result) - .map_err(EstimateGasPriceError::SerdeJSONError)?[2..], + serde_json::from_value::(result.result) + .map_err(EstimateGasPriceError::SerdeJSONError)? + .get(2..) + .ok_or(EstimateGasPriceError::Custom( + "Failed to slice index response in estimate_gas".to_owned(), + ))?, 16, ) .map_err(EstimateGasPriceError::ParseIntError) @@ -398,9 +404,20 @@ impl EthClient { "Failed to hex::decode in estimate_gas".to_owned(), ) })?; - let string_length = U256::from_big_endian(&abi_decoded_error_data[36..68]); - let string_data = - &abi_decoded_error_data[68..68 + string_length.as_usize()]; + let string_length = U256::from_big_endian( + abi_decoded_error_data + .get(36..68) + .ok_or(EthClientError::Custom( + "Failed to slice index abi_decoded_error_data in estimate_gas" + .to_owned(), + ))?, + ); + let string_data = abi_decoded_error_data + .get(68..68 + string_length.as_usize()) + .ok_or(EthClientError::Custom( + "Failed to slice index abi_decoded_error_data in estimate_gas" + .to_owned(), + ))?; String::from_utf8(string_data.to_vec()).map_err(|_| { EthClientError::Custom( "Failed to String::from_utf8 in estimate_gas".to_owned(), @@ -449,8 +466,12 @@ impl EthClient { match self.send_request(request).await { Ok(RpcResponse::Success(result)) => u64::from_str_radix( - &serde_json::from_value::(result.result) - .map_err(GetNonceError::SerdeJSONError)?[2..], + serde_json::from_value::(result.result) + .map_err(GetNonceError::SerdeJSONError)? + .get(2..) + .ok_or(EthClientError::Custom( + "Failed to deserialize get_nonce request".to_owned(), + ))?, 16, ) .map_err(GetNonceError::ParseIntError) diff --git a/crates/l2/utils/merkle_tree.rs b/crates/l2/utils/merkle_tree.rs index d52ffb3648..4fe4898ae2 100644 --- a/crates/l2/utils/merkle_tree.rs +++ b/crates/l2/utils/merkle_tree.rs @@ -1,7 +1,16 @@ use keccak_hash::{keccak, H256}; +use serde::{Deserialize, Serialize}; use tracing::info; -pub fn merkelize(data: Vec) -> H256 { +#[derive(Debug, thiserror::Error, Clone, Serialize, Deserialize)] +pub enum MerkleError { + #[error("Left element is None")] + LeftElementIsNone(), + #[error("Data vector is empty")] + DataVectorIsEmpty(), +} + +pub fn merkelize(data: Vec) -> Result { info!("Merkelizing {:?}", data); let mut data = data; let mut first = true; @@ -9,19 +18,21 @@ pub fn merkelize(data: Vec) -> H256 { first = false; data = data .chunks(2) - .map(|chunk| { - let left = chunk[0]; - let right = *chunk.get(1).unwrap_or(&left); - keccak([left.as_bytes(), right.as_bytes()].concat()) + .flat_map(|chunk| -> Result { + let left = chunk.first().ok_or(MerkleError::LeftElementIsNone())?; + let right = *chunk.get(1).unwrap_or(left); + Ok(keccak([left.as_bytes(), right.as_bytes()].concat())) }) .collect(); } - data[0] + data.first() + .copied() + .ok_or(MerkleError::DataVectorIsEmpty()) } -pub fn merkle_proof(data: Vec, base_element: H256) -> Option> { +pub fn merkle_proof(data: Vec, base_element: H256) -> Result>, MerkleError> { if !data.contains(&base_element) { - return None; + return Ok(None); } let mut proof = vec![]; @@ -34,9 +45,12 @@ pub fn merkle_proof(data: Vec, base_element: H256) -> Option> { let current_target = target_hash; data = data .chunks(2) - .map(|chunk| { - let left = chunk[0]; - let right = *chunk.get(1).unwrap_or(&left); + .flat_map(|chunk| -> Result { + let left = chunk + .first() + .copied() + .ok_or(MerkleError::LeftElementIsNone())?; + let right = chunk.get(1).copied().unwrap_or(left); let result = keccak([left.as_bytes(), right.as_bytes()].concat()); if left == current_target { proof.push(right); @@ -45,10 +59,10 @@ pub fn merkle_proof(data: Vec, base_element: H256) -> Option> { proof.push(left); target_hash = result; } - result + Ok(result) }) .collect(); } - Some(proof) + Ok(Some(proof)) } diff --git a/crates/l2/utils/mod.rs b/crates/l2/utils/mod.rs index 293effdd50..eec7adeaad 100644 --- a/crates/l2/utils/mod.rs +++ b/crates/l2/utils/mod.rs @@ -1,5 +1,4 @@ -use std::array::TryFromSliceError; - +use crate::utils::eth_client::errors::EthClientError; use ethrex_core::Address; use keccak_hash::{keccak, H256}; use secp256k1::SecretKey; @@ -26,14 +25,23 @@ where hex.serialize(serializer) } -pub fn get_address_from_secret_key(secret_key: &SecretKey) -> Result { +pub fn get_address_from_secret_key(secret_key: &SecretKey) -> Result { let public_key = secret_key .public_key(secp256k1::SECP256K1) .serialize_uncompressed(); let hash = keccak(&public_key[1..]); // Get the lat 20 bytes of the hash - let address_bytes: [u8; 20] = hash[12..32].try_into()?; + let address_bytes: [u8; 20] = hash + .as_ref() + .get(12..32) + .ok_or(EthClientError::Custom( + "Failed to get_address_from_secret_key: error slicing address_bytes".to_owned(), + ))? + .try_into() + .map_err(|err| { + EthClientError::Custom(format!("Failed to get_address_from_secret_key: {err}")) + })?; Ok(Address::from(address_bytes)) }