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

Remove Union from ExecutionPayload transaction type #2684

Merged
merged 2 commits into from
Oct 20, 2021
Merged

Remove Union from ExecutionPayload transaction type #2684

merged 2 commits into from
Oct 20, 2021

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Oct 19, 2021

It seems that we are at an impasse as to how to properly use a Union-type for Transactions that maps well to the execution layer native typing. (based on convo here -- #2608)

This PR removes the Union type completely, making the execution layer transactions just opaque bytes. This is nice because we don't tightly couple Consensus Layer changes if/when any new Execution Layer txs are added/modified.

This keeps a cleaner separation of concerns and allows the layers to be specified (and even upgraded) relatively independently. I'm personally pro keeping this divide clean where possible. The fact that we can't agree on a proper Union selector typing is because of decisions that need to be made in EL (or that CL would imply a decision needs to be made in a certain way eventually). This goes to show us that crossing these layers in specs is complication inducing and imo should be avoided when possible.


additionally, I added the Execution presets to the associated yaml files

@djrtwo
Copy link
Contributor Author

djrtwo commented Oct 19, 2021

cc: @MicahZoltu

Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice simplification, and having a selector/value pair in every transaction JSON was going to get bad anyways, if we wanted a Union that doesn't exactly match the typed-envelope EIP and thus required encoding the selector (like Micah said, 0 isn't the legacy tx ID, and splitting the legacy transactions into more consensus types would cause it to not fit as easily).

The drawback is that we don't get type-structured merkleization of the transaction, which would have been nice. Merkleization of the encoded tx is still good though, and we don't leak execution layer logic into the consensus layer. Worst case, if we realize we need custom merkleization in the future, we can do an approach like the commitment-container PR, but for the transaction byte list.

@MicahZoltu
Copy link
Contributor

I agree with what @protolambda's comments. It makes me sad to lose merklization of transactions, but I also have a strong appreciation of the simplification this provides. I think the simplification wins out for me as well, but one of those wins where you walk away sad.

@djrtwo djrtwo merged commit a20f6f7 into dev Oct 20, 2021
@djrtwo djrtwo deleted the merge-txs branch October 20, 2021 14:59
# 5,000
MIN_GAS_LIMIT: 5000
# 2**5 (= 32)
MAX_EXTRA_DATA_BYTES: 32
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't BYTES_PER_LOGS_BLOOM and MAX_EXTRA_DATA_BYTES be constant values disregarding the network, i.e. out of the scope of any configuration, even the preset one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. Presets offer compile-time configuration for clients that want to optimize usage of these and/or use static typing. They are not configurable per network, unless the testnet warrants a customized preset and clients compile with that. Flexible enough if we ever need a network that changes them, but not part of the default runtime config. See config/preset docs for more info.

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

Successfully merging this pull request may close these issues.

4 participants