contracts-bedrock: prevent too low of gas config by tynes · Pull Request #5112 · ethereum-optimism/optimism

tynes · 2023-03-11T03:06:59Z

Description

The chain liveliness depends on the L2 gas limit being larger than the gas consumed by a deposit. If a deposit consumes more gas than what is possible in an L2 block, then the chain will halt and be unable to produce blocks.

This was previously handled by having a hardcoded constant in the system config contract. This constant must be kept in sync with the value in the optimism portal. It is also possible to deploy a new optimism portal such that it has a different resource limit. To remove the need to always having to keep these values in sync, a handle to the portal is given to the system config contract. The system config contract will call the portal to get the value dynamically.

This ensures that we do not need to remember what the value is and update the contracts in tandem.

changeset-bot · 2023-03-11T03:07:02Z

⚠️ No Changeset found

Latest commit: 640664b

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

tynes · 2023-03-11T03:35:20Z

If we opt into this solution, then I will need to modify the L1 deploy process such that the correct optimism portal contract exists at the correct location before the deployment of the system config contract. Will require a refactor of how the L1 deployment works so that contracts are deployed + initialized + moved to the account one at a time, in dependency order.

packages/contracts-bedrock/contracts/L1/SystemConfig.sol

netlify · 2023-03-14T22:57:09Z

✅ Deploy Preview for opstack-docs ready!

Name	Link
🔨 Latest commit	`640664b`
🔍 Latest deploy log	https://app.netlify.com/sites/opstack-docs/deploys/6419109c47ee250008b097ac
😎 Deploy Preview	https://deploy-preview-5112--opstack-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

semgrep-app · 2023-03-15T01:34:57Z

Semgrep found 1 unchecked-type-assertion finding:

op-bindings/bindings/systemconfig.go: L463

Unchecked type assertion.

_{Created by unchecked-type-assertion.}

protolambda

LGTM. Just a few comments about future contract dependency / checks. The PR description makes sense, but I'm not sure if future additional configurable values change this.

packages/contracts-bedrock/deploy/017-SystemConfigImpl.ts

packages/contracts-bedrock/contracts/L1/SystemConfig.sol

codecov · 2023-03-15T20:00:03Z

Codecov Report

Merging #5112 (640664b) into develop (7d0c54c) will decrease coverage by 0.55%.
The diff coverage is 74.07%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5112      +/-   ##
===========================================
- Coverage    41.59%   41.05%   -0.55%     
===========================================
  Files          358      337      -21     
  Lines        22385    21769     -616     
  Branches       776      660     -116     
===========================================
- Hits          9312     8937     -375     
+ Misses       12366    12123     -243     
- Partials       707      709       +2

Flag	Coverage Δ
bedrock-go-tests	`37.66% <75.00%> (-0.01%)`	⬇️
contracts-bedrock-tests	`51.35% <66.66%> (+0.18%)`	⬆️
contracts-tests	`98.86% <ø> (ø)`
core-utils-tests	`?`
dtl-tests	`47.15% <ø> (ø)`
fault-detector-tests	`33.88% <ø> (ø)`
sdk-tests	`39.05% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
op-chain-ops/genesis/config.go	`30.58% <0.00%> (-0.29%)`	⬇️
...es/contracts-bedrock/contracts/L1/SystemConfig.sol	`71.42% <66.66%> (+4.76%)`	⬆️
op-chain-ops/genesis/layer_one.go	`75.13% <85.71%> (-0.21%)`	⬇️

... and 22 files with indirect coverage changes

packages/contracts-bedrock/contracts/L1/SystemConfig.sol

tynes · 2023-03-16T15:18:10Z

I've move the min gas check outside of the constructor and added a comment that it should happen offchain. I added a check to the deployment code. It would require a large refactor of how we do deployments/initializations to be able to allow a contract to make a call in the constructor.

mergify · 2023-03-17T22:11:10Z

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

Ensures that things are smooth once #5112 is merged. A too small of gas limit will break things after that PR is merged. We will run with 30 million limit on mainnet, so we should ensure that our devnet config reflects that.

maurelian

Code looks solid, just looking for some small tweaks to comments then will approve.

packages/contracts-bedrock/contracts/L1/SystemConfig.sol

maurelian · 2023-03-18T15:00:36Z

packages/contracts-bedrock/contracts/L1/SystemConfig.sol

+     * @notice Returns the minimum gas limit allowed by the system. If the L2
+     *         gas limit is set to a value smaller than this, then it is
+     *         possible for a block to be produced that uses more gas than what
+     *         is allowed on L2, resulting in a liveness failure.


Is that what would happen, or would deposit blocks be rejected or invalid? I honestly don't know.

According to @trianglesphere it would cause the chain to halt. I don't believe we have test coverage of this case

How difficult would it be to add a test for this case in op-e2e? Is this something we could do prior to mainnet launch, or okay to put in the backlog in your opinion?

packages/contracts-bedrock/contracts/L1/SystemConfig.sol

op-chain-ops/genesis/layer_one.go

packages/contracts-bedrock/deploy/017-SystemConfigImpl.ts

The chain liveliness depends on the L2 gas limit being larger than the gas consumed by a deposit. If a deposit consumes more gas than what is possible in an L2 block, then the chain will halt and be unable to produce blocks. This was previously handled by having a hardcoded constant in the system config contract. This constant must be kept in sync with the value in the optimism portal. It is also possible to deploy a new optimism portal such that it has a different resource limit. To remove the need to always having to keep these values in sync, a handle to the portal is given to the system config contract. The system config contract will call the portal to get the value dynamically. This ensures that we do not need to remember what the value is and update the contracts in tandem.

smartcontracts · 2023-03-21T18:22:37Z

packages/contracts-bedrock/contracts/L1/SystemConfig.sol

+     *         gas the system transaction can consume in a block.
+     */
+    function minimumGasLimit() public view returns (uint256) {
+        uint256 maxResourceLimit = uint256(ResourceMetering(PORTAL).MAX_RESOURCE_LIMIT());


I find this relationship pretty ugly. If anything, ResourceMetering should be pulling the max resource limit from SystemConfig rather than the other way around. That way you don't need a minimum gas limit like this. You just derive the maximum available deposit gas from the maximum resource gas as defined in this contract.

Considered this design but decided against it balancing amount of diff

Also seems like a cleaner layout to me, but this feels like a change that should be done after we ship Bedrock (along with broader SystemConfig enhancements, like adding in immutables for all of the protocol's proxies etc. similar to curve's AddressProvider)

clabby

Looks good to me

mergify · 2023-03-24T15:31:54Z

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

trianglesphere reviewed Mar 13, 2023

View reviewed changes

packages/contracts-bedrock/contracts/L1/SystemConfig.sol Outdated Show resolved Hide resolved

tynes force-pushed the fix/syscfg-prevent-low-gaslimit branch from 2af93fc to 80a8f2a Compare March 14, 2023 23:10

tynes marked this pull request as ready for review March 14, 2023 23:25

tynes requested review from a team as code owners March 14, 2023 23:25

tynes requested review from maurelian and mslipper March 14, 2023 23:25

protolambda approved these changes Mar 15, 2023

View reviewed changes

packages/contracts-bedrock/deploy/017-SystemConfigImpl.ts Show resolved Hide resolved

packages/contracts-bedrock/contracts/L1/SystemConfig.sol Outdated Show resolved Hide resolved

tynes force-pushed the fix/syscfg-prevent-low-gaslimit branch from 80a8f2a to 8f47ddd Compare March 15, 2023 18:14

maurelian reviewed Mar 15, 2023

View reviewed changes

packages/contracts-bedrock/contracts/L1/SystemConfig.sol Outdated Show resolved Hide resolved

packages/contracts-bedrock/contracts/L1/SystemConfig.sol Outdated Show resolved Hide resolved

mergify bot added the conflict label Mar 17, 2023

tynes force-pushed the fix/syscfg-prevent-low-gaslimit branch from 39c384f to 86eb055 Compare March 17, 2023 22:14

mergify bot removed the conflict label Mar 17, 2023

tynes mentioned this pull request Mar 18, 2023

config: set the L2 gaslimit to 30 million in config #5194

Merged

maurelian reviewed Mar 18, 2023

View reviewed changes

tynes commented Mar 20, 2023

View reviewed changes

op-chain-ops/genesis/layer_one.go Outdated Show resolved Hide resolved

tynes commented Mar 20, 2023

View reviewed changes

packages/contracts-bedrock/deploy/017-SystemConfigImpl.ts Outdated Show resolved Hide resolved

tynes added 5 commits March 20, 2023 19:04

op-chain-ops: fix deployment of L1 contracts

e3070a5

contracts-bedrock: cleanup

686c583

contracts-bedrock: fix test

307a55d

contracts-bedrock: assert deploy config

51723d8

tynes added 12 commits March 20, 2023 19:04

contracts-bedrock: regenerate invariant docs

174f921

contracts-bedrock: clean up deployment

f3f0a40

lint: fix

bb8f013

lint: more fix

3b39f0b

contracts-bedrock: cleanup

b048e5e

contracts-bedrock: fixup

d435601

op-chain-ops: fixup

30c4458

contracts-bedrock: refactor for safer deploy

c554fbc

fix: invariant docs

0173a18

op-chain-ops: clean up dead code

8039cb1

contracts-bedrock: fix comments

e7ed939

contracts-bedrock: add assert to deploy script

640664b

tynes force-pushed the fix/syscfg-prevent-low-gaslimit branch from 7d62d4e to 640664b Compare March 21, 2023 02:04

smartcontracts reviewed Mar 21, 2023

View reviewed changes

clabby approved these changes Mar 21, 2023

View reviewed changes

mergify bot added the conflict label Mar 24, 2023

tynes closed this Mar 24, 2023

tynes deleted the fix/syscfg-prevent-low-gaslimit branch March 24, 2023 15:32

mergify bot removed the conflict label Mar 24, 2023

Conversation

tynes commented Mar 11, 2023

Uh oh!

changeset-bot bot commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

tynes commented Mar 11, 2023

Uh oh!

Uh oh!

netlify bot commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for opstack-docs ready!

Uh oh!

semgrep-app bot commented Mar 15, 2023

Uh oh!

protolambda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

tynes commented Mar 16, 2023

Uh oh!

mergify bot commented Mar 17, 2023

Uh oh!

maurelian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maurelian Mar 18, 2023

Choose a reason for hiding this comment

Uh oh!

tynes Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

clabby Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smartcontracts Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

tynes Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

clabby Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clabby left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

changeset-bot bot commented Mar 11, 2023 •

edited

Loading

netlify bot commented Mar 14, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading

maurelian left a comment •

edited

Loading

clabby Mar 21, 2023 •

edited

Loading