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

internal/ethapi: handle blobs in API methods #28786

Merged
merged 27 commits into from
Jan 17, 2024
Merged

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Jan 10, 2024

EIP-4844 adds a new transaction type for blobs. Users can submit such transactions via eth_sendRawTransaction. In this PR we refrain from adding support to eth_sendTransaction and in fact it will fail if the user passes in a blob hash.

However since the chain can handle such transactions it makes sense to allow simulating them. E.g. an L2 operator should be able to simulate submitting a rollup blob and updating the L2 state. Most methods that take in a transaction object should recognize blobs.The change boils down to adding blobVersionedHashes and maxFeePerBlobGas to TransactionArgs. In summary:

  • eth_sendTransaction: will fail for blob txes
  • eth_signTransaction: will fail for blob txes

As geth moves away from user-management responsibilities, the methods that sign txes will not support the new EIP-4844 transaction types. Please use eth_sendRawTransaction instead. Resuming the summary:

  • eth_sendRawTransaction: can send blob txes
  • eth_fillTransaction: will fill in a blob tx. Note: here we simply fill in normal transaction fields + possibly maxFeePerBlobGas when blobs are present. One can imagine a more elaborate set-up where users can submit blobs themselves and we fill in proofs and commitments and such. Left for future PRs if desired.
  • eth_call: can simulate blob messages
  • eth_estimateGas: blobs have no effect here. They have a separate unit of gas which is not tunable in the transaction.

internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
s1na and others added 5 commits January 11, 2024 15:05
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: Martin HS <martin@swende.se>
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@s1na
Copy link
Contributor Author

s1na commented Jan 16, 2024

I rebased to fix the merge conflicts. Note that this PR is in accord with the spec. As you can see GenericTransaction in the spec takes in maxFeePerBlobGas, blobVersionedHashes (and blobs but I propose we remove that from the spec).

https://github.com/ethereum/execution-apis/blob/cea7eeb642052f4c2e03449dc48296def4aafc24/src/schemas/transaction.yaml#L408C15-L413

@s1na
Copy link
Contributor Author

s1na commented Jan 16, 2024

I changed eth_signTransaction so it also doesn't support blob-txes to be consistent with eth_sendTransaction. This is a light nudge towards using external account-management.

@holiman
Copy link
Contributor

holiman commented Jan 16, 2024

Triage discussion:

eth_sendTransaction: will fail for blob txes
eth_signTransaction: will fail for blob txes

In a future PR, we should make these not fail: geth should be able to produce anything that is ethereum mainnet relevant/acceptable.

internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
Co-authored-by: Martin HS <martin@swende.se>
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.13.11 milestone Jan 17, 2024
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Please undo the change to tests/testdata, I guess you committed it unintentionally.

This reverts commit 402313c.
@s1na
Copy link
Contributor Author

s1na commented Jan 17, 2024

Please undo the change to tests/testdata, I guess you committed it unintentionally.

Oops, fixed.

@fjl fjl added the cancun label Jan 17, 2024
@holiman holiman merged commit e5d5e09 into ethereum:master Jan 17, 2024
3 checks passed
@s1na s1na mentioned this pull request Jan 18, 2024
5 tasks
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
EIP-4844 adds a new transaction type for blobs. Users can submit such transactions via `eth_sendRawTransaction`. In this PR we refrain from adding support to `eth_sendTransaction` and in fact it will fail if the user passes in a blob hash.

However since the chain can handle such transactions it makes sense to allow simulating them. E.g. an L2 operator should be able to simulate submitting a rollup blob and updating the L2 state. Most methods that take in a transaction object should recognize blobs. The change boils down to adding `blobVersionedHashes` and `maxFeePerBlobGas` to `TransactionArgs`. In summary:

- `eth_sendTransaction`: will fail for blob txes
- `eth_signTransaction`: will fail for blob txes

The methods that sign txes does not, as of this PR, add support the for new EIP-4844 transaction types. Resuming the summary:

- `eth_sendRawTransaction`: can send blob txes
- `eth_fillTransaction`: will fill in a blob tx. Note: here we simply fill in normal transaction fields + possibly `maxFeePerBlobGas` when blobs are present. One can imagine a more elaborate set-up where users can submit blobs themselves and we fill in proofs and commitments and such. Left for future PRs if desired.
- `eth_call`: can simulate blob messages
- `eth_estimateGas`: blobs have no effect here. They have a separate unit of gas which is not tunable in the transaction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants