Remove signature.v parity before calculating tx hash

[kpp]

Motivation

I noticed that TxEnvelope::encode/decode didn't produce the same TxEnvelope. The difference was the hash only.
I fixed the issue for TxEip1559, TxEip2930, TxEip4844, TxEip4844WithSidecar and TxEip4844Variant.

Context

Related to: #150
Fixes #897

[prestwich]

I noticed that TxEnvelope::encode/decode didn't produce the same TxEnvelope. The difference was the hash only.

can you provide a minimal repro of this so we can be sure we understand the observed behavior?

[kpp]

Sure! My minimal repro is very close to unit tests, except I used LocalWallet to get a signature for tx. You can observe the incorrect behavior by reverting my changes and running tests.

Revert:

diff --git a/crates/consensus/src/transaction/eip1559.rs b/crates/consensus/src/transaction/eip1559.rs
index d195edb9..729d811b 100644
--- a/crates/consensus/src/transaction/eip1559.rs
+++ b/crates/consensus/src/transaction/eip1559.rs
@@ -308,13 +308,13 @@ impl SignableTransaction<Signature> for TxEip1559 {
         // 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 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);
 
-        Signed::new_unchecked(self, signature, hash)
+        Signed::new_unchecked(self, signature.with_parity_bool(), hash)
     }
 }

Failed test:

---- transaction::envelope::tests::test_encode_decode_eip1559_parity_eip155 stdout ----
thread 'transaction::envelope::tests::test_encode_decode_eip1559_parity_eip155' panicked at crates/consensus/src/transaction/envelope.rs:478:9:
assertion `left == right` failed
  left: Eip1559(Signed { tx: TxEip1559 { chain_id: 1, nonce: 2, gas_limit: 5, max_fee_per_gas: 3, max_priority_fee_per_gas: 4, to: Call(0x0000000000000000000000000000000000000006), value: 7, access_list: AccessList([]), input: 0x08 }, signature: Signature { inner: ecdsa::Signature<Secp256k1>(840CFC572845F5786E702984C2A582528CAD4B49B2A10B9DB1BE7FCA9005856525E7109CEB98168D95B09B18BBF6B685130E0562F233877D492B94EEE0C5B6D1), v: Parity(true), r: 59728239767604526217550949535444980007647751110991992555989432467883920033125, s: 17143831728048845831134990513685258884275296176139135115431336905099564136145 }, hash: 0xbd9e80b9d3c7027f149869eff2037c2e3ec8045c007ff4819522f67538246e90 })
 right: Eip1559(Signed { tx: TxEip1559 { chain_id: 1, nonce: 2, gas_limit: 5, max_fee_per_gas: 3, max_priority_fee_per_gas: 4, to: Call(0x0000000000000000000000000000000000000006), value: 7, access_list: AccessList([]), input: 0x08 }, signature: Signature { inner: ecdsa::Signature<Secp256k1>(840CFC572845F5786E702984C2A582528CAD4B49B2A10B9DB1BE7FCA9005856525E7109CEB98168D95B09B18BBF6B685130E0562F233877D492B94EEE0C5B6D1), v: Parity(true), r: 59728239767604526217550949535444980007647751110991992555989432467883920033125, s: 17143831728048845831134990513685258884275296176139135115431336905099564136145 }, hash: 0xa4b327fdd449fb3dcae8511f0f1c32af0d6b24c6199a67a4b099f957c9478bc2 })

[prestwich]

thank you! I'll be looking at this and #897 this morning

[prestwich]

per #897 (comment) can you add the following to the legacy tx into_signed

    fn into_signed(self, signature: Signature) -> Signed<Self> {
        debug_assert_eq!(signature.v().chain_id(), self.chain_id);       

this prevents most issues while we move towards a larger refactor

[kpp]

per #897 (comment) can you add the following to the legacy tx into_signed

    fn into_signed(self, signature: Signature) -> Signed<Self> {
        debug_assert_eq!(signature.v().chain_id(), self.chain_id);       

this prevents most issues while we move towards a larger refactor

Done

[kpp]

I did but CI failed with:

thread 'transaction::envelope::tests::test_serde_roundtrip_legacy' panicked at crates/consensus/src/transaction/legacy.rs:253:9:
assertion `left == right` failed
  left: None
 right: Some(1)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

So I am reverting that assert.

[kpp]

Hi! Is there anything else I can do in this PR?

[kpp]

Hello! Is there any progress? @prestwich

[kpp]

@onbjerg @prestwich @gakonst @mattsse @Evalir @DaniPopes

ping

[mattsse]

ty!