Remove signature.v parity before calculating tx hash (#893)

* Remove signature.v parity before calculating tx hash

* Fix tx_hash parity for eip4844

* Fix review notes

* Add signature chainid guard for a legacy tx

* Revert "Add signature chainid guard for a legacy tx"

This reverts commit a3fa3f8.

crates/consensus/src/transaction/eip1559.rs

@@ -305,14 +305,16 @@ impl SignableTransaction<Signature> for TxEip1559 {
     }
 
     fn into_signed(self, signature: Signature) -> Signed<Self> {
+        // 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)
     }
 }
 

crates/consensus/src/transaction/eip2930.rs

@@ -272,14 +272,16 @@ impl SignableTransaction<Signature> for TxEip2930 {
     }
 
     fn into_signed(self, signature: Signature) -> Signed<Self> {
+        // 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)
     }
 }
 

crates/consensus/src/transaction/eip4844.rs

@@ -268,16 +268,18 @@ impl SignableTransaction<Signature> for TxEip4844Variant {
     }
 
     fn into_signed(self, signature: Signature) -> Signed<Self> {
+        // 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)
     }
 }
 
@@ -617,14 +619,16 @@ impl SignableTransaction<Signature> for TxEip4844 {
     }
 
     fn into_signed(self, signature: Signature) -> Signed<Self> {
+        // 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)
     }
 }
 
@@ -847,6 +851,11 @@ impl SignableTransaction<Signature> for TxEip4844WithSidecar {
     }
 
     fn into_signed(self, signature: Signature) -> Signed<Self, Signature> {
+        // 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.
@@ -856,10 +865,7 @@ impl SignableTransaction<Signature> 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)
     }
 }
 

crates/consensus/src/transaction/envelope.rs

@@ -430,8 +430,11 @@ impl Transaction for TxEnvelope {
 mod tests {
     use super::*;
     use crate::transaction::SignableTransaction;
-    use alloy_eips::eip2930::{AccessList, AccessListItem};
-    use alloy_primitives::{hex, Address, Signature, U256};
+    use alloy_eips::{
+        eip2930::{AccessList, AccessListItem},
+        eip4844::BlobTransactionSidecar,
+    };
+    use alloy_primitives::{hex, Address, Parity, Signature, U256};
     #[allow(unused_imports)]
     use alloy_primitives::{Bytes, TxKind};
     use std::{fs, path::PathBuf, vec};
@@ -525,11 +528,13 @@ mod tests {
         assert_eq!(from, address!("A83C816D4f9b2783761a22BA6FADB0eB0606D7B2"));
     }
 
-    fn test_encode_decode_roundtrip<T: SignableTransaction<Signature>>(tx: T)
-    where
+    fn test_encode_decode_roundtrip<T: SignableTransaction<Signature>>(
+        tx: T,
+        signature: Option<Signature>,
+    ) where
         Signed<T>: Into<TxEnvelope>,
     {
-        let signature = 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();
@@ -551,7 +556,113 @@ 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]
+    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]
+    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]
@@ -569,7 +680,7 @@ mod tests {
                 storage_keys: vec![B256::left_padding_from(&[9])],
             }]),
         };
-        test_encode_decode_roundtrip(tx);
+        test_encode_decode_roundtrip(tx, None);
     }
 
     #[test]