-
Notifications
You must be signed in to change notification settings - Fork 48
Add L2 Contract Upgrades design doc #351
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
base: main
Are you sure you want to change the base?
Conversation
protocol/l2-contract-upgrades.md
Outdated
| 1. deploy new implementations | ||
| 2. deploy a new `L2ContractsManager` | ||
| 3. update the `ProxyAdmin` (This step will only be needed the first time this scheme is used) | ||
| 4. execute a call to `ProxyAdmin.upgradePredeploys()` | ||
| 1. This function will `DELEGATECALL` the new `L2ContractsManager`'s `upgrade()` function. | ||
| 2. For all predeploys being upgrade, `L2ContractsManager.upgrade()` will make a call to that predeploy's `upgradeTo()` function |
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.
There is currently 1M gas available to do all of this in a way that is guaranteed to be safe. This is because upgrade transactions must be included in the block but also must fit within the block gas limit. Typically the L2 block gas limit is set pretty high and there's lots of room for upgrade transactions, but it's only guaranteed to be at least SystemConfig.minimumGasLimit() and _resourceConfig.systemTxMaxGas is set to 1000000 (though not sure how guaranteed that is across chains).
The simplest fix would be to increase systemTxMaxGas to ensure L2 block gas limits are set to something reasonable and that we can keep upgrade transactions below. Otherwise there was a suggestion from Seb to automatically increase the block gas limit to ensure upgrade transactions fit which would also work.
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.
Ouch, there is almost no way that 1M is going to be sufficient, thanks for calling that out.
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.
I will work with Seb to properly address this. It's not expected to be a blocker.
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.
Yes, we could increase the systemTxMaxGas, but then that change would also need to be propagated on L1 and I don't know which other places. And it would still not be future proof. Do we set it to 10M? What happens if we suddenly need 15M? Do we raise it again, including all the L1 work to do this?
Another solution that would always guarantee there's enough gas for the upgrade transactions, no matter how small the regular gasLimit, is to introduce the concept of additional upgrade gas (or maxing with the gas limit). I've implemented this for Isthmus already but it was then decided not to include: ethereum-optimism/optimism#14797 (The user tx omission part of this has now been included in Jovian). The actual changes necessary for upgrade gas are in file op-node/rollup/derive/attributes.go, it's only a few lines of code.
|
|
||
| A given set of upgrade transactions will typically perform the following actions: | ||
|
|
||
| 1. deploy new `DeployerWrapper` contract |
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.
This will probably only be needed for the first time right?
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.
Should the DeployerWrapper be part of the NUTs (and why?) or should it be handled as a separate deployment?
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.
It should be deployed in the NUTs. It will always be needed to prevent CREATE2 collisions.
| The `ProxyAdmin` predeploy (at `0x42...18`) will be upgraded with a new implementation which: | ||
|
|
||
| 1. maintains the current interface for backwards compatibility | ||
| 2. removes code which is necessary to support the Chugsplash and Resolved Delegate proxies (Optional, can be cut from scope) |
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.
I think it's safe to remove this logic from the L2 Proxy Admin. We can actually check that only ERC-1967 proxies should be present in L2 by looking at the L2Genesis, I still confirmed this in a couple L2s (OP Mainnet, Base, Unichain) by querying the ProxyAdmin::proxyType with all the proxies. As long as we keep the public interface of the ProxyAdmin we should be good.
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.
I agree. We should just do it.
Purpose
To enable deterministic, well tested, hard fork driven upgrades of L2 contracts, with both the implementation and upgrade path written in Solidity.
The customers for this work include:
Summary
The proposed design maintains the current method of injecting Network Upgrade Transactions (NUTs) at a specific fork block height, while improving on the development and testing process in order to better enable safe, well-tested, multi-client upgrades of L2 contracts.
This is achieved by putting the transaction data into a JSON file, and then executing it at the specified fork block.
The design aims to create a solution which parallels the OPCM, but for L2.