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

Add EIP-4844 transaction and receipt #398

Merged
merged 13 commits into from
Oct 14, 2023

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Apr 5, 2023

As discussed in recent 4844 implementers calls, this proposes to add optional fields for blobGasUsed and blobGasPrice to the transaction receipt object specific to 4844 blob transactions.

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, should it also not be part of the encoding as noticed by @jochem-brouwer

@roberto-bayardo
Copy link

roberto-bayardo commented Apr 6, 2023

looks good, should it also not be part of the encoding as noticed by @jochem-brouwer

I don't think we'd want it encoded since the values can be derived from protocol rules. The field is being added more for convenience. And to that end, I wonder if the value of data_gasprice should be in there too, in order to be able to conveniently compute the gas cost component incurred by the blobs. Without this you'd have to re-implement get_data_gasprice from the spec and hope the parameters never change.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

This change seems fine, but there are other large changes to the RPC that need to be made for cancun so I am going to hold off merging this until that solidifies. We don't really have much ability on execution rpc side of things to denote fork-related info.

@LukaszRozmej
Copy link

Is this change only for RPC and not for receipts RLP in devp2p?

@acolytec3
Copy link
Contributor Author

Is this change only for RPC and not for receipts RLP in devp2p?

That was my understanding from the discussion on the last implementers call.

@roberto-bayardo
Copy link

Can we add dataGasPrice too? I think without it this change isn't very helpful. Having both fields allows one to calculate total gas cost easily from the tx receipt in a way that will be forward compatible with any 4844 parameter tweaks:

(effectiveGasPrice * gasUsed) + (dataGasPrice * dataGasUsed)

@acolytec3 acolytec3 changed the title Add dataGasUsed to receipts for 4844 txs Add dataGasUsed and dataGasPrice to receipts for 4844 txs Apr 13, 2023
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

some punctuation nits

src/schemas/receipt.yaml Outdated Show resolved Hide resolved
src/schemas/receipt.yaml Outdated Show resolved Hide resolved
src/schemas/receipt.yaml Outdated Show resolved Hide resolved
@karalabe
Copy link
Member

We should rename these to blobGasUsed and blobGasPrice

@yperbasis
Copy link
Member

We should rename these to blobGasUsed and blobGasPrice

Yes, please. See ethereum/EIPs#7354

@acolytec3
Copy link
Contributor Author

We should rename these to blobGasUsed and blobGasPrice

Done. Thanks for the reminder.

@acolytec3 acolytec3 changed the title Add dataGasUsed and dataGasPrice to receipts for 4844 txs Add blobGasUsed and blobGasPrice to receipts for 4844 txs Jul 28, 2023
@lightclient lightclient changed the title Add blobGasUsed and blobGasPrice to receipts for 4844 txs Add EIP-4844 transaction and receipt Sep 5, 2023
@lightclient
Copy link
Member

lightclient commented Sep 5, 2023

@acolytec3 I'm co-opting this PR a bit to push through all the EIP-4844 changes together.

@lightclient lightclient changed the base branch from main to eth-cancun October 14, 2023 02:19
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

LGTM! Going to merge this into eth-cancun to push up the combined tests.

@lightclient lightclient merged commit ab83ce8 into ethereum:eth-cancun Oct 14, 2023
3 checks passed
lightclient added a commit that referenced this pull request Oct 14, 2023
* Add dataGasUsed to receipt

* Make dataGasUsed optional

* Add dataGasPrice

* Update src/schemas/receipt.yaml

Co-authored-by: Sally MacFarlane <[email protected]>

* Update src/schemas/receipt.yaml

Co-authored-by: Sally MacFarlane <[email protected]>

* Update src/schemas/receipt.yaml

Co-authored-by: Sally MacFarlane <[email protected]>

* rename data gas to blob gas - eip 7354

* schemas/tx: add 4844 tx

* schemas/tx: add 4844 blob fields to generic transaction

* eth/submit: make note that 4844 txs must be in network form for sendRawTransaction

* schemas: fix typo in 4844 tx uint ref

* schemas: remove some extra spacing

* schema: update required fields for blob tx repr

---------

Co-authored-by: Sally MacFarlane <[email protected]>
Co-authored-by: lightclient <[email protected]>
lightclient added a commit that referenced this pull request Oct 18, 2023
* Add Parent Beacon Block Root to Block (#450)

* Add EIP-4844 transaction and receipt (#398)

* Add dataGasUsed to receipt

* Make dataGasUsed optional

* Add dataGasPrice

* Update src/schemas/receipt.yaml

Co-authored-by: Sally MacFarlane <[email protected]>

* Update src/schemas/receipt.yaml

Co-authored-by: Sally MacFarlane <[email protected]>

* Update src/schemas/receipt.yaml

Co-authored-by: Sally MacFarlane <[email protected]>

* rename data gas to blob gas - eip 7354

* schemas/tx: add 4844 tx

* schemas/tx: add 4844 blob fields to generic transaction

* eth/submit: make note that 4844 txs must be in network form for sendRawTransaction

* schemas: fix typo in 4844 tx uint ref

* schemas: remove some extra spacing

* schema: update required fields for blob tx repr

---------

Co-authored-by: Sally MacFarlane <[email protected]>
Co-authored-by: lightclient <[email protected]>

* eth: add new 4844 header fields

* tests: update for cancun

* tests: non-zero parent beacon block root

---------

Co-authored-by: Ahmad Bitar <[email protected]>
Co-authored-by: acolytec3 <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-spec Area: specification F-cancun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants