From 8661477675b4ff0d704e94c499fea38a0649b3b8 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Tue, 7 Feb 2023 21:32:36 -0500 Subject: [PATCH 1/5] use hex decode instead of parse --- beacon_node/execution_layer/src/lib.rs | 77 ++++++++++++++++++-------- consensus/types/src/transaction.rs | 13 +++++ 2 files changed, 68 insertions(+), 22 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 3a1365db8..7f914c50b 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -14,7 +14,7 @@ pub use engine_api::{http, http::deposit_methods, http::HttpJsonRpc}; use engines::{Engine, EngineError}; pub use engines::{EngineState, ForkchoiceState}; use eth2::types::{builder_bid::SignedBuilderBid, ForkVersionedResponse}; -use ethers_core::types::Transaction as EthersTransaction; +use ethers_core::types::{Transaction as EthersTransaction, U64}; use fork_choice::ForkchoiceUpdateParameters; use lru::LruCache; use payload_status::process_payload_status; @@ -39,8 +39,10 @@ use tokio::{ }; use tokio_stream::wrappers::WatchStream; use types::consts::eip4844::BLOB_TX_TYPE; -use types::transaction::{AccessTuple, BlobTransaction}; -use types::{AbstractExecPayload, BeaconStateError, Blob, ExecPayload, KzgCommitment}; +use types::transaction::{AccessTuple, BlobTransaction, EcdsaSignature, SignedBlobTransaction}; +use types::{ + AbstractExecPayload, BeaconStateError, Blob, ExecPayload, KzgCommitment, VersionedHash, +}; use types::{ BlindedPayload, BlockType, ChainSpec, Epoch, ExecutionBlockHash, ForkName, ProposerPreparationData, PublicKeyBytes, Signature, SignedBeaconBlock, Slot, Transaction, @@ -2030,10 +2032,14 @@ pub enum BlobTxConversionError { MaxFeePerDataGasMissing, /// Missing the `blob_versioned_hashes` field. BlobVersionedHashesMissing, + /// `y_parity` field was greater than one. + InvalidYParity, /// There was an error converting the transaction to SSZ. SszError(ssz_types::Error), /// There was an error converting the transaction from JSON. SerdeJson(serde_json::Error), + /// There was an error converting the transaction from hex. + FromHexError(String), } impl From for BlobTxConversionError { @@ -2096,23 +2102,36 @@ fn ethers_tx_to_bytes( }) .collect::, BlobTxConversionError>>()?, )?; - let max_fee_per_data_gas = transaction + let max_fee_per_data_gas = Uint256::from_big_endian( + ð2_serde_utils::hex::decode( + transaction + .other + .get("maxFeePerDataGas") + .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)? + .as_str() + .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)?, + ) + .map_err(BlobTxConversionError::FromHexError)?, + ); + let blob_versioned_hashes = transaction .other - .get("maxFeePerDataGas") - .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)? - .as_str() - .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)? - .parse() - .map_err(|_| BlobTxConversionError::MaxFeePerDataGasMissing)?; - let blob_versioned_hashes = serde_json::from_str( - transaction - .other - .get("blobVersionedHashes") - .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)? - .as_str() - .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)?, - )?; - BlobTransaction { + .get("blobVersionedHashes") + .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)? + .as_array() + .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)? + .into_iter() + .map(|versioned_hash| { + Ok(Hash256::from_slice( + ð2_serde_utils::hex::decode( + versioned_hash + .as_str() + .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)?, + ) + .map_err(BlobTxConversionError::FromHexError)?, + )) + }) + .collect::, BlobTxConversionError>>()?; + let message = BlobTransaction { chain_id, nonce, max_priority_fee_per_gas, @@ -2123,9 +2142,23 @@ fn ethers_tx_to_bytes( data, access_list, max_fee_per_data_gas, - blob_versioned_hashes, - } - .as_ssz_bytes() + blob_versioned_hashes: VariableList::new(blob_versioned_hashes)?, + }; + + let y_parity = if transaction.v == U64::zero() { + false + } else if transaction.v == U64::one() { + true + } else { + return Err(BlobTxConversionError::InvalidYParity); + }; + let r = transaction.r; + let s = transaction.s; + let signature = EcdsaSignature { y_parity, r, s }; + + let mut signed_tx = SignedBlobTransaction { message, signature }.as_ssz_bytes(); + signed_tx.insert(0, BLOB_TX_TYPE); + signed_tx } else { transaction.rlp().to_vec() }; diff --git a/consensus/types/src/transaction.rs b/consensus/types/src/transaction.rs index 797ee1dae..ee0af981b 100644 --- a/consensus/types/src/transaction.rs +++ b/consensus/types/src/transaction.rs @@ -9,6 +9,12 @@ pub type MaxAccessListSize = U16777216; pub type MaxVersionedHashesListSize = U16777216; pub type MaxAccessListStorageKeys = U16777216; +#[derive(Debug, Clone, PartialEq, Encode, Decode)] +pub struct SignedBlobTransaction { + pub message: BlobTransaction, + pub signature: EcdsaSignature, +} + #[derive(Debug, Clone, PartialEq, Encode, Decode)] pub struct BlobTransaction { pub chain_id: Uint256, @@ -29,3 +35,10 @@ pub struct AccessTuple { pub address: Address, pub storage_keys: VariableList, } + +#[derive(Debug, Clone, PartialEq, Encode, Decode)] +pub struct EcdsaSignature { + pub y_parity: bool, + pub r: Uint256, + pub s: Uint256, +} From dd40adc5c07041ad5b4b9ec5a4321bff5c2e49b5 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 8 Feb 2023 10:38:45 -0500 Subject: [PATCH 2/5] check byte length when converting to uint256 and hash256 from bytes. Add comments --- beacon_node/execution_layer/src/lib.rs | 80 ++++++++++++++++++++------ 1 file changed, 61 insertions(+), 19 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 7f914c50b..bdf74d0a5 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -2040,6 +2040,10 @@ pub enum BlobTxConversionError { SerdeJson(serde_json::Error), /// There was an error converting the transaction from hex. FromHexError(String), + /// The `max_fee_per_data_gas` field did not contains 32 bytes. + InvalidDataGasBytesLen, + /// A `versioned_hash` did not contain 32 bytes. + InvalidVersionedHashBytesLen, } impl From for BlobTxConversionError { @@ -2064,29 +2068,49 @@ fn ethers_tx_to_bytes( .transaction_type .ok_or(BlobTxConversionError::NoTransactionType)? .as_u64(); + let tx = if BLOB_TX_TYPE as u64 == tx_type { + // ******************** BlobTransaction fields ******************** + + // chainId let chain_id = transaction .chain_id .ok_or(BlobTxConversionError::NoChainId)?; + + // nonce let nonce = if transaction.nonce > Uint256::from(u64::MAX) { return Err(BlobTxConversionError::NonceTooLarge); } else { transaction.nonce.as_u64() }; + + // maxPriorityFeePerGas let max_priority_fee_per_gas = transaction .max_priority_fee_per_gas .ok_or(BlobTxConversionError::MaxPriorityFeePerGasMissing)?; + + // maxFeePerGas let max_fee_per_gas = transaction .max_fee_per_gas .ok_or(BlobTxConversionError::MaxFeePerGasMissing)?; + + // gas let gas = if transaction.gas > Uint256::from(u64::MAX) { return Err(BlobTxConversionError::GasTooHigh); } else { transaction.gas.as_u64() }; + + // to let to = transaction.to; + + // value let value = transaction.value; + + // data (a.k.a input) let data = VariableList::new(transaction.input.to_vec())?; + + // accessList let access_list = VariableList::new( transaction .access_list @@ -2102,17 +2126,29 @@ fn ethers_tx_to_bytes( }) .collect::, BlobTxConversionError>>()?, )?; - let max_fee_per_data_gas = Uint256::from_big_endian( - ð2_serde_utils::hex::decode( - transaction - .other - .get("maxFeePerDataGas") - .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)? - .as_str() - .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)?, - ) - .map_err(BlobTxConversionError::FromHexError)?, - ); + + // ******************** BlobTransaction `other` fields ******************** + // + // Here we use the `other` field in the `ethers-rs` `Transaction` type because + // `ethers-rs` does not yet support SSZ and therefore the blobs transaction type. + + // maxFeePerDataGas + let data_gas_bytes = eth2_serde_utils::hex::decode( + transaction + .other + .get("maxFeePerDataGas") + .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)? + .as_str() + .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)?, + ) + .map_err(BlobTxConversionError::FromHexError)?; + let max_fee_per_data_gas = if data_gas_bytes.len() != Uint256::ssz_fixed_len() { + Err(BlobTxConversionError::InvalidDataGasBytesLen) + } else { + Ok(Uint256::from_big_endian(&data_gas_bytes)) + }?; + + // blobVersionedHashes let blob_versioned_hashes = transaction .other .get("blobVersionedHashes") @@ -2121,14 +2157,17 @@ fn ethers_tx_to_bytes( .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)? .into_iter() .map(|versioned_hash| { - Ok(Hash256::from_slice( - ð2_serde_utils::hex::decode( - versioned_hash - .as_str() - .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)?, - ) - .map_err(BlobTxConversionError::FromHexError)?, - )) + let hash_bytes = eth2_serde_utils::hex::decode( + versioned_hash + .as_str() + .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)?, + ) + .map_err(BlobTxConversionError::FromHexError)?; + if hash_bytes.len() != Hash256::ssz_fixed_len() { + Err(BlobTxConversionError::InvalidVersionedHashBytesLen) + } else { + Ok(Hash256::from_slice(&hash_bytes)) + } }) .collect::, BlobTxConversionError>>()?; let message = BlobTransaction { @@ -2145,6 +2184,8 @@ fn ethers_tx_to_bytes( blob_versioned_hashes: VariableList::new(blob_versioned_hashes)?, }; + // ******************** EcdsaSignature fields ******************** + let y_parity = if transaction.v == U64::zero() { false } else if transaction.v == U64::one() { @@ -2156,6 +2197,7 @@ fn ethers_tx_to_bytes( let s = transaction.s; let signature = EcdsaSignature { y_parity, r, s }; + // The `BLOB_TX_TYPE` should prepend the SSZ encoded `SignedBlobTransaction`. let mut signed_tx = SignedBlobTransaction { message, signature }.as_ssz_bytes(); signed_tx.insert(0, BLOB_TX_TYPE); signed_tx From 902f64a94692d6c87975e27761b2ad46571cb955 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 8 Feb 2023 10:59:48 -0500 Subject: [PATCH 3/5] remove clone of access lists --- beacon_node/execution_layer/src/lib.rs | 105 ++++++++++++++----------- 1 file changed, 60 insertions(+), 45 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index bdf74d0a5..50f01893d 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -14,6 +14,7 @@ pub use engine_api::{http, http::deposit_methods, http::HttpJsonRpc}; use engines::{Engine, EngineError}; pub use engines::{EngineState, ForkchoiceState}; use eth2::types::{builder_bid::SignedBuilderBid, ForkVersionedResponse}; +use ethers_core::types::transaction::eip2930::AccessListItem; use ethers_core::types::{Transaction as EthersTransaction, U64}; use fork_choice::ForkchoiceUpdateParameters; use lru::LruCache; @@ -51,6 +52,7 @@ use types::{ use types::{ ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge, }; +use warp::hyper::body::HttpBody; mod block_hash; mod engine_api; @@ -1689,13 +1691,15 @@ impl ExecutionLayer { return Ok(None); }; - let transactions = VariableList::from( - block - .transactions() - .into_iter() - .map(ethers_tx_to_bytes::) - .collect::, BlobTxConversionError>>()?, - ); + let convert_transactions = |transactions: Vec| { + VariableList::new( + transactions + .into_iter() + .map(ethers_tx_to_bytes::) + .collect::, BlobTxConversionError>>()?, + ) + .map_err(BlobTxConversionError::SszError) + }; let payload = match block { ExecutionBlockWithTransactions::Merge(merge_block) => { @@ -1713,7 +1717,7 @@ impl ExecutionLayer { extra_data: merge_block.extra_data, base_fee_per_gas: merge_block.base_fee_per_gas, block_hash: merge_block.block_hash, - transactions, + transactions: convert_transactions(merge_block.transactions)?, }) } ExecutionBlockWithTransactions::Capella(capella_block) => { @@ -1739,7 +1743,7 @@ impl ExecutionLayer { extra_data: capella_block.extra_data, base_fee_per_gas: capella_block.base_fee_per_gas, block_hash: capella_block.block_hash, - transactions, + transactions: convert_transactions(capella_block.transactions)?, withdrawals, }) } @@ -1767,7 +1771,7 @@ impl ExecutionLayer { base_fee_per_gas: eip4844_block.base_fee_per_gas, excess_data_gas: eip4844_block.excess_data_gas, block_hash: eip4844_block.block_hash, - transactions, + transactions: convert_transactions(eip4844_block.transactions)?, withdrawals, }) } @@ -2062,7 +2066,7 @@ impl From for BlobTxConversionError { /// on transaction type. That means RLP encoding if this is a transaction other than a /// `BLOB_TX_TYPE` transaction in which case, SSZ encoding will be used. fn ethers_tx_to_bytes( - transaction: &EthersTransaction, + transaction: EthersTransaction, ) -> Result, BlobTxConversionError> { let tx_type = transaction .transaction_type @@ -2070,58 +2074,73 @@ fn ethers_tx_to_bytes( .as_u64(); let tx = if BLOB_TX_TYPE as u64 == tx_type { + + let EthersTransaction { + hash, + nonce, + block_hash, + block_number, + transaction_index, + from, + to, + value, + gas_price, + gas, + input, + v, + r, + s, + transaction_type, + access_list, + max_priority_fee_per_gas, + max_fee_per_gas, + chain_id, + other, + } = transaction; + // ******************** BlobTransaction fields ******************** // chainId - let chain_id = transaction - .chain_id - .ok_or(BlobTxConversionError::NoChainId)?; + let chain_id = chain_id.ok_or(BlobTxConversionError::NoChainId)?; // nonce - let nonce = if transaction.nonce > Uint256::from(u64::MAX) { + let nonce = if nonce > Uint256::from(u64::MAX) { return Err(BlobTxConversionError::NonceTooLarge); } else { - transaction.nonce.as_u64() + nonce.as_u64() }; // maxPriorityFeePerGas - let max_priority_fee_per_gas = transaction - .max_priority_fee_per_gas - .ok_or(BlobTxConversionError::MaxPriorityFeePerGasMissing)?; + let max_priority_fee_per_gas = + max_priority_fee_per_gas.ok_or(BlobTxConversionError::MaxPriorityFeePerGasMissing)?; // maxFeePerGas - let max_fee_per_gas = transaction - .max_fee_per_gas - .ok_or(BlobTxConversionError::MaxFeePerGasMissing)?; + let max_fee_per_gas = max_fee_per_gas.ok_or(BlobTxConversionError::MaxFeePerGasMissing)?; // gas - let gas = if transaction.gas > Uint256::from(u64::MAX) { + let gas = if gas > Uint256::from(u64::MAX) { return Err(BlobTxConversionError::GasTooHigh); } else { - transaction.gas.as_u64() + gas.as_u64() }; - // to - let to = transaction.to; - - // value - let value = transaction.value; - // data (a.k.a input) - let data = VariableList::new(transaction.input.to_vec())?; + let data = VariableList::new(input.to_vec())?; // accessList let access_list = VariableList::new( - transaction - .access_list - .as_ref() + access_list .ok_or(BlobTxConversionError::AccessListMissing)? .0 - .iter() + .into_iter() .map(|access_tuple| { + let AccessListItem { + address, + storage_keys, + } = access_tuple; Ok(AccessTuple { - address: access_tuple.address, - storage_keys: VariableList::new(access_tuple.storage_keys.clone())?, + address, + storage_keys: VariableList::new(storage_keys)?, }) }) .collect::, BlobTxConversionError>>()?, @@ -2134,8 +2153,7 @@ fn ethers_tx_to_bytes( // maxFeePerDataGas let data_gas_bytes = eth2_serde_utils::hex::decode( - transaction - .other + other .get("maxFeePerDataGas") .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)? .as_str() @@ -2149,8 +2167,7 @@ fn ethers_tx_to_bytes( }?; // blobVersionedHashes - let blob_versioned_hashes = transaction - .other + let blob_versioned_hashes = other .get("blobVersionedHashes") .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)? .as_array() @@ -2186,15 +2203,13 @@ fn ethers_tx_to_bytes( // ******************** EcdsaSignature fields ******************** - let y_parity = if transaction.v == U64::zero() { + let y_parity = if v == U64::zero() { false - } else if transaction.v == U64::one() { + } else if v == U64::one() { true } else { return Err(BlobTxConversionError::InvalidYParity); }; - let r = transaction.r; - let s = transaction.s; let signature = EcdsaSignature { y_parity, r, s }; // The `BLOB_TX_TYPE` should prepend the SSZ encoded `SignedBlobTransaction`. From 99da11e9f4ca804f2cf993f87355972568cbaaed Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 8 Feb 2023 11:03:34 -0500 Subject: [PATCH 4/5] fix lints --- beacon_node/execution_layer/src/lib.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 50f01893d..d60082b91 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -52,7 +52,6 @@ use types::{ use types::{ ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge, }; -use warp::hyper::body::HttpBody; mod block_hash; mod engine_api; @@ -2074,23 +2073,22 @@ fn ethers_tx_to_bytes( .as_u64(); let tx = if BLOB_TX_TYPE as u64 == tx_type { - let EthersTransaction { - hash, + hash: _, nonce, - block_hash, - block_number, - transaction_index, - from, + block_hash: _, + block_number: _, + transaction_index: _, + from: _, to, value, - gas_price, + gas_price: _, gas, input, v, r, s, - transaction_type, + transaction_type: _, access_list, max_priority_fee_per_gas, max_fee_per_gas, From a68e3eac2c15189574742eddeb11d4c7ea2a92a2 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Fri, 10 Feb 2023 08:25:42 -0500 Subject: [PATCH 5/5] pr feedback --- beacon_node/execution_layer/src/lib.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index d60082b91..bbf45b183 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -14,6 +14,7 @@ pub use engine_api::{http, http::deposit_methods, http::HttpJsonRpc}; use engines::{Engine, EngineError}; pub use engines::{EngineState, ForkchoiceState}; use eth2::types::{builder_bid::SignedBuilderBid, ForkVersionedResponse}; +use ethers_core::abi::ethereum_types::FromStrRadixErr; use ethers_core::types::transaction::eip2930::AccessListItem; use ethers_core::types::{Transaction as EthersTransaction, U64}; use fork_choice::ForkchoiceUpdateParameters; @@ -2042,9 +2043,9 @@ pub enum BlobTxConversionError { /// There was an error converting the transaction from JSON. SerdeJson(serde_json::Error), /// There was an error converting the transaction from hex. - FromHexError(String), - /// The `max_fee_per_data_gas` field did not contains 32 bytes. - InvalidDataGasBytesLen, + FromHex(String), + /// There was an error converting the transaction from hex. + FromStrRadix(FromStrRadixErr), /// A `versioned_hash` did not contain 32 bytes. InvalidVersionedHashBytesLen, } @@ -2150,19 +2151,15 @@ fn ethers_tx_to_bytes( // `ethers-rs` does not yet support SSZ and therefore the blobs transaction type. // maxFeePerDataGas - let data_gas_bytes = eth2_serde_utils::hex::decode( + let max_fee_per_data_gas = Uint256::from_str_radix( other .get("maxFeePerDataGas") .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)? .as_str() .ok_or(BlobTxConversionError::MaxFeePerDataGasMissing)?, + 16, ) - .map_err(BlobTxConversionError::FromHexError)?; - let max_fee_per_data_gas = if data_gas_bytes.len() != Uint256::ssz_fixed_len() { - Err(BlobTxConversionError::InvalidDataGasBytesLen) - } else { - Ok(Uint256::from_big_endian(&data_gas_bytes)) - }?; + .map_err(BlobTxConversionError::FromStrRadix)?; // blobVersionedHashes let blob_versioned_hashes = other @@ -2177,7 +2174,7 @@ fn ethers_tx_to_bytes( .as_str() .ok_or(BlobTxConversionError::BlobVersionedHashesMissing)?, ) - .map_err(BlobTxConversionError::FromHexError)?; + .map_err(BlobTxConversionError::FromHex)?; if hash_bytes.len() != Hash256::ssz_fixed_len() { Err(BlobTxConversionError::InvalidVersionedHashBytesLen) } else {