tx: consolidate generic tx capabilities #2993

gabrocheleau · 2023-08-26T18:38:19Z

This PR resolves #2987

codecov · 2023-08-30T15:22:47Z

Codecov Report

Merging #2993 (339dc1f) into master (d6d9391) will decrease coverage by 0.08%.
The diff coverage is 95.53%.

Additional details and impacted files

Flag	Coverage Δ
block	`88.73% <ø> (ø)`
blockchain	`92.58% <ø> (ø)`
client	`87.35% <ø> (-0.01%)`	⬇️
common	`98.18% <ø> (ø)`
ethash	`∅ <ø> (∅)`
evm	`70.43% <ø> (ø)`
rlp	`∅ <ø> (∅)`
statemanager	`84.46% <ø> (ø)`
trie	`89.82% <ø> (-0.58%)`	⬇️
tx	`96.38% <95.53%> (+0.41%)`	⬆️
util	`?`
vm	`79.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

acolytec3 · 2023-08-30T17:45:35Z

General question on this PR. What is the advantage of declaring these helpers as
export function serialize(this: TypedTransactionEIP2930): Uint8Array and then using
return EIP2930.serialize.bind(this)() inside each Transaction subclass over just declaring the functions as generic pure functions like:
export function serialize(tx: TypedTransactionEIP2930): Uint8Array
and then calling them with
return EIP2930.serialize(this)

If there's some SO thread of something that explains the motivation for this, I'd be happy to read if that's easier to do.

acolytec3

This looks great!

acolytec3 · 2023-08-30T18:04:16Z

packages/tx/src/capabilities/generic.ts

+import { SECP256K1_ORDER_DIV_2, bigIntToUnpaddedBytes, ecrecover } from '@ethereumjs/util'
+import { keccak256 } from 'ethereum-cryptography/keccak.js'
+
+import { Capability, type TypedTransaction } from '../types.js'


Ah, I didn't know you could import a type inside a general import statement like this. Neat!

Found that out recently too!

holgerd77 · 2023-08-31T08:30:29Z

Super great work, thanks a lot! 🤩

I would really like to put an emphasis here that we get the associations right, so to expand: that we a) name the capability files correctly and b) associate/integrate the correct methods, so that this whole things directly structure things in a right way along, I have some remarks on this, will do along the code.

The most basic specific thing here: EIP-2930 is really only this specific (very much unused) transaction type with access lists. So this should - if including anything - only include access list specific functionality (which we have already extracted in a similar way in former work in util.ts and - while it would likely make sense to align this - we likely want/need to skip for now due to backwards compatibility reasons).

All the generic tx type specific functionality has been introduced in EIP-2718 and I would therefore strongly lean towards also labeling the capability file this way. Your local transaction type at the beginning of the file is actually the very definition of EIP-2718 typed transactions 🙂:

type TypedTransactionEIP2930 = Exclude<TypedTransaction, Transaction[TransactionType.Legacy]>

Will do some more comments on the files.

holgerd77

Ok, gone through the list of capability methods with various comments.

holgerd77 · 2023-08-31T08:40:16Z

packages/tx/src/capabilities/eip1559.ts

+
+type TypedTransactionEIP1559 =
+  | Transaction[TransactionType.BlobEIP4844]
+  | Transaction[TransactionType.FeeMarketEIP1559]


Is it generally possible to type in a way that these methods can also be called by custom transaction type implementations? 🤔 So without the need to predefine all the existing tx types which come into question atm here?

Also coming to surface here again: we really really need to be super careful to make the distinction clear between "EIP-1559 -> the specific transaction" and "Capabilities derived from EIP-1559".

If we do not make this very clear things will get blurry over time and mixed up all the time I fear. In this context the naming above "TypedTransactionEIP1559" is too close to the wrong association thing and we should rename these throughout the files for clarity.

Just need to think right now myself: hmm. I would find EIP1559CompatibleTx already better. 🤔 Not sure if it is the last word of wisdom here though.

(maybe this whole discussion interplays with an eventually possible re-typing though)

I'll think about this a bit more about how to architect this. You're right that these "capabilities" are currently tightly paired with "transaction types", whereas they shouldn't. I'm currently wondering if we make use of the existing tx Capability enum. Will reflect a bit more and follow up...

Here's what I feel is a more sound approach: #3010, curious to hear your thoughts!

holgerd77 · 2023-08-31T08:42:29Z

packages/tx/src/capabilities/eip1559.ts

+  const inclusionFeePerGas = prio < maxBase ? prio : maxBase
+  const gasPrice = inclusionFeePerGas + baseFee
+  return tx.gasLimit * gasPrice + tx.value
+}


Ok, I will just go through the methods one-by-one. Think this initial structure is really crucial so that things can be build cleanly on top of this work.

So: getUpfrontCost() should be ok here.

holgerd77 · 2023-08-31T08:45:02Z

packages/tx/src/capabilities/eip2930.ts

+  }
+
+  return cost
+}


I would cautiously say this one is actually well placed in a eip2930.ts capability file (?).

And I would - also cautiously - say that the "other" version of getDataFee() would actually be a candidate for a legacy.ts capability file?

Also to re-iterate here: so legacy.ts also would include capabilities which were introduced by the legacy transaction and would therefore be wider than the specific legacy transaction itself.

Since also typed transactions have just inherited specific functionality from the legacy transaction, it would very much be ok if typed transactions call into the legacy.ts capability file (same logic as an EIP4844 tx calls into the EIP1559 capability file).

Distinction between generic.ts and legacy.ts would just be that legacy.ts only includes functionality (like getUpfrontCost()) which then changed along the introduction of a new transaction type.

When thinking about it: when staying in this logic it might be even more consistent to skip generic.ts alltogether and just do legacy.ts and move everything being now in generic.ts over there. These is even more consistent with this capabilities-thinking.

holgerd77 · 2023-08-31T08:53:33Z

packages/tx/src/capabilities/eip2930.ts

+  const base = tx.raw()
+  const txTypeBytes = hexToBytes('0x' + tx.type.toString(16).padStart(2, '0'))
+  return concatBytes(txTypeBytes, RLP.encode(base))
+}


Yeah, so this is definitely an EIP-2718 thing.

holgerd77 · 2023-08-31T08:55:40Z

packages/tx/src/capabilities/eip2930.ts

+
+export function getHashedMessageToSign(tx: TypedTransactionEIP2930): Uint8Array {
+  return keccak256(tx.getMessageToSign())
+}


-> EIP-2718.

(just to mention: there is also a legacy version of this)

holgerd77 · 2023-08-31T08:56:47Z

packages/tx/src/capabilities/generic.ts

+  } else {
+    return true
+  }
+}


Yes, guess ok, respectively: -> legacy.ts

holgerd77 · 2023-08-31T08:58:39Z

packages/tx/src/capabilities/generic.ts

+  }
+
+  return keccak256(tx.serialize())
+}


If I see correctly this is actually also an EIP-2718 thing, so this specific implementation (sorry: I suggested this placing wrongly initially)

holgerd77 · 2023-08-31T08:59:17Z

packages/tx/src/capabilities/generic.ts

+    )
+    throw new Error(msg)
+  }
+}


-> legacy.ts

(I think I am getting more and more a fan of the idea to remove generic.ts while going through here)

holgerd77 · 2023-08-31T08:59:41Z

packages/tx/src/capabilities/generic.ts

+    const msg = errorMsg(tx, 'The y-parity of the transaction should either be 0 or 1')
+    throw new Error(msg)
+  }
+}


This is actually an EIP-2718 thing and definitely not generic.

holgerd77 · 2023-08-31T09:01:34Z

packages/tx/src/capabilities/generic.ts

+    const msg = errorMsg(tx, 'Invalid Signature')
+    throw new Error(msg)
+  }
+}


-> legacy.ts

gabrocheleau added PR state: WIP type: refactor package: tx labels Aug 26, 2023

tx: consolidate generic tx capabilities

dd5c76f

gabrocheleau force-pushed the tx/consolidate-functionalities branch from 414afdc to dd5c76f Compare August 30, 2023 14:41

tx: refactor highS and YParity validation into generic

fa6549b

gabrocheleau added 3 commits August 30, 2023 11:52

tx: refactor getDataFee

78dc797

tx: refactor getUpfrontCost capability

418541f

tx: refactor getHashedMessageToSign and serialize methods

84280e7

gabrocheleau marked this pull request as ready for review August 30, 2023 17:23

gabrocheleau added PR state: needs review and removed PR state: WIP labels Aug 30, 2023

tx: refactor usage of this

339dc1f

acolytec3 approved these changes Aug 30, 2023

View reviewed changes

acolytec3 merged commit f5c6769 into master Aug 30, 2023

holgerd77 deleted the tx/consolidate-functionalities branch August 30, 2023 20:26

holgerd77 reviewed Aug 31, 2023

View reviewed changes

gabrocheleau mentioned this pull request Sep 5, 2023

tx: improve tx capability handling #3010

Merged

jochem-brouwer mentioned this pull request Oct 9, 2024

TX refactor PR #3733

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tx: consolidate generic tx capabilities #2993

tx: consolidate generic tx capabilities #2993

gabrocheleau commented Aug 26, 2023

codecov bot commented Aug 30, 2023 •

edited

Loading

acolytec3 commented Aug 30, 2023

acolytec3 left a comment

acolytec3 Aug 30, 2023

gabrocheleau Aug 30, 2023

holgerd77 commented Aug 31, 2023

holgerd77 left a comment

holgerd77 Aug 31, 2023

gabrocheleau Sep 5, 2023

gabrocheleau Sep 5, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

holgerd77 Aug 31, 2023

tx: consolidate generic tx capabilities #2993

tx: consolidate generic tx capabilities #2993

Conversation

gabrocheleau commented Aug 26, 2023

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

acolytec3 commented Aug 30, 2023

acolytec3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Aug 31, 2023

holgerd77 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

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

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

codecov bot commented Aug 30, 2023 •

edited

Loading