Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transaction Type representation in the Merge #2608

Closed
protolambda opened this issue Sep 17, 2021 · 14 comments
Closed

Transaction Type representation in the Merge #2608

protolambda opened this issue Sep 17, 2021 · 14 comments
Labels
Bellatrix CL+EL Merge

Comments

@protolambda
Copy link
Contributor

protolambda commented Sep 17, 2021

Problem

The transaction typing forward-compatibility is broken:
due to double transaction type identification, and the execution engine only recognizing the first,
introducing additional types to the current Union definition is not possible.

Background

The current Merge spec defines transactions and payloads as:

OpaqueTransaction = ByteList[MAX_BYTES_PER_OPAQUE_TRANSACTION]  # typed transaction envelope: TransactionType || TransactionPayload
Transaction = Union[OpaqueTransaction]  # Union can introduce more union options, but *only consensus layer would be aware*
transactions: List[Transaction, MAX_TRANSACTIONS_PER_PAYLOAD]

So in JSON this looks like:

{
transactions: [
  {selector: 0, value: "0x??...."},  // legacy transaction with RLP byte in range 0xc0 <= id <= 0xfe
  {selector: 0, value: "0x01...."},  // eip-2930 (optional access lists) in OpaqueTransaction
  {selector: 0, value: "0x02...."},  // eip-1559 in OpaqueTransaction
  
  {selector: 1, value: {"foo": 123, "bar": 42}},  // if we were to add a FooBar type, i.e. Union[OpaqueTransaction, FooBar] 
]
}

The Merge API then just uses:

  • transactions: Array of DATA - Array of transaction objects,
    each object is a byte list (DATA) representing TransactionType || TransactionPayload or LegacyTransaction
    as defined in EIP-2718

Read: Merge API uses a list of OpaqueTransaction, not a list of Union[OpaqueTransaction]

And the block_hash (execution layer hash, same as eth1 today) is constructed by hashing the header,
which includes a transactions-trie, which is built from EIP-2718 transactions, not from Transaction with Union selector data.


"Why do we even have this Union?" you may ask, well:

The benefit of Union:

  • Merkleize the selector and value as two separate binary tree nodes; elegant proof to get the type, then handle the value subtree appropriately
  • Option to specify other transactions in the future as SSZ type (nice merkleization of transaction contents, no RLP, etc.)

The problems of Union:

  • selector byte and EIP-2718 byte are different, can be confusing
  • Not specified how additional SSZ typed transactions would make their way into the execution engine, or even affect the transaction trie or block hash

This is kind of messy, and for new things like Rollups using the same Execution Engine API,
we definitely want typed transactions to just work and not break the next hardfork after the Merge.

Proposal to fix

There are a few ways this can improve:

  1. Translate EIP-2718 transaction IDs into the Union type, and translate back before insertion into execution layer
  2. Update the execution layer to parse Union[OpaqueTransaction] with union-selector, keep 2 layers of ids
  3. Drop the Union idea

1. Translate EIP-2718 transaction IDs into the Union type, and translate back before insertion into execution layer

  • 100% compatible with current execution engines, no changes or additions to eth1
  • Consistent identifiers between Union and EIP-2718
  • Get to keep the Union type benefits
  • Forward-compatible for SSZ typed transactions
# Define EIP-2718 base types
LegacyTransaction = ByteList[MAX_BYTES_PER_TRANSACTION_PAYLOAD]  # contains the full RLP, including first byte
TransactionPayload = ByteList[MAX_BYTES_PER_TRANSACTION_PAYLOAD]  # excludes the TransactionType byte
TransactionType = uint8

LEGACY_TRANSACTION = TransactionType(0)
EIP_2930_TRANSACTION = TransactionType(1)
EIP_1559_TRANSACTION = TransactionType(2)

Transaction = Union[{
  LEGACY_TRANSACTION: LegacyTransaction,
  EIP_2930_TRANSACTION: TransactionPayload,
  EIP_1559_TRANSACTION: TransactionPayload,

  FUTURE_FOOBAR_TRANSACTION: FooBar,  # extension for illustration
}]

transactions: List[Transaction, MAX_TRANSACTIONS_PER_PAYLOAD]

def transaction_envelope(tx: Transaction) -> bytes:
    if tx.selector == LEGACY_TRANSACTION:
        assert len(tx.value) > 0 and 0xc0 <= tx.value[0] <= 0xfe  # don't allow e.g. eip-1559 transactions in the legacy transaction union selector
        return tx.value
    transaction_type = bytes([tx.selector])
    transaction_payload = serialize(tx.value)  # result is just as-is if a ByteList, but structured SSZ is flattened
    return transaction_type + transaction_payload

# transaction_envelope is used to convert the ExecutionPayload to the EIP-2718 inputs sent to the execution engine.
# Note: receipt trie is unaffected, EIP-2718 specifies how EIP-2718 transaction receipts are handled, and no execution engine changes are necessary to support this approach

2. Update the execution layer to parse Union[OpaqueTransaction] with union-selector, keep 2 layers of ids

This likely requires another transaction typing EIP, and seems way more complex and unnecessary than the other solutions.
But this is what it is like if we keep Union and expect the execution engine to just handle it.

Just for illustration:

  • EIP-2718 version 2
  • A changed transaction trie, to have a trie of transaction index -> Union[OpaqueTransaction]
  • receipts that handle this additional selector ID
  • JSON RPC changes to support a wrapper type with selector and value fields everywhere

3. Drop the Union idea

  • No benefits of Union (losing type-structured merkleization of future types, losing transaction-type mix-in for easy type proof, etc.)
  • Transactions defined in execution layer, very opaque to consensus layer (somewhat of a benefit, but also limiting future changes significantly, e.g. if we need a special transaction type for withdrawals or shard-builder accounting)
  • No ability to utilize any SSZ (incl. CommitmentContainer proposal) benefits
  • Simple
OpaqueTransaction = ByteList[MAX_BYTES_PER_OPAQUE_TRANSACTION]  # typed transaction envelope: TransactionType || TransactionPayload
transactions: List[OpaqueTransaction, MAX_TRANSACTIONS_PER_PAYLOAD]

My opinion

  • Option 1 would get things far, with no execution engine changes, just changes to the still alpha-version Merge spec. It's closest to the original intention of the Union
  • Option 2 is more of a thought experiment than an option
  • Option 3 simplifies at the cost of later technical debt

And you could think of variants of Option 1 that try and combine transaction types 0, 1 and 2 but consistency between Union selector and EIP-2718 type identifier seems more important.

@protolambda protolambda added the Bellatrix CL+EL Merge label Sep 17, 2021
@protolambda
Copy link
Contributor Author

@mkalinin @MicahZoltu @djrtwo please take a look

@MicahZoltu
Copy link
Contributor

I like option 1. Any thoughts on having 2 types for "legacy transactions" though, one for pre-155 (without chainID) and one for post-155 (with chainID)? If we are going to give a type to legacy transactions, it would be nice to have them be separate types.

@mkalinin
Copy link
Contributor

Option 1 seems the most reasonable to me.

@MicahZoltu why do you think we should care about pre-155 transactions after the Merge?

@MicahZoltu
Copy link
Contributor

They are technically different transaction types, they just have the same shape so they are difficult to differentiate. If we are going to give them type numbers, we should just do it right and give both types their own number to ease differentiation across the ecosystem.

@mkalinin
Copy link
Contributor

They are technically different transaction types, they just have the same shape so they are difficult to differentiate. If we are going to give them type numbers, we should just do it right and give both types their own number to ease differentiation across the ecosystem.

I was thinking that pre-155 transaction format is not used by anyone as migrating to post-155 doesn't require any changes on the dapp layer and is rather a matter of software upgrade. Thus, LegacyTransaction should be of post-155 format.

@MicahZoltu
Copy link
Contributor

Pre-155 is still used when you want a transaction that is intentionally replayable across networks to allow for deterministic contract addresses. It would be cool to introduce this as a built-in feature like making a CREATE2 pre-compile, but at the moment we don't have any such thing so we cannot deprecate 155.

@mkalinin
Copy link
Contributor

Pre-155 is still used when you want a transaction that is intentionally replayable across networks to allow for deterministic contract addresses. It would be cool to introduce this as a built-in feature like making a CREATE2 pre-compile, but at the moment we don't have any such thing so we cannot deprecate 155.

I see! Then why don't we have this two types distinguished by the EIP? I would not stem from the EIP designing this part of the beacon chain.

@MicahZoltu
Copy link
Contributor

EIP-155 was before we had typed transactions. When we introduced EIP-2718: Typed Transactions there was discussion of creating a new transaction type for legacy transactions or two new transaction types for pre-155 and post-155. I think we didn't follow through with it just because it was deemed unnecessary since we would still need to support legacy transactions for some indefinite period of time.

With the introduction of the beacon chain however, the argument that "we have to support legacy forever" no longer holds since beacon chain is starting from a clean slate. In this case, I would prefer to start from the cleanest slate possible which IMO would mean separate pre-155 and post-155 transaction types.

@mkalinin
Copy link
Contributor

You mean we do the following at the Merge:

  • Deprecate LegacyTransaction
  • Add a couple of new types: Pre155Transaction, Post155Transaction

And we would need these changes to be specified in a separate EIP. Did I get this right?

Also, Pre155Transaction doesn't sound. From the above use case I can conclude that this is a transaction where chainId=* and pre-155 format perfectly fits an implementation of such transaction type. Potentially it is MultiChainTransaction or something like that.

@MicahZoltu
Copy link
Contributor

I suspect that convincing the client teams to fully deprecate legacy transactions before or with the merge will not go over well, so I propose we only implement this new typing scheme in the consensus layer stuff which doesn't currently have the legacy baggage. At a later point in time we can apply a similar change (using the same numbers) to execution client gossip, transaction trie, and receipt trie.

This would mean that for now we just need to pick two numbers (0 and 4 are both available) for legacy transaction types (multichain and singlechain) used in anything that touches the consensus client.

@mkalinin
Copy link
Contributor

I suspect that convincing the client teams to fully deprecate legacy transactions before or with the merge will not go over well, so I propose we only implement this new typing scheme in the consensus layer stuff which doesn't currently have the legacy baggage. At a later point in time we can apply a similar change (using the same numbers) to execution client gossip, transaction trie, and receipt trie.

This would mean that for now we just need to pick two numbers (0 and 4 are both available) for legacy transaction types (multichain and singlechain) used in anything that touches the consensus client.

I would suggest to have them specified on the execution layer and passing the ACD governance first. Adding these numbers on the consensus layer would be straightforward (requiring HF though). Starting to make this change from the consensus layer looks like an inversion and seems useless without the corresponding update on the execution layer.

@MicahZoltu
Copy link
Contributor

IIUC, consensus layer needs a decision prior to the merge (code needs to be written), and the execution layer refuses to write any new code (or even discuss execution layer changes) prior to the merge, which is why I think it is politically not realistic to make the decision at the execution layer first.

Maybe since the consensus layer needs a decision we can force the execution teams to think about the problem as part of required The Merge work?

@mkalinin
Copy link
Contributor

IIUC, consensus layer needs a decision prior to the merge (code needs to be written), and the execution layer refuses to write any new code (or even discuss execution layer changes) prior to the merge, which is why I think it is politically not realistic to make the decision at the execution layer first.

Maybe since the consensus layer needs a decision we can force the execution teams to think about the problem as part of required The Merge work?

We may introduce the change proposed by this issue (Option 1) and then when new transaction types are decided to be added to the execution layer it may also be reflected on the consensus layer. There is no need to introduce new transaction types in advance.

@protolambda
Copy link
Contributor Author

Resolved in #2684, closing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bellatrix CL+EL Merge
Projects
None yet
Development

No branches or pull requests

3 participants