feat: migrate to alloy TxLegacy #9593

leruaa · 2024-07-17T20:23:38Z

mattsse

amazing work so far!

crates/primitives/src/transaction/legacy.rs

mattsse · 2024-07-17T21:01:25Z

crates/primitives/src/transaction/mod.rs

@@ -1172,7 +1176,8 @@ impl TransactionSigned {
    /// only `true`.
    pub(crate) fn payload_len_inner(&self) -> usize {
        match &self.transaction {
-            Transaction::Legacy(legacy_tx) => legacy_tx.payload_len_with_signature(&self.signature),
+            Transaction::Legacy(legacy_tx) => legacy_tx
+                .encoded_len_with_signature(&self.signature.as_alloy_signature(legacy_tx.chain_id)),


maybe worth exploring if we can phase out the reth-prim signature type?

That would be interesting, but can it be done in a followup?

crates/primitives/src/transaction/signature.rs

crates/storage/codecs/src/alloy/transaction.rs

leruaa · 2024-07-18T07:31:05Z

@mattsse the test test_sending_invalid_transactions is failling because it's creating an invalid signature, but now with the conversion to alloy Signature we create a k256::ecdsa::Signature and it fails.

Should as_alloy_signature() returns a Result and also update the callers to also returns a Result?

Or maybe it's time to migrate to alloy signature per your comment above: #9593 (comment), but it wouldn't be possible to create invalid signatures anymore.

mattsse · 2024-07-18T09:50:41Z

but now with the conversion to alloy Signature we create a k256::ecdsa::Signature and it fails.

can you elaborate? unclear what fails, the default alloy Signature type is feature gated, imo we should use Signature<()> here

it wouldn't be possible to create invalid signatures anymore.

I think with Signature<()> this would still be possible, but perhaps we need to tune something for this type?

leruaa · 2024-07-18T11:24:35Z

@mattsse Unfortunately alloy-primitives "k256" feature is enabled due to this in alloy-signer: https://github.com/alloy-rs/alloy/blob/423a4c53c1bffdfb2acca2daee60215194f7711d/crates/signer/Cargo.toml#L22C7-L22C18

And also this in revm-primitives: https://github.com/leruaa/revm/blob/1adc908dfa924a9cd5673b526ecbaaec7298d60f/crates/primitives/Cargo.toml#L25

leruaa · 2024-07-19T09:09:45Z

To add a bit of context:

The issue with the test test_sending_invalid_transactions is that it create an invalid signature here:

reth/crates/net/network/tests/it/txgossip.rs

Line 132 in 4a19161

    
           let tx = TransactionSigned::from_transaction_and_signature(tx.into(), Default::default());

Deeper in the code this signature is converted to an alloy Signature, but as the k256 feature is on, a k256::ecdsa::Signature is needed but can't be created from an invalid signature.

I fixed test_sending_invalid_transactions() by creating a valid signature, but more generally I guess we should be able to convert reth signatures to alloy ones even if they are invalid.

I think the main issue here is that the k256 feature on alloy-primitives is not additive (i.e. it remove the Signature<()> type when activated), and that is an issue in reth because we won't be able to make it off (it's needed in revm).

So I think we need to update alloy-primitives to never remove Signature<()> and rename Signature when k256 feature is enabled to something else.

@mattsse wdyt? I hope to have been clear :)

mattsse · 2024-07-19T09:31:16Z

i.e. it remove the Signature<()> type when activated), and that is an issue in reth because we won't be able to make it off (it's needed in revm).

yeah, tbh I find this behaviour in alloy-primitives a bit strange because this obfuscates things.
I'd like if we could add a simple alias for Signature<()> to alloy-primitives, like RawSignature or something,

but maybe you can use a type alias for this here for now?

we also need to change these functions

https://github.com/alloy-rs/alloy/blob/main/crates/consensus/src/transaction/legacy.rs#L120-L120

so they accept any Signature<S> because we only need the rsv fields

leruaa · 2024-07-19T09:51:49Z

Unfortunately we can't use a type alias because Signature<S> is private.

mattsse · 2024-07-19T09:53:43Z

this is kinda stupid lol

let's change that

we also need an infallible constructor for Signature<S>::rsv

I find this type a bit strange...

mattsse · 2024-08-01T11:23:00Z

@leruaa just checking in, is this now only blocked by a new alloy release?

mattsse

this is a pretty huge unlock and should unblock further alloy changes.

ptal at the codec things @joshieDo

joshieDo

LGTM, tried locally with no issues

mattsse · 2024-08-05T18:11:14Z

sweet, blocking this to go in after tmrw's release

really nice work @leruaa this unblocks us from migrating the other variants aswell

…nsaction now (following bad merge)

crates/primitives/src/transaction/mod.rs

mattsse

alright, let's finally do this.

tysm for kickstarting this.

only have two more qs

crates/primitives/src/transaction/mod.rs

mattsse · 2024-08-21T07:14:11Z

crates/primitives/src/transaction/signature.rs

-    #[test]
-    fn test_payload_len_with_eip155_chain_id() {
-        // Select 1 as an arbitrary nonzero value for R and S, as v() always returns 0 for (0, 0).
-        let signature = Signature { r: U256::from(1), s: U256::from(1), odd_y_parity: false };
-
-        assert_eq!(3, signature.payload_len_with_eip155_chain_id(None));
-        assert_eq!(3, signature.payload_len_with_eip155_chain_id(Some(1)));
-        assert_eq!(4, signature.payload_len_with_eip155_chain_id(Some(47)));
-    }


why was this test removed?

The corresponding method payload_len_with_eip155_chain_id() has been removed.

mattsse · 2024-08-21T07:14:17Z

crates/primitives/src/transaction/signature.rs

-    #[test]
-    fn test_encode_and_decode_with_eip155_chain_id() {


encode_with_eip155_chain_id() has also been removed

mattsse

sorry I have to ask more questions because I'm a bit lost with the signature types

also would appreciate a closer look by @Rjected

mattsse · 2024-08-21T12:30:53Z

crates/primitives/src/transaction/signature.rs

+        alloy_primitives::Parity::NonEip155(self.odd_y_parity)
+    }


hmm unsure if this is correct.

This was to reflect the "else" case here:

reth/crates/primitives/src/transaction/signature.rs

Lines 88 to 103 in ae8ce20

pub fn v(&self, chain_id: Option<u64>) -> u64 {

if let Some(chain_id) = chain_id {

// EIP-155: v = {0, 1} + CHAIN_ID * 2 + 35

self.odd_y_parity as u64 + chain_id * 2 + 35

} else {

#[cfg(feature = "optimism")]

// pre bedrock system transactions were sent from the zero address as legacy

// transactions with an empty signature

//

// NOTE: this is very hacky and only relevant for op-mainnet pre bedrock

if *self == Self::optimism_deposit_tx_signature() {

return 0

}

self.odd_y_parity as u64 + 27

}

}

mattsse · 2024-08-21T12:33:19Z

crates/primitives/src/transaction/signature.rs

+    /// Returns a signature with the given chain ID applied to the `v` value.
+    pub(crate) fn as_signature_with_eip155_parity(
+        &self,
+        chain_id: Option<u64>,
+    ) -> SignatureWithParity {
+        let parity = match chain_id {
+            Some(chain_id) => EncodableSignature::v(self).with_chain_id(chain_id),
+            None => EncodableSignature::v(self),
+        };
+
+        SignatureWithParity::new(self.r(), self.s(), parity)


why do we need this?

I did that to encode the tx + signature the exact same way as before, in encode_with_signature():

reth/crates/primitives/src/transaction/legacy.rs

Lines 94 to 101 in ae8ce20

pub(crate) fn encode_with_signature(&self, signature: &Signature, out: &mut dyn bytes::BufMut) {

let payload_length =

self.fields_len() + signature.payload_len_with_eip155_chain_id(self.chain_id);

let header = Header { list: true, payload_length };

header.encode(out);

self.encode_fields(out);

signature.encode_with_eip155_chain_id(out, self.chain_id);

}

So I needed a way to carry the chain_id.

leruaa · 2024-08-21T14:46:15Z

I realize this is not taken into account anymore:

reth/crates/primitives/src/transaction/signature.rs

Lines 93 to 100 in ae8ce20

    
           #[cfg(feature = "optimism")] 
        
           // pre bedrock system transactions were sent from the zero address as legacy 
        
           // transactions with an empty signature 
        
           // 
        
           // NOTE: this is very hacky and only relevant for op-mainnet pre bedrock 
        
           if *self == Self::optimism_deposit_tx_signature() { 
        
               return 0 
        
           }

Let me add a commit

mattsse

lgtm, let's send it this finally @klkvr

feat: migrate to alloy TxLegacy

f561f8e

leruaa requested review from mattsse, joshieDo, rakita, Rjected, emhane and gakonst as code owners July 17, 2024 20:23

leruaa added 2 commits July 17, 2024 20:29

fix: move TxLegacy bitflag_encoded_bytes test for optimism

4e7940c

fix: add optimism feature

8eacec3

leruaa marked this pull request as draft July 17, 2024 20:52

mattsse requested changes Jul 17, 2024

View reviewed changes

leruaa added 5 commits July 17, 2024 21:51

fix: typo in as_alloy_signature()

fdb5636

fix remove useless derive

51f257c

fix: Don't put EIP-155 parity to create signatures for legacy txs

d1aadb1

Merge branch 'main' into alloy_tx_legacy

2667755

fix: update AlloyTxLegacy Compact impl

9ea88a6

fix: set a valid signature for test_sending_invalid_transactions()

a51f350

leruaa mentioned this pull request Jul 19, 2024

feat: expose Signature<T> alloy-rs/core#689

Closed

3 tasks

leruaa added 4 commits July 21, 2024 18:50

fix: set correct parity

c9749c9

feat: impl EncodableSignature for Signature

92450b0

Merge branch 'main' into alloy_tx_legacy

e72d217

fix: revert modified test

622f1e7

mattsse added the C-debt Refactor of code section that is hard to understand or maintain label Aug 5, 2024

mattsse reviewed Aug 5, 2024

View reviewed changes

joshieDo approved these changes Aug 5, 2024

View reviewed changes

mattsse added the S-blocked This cannot more forward until something else changes label Aug 5, 2024

leruaa mentioned this pull request Aug 11, 2024

feat: migrate to alloy TxEip1559 #10262

Merged

leruaa added 4 commits August 17, 2024 00:43

Merge remote-tracking branch 'origin/main' into alloy_tx_legacy

194b873

fix: remove add_arbitrary_tests on Transaction as they are on MockTra…

b68146b

…nsaction now (following bad merge)

fix: same for TransactionSignedNoHash

cd45602

fix: fmt

85d840e

emhane requested changes Aug 17, 2024

View reviewed changes

crates/primitives/src/transaction/mod.rs Outdated Show resolved Hide resolved

fix: fmt

10b1ed9

emhane approved these changes Aug 20, 2024

View reviewed changes

mattsse requested changes Aug 21, 2024

View reviewed changes

nit: move mocks in tests mod

e40b470

mattsse reviewed Aug 21, 2024

View reviewed changes

leruaa and others added 7 commits August 21, 2024 16:47

fix: handle optimism pre bedrock system transactions

9eb604e

fix: embed chain_id in optimism deposit tx signature

5b89127

rm impl EncodableSignature for Signature

2675355

Merge branch 'main' into pr/leruaa/9593

7bf8e72

fmt

2503b78

Merge branch 'main' into alloy_tx_legacy

d3d8a7d

rm mock types

a8d154b

mattsse approved these changes Aug 30, 2024

View reviewed changes

klkvr added this pull request to the merge queue Aug 30, 2024

Merged via the queue into paradigmxyz:main with commit 71e0178 Aug 30, 2024
34 checks passed

This was referenced Aug 30, 2024

use alloy transaction types #9484

Closed

eip7702: use alloy TxEip7702 #10617

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate to alloy TxLegacy #9593

feat: migrate to alloy TxLegacy #9593

leruaa commented Jul 17, 2024

mattsse left a comment

mattsse Jul 17, 2024

leruaa Aug 4, 2024

leruaa commented Jul 18, 2024 •

edited

Loading

mattsse commented Jul 18, 2024

leruaa commented Jul 18, 2024 •

edited

Loading

leruaa commented Jul 19, 2024

mattsse commented Jul 19, 2024 •

edited

Loading

leruaa commented Jul 19, 2024

mattsse commented Jul 19, 2024 •

edited

Loading

mattsse commented Aug 1, 2024

mattsse left a comment

joshieDo left a comment

mattsse commented Aug 5, 2024 •

edited

Loading

mattsse left a comment

mattsse Aug 21, 2024

leruaa Aug 21, 2024

mattsse Aug 21, 2024

leruaa Aug 21, 2024

mattsse left a comment

mattsse Aug 21, 2024

leruaa Aug 21, 2024

mattsse Aug 21, 2024

leruaa Aug 21, 2024

leruaa commented Aug 21, 2024

mattsse left a comment

	pub fn v(&self, chain_id: Option<u64>) -> u64 {
	if let Some(chain_id) = chain_id {
	// EIP-155: v = {0, 1} + CHAIN_ID * 2 + 35
	self.odd_y_parity as u64 + chain_id * 2 + 35
	} else {
	#[cfg(feature = "optimism")]
	// pre bedrock system transactions were sent from the zero address as legacy
	// transactions with an empty signature
	//
	// NOTE: this is very hacky and only relevant for op-mainnet pre bedrock
	if *self == Self::optimism_deposit_tx_signature() {
	return 0
	}
	self.odd_y_parity as u64 + 27
	}
	}

	pub(crate) fn encode_with_signature(&self, signature: &Signature, out: &mut dyn bytes::BufMut) {
	let payload_length =
	self.fields_len() + signature.payload_len_with_eip155_chain_id(self.chain_id);
	let header = Header { list: true, payload_length };
	header.encode(out);
	self.encode_fields(out);
	signature.encode_with_eip155_chain_id(out, self.chain_id);
	}

feat: migrate to alloy TxLegacy #9593

feat: migrate to alloy TxLegacy #9593

Conversation

leruaa commented Jul 17, 2024

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leruaa commented Jul 18, 2024 • edited Loading

mattsse commented Jul 18, 2024

leruaa commented Jul 18, 2024 • edited Loading

leruaa commented Jul 19, 2024

mattsse commented Jul 19, 2024 • edited Loading

leruaa commented Jul 19, 2024

mattsse commented Jul 19, 2024 • edited Loading

mattsse commented Aug 1, 2024

mattsse left a comment

Choose a reason for hiding this comment

joshieDo left a comment

Choose a reason for hiding this comment

mattsse commented Aug 5, 2024 • edited Loading

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leruaa commented Aug 21, 2024

mattsse left a comment

Choose a reason for hiding this comment

leruaa commented Jul 18, 2024 •

edited

Loading

leruaa commented Jul 18, 2024 •

edited

Loading

mattsse commented Jul 19, 2024 •

edited

Loading

mattsse commented Jul 19, 2024 •

edited

Loading

mattsse commented Aug 5, 2024 •

edited

Loading