From ae24b7cadb1c018ed72f03107472c31e80655f4d Mon Sep 17 00:00:00 2001 From: Roman Proskuryakoff Date: Fri, 14 Jun 2024 14:22:24 +0300 Subject: [PATCH 1/5] Remove signature.v parity before calculating tx hash --- crates/consensus/src/transaction/eip1559.rs | 10 +-- crates/consensus/src/transaction/eip2930.rs | 10 +-- crates/consensus/src/transaction/envelope.rs | 70 ++++++++++++++++++-- 3 files changed, 76 insertions(+), 14 deletions(-) diff --git a/crates/consensus/src/transaction/eip1559.rs b/crates/consensus/src/transaction/eip1559.rs index 585f14bfa2b..d195edb9444 100644 --- a/crates/consensus/src/transaction/eip1559.rs +++ b/crates/consensus/src/transaction/eip1559.rs @@ -305,14 +305,16 @@ impl SignableTransaction for TxEip1559 { } fn into_signed(self, signature: Signature) -> Signed { + // Drop any v chain id value to ensure the signature format is correct at the time of + // combination for an EIP-1559 transaction. V should indicate the y-parity of the + // signature. + let signature = signature.with_parity_bool(); + let mut buf = Vec::with_capacity(self.encoded_len_with_signature(&signature, false)); self.encode_with_signature(&signature, &mut buf, false); let hash = keccak256(&buf); - // Drop any v chain id value to ensure the signature format is correct at the time of - // combination for an EIP-1559 transaction. V should indicate the y-parity of the - // signature. - Signed::new_unchecked(self, signature.with_parity_bool(), hash) + Signed::new_unchecked(self, signature, hash) } } diff --git a/crates/consensus/src/transaction/eip2930.rs b/crates/consensus/src/transaction/eip2930.rs index 8ae2a111098..85bb370c178 100644 --- a/crates/consensus/src/transaction/eip2930.rs +++ b/crates/consensus/src/transaction/eip2930.rs @@ -272,14 +272,16 @@ impl SignableTransaction for TxEip2930 { } fn into_signed(self, signature: Signature) -> Signed { + // Drop any v chain id value to ensure the signature format is correct at the time of + // combination for an EIP-2930 transaction. V should indicate the y-parity of the + // signature. + let signature = signature.with_parity_bool(); + let mut buf = Vec::with_capacity(self.encoded_len_with_signature(&signature, false)); self.encode_with_signature(&signature, &mut buf, false); let hash = keccak256(&buf); - // Drop any v chain id value to ensure the signature format is correct at the time of - // combination for an EIP-2930 transaction. V should indicate the y-parity of the - // signature. - Signed::new_unchecked(self, signature.with_parity_bool(), hash) + Signed::new_unchecked(self, signature, hash) } } diff --git a/crates/consensus/src/transaction/envelope.rs b/crates/consensus/src/transaction/envelope.rs index b586efffcb3..3a8d25422a2 100644 --- a/crates/consensus/src/transaction/envelope.rs +++ b/crates/consensus/src/transaction/envelope.rs @@ -366,7 +366,7 @@ mod tests { use super::*; use crate::transaction::SignableTransaction; use alloy_eips::eip2930::{AccessList, AccessListItem}; - use alloy_primitives::{hex, Address, Signature, U256}; + use alloy_primitives::{hex, Address, Parity, Signature, U256}; #[allow(unused_imports)] use alloy_primitives::{Bytes, TxKind}; use std::{fs, path::PathBuf, vec}; @@ -460,11 +460,13 @@ mod tests { assert_eq!(from, address!("A83C816D4f9b2783761a22BA6FADB0eB0606D7B2")); } - fn test_encode_decode_roundtrip>(tx: T) - where + fn test_encode_decode_roundtrip>( + tx: T, + signature: Option, + ) where Signed: Into, { - let signature = Signature::test_signature(); + let signature = signature.unwrap_or(Signature::test_signature()); let tx_signed = tx.into_signed(signature); let tx_envelope: TxEnvelope = tx_signed.into(); let encoded = tx_envelope.encoded_2718(); @@ -486,7 +488,63 @@ mod tests { input: vec![8].into(), access_list: Default::default(), }; - test_encode_decode_roundtrip(tx); + test_encode_decode_roundtrip(tx, None); + } + + #[test] + fn test_encode_decode_eip1559_parity_eip155() { + let tx = TxEip1559 { + chain_id: 1u64, + nonce: 2, + max_fee_per_gas: 3, + max_priority_fee_per_gas: 4, + gas_limit: 5, + to: Address::left_padding_from(&[6]).into(), + value: U256::from(7_u64), + input: vec![8].into(), + access_list: Default::default(), + }; + let signature = Signature::test_signature().with_parity(Parity::Eip155(42)); + test_encode_decode_roundtrip(tx, Some(signature)); + } + + #[test] + fn test_encode_decode_eip2930_parity_eip155() { + let tx = TxEip2930 { + chain_id: 1u64, + nonce: 2, + gas_price: 3, + gas_limit: 4, + to: Address::left_padding_from(&[5]).into(), + value: U256::from(6_u64), + input: vec![7].into(), + access_list: Default::default(), + }; + let signature = Signature::test_signature().with_parity(Parity::Eip155(42)); + test_encode_decode_roundtrip(tx, Some(signature)); + } + + #[test] + #[ignore] + fn test_encode_decode_eip4844_parity_eip155() { + let tx = TxEip4844 { + chain_id: 1, + nonce: 100, + max_fee_per_gas: 50_000_000_000, + max_priority_fee_per_gas: 1_000_000_000_000, + gas_limit: 1_000_000, + to: Address::random(), + value: U256::from(10e18), + input: Bytes::new(), + access_list: AccessList(vec![AccessListItem { + address: Address::random(), + storage_keys: vec![B256::random()], + }]), + blob_versioned_hashes: vec![B256::random()], + max_fee_per_blob_gas: 0, + }; + let signature = Signature::test_signature().with_parity(Parity::Eip155(42)); + test_encode_decode_roundtrip(tx, Some(signature)); } #[test] @@ -504,7 +562,7 @@ mod tests { storage_keys: vec![B256::left_padding_from(&[9])], }]), }; - test_encode_decode_roundtrip(tx); + test_encode_decode_roundtrip(tx, None); } #[test] From acd0eec22933d92eb870c76a844917471cf5aeb0 Mon Sep 17 00:00:00 2001 From: Roman Proskuryakoff Date: Fri, 14 Jun 2024 14:56:03 +0300 Subject: [PATCH 2/5] Fix tx_hash parity for eip4844 --- crates/consensus/src/transaction/eip4844.rs | 30 ++++++----- crates/consensus/src/transaction/envelope.rs | 57 +++++++++++++++++++- 2 files changed, 73 insertions(+), 14 deletions(-) diff --git a/crates/consensus/src/transaction/eip4844.rs b/crates/consensus/src/transaction/eip4844.rs index f93273314cb..670c7d9c6b9 100644 --- a/crates/consensus/src/transaction/eip4844.rs +++ b/crates/consensus/src/transaction/eip4844.rs @@ -259,16 +259,18 @@ impl SignableTransaction for TxEip4844Variant { } fn into_signed(self, signature: Signature) -> Signed { + // Drop any v chain id value to ensure the signature format is correct at the time of + // combination for an EIP-4844 transaction. V should indicate the y-parity of the + // signature. + let signature = signature.with_parity_bool(); + let payload_length = 1 + self.fields_len() + signature.rlp_vrs_len(); let mut buf = Vec::with_capacity(payload_length); // we use the inner tx to encode the fields self.tx().encode_with_signature(&signature, &mut buf, false); let hash = keccak256(&buf); - // Drop any v chain id value to ensure the signature format is correct at the time of - // combination for an EIP-4844 transaction. V should indicate the y-parity of the - // signature. - Signed::new_unchecked(self, signature.with_parity_bool(), hash) + Signed::new_unchecked(self, signature, hash) } fn encode_for_signing(&self, out: &mut dyn alloy_rlp::BufMut) { @@ -613,14 +615,16 @@ impl SignableTransaction for TxEip4844 { } fn into_signed(self, signature: Signature) -> Signed { + // Drop any v chain id value to ensure the signature format is correct at the time of + // combination for an EIP-4844 transaction. V should indicate the y-parity of the + // signature. + let signature = signature.with_parity_bool(); + let mut buf = Vec::with_capacity(self.encoded_len_with_signature(&signature, false)); self.encode_with_signature(&signature, &mut buf, false); let hash = keccak256(&buf); - // Drop any v chain id value to ensure the signature format is correct at the time of - // combination for an EIP-4844 transaction. V should indicate the y-parity of the - // signature. - Signed::new_unchecked(self, signature.with_parity_bool(), hash) + Signed::new_unchecked(self, signature, hash) } fn encode_for_signing(&self, out: &mut dyn alloy_rlp::BufMut) { @@ -841,6 +845,11 @@ impl SignableTransaction for TxEip4844WithSidecar { } fn into_signed(self, signature: Signature) -> Signed { + // Drop any v chain id value to ensure the signature format is correct at the time of + // combination for an EIP-4844 transaction. V should indicate the y-parity of the + // signature. + let signature = signature.with_parity_bool(); + let mut buf = Vec::with_capacity(self.tx.encoded_len_with_signature(&signature, false)); // The sidecar is NOT included in the signed payload, only the transaction fields and the // type byte. Include the type byte. @@ -850,10 +859,7 @@ impl SignableTransaction for TxEip4844WithSidecar { self.tx.encode_with_signature(&signature, &mut buf, false); let hash = keccak256(&buf); - // Drop any v chain id value to ensure the signature format is correct at the time of - // combination for an EIP-4844 transaction. V should indicate the y-parity of the - // signature. - Signed::new_unchecked(self, signature.with_parity_bool(), hash) + Signed::new_unchecked(self, signature, hash) } fn payload_len_for_signature(&self) -> usize { diff --git a/crates/consensus/src/transaction/envelope.rs b/crates/consensus/src/transaction/envelope.rs index 3a8d25422a2..93164122876 100644 --- a/crates/consensus/src/transaction/envelope.rs +++ b/crates/consensus/src/transaction/envelope.rs @@ -365,7 +365,10 @@ impl Encodable2718 for TxEnvelope { mod tests { use super::*; use crate::transaction::SignableTransaction; - use alloy_eips::eip2930::{AccessList, AccessListItem}; + use alloy_eips::{ + eip2930::{AccessList, AccessListItem}, + eip4844::BlobTransactionSidecar, + }; use alloy_primitives::{hex, Address, Parity, Signature, U256}; #[allow(unused_imports)] use alloy_primitives::{Bytes, TxKind}; @@ -525,7 +528,6 @@ mod tests { } #[test] - #[ignore] fn test_encode_decode_eip4844_parity_eip155() { let tx = TxEip4844 { chain_id: 1, @@ -547,6 +549,57 @@ mod tests { test_encode_decode_roundtrip(tx, Some(signature)); } + #[test] + fn test_encode_decode_eip4844_sidecar_parity_eip155() { + let tx = TxEip4844 { + chain_id: 1, + nonce: 100, + max_fee_per_gas: 50_000_000_000, + max_priority_fee_per_gas: 1_000_000_000_000, + gas_limit: 1_000_000, + to: Address::random(), + value: U256::from(10e18), + input: Bytes::new(), + access_list: AccessList(vec![AccessListItem { + address: Address::random(), + storage_keys: vec![B256::random()], + }]), + blob_versioned_hashes: vec![B256::random()], + max_fee_per_blob_gas: 0, + }; + let sidecar = BlobTransactionSidecar { + blobs: vec![[2; 131072].into()], + commitments: vec![[3; 48].into()], + proofs: vec![[4; 48].into()], + }; + let tx = TxEip4844WithSidecar { tx, sidecar }; + let signature = Signature::test_signature().with_parity(Parity::Eip155(42)); + test_encode_decode_roundtrip(tx, Some(signature)); + } + + #[test] + fn test_encode_decode_eip4844_variant_parity_eip155() { + let tx = TxEip4844 { + chain_id: 1, + nonce: 100, + max_fee_per_gas: 50_000_000_000, + max_priority_fee_per_gas: 1_000_000_000_000, + gas_limit: 1_000_000, + to: Address::random(), + value: U256::from(10e18), + input: Bytes::new(), + access_list: AccessList(vec![AccessListItem { + address: Address::random(), + storage_keys: vec![B256::random()], + }]), + blob_versioned_hashes: vec![B256::random()], + max_fee_per_blob_gas: 0, + }; + let tx = TxEip4844Variant::TxEip4844(tx); + let signature = Signature::test_signature().with_parity(Parity::Eip155(42)); + test_encode_decode_roundtrip(tx, Some(signature)); + } + #[test] fn test_encode_decode_eip2930() { let tx = TxEip2930 { From 4fc017deb091a646d6ca066ddd7c5f6043f8632b Mon Sep 17 00:00:00 2001 From: Roman Proskuryakoff Date: Fri, 14 Jun 2024 17:19:02 +0300 Subject: [PATCH 3/5] Fix review notes --- crates/consensus/src/transaction/envelope.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/consensus/src/transaction/envelope.rs b/crates/consensus/src/transaction/envelope.rs index 93164122876..bab50dcffd9 100644 --- a/crates/consensus/src/transaction/envelope.rs +++ b/crates/consensus/src/transaction/envelope.rs @@ -469,7 +469,7 @@ mod tests { ) where Signed: Into, { - let signature = signature.unwrap_or(Signature::test_signature()); + let signature = signature.unwrap_or_else(Signature::test_signature); let tx_signed = tx.into_signed(signature); let tx_envelope: TxEnvelope = tx_signed.into(); let encoded = tx_envelope.encoded_2718(); From a3fa3f8057c1aa2c5a9ccb6c798df8c5b33c831b Mon Sep 17 00:00:00 2001 From: Roman Proskuryakoff Date: Wed, 19 Jun 2024 11:33:39 +0300 Subject: [PATCH 4/5] Add signature chainid guard for a legacy tx --- crates/consensus/src/transaction/legacy.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/consensus/src/transaction/legacy.rs b/crates/consensus/src/transaction/legacy.rs index c39abfbf186..7de6515df79 100644 --- a/crates/consensus/src/transaction/legacy.rs +++ b/crates/consensus/src/transaction/legacy.rs @@ -250,6 +250,7 @@ impl SignableTransaction for TxLegacy { } fn into_signed(self, signature: Signature) -> Signed { + debug_assert_eq!(signature.v().chain_id(), self.chain_id); let mut buf = Vec::with_capacity(self.encoded_len_with_signature(&signature)); self.encode_with_signature_fields(&signature, &mut buf); let hash = keccak256(&buf); From 55d0c5b3e8d58cb63081ecc5f316eadf368f204b Mon Sep 17 00:00:00 2001 From: Roman Proskuryakoff Date: Wed, 19 Jun 2024 14:51:42 +0300 Subject: [PATCH 5/5] Revert "Add signature chainid guard for a legacy tx" This reverts commit a3fa3f8057c1aa2c5a9ccb6c798df8c5b33c831b. --- crates/consensus/src/transaction/legacy.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/consensus/src/transaction/legacy.rs b/crates/consensus/src/transaction/legacy.rs index 7de6515df79..c39abfbf186 100644 --- a/crates/consensus/src/transaction/legacy.rs +++ b/crates/consensus/src/transaction/legacy.rs @@ -250,7 +250,6 @@ impl SignableTransaction for TxLegacy { } fn into_signed(self, signature: Signature) -> Signed { - debug_assert_eq!(signature.v().chain_id(), self.chain_id); let mut buf = Vec::with_capacity(self.encoded_len_with_signature(&signature)); self.encode_with_signature_fields(&signature, &mut buf); let hash = keccak256(&buf);