feat: backend mechanics for OVM self-upgrades by ben-chain · Pull Request #682 · ethereum-optimism/optimism

ben-chain · 2021-04-28T22:42:31Z

Description
This PR to the 0.3.0 release candidate gives the execution manager access to directly setting account code and storage, so that versioning can be performed from within the L2 state transitions.

Additional context

Builds on the 0.3.0-rc.
Does not yet have authentication for the OVM_UpgradeExecutor.
Most pressing next steps I see:
- Add integration test which interacts with/uses the new storage and code to make sure we're not just persisting these updates at the eth_getCode/eth_getStorageAt RPC level.
- Add integration test for upgrading the EM (how to do this? Can I just upgrade to something with a constant return value? This would brick the chain but it's harder to validate/access a "different" EM whose version we can check)

get tests passing

changeset-bot · 2021-04-28T22:42:34Z

⚠️ No Changeset found

Latest commit: ae262be

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

ben-chain · 2021-04-28T22:59:04Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

+    )
+        override
+        external
+        onlyCallableBy(0x420000000000000000000000000000000000000A)


Resolving from the L2-compiled AM was a pain here since you need to construct an ovmCALL to the AM to load storage from the correct account. I figured since this will be chugsplash-ified and moved into a storage contract anyways, it's fine to leave for now.

Think you forgot to finish the sentence :D chugsplash-ified -> what does this mean?

Maybe better to refactor the address to a constant?

Edited -- TLDR: it will be moved to storage.

Can we use storage in the execution manager? If not, I think we should just make it a constant.

Or an immutable.

gakonst · 2021-04-29T14:28:24Z

Tests are failing due to the gas cost changing.

packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol

smartcontracts · 2021-04-29T17:48:01Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

+    )
+        override
+        external
+        onlyCallableBy(0x420000000000000000000000000000000000000A)


Can we use storage in the execution manager? If not, I think we should just make it a constant.

maurelian · 2021-04-29T18:30:21Z

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_StateManager.sol

+    )
+        override
+        public
+        authenticated


The authenticated modifier allows calls from either the EM or StateTransitioner.

I think we could reduce the attack surface, and avoid 'implicit behavior' by breaking that check into two modifiers, or perhaps a single modifier which accepts an array of authorized callers (more like the new onlyCallableBy() modifier above).

@maurelian Can you make a quick little issue for this? Think it should be dealt with in a separate PR.

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

maurelian · 2021-04-29T18:34:57Z

packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol

+            _value
+        );
+    }
+}


Shouldn't there be some access control on each of these functions?

Yeah, this is just a placeholder for the sake of integration testing. I'll add a comment to make this clearer.

smartcontracts · 2021-04-29T19:26:19Z

This PR is now the base of the v0.4.0 branch. I'm going to update #653 to point to that branch.

smartcontracts · 2021-04-29T21:46:41Z

Going to close this in favor of simply having it be the base of v0.4.0. Will be fixing any remaining issues with this PR in a separate PR against that branch.

smartcontracts · 2021-05-04T18:10:50Z

As per conversation with @ben-chain, @snario, and @karlfloersch, we're going to reopen this PR and try to merge it into v0.4.0-rc.

tynes · 2021-05-05T03:36:37Z

l2geth/core/vm/ovm_state_manager.go

+	}
+
+	// save the block number and address with modified key if it's not an eth_call
+	if evm.Context.EthCallSender == nil {


Note that during debug_traceTransaction the EthCallSender is nil and will hit this block. We don't have a good solution to this right now

What is the problem you are referring to a solution of here? I added this because the diff proof should include changed accounts if we want to prove fraud on an upgrade tx.

tynes · 2021-05-05T03:38:14Z

l2geth/core/vm/ovm_state_manager.go

+		}
+	}
+	evm.StateDB.SetCode(address, code)
+	log.Debug("Put account code", "address", address.Hex(), "code", hex.EncodeToString(code))


This logline can become very expensive and will pollute the logs with large amounts of code. Could we remove the second half of the logline? code + hex.EncodeToString

ben-chain · 2021-05-06T19:23:45Z

I have rebased and renamed this branch to reflect intended release version. Closing this one and will re-open there.

ben-chain added 3 commits April 28, 2021 15:31

feat: add OVM Upgrade Functions with stub executor

eebead8

test: smocked unit tests for upgrade paths

111b63f

get tests passing

feat(l2geth): support putAccountCode

4031292

ben-chain requested review from karlfloersch, maurelian, smartcontracts and tynes as code owners April 28, 2021 22:42

test: integration test unathenticated executor

9c94a18

ben-chain force-pushed the v0.3.0-ovm_su branch from 05facb6 to 9c94a18 Compare April 28, 2021 22:47

ben-chain commented Apr 28, 2021

View reviewed changes

fix: update gas estimates in integration tests

12d63cc

smartcontracts reviewed Apr 29, 2021

View reviewed changes

maurelian reviewed Apr 29, 2021

View reviewed changes

smartcontracts closed this Apr 29, 2021

smartcontracts reopened this May 4, 2021

smartcontracts changed the base branch from v0.3.0-rc to v0.4.0-rc May 4, 2021 18:09

Merge branch 'v0.4.0-rc' into v0.3.0-ovm_su

ae262be

smartcontracts added A-contracts C-feature Category: features labels May 4, 2021

smartcontracts mentioned this pull request May 4, 2021

feat[contracts]: Add L2ChugSplashDeployer contract to manage L2 deployments #755

Merged

tynes reviewed May 5, 2021

View reviewed changes

ben-chain closed this May 6, 2021

ben-chain deleted the v0.3.0-ovm_su branch May 6, 2021 19:23

ben-chain mentioned this pull request May 6, 2021

feat[contracts, l2geth]: backend mechanics for OVM self-upgrades #793

Closed

theochap pushed a commit that referenced this pull request Dec 10, 2025

chore(derive): remove span batch todo comments (#682)

cd2fe99

Conversation

ben-chain commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

ben-chain Apr 28, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gakonst commented Apr 29, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smartcontracts commented Apr 29, 2021

Uh oh!

smartcontracts commented Apr 29, 2021

Uh oh!

smartcontracts commented May 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-chain commented May 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

ben-chain commented Apr 28, 2021 •

edited

Loading

changeset-bot bot commented Apr 28, 2021 •

edited

Loading

ben-chain Apr 28, 2021 •

edited

Loading