Conversation
Add some polish
| commits, the command will also accept a tag locator, with the following invocation: | ||
|
|
||
| ``` | ||
| op-deployer verify-bytecode --dangerously-use-remote-artifacts --artifacts-locator tag://op-contracts/vX.Y.Z |
There was a problem hiding this comment.
nit, could combine these two args?
| op-deployer verify-bytecode --dangerously-use-remote-artifacts --artifacts-locator tag://op-contracts/vX.Y.Z | |
| op-deployer verify-bytecode --dangerously-set-artifacts-locator tag://op-contracts/vX.Y.Z |
There was a problem hiding this comment.
I somewhat prefer it as is because --artifacts-locator already exists, so we're just adding a new simple boolean flag, rather than a flag that is kind of an alternative to an existing one.
|
|
||
| The specific points that we should include the process are: | ||
|
|
||
| 1. Automated in superchain-registry CI (as discussed above). |
There was a problem hiding this comment.
Does this create a circular dependency, in that the monorepo depends on the superchain-registry and now the superchain-registry depends on the monorepo for op-deployer? i.e. how do we know this will work in practice?
More broadly, what value does the op-deployer wrapper add? I can instead imagine the superchain-registry CI just cloning the monorepo at the required commit/tag, then running the forge script VerifyOPCM.s.sol script in CI
There was a problem hiding this comment.
Good questions which clarify the reason for the op-deployer wrapper:
op-deployercan be easily/quickly installed from a binary by downloading, so we don't actually introduce a circular dependency.- I expect it will be pretty slow to clone/check out the monorepo at a commit, build the contracts, then run the script.
Although there is a sense in which rebuilding is safer than relying on a download, in both cases the build is occurring on cloud infrastructure, so I don't think we get a ton of benefit from the rebuild. That's why it is important to have it run in at least step 2 here.
Co-authored-by: Matt Solomon <matt@mattsolomon.dev>
| By default, `op-deployer verify-bytecode` will use locally built forge-artifacts to check bytecode. | ||
| In order to facilitate quickly running in CI, without having to checkout and rebuild different | ||
| commits, the command will also accept a tag locator, with the following invocation: |
There was a problem hiding this comment.
This seems contradictory to the Problem Statement, which states:
Currently, there is no enforced mechanism to ensure that the OPCM used in an upgrade is built from a
trusted commit, which should be one labelled as an `op-contracts/vX.Y.Z` tag, and approved by
governance.
If the above statement is true, it seems the command should default to verifying bytecode against an op-contracts git tag + remote artifacts and the flag should be --dangerously-use-local-artifacts
There was a problem hiding this comment.
In other words, which do we think is more dangerous: verifying bytecode against local or remote artifacts?
| Superchain-registry flag: | ||
|
|
||
| ``` | ||
| --superchain-registry <path/to/registry> |
There was a problem hiding this comment.
If this flag is passed, where are the addresses pulled from? Assuming we'd still need the following addresses:
- upgrade-controller
- superchain-config: from
standard-versions-<network>.toml> - protocol-versions: from
standard-versions-<network>.toml> - superchain-proxy-admin
| It is important to ensure that the bytecode verification process is run by multiple people and on | ||
| multiple different machines. |
There was a problem hiding this comment.
Why is it important for multiple people to run this? What are the risks if only one person runs the verification?
| #### 1. Automated in superchain-registry CI (as discussed above). | ||
|
|
||
| For each `$TAG` listed in `standard-versions-[mainnet|sepolia].toml`, we should run the following | ||
| command: | ||
|
|
||
| ``` | ||
| op-deployer verify-bytecode \ | ||
| --dangerously-use-remote-artifacts \ | ||
| --artifacts-locator tag://$TAG \ | ||
| --superchain-registry . | ||
| ``` |
There was a problem hiding this comment.
Should this ci job only be triggered when the standard-versions-[mainnet|sepolia].toml files are modified?
There was a problem hiding this comment.
Yes, that makes sense.
There was a problem hiding this comment.
More detail:
- scheduled job that looks at each tag in the SCR's standard-versions*.toml
- uses tag locator to get artifacts
- calculates checksum and artifacts hash
- compares to those in standard.go
Ideally we begin storing these hashes in the SCR.
| version of the OPCM (or the implementation contracts it sets). It also presents a risk of a failed | ||
| upgrade resulting from a misconfigured OPCM (ie. if any [constructor | ||
| vars](https://github.com/ethereum-optimism/optimism/blob/a10fd5259a3af9a465955b035e16f516327d51d5/packages/contracts-bedrock/src/L1/OPContractsManager.sol#L266-L269) | ||
| are set incorrectly). |
There was a problem hiding this comment.
We need a strategy for ensuring that VerifyOPCM remains complete, so that if a new contract is added to the system, that will be identified.
| The flag `--dangerously-use-remote-artifacts` is intended to discourge the use of remote artifacts | ||
| when running locally, while still enabling a fast mechanism to run in CI. |
There was a problem hiding this comment.
Ideally we want to be ensuring parity between:
- the remote tagged artifact
- the locally built artifact
- the actual deployed contract.
We should consider parallelization so that we can have that property in CI.
There was a problem hiding this comment.
worst case, have the local process be the more complete of the two.
| #### 3. Additionally for consideration: run by signers during the upgrade process. | ||
|
|
||
| The command used here would be the same as the one used in the previous step. |
There was a problem hiding this comment.
- Optional for signers but documented how to run
opd verify-bytecode - Need a process in ops repo CI to ensure that the opcm being used in the config.toml is the correct one for the upgrade tag.
|
|
||
| ## Risks & Uncertainties | ||
|
|
||
| - Reliance on Etherscan APIs. |
There was a problem hiding this comment.
in order to remove this reliance, we need to get the initcode another way.
Can do that with binary search on create2 deployer calls.
There was a problem hiding this comment.
general note to add codehash verification, with regard to this comment.
A proposal for incorporating bytecode verification into our release process.