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

Update EIP-4844: de-sszify spec #6985

Merged
merged 7 commits into from
May 16, 2023
Merged

Conversation

lightclient
Copy link
Member

I'd like to propose removing SSZ entirely from the EL side of 4844.

The only real value in blob transactions being SSZ encoded (as specified) is that the CL is able to parse the transaction and easily access blob_versioned_hashes. I don't have a great solution to this if they aren't SSZ encoded. Ultimately it seems that either i) one of the layers implements the other layer's serialization scheme or ii) the engine api is extended to resolve blob hashes given a blob tx (may not even be efficient enough).

I think adding RLP into the CL requires much less decision making than supporting SSZ in the EL. It's already clear how to lay transactions out, calculate their sig hashes and standard hashes, etc. CLs simply need to search through the opaque blobs, find the blob txs, then RLP decode them.

If we do end up going forward with SSZ in 4844, it is almost a certainty that either i) we have to change something about it in the future (defeating the purpose a bit) or ii) we are unnecessarily constrained when we do decide make the full transition to SSZ.

The least risky option appears to be making 4844 transactions RLP. I would love if we were to spend time and figure out how to apply SSZ properly (à la EIP-6404), but it doesn't seem there is interest in that route at the moment. So given all that, I believe RLP is the best path forward.

@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels May 3, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented May 3, 2023

✅ All reviewers have approved.

@eth-bot eth-bot changed the title 4844: de-sszify spec Update EIP-4844: de-sszify spec May 3, 2023
EIPS/eip-4844.md Outdated Show resolved Hide resolved
g11tech
g11tech previously approved these changes May 11, 2023
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

looks good to me

EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
dankrad
dankrad previously approved these changes May 16, 2023
@lightclient
Copy link
Member Author

@dankrad previously approved, but I didn't realize I needed to rebase. I will go ahead with merge since the rebase was rather straightforward.

@lightclient lightclient marked this pull request as ready for review May 16, 2023 10:42
@lightclient lightclient requested a review from eth-bot as a code owner May 16, 2023 10:42
@lightclient lightclient reopened this May 16, 2023
@eth-bot eth-bot enabled auto-merge (squash) May 16, 2023 10:52
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...


The signature is verified and `tx.origin` is calculated as follows:
The `max_fee_per_data_gas` is `uint256` and the `blob_versioned_hashes` field represents a list hash outputs from `kzg_to_versioned_hash`.
Copy link
Contributor

Choose a reason for hiding this comment

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

"represents a list hash outputs" something is wrong here. 🤔

This EIP introduces a transaction type that has a distinct mempool version (`BlobTransactionNetworkWrapper`) and execution-payload version (`SignedBlobTransaction`),
with only one-way convertibility between the two. The blobs are in the `BlobTransactionNetworkWrapper` and not in the `SignedBlobTransaction`;
This EIP introduces a transaction type that has a distinct mempool version and execution-payload version,
with only one-way convertibility between the two. The blobs are in the network representation and not in the consensus representation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I misread, but should this read "not in the execution implementation"? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants