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.

Sign up for GitHub

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

Jump to bottom

Experimental EIP-4844 support #7349

Closed
wants to merge 2 commits into from
Closed

Experimental EIP-4844 support #7349

wants to merge 2 commits into from

Conversation

protolambda
Copy link
Contributor

@protolambda protolambda commented Sep 22, 2023 •
edited
Loading

Description

This PR depends on the L1 Dencun support changes of #8131

This implements experimental EIP-4844 DA support.

Changes:

  • introduce a Beacon-API endpoint option to the op-node, to fetch blobs from
  • introduce deploy-config updates for:
    • L1 HF activation of Cancun (l1CancunTimeOffset relative to genesis)
    • L2 HF activation for Blobs processing (l2BlobsUpgradeTimeOffset relative to genesis)
  • refactor the derivation slightly to have an optional blobs data source, switched over when the HF is active. The old calldata inbox still works, as fallback if the network experiences any problems post-4844.
  • op-e2e utils extension: (cherry-picked into L1 dencun support PRs)
    • changes to build Cancun L1 blocks with the fake-proof-of-stake geth block builder
    • introduce a fake beacon node, which serves the minimal subset of the beacon-API to serve blobs, and holds on to the blobs after the geth node puts them in a block.
  • op-node support for L1 Cancun block-header validation
  • op-batcher changes to submit the data as blobs
  • op-service tx manager changes to support blobs as tx-candidate option
  • some new types to handle the blob and commitments in the op-node. (cherry-picked into L1 dencun support PRs)

TODO:

  • more e2e testing
  • blob-gas fee estimation for tx-manager
  • utilize the blob-commitments/proofs of the sidecar, to verify the blob contents against the versioned hashes faster and more efficiently.
  • review upstream beacon-API changes. The op-e2e test relies on the beacon-API as it looked in an earlier 4844 prototype, this likely has changed.

Tests

Adds an op-e2e test that confirms an L2 transaction is included in a safe block, on a chain with 4844 blobs as DA!

@mergify mergify bot added A-op-batcher Area: op-batcher A-op-chain-ops Area: op-chain-ops A-op-e2e Area: op-e2e A-op-node Area: op-node A-op-program Area: op-program A-op-service Area: op-service labels Sep 22, 2023
@mergify mergify bot added the A-pkg-contracts-bedrock Area: packages/contracts-bedrock label Sep 22, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2023

Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the S-conflict Status: A conflict is present label Sep 22, 2023
semgrep-app[bot]
semgrep-app bot reviewed Sep 25, 2023
View reviewed changes
op-service/eth/blob.go
// The remaining field elements each encode 31 bytes of the remaining input data, up until the end
// of the input.
//
// TODO: version the encoding format to allow for future encoding changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a Linear ticket for this TODO.

Ignore this finding from todos_require_linear.

semgrep-app[bot]
semgrep-app bot reviewed Sep 25, 2023
View reviewed changes
op-service/eth/blob.go
//
// First, field elements are encoded as big-endian uint256 in BLS modulus range. To avoid modulus
// overflow, we can't use the full 32 bytes, so we write data only to the topmost 31 bytes of each.
// TODO: we can optimize this to get a bit more data from the blobs by using the top byte
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a Linear ticket for this TODO.

Ignore this finding from todos_require_linear.

Base automatically changed from op-geth-v1_13_1 to develop September 25, 2023 18:25
@mergify mergify bot removed the S-conflict Status: A conflict is present label Sep 25, 2023
@mergify mergify bot mentioned this pull request Sep 25, 2023
Merged
@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2023

Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the S-conflict Status: A conflict is present label Sep 27, 2023
@semgrep-app
Copy link
Contributor

semgrep-app bot commented Sep 28, 2023

Semgrep found 14 third-party-action-not-pinned-to-commit-sha findings:

An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.

Ignore this finding from third-party-action-not-pinned-to-commit-sha.

Semgrep found 1 missing-user finding:

  • ufm-test-services/metamask/Dockerfile: L17

By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.

Ignore this finding from missing-user.

@mergify mergify bot added A-op-bindings Area: op-bindings A-op-challenger Area: op-challenger A-op-proposer Area: op-proposer and removed S-conflict Status: A conflict is present labels Oct 2, 2023
@mergify mergify bot mentioned this pull request Oct 2, 2023
Merged
@protolambda protolambda force-pushed the eip4844 branch 2 times, most recently from 72a93cb to a40a24a Compare October 2, 2023 18:48
@tynes
Copy link
Contributor

tynes commented Oct 10, 2023

I am concerned the fee pricing is being overlooked here. We need to have the blobbasefee pushed to L2 through the system tx into the L1Block contract as well as updating the L1CostFunc in op-geth to new logic. There isn't consensus on what this logic should be yet but my proposal is to compute the fee using both calldata and blobspace and then take the min of the 2. This would allow op-node to still parse data from both calldata and blobspace after the upgrade and assumes the batcher is smart enough to use the cheaper resource at the time. This makes the spec more complex but would not break support for base chains that do not have blobspace. Also the implementation of the GasPriceOracle smart contract on L2 would need to be updated with the new L1CostFunc logic for gas estimation

@protolambda protolambda force-pushed the eip4844 branch 2 times, most recently from c412a13 to bcbf0ea Compare October 25, 2023 20:39
@semgrep-app
Copy link
Contributor

semgrep-app bot commented Oct 25, 2023

Semgrep found 1 todos_require_linear finding:

  • op-batcher/batcher/config.go: L99

Please create a Linear ticket for this TODO.

Ignore this finding from todos_require_linear.

@EvanJRichard EvanJRichard mentioned this pull request Nov 1, 2023
Merged
@EvanJRichard EvanJRichard mentioned this pull request Nov 8, 2023
Closed
3 tasks
EvanJRichard pushed a commit that referenced this pull request Nov 8, 2023
@protolambda @roberto-bayardo
Grab just the diffs of #7349.
0e8441b 
Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: Roberto Bayardo <bayardo@alum.mit.edu>
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 9, 2023

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Nov 9, 2023
@ghost ghost mentioned this pull request Nov 9, 2023
Closed
@EvanJRichard EvanJRichard mentioned this pull request Nov 9, 2023
Merged
@protolambda protolambda removed the Stale label Nov 10, 2023
protolambda and others added 2 commits November 10, 2023 15:18
@protolambda @roberto-bayardo
op-node: EIP-4844 support
9af4891 
Original rebased prototype by proto, plus changes from Roberto:
- encapsulate data <-> blob conversion code, add unit tests
- update 4844 code to be compatible with latest beacon node api
- remove stray file, include one more invalid blob test
- misc other improvements

Co-authored-by: Roberto Bayardo <bayardo@alum.mit.edu>
@roberto-bayardo @protolambda
fix fee bumping & blob fee estimation for blob transactions
0fb737e
@protolambda protolambda changed the base branch from develop to l1-dencun-support November 10, 2023 14:21
Base automatically changed from l1-dencun-support to develop November 16, 2023 05:00
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 1, 2023

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Dec 1, 2023
@protolambda protolambda removed the Stale label Dec 1, 2023
@protolambda
Copy link
Contributor Author

Closing in favor of #8434

@protolambda protolambda closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

Assignees
No one assigned
Labels
A-op-batcher Area: op-batcher A-op-bindings Area: op-bindings A-op-chain-ops Area: op-chain-ops A-op-challenger Area: op-challenger A-op-e2e Area: op-e2e A-op-node Area: op-node A-op-program Area: op-program A-op-proposer Area: op-proposer A-op-service Area: op-service A-pkg-contracts-bedrock Area: packages/contracts-bedrock
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@protolambda @tynes @roberto-bayardo