-
Notifications
You must be signed in to change notification settings - Fork 4k
feat: backend mechanics for OVM self-upgrades #682
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
Changes from 5 commits
eebead8
111b63f
4031292
9c94a18
12d63cc
ae262be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| import { expect } from 'chai' | ||
| import { Wallet, utils, BigNumber, Contract } from 'ethers' | ||
| import { Direction } from './shared/watcher-utils' | ||
|
|
||
| import { OptimismEnv } from './shared/env' | ||
|
|
||
| import { getContractInterface } from '@eth-optimism/contracts' | ||
| import { l2Provider, OVM_ETH_ADDRESS } from './shared/utils' | ||
| import { ethers } from 'hardhat' | ||
|
|
||
|
|
||
| // TODO: use actual imported Chugsplash type | ||
|
|
||
| interface SetCodeInstruction { | ||
| target: string // address | ||
| code: string // bytes memory | ||
| } | ||
|
|
||
| interface SetStorageInstruction { | ||
| target: string // address | ||
| key: string // bytes32 | ||
| value: string // bytes32 | ||
| } | ||
|
|
||
| type ChugsplashInstruction = SetCodeInstruction | SetStorageInstruction | ||
|
|
||
| // Just an array of the above two instruction types. | ||
| type ChugSplashInstructions = Array<ChugsplashInstruction> | ||
|
|
||
| const isSetStorageInstruction = (instr: ChugsplashInstruction): instr is SetStorageInstruction => { | ||
| return !instr["code"] | ||
| } | ||
|
|
||
| describe('OVM Self-Upgrades', async () => { | ||
| let env: OptimismEnv | ||
| let l2Wallet: Wallet | ||
| let OVM_UpgradeExecutor: Contract | ||
|
|
||
| const applyChugsplashInstructions = async (instructions: ChugSplashInstructions) => { | ||
| for (const instruction of instructions) { | ||
| let res | ||
| if (isSetStorageInstruction(instruction)) { | ||
| res = await OVM_UpgradeExecutor.setStorage( | ||
| instruction.target, | ||
| instruction.key, | ||
| instruction.value | ||
| ) | ||
| } else { | ||
| res = await OVM_UpgradeExecutor.setCode( | ||
| instruction.target, | ||
| instruction.code | ||
| ) | ||
| } | ||
| await res.wait() // TODO: promise.all | ||
| } | ||
| } | ||
|
|
||
| const checkChugsplashInstructionsApplied = async (instructions: ChugSplashInstructions) => { | ||
| for (const instruction of instructions) { | ||
| // TODO: promise.all this for with a map for efficiency | ||
| if (isSetStorageInstruction(instruction)) { | ||
| const actualStorage = await l2Provider.getStorageAt( | ||
| instruction.target, | ||
| instruction.key | ||
| ) | ||
| expect(actualStorage).to.deep.eq(instruction.value) | ||
| } else { | ||
| const actualCode = await l2Provider.getCode( | ||
| instruction.target | ||
| ) | ||
| expect(actualCode).to.deep.eq(instruction.code) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const applyAndVerifyUpgrade = async (instructions: ChugSplashInstructions) => { | ||
| await applyChugsplashInstructions(instructions) | ||
| await checkChugsplashInstructionsApplied(instructions) | ||
| } | ||
|
|
||
| before(async () => { | ||
| env = await OptimismEnv.new() | ||
| l2Wallet = env.l2Wallet | ||
|
|
||
| OVM_UpgradeExecutor = new Contract( | ||
| '0x420000000000000000000000000000000000000a', | ||
| getContractInterface('OVM_UpgradeExecutor', true), | ||
| l2Wallet | ||
| ) | ||
| }) | ||
|
|
||
| describe('setStorage and setCode are correctly applied', () => { | ||
| it('Should execute a basic storage upgrade', async () => { | ||
| const basicStorageUpgrade: ChugSplashInstructions = [ | ||
| { | ||
| target: OVM_ETH_ADDRESS, | ||
| key: '0x1234123412341234123412341234123412341234123412341234123412341234', | ||
| value: '0x6789123412341234123412341234123412341234123412341234678967896789', | ||
| } | ||
| ] | ||
| await applyAndVerifyUpgrade(basicStorageUpgrade) | ||
| }) | ||
|
|
||
| it('Should execute a basic upgrade overwriting existing deployed code', async () => { | ||
| const DummyContract = await ( | ||
| await ethers.getContractFactory('SimpleStorage', l2Wallet) | ||
| ).deploy() | ||
| await DummyContract.deployTransaction.wait() | ||
|
|
||
| const basicCodeUpgrade: ChugSplashInstructions = [ | ||
| { | ||
| target: DummyContract.address, | ||
| code: '0x1234123412341234123412341234123412341234123412341234123412341234', | ||
| } | ||
| ] | ||
| await applyAndVerifyUpgrade(basicCodeUpgrade) | ||
| }) | ||
|
|
||
| it('Should execute a basic code upgrade which is not overwriting an existing account', async () => { | ||
| // TODO: fix me? Currently breaks due to nil pointer dereference; triggerd by evm.StateDB.SetCode(...) in ovm_state_manager.go ? | ||
| // More recent update: I cannot get this to error out any more. | ||
| const emptyAccountCodeUpgrade: ChugSplashInstructions = [ | ||
| { | ||
| target: '0x5678657856785678567856785678567856785678', | ||
| code: '0x1234123412341234123412341234123412341234123412341234123412341234', | ||
| } | ||
| ] | ||
| await applyAndVerifyUpgrade(emptyAccountCodeUpgrade) | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package vm | ||
|
|
||
| import ( | ||
| "encoding/hex" | ||
| "errors" | ||
| "fmt" | ||
| "math/big" | ||
|
|
@@ -19,6 +20,7 @@ var funcs = map[string]stateManagerFunction{ | |
| "getAccountEthAddress": getAccountEthAddress, | ||
| "getContractStorage": getContractStorage, | ||
| "putContractStorage": putContractStorage, | ||
| "putAccountCode": putAccountCode, | ||
| "isAuthenticated": nativeFunctionTrue, | ||
| "hasAccount": nativeFunctionTrue, | ||
| "hasEmptyAccount": hasEmptyAccount, | ||
|
|
@@ -156,6 +158,32 @@ func putContractStorage(evm *EVM, contract *Contract, args map[string]interface{ | |
| return []interface{}{}, nil | ||
| } | ||
|
|
||
| func putAccountCode(evm *EVM, contract *Contract, args map[string]interface{}) ([]interface{}, error) { | ||
| address, ok := args["_address"].(common.Address) | ||
| if !ok { | ||
| return nil, errors.New("Could not parse address arg in putAccountCode") | ||
| } | ||
| code, ok := args["_code"].([]byte) | ||
| if !ok { | ||
| return nil, errors.New("Could not parse code arg in putAccountCode") | ||
| } | ||
|
|
||
| // save the block number and address with modified key if it's not an eth_call | ||
| if evm.Context.EthCallSender == nil { | ||
| err := evm.StateDB.SetDiffAccount( | ||
| evm.Context.BlockNumber, | ||
| address, | ||
| ) | ||
| if err != nil { | ||
| log.Error("Cannot set diff key", "err", err) | ||
| } | ||
| } | ||
| evm.StateDB.SetCode(address, code) | ||
| log.Debug("Put account code", "address", address.Hex(), "code", hex.EncodeToString(code)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
|
||
| return []interface{}{}, nil | ||
| } | ||
|
|
||
| func testAndSetAccount(evm *EVM, contract *Contract, args map[string]interface{}) ([]interface{}, error) { | ||
| address, ok := args["_address"].(common.Address) | ||
| if !ok { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,6 +146,18 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { | |
| _; | ||
| } | ||
|
|
||
| /** | ||
| * Modifies an EM method so that only a given address can invoke it. | ||
| */ | ||
| modifier onlyCallableBy( | ||
| address _allowed | ||
| ) { | ||
| if (ovmADDRESS() != _allowed) { | ||
| _revertWithFlag(RevertFlag.CALLER_NOT_ALLOWED); | ||
| } | ||
| _; | ||
| } | ||
|
smartcontracts marked this conversation as resolved.
|
||
|
|
||
|
|
||
| /************************************ | ||
| * Transaction Execution Entrypoint * | ||
|
|
@@ -1872,6 +1884,52 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { | |
| ovmStateManager = iOVM_StateManager(address(0)); | ||
| } | ||
|
|
||
|
|
||
| /********************* | ||
| * Upgrade Functions * | ||
| *********************/ | ||
|
|
||
| /** | ||
| * Sets the code of an ovm contract. | ||
| * @param _address Address to update the code of. | ||
| * @param _code Bytecode to put into the ovm account. | ||
| */ | ||
| function ovmSETCODE( | ||
| address _address, | ||
| bytes memory _code | ||
| ) | ||
| override | ||
| external | ||
| onlyCallableBy(0x420000000000000000000000000000000000000A) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolving from the L2-compiled AM was a pain here since you need to construct an
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think you forgot to finish the sentence :D
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe better to refactor the address to a constant?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edited -- TLDR: it will be moved to storage.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use storage in the execution manager? If not, I think we should just make it a constant.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or an |
||
| { | ||
| _checkAccountLoad(_address); | ||
| ovmStateManager.putAccountCode(_address, _code); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Sets the storage slot of an OVM contract. | ||
| * @param _address OVM account to set storage of. | ||
| * @param _key Key to set set. | ||
| * @param _value Value to store at the given key. | ||
| */ | ||
| function ovmSETSTORAGE( | ||
| address _address, | ||
| bytes32 _key, | ||
| bytes32 _value | ||
| ) | ||
| override | ||
| external | ||
| onlyCallableBy(0x420000000000000000000000000000000000000A) | ||
| { | ||
| _putContractStorage( | ||
| _address, | ||
| _key, | ||
| _value | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| /***************************** | ||
| * L2-only Helper Functions * | ||
| *****************************/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,6 +143,23 @@ contract OVM_StateManager is iOVM_StateManager { | |
| account.codeHash = EMPTY_ACCOUNT_CODE_HASH; | ||
| } | ||
|
|
||
| /** | ||
| * Inserts the given bytecode into the given OVM account. | ||
| * @param _address Address of the account to overwrite code onto. | ||
| * @param _code Bytecode to put at the address. | ||
| */ | ||
| function putAccountCode( | ||
| address _address, | ||
| bytes memory _code | ||
| ) | ||
| override | ||
| public | ||
| authenticated | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maurelian Can you make a quick little issue for this? Think it should be dealt with in a separate PR. |
||
| { | ||
| Lib_OVMCodec.Account storage account = accounts[_address]; | ||
| account.codeHash = keccak256(_code); | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves an account from the state. | ||
| * @param _address Address of the account to retrieve. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| // @unsupported: evm | ||
| pragma solidity >0.5.0 <0.8.0; | ||
|
|
||
| /* Library Imports */ | ||
| import { Lib_ExecutionManagerWrapper } from "../../libraries/wrappers/Lib_ExecutionManagerWrapper.sol"; | ||
|
|
||
| /** | ||
| * @title OVM_UpgradeExecutor | ||
| * @dev The OVM_UpgradeExecutor is the contract which authenticates and executes (i.e. | ||
|
smartcontracts marked this conversation as resolved.
|
||
| * calls the relevant Execution Manager upgrade functions) upgrades to the OVM State. | ||
| * This enables us to update the predeploy and execution contracts directly from within | ||
| * L2 when there is a new release. | ||
| * | ||
| * Compiler used: optimistic-solc | ||
| * Runtime target: OVM | ||
| */ | ||
| contract OVM_UpgradeExecutor { | ||
| function setCode( | ||
| address _address, | ||
| bytes memory _code | ||
| ) | ||
| external | ||
| { | ||
| Lib_ExecutionManagerWrapper.ovmSETCODE( | ||
| _address, | ||
| _code | ||
| ); | ||
| } | ||
|
|
||
| function setStorage( | ||
| address _address, | ||
| bytes32 _key, | ||
| bytes32 _value | ||
| ) | ||
| external | ||
| { | ||
| Lib_ExecutionManagerWrapper.ovmSETSTORAGE( | ||
| _address, | ||
| _key, | ||
| _value | ||
| ); | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't there be some access control on each of these functions?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is just a placeholder for the sake of integration testing. I'll add a comment to make this clearer. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that during
debug_traceTransactiontheEthCallSenderisniland will hit this block. We don't have a good solution to this right nowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.