Skip to content

op-node,contracts-bedrock: dynamic gas limit via SystemConfig#3814

Merged
mslipper merged 7 commits intodevelopfrom
dynamic-gas-limit
Nov 4, 2022
Merged

op-node,contracts-bedrock: dynamic gas limit via SystemConfig#3814
mslipper merged 7 commits intodevelopfrom
dynamic-gas-limit

Conversation

@protolambda
Copy link
Copy Markdown
Contributor

@protolambda protolambda commented Oct 28, 2022

Description

Dynamic gas limit: we extend the SystemConfig on L1 with a setter that emits gas limit changes, which can then be picked up during derivation, and applied to the payload attributes for block building.

Changes:

  • extend SystemConfig contract with gaslimit setter, and require minimum gas limit for deposit / chain safety
  • update chainops to deploy L1 contracts with gas limit
  • new action test to test gas limit update via system config contract
  • minor e2e / action test updates to handle new gas limit attribute and minimum gas limit
  • update engine API types with new gas limit field
  • extend system config with gas limit field
  • check gas limit field when comparing payload attributes and block
  • handle gas limit update events
  • spec updates to describe config value, derivation and engine API usage.
  • bindings update
  • system config storage layout update
  • minor test/utils updates to handle new field

This PR depends on ethereum-optimism/op-geth#22

Once the geth PR is merged we can update the geth dependencies, and marge this PR. A temporary go.work entry points to the geth PR for now to make CI run.

Tests

Added an action test to cover the derivation changes, and the block body validation changes in the engine.

Metadata

Fix ENG-2935
Fix ENG-2957

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Oct 28, 2022

⚠️ No Changeset found

Latest commit: ca5a246

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Oct 28, 2022
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 28, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@protolambda protolambda force-pushed the sys-config-action-tests branch from 85839b2 to 6de1f33 Compare November 1, 2022 10:42
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Nov 1, 2022

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

@mergify mergify bot added the conflict label Nov 1, 2022
@protolambda protolambda force-pushed the sys-config-action-tests branch from 6de1f33 to 66589e8 Compare November 3, 2022 14:27
@protolambda protolambda marked this pull request as ready for review November 3, 2022 15:15
@mergify mergify bot removed the conflict label Nov 3, 2022
@protolambda
Copy link
Copy Markdown
Contributor Author

This PR depends on ethereum-optimism/op-geth#22

I can pull in that commit in the go.work, but I'd prefer to first resolve the dependency stack of this PR, which also depend on open geth changes.

Base automatically changed from sys-config-action-tests to sys-config-bindings November 3, 2022 19:37
Base automatically changed from sys-config-bindings to sys-config-contract November 3, 2022 19:37
Base automatically changed from sys-config-contract to derive-with-sys-config November 3, 2022 19:37
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Nov 3, 2022

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

@mergify mergify bot added the conflict label Nov 3, 2022
@trianglesphere
Copy link
Copy Markdown
Contributor

hey @protolambda, it seems like there's a lot of system config changes, this PR should just be the dynamic gas limit

@protolambda
Copy link
Copy Markdown
Contributor Author

@trianglesphere yea, I was planning to rebase this once the sys config changes land (so close, just need op-geth PR). Github just exploded the diff here because the base branch changes. The actual changes are much smaller. I'll rebase it again

@mergify mergify bot removed the conflict label Nov 3, 2022
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Nov 4, 2022

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

@mergify mergify bot added the conflict label Nov 4, 2022
Base automatically changed from derive-with-sys-config to develop November 4, 2022 02:46
@mergify mergify bot removed the conflict label Nov 4, 2022
@protolambda protolambda force-pushed the dynamic-gas-limit branch 2 times, most recently from 951a61a to 8a89e24 Compare November 4, 2022 13:46
@protolambda
Copy link
Copy Markdown
Contributor Author

Hmm, devnet is failing on contract deployment, meaning it's failing to deploy the system config. Likely hitting the new require statement on minimum gaslimit. What gas limit do we start it with?

@protolambda
Copy link
Copy Markdown
Contributor Author

Found the issue: the L2 gas limit is set to 0, the genesis generator then uses 15M when it's unspecified, but does not implement this same fallback when inserting the gaslimit value into the initialize args.

Copy link
Copy Markdown
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. One contracts question to see if we can limit the foot guns with setting the L2 gas limit.

@mslipper
Copy link
Copy Markdown
Contributor

mslipper commented Nov 4, 2022

Merging so we can fix Hive.

@mslipper mslipper merged commit 1606b3b into develop Nov 4, 2022
@mslipper mslipper deleted the dynamic-gas-limit branch November 4, 2022 21:08
@mslipper mslipper mentioned this pull request Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-pkg-contracts-bedrock Area: packages/contracts-bedrock

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants