Espresso 1: Contracts#443
Conversation
QuentinI
force-pushed
the
ag/contracts
branch
2 times, most recently
from
4bc5939 to
cbd97f9
Compare
Adds the Espresso-introduced contracts and the minimum supporting changes
required for them to compile, test, and pass the contract checks.
New contracts and scripts:
- src/L1/BatchAuthenticator.sol and interfaces/L1/IBatchAuthenticator.sol
(upgradeable contract that authenticates batch transactions, with switching
between Espresso and fallback batchers)
- scripts/deploy/DeployBatchAuthenticator.s.sol and
scripts/deploy/DeployEspresso.s.sol
- test/L1/BatchAuthenticator.t.sol and test/mocks/MockEspressoTEEVerifiers.sol
- snapshots/{abi,storageLayout}/BatchAuthenticator.json
- snapshots/semver-lock.json entry for BatchAuthenticator
New submodules:
- lib/espresso-tee-contracts (interfaces required by BatchAuthenticator)
- lib/openzeppelin-contracts-upgradeable-v5 (OZ v5 used by BatchAuthenticator
via OwnableUpgradeable)
Supporting changes (Espresso-driven):
- foundry.toml: remappings for OZ v5 and espresso-tee-contracts; ignored
warning codes for vendored libs; OOM-safe jobs settings; via-ir profile.
- justfile: fix-proxy-artifact recipe to handle OZ v5 shadowing Proxy/ProxyAdmin
artifacts; build/coverage hooks.
- src/universal/Proxy.sol, src/universal/ProxyAdmin.sol: pin pragma to exact
0.8.15 so they stay in their own compilation group and never emit PUSH0.
- src/universal/ReinitializableBase.sol: loosen pragma to ^0.8.15 so
BatchAuthenticator (compiled with OZ v5) can import it.
- scripts/* and test/*: disambiguate Proxy artifact lookups to
src/universal/Proxy.sol:Proxy (avoids OZ v5 proxy/Proxy.sol shadow).
- scripts/checks: bypass interface checks for artifacts originating from lib/;
add Espresso-related contract names to exclude lists; pragma exclusions for
Proxy/ProxyAdmin/BatchAuthenticator.
- test/vendor/Initializable.t.sol: exclude BatchAuthenticator (deployed by a
separate Espresso script).
Co-authored-by: OpenCode <noreply@opencode.ai>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee2d984108
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: piersy <pierspowlesland@gmail.com>
| @@ -0,0 +1,287 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity 0.8.25; | |||
There was a problem hiding this comment.
I would recommend to double check versioning in each file for consistency.
OP source contracts follow 0.8.15, but files like BatchAuthenticator.sol & contracts in espresso-tee-contracts follow ^0.8.0 (which is fine I guess).
OP scripts usually follow ^0.8.0, but here scripts follow 0.8.25 - could you tell more about the motivation for it?
There was a problem hiding this comment.
Relaxed to ^0.8.0 - I don't remember any specific reason for us doing this, going to assume there wasn't one since nothing seems to break doing this.
| address payable _addr; | ||
| assembly { _addr := create(0, add(_initCode, 0x20), mload(_initCode)) } | ||
| require(_addr != address(0), "DeployBatchAuthenticator: ProxyAdmin deployment failed"); | ||
| proxyAdmin = IProxyAdmin(_addr); |
There was a problem hiding this comment.
Should separate ProxyAdmin be deployed just for BatchAuthenticator or would it be possible to re-use existing ProxyAdmin from our OP Stack deployment?
There was a problem hiding this comment.
I don't think there's a technical reason
cc @shenkeyao is there a governance reason for this?
There was a problem hiding this comment.
I think technically we can reuse the existing ProxyAdmin, but that would require the OP stack to participate in every future batchAuthenticator upgrade, which is an undesirable governance dependency. Looks like this was introduced in this commit, so cc @jjeangal, who might have more context.
There was a problem hiding this comment.
This script was essentially built to deploy the contracts required for running solely the batch poster on the devnet. I believe it is technically possible to re-use the ProxyAdmin from your OP stack deployment. The decision to do so is more operational than architectural.
We can open a discussion as to whether this is something we want to do!
| checkOutput(_output); | ||
| } | ||
|
|
||
| function deployBatchAuthenticator( |
There was a problem hiding this comment.
I see this is the second place that can deploy BatchAuthenticator, so the same question: should separate ProxyAdmin be freshly deployed just for this contract? Or is it possible to use already deployed ProxyAdmin for Celo's OP Stack?
There was a problem hiding this comment.
DeployEspresso.s.sol is the script invoked by the Espresso-specific stage of the op-deployer apply pipeline so it would never be used in production.
| return nil, []error{fmt.Errorf("%s: Interface does not start with 'I'", contractName)} | ||
| } | ||
|
|
||
| // Espresso: skip interface artifacts that originate from lib/ (e.g. OZ v5 interfaces |
There was a problem hiding this comment.
I'm not the biggest fan of all this artifact handling.
I think there's a way around it: 0322873
Let me know what you think.
There was a problem hiding this comment.
This makes sense to me
cc @shenkeyao I think you have more context here
There was a problem hiding this comment.
Makes sense to cherry-pick 0322873! This would help clean up many artifact-related changes.
However, the lib/ skip in this file would still be necessary, because OZ v5 interfaces compiled transitively from espresso-tee-contracts would still land in forge-artifacts/.
| address _owner | ||
| ) | ||
| external | ||
| reinitializer(initVersion()) |
There was a problem hiding this comment.
initialize() is non-idempotent: activeIsEspresso = true on L73 re-runs on every reinitializer(initVersion()) bump and silently undoes any prior switchBatcher() (e.g. a guardian flip to Fallback).
- strict-pragma: remove unneeded exclusions for src/universal/Proxy.sol and src/universal/ProxyAdmin.sol — both already use strict 'pragma solidity 0.8.15;', so the entries (and their misleading comment claiming '^') were dead. - interfaces: move the Espresso excludeContracts block out of the upstream-shared area and down next to the Celo block, with one entry per line to match the surrounding style. Localizes future rebase deltas. Co-authored-by: OpenCode <noreply@opencode.ai>
Inline the EspressoTEEVerifier deployment in DeployEspresso.s.sol so it
no longer imports lib/espresso-tee-contracts/scripts/DeployTEEVerifier.s.sol
or DeployNitroTEEVerifier.s.sol. The upstream scripts pulled OZ v5's
TransparentUpgradeableProxy (and its auto-deployed ProxyAdmin) into the
OP artifact tree, shadowing src/universal/ProxyAdmin.sol and forcing a
~90-line fix-proxy-artifact justfile recipe.
The TEEVerifier is now deployed behind src/universal/Proxy.sol +
src/universal/ProxyAdmin.sol, matching how BatchAuthenticator is
deployed in the same script. ERC-1967 slots are unchanged, so external
callers see no difference.
The raw vm.getCode("ProxyAdmin") lookups in the deploy scripts and
BatchAuthenticator tests are switched to the explicit artifact path
vm.getCode("forge-artifacts/ProxyAdmin.sol/ProxyAdmin.json") to
deterministically resolve the default compilation profile's bytecode
(the dispute profile transitively compiles ProxyAdmin at optimizer_runs=5000,
creating a second artifact that broke unqualified lookups).
The fix-proxy-artifact recipe and its 5 callsites are removed.
Cherry-picked from piersy's commit 5d0a803 on PR ethereum-optimism#443. Walks the dual-batcher state machine: Espresso path → switchBatcher → fallback path → switchBatcher → Espresso path. Asserts every transition emits the expected event, that signer registration survives the round-trip, and that re-issuing the same call after a mode flip changes the outcome (the previously-valid Espresso signature is no longer consulted on the fallback path). Co-authored-by: Piers Powlesland <pierspowlesland@gmail.com> Co-authored-by: OpenCode <noreply@opencode.ai>
| } | ||
|
|
||
| function authenticateBatchInfo(bytes32 _commitment, bytes calldata _signature) external { | ||
| if (activeIsEspresso) { |
There was a problem hiding this comment.
Don't we need to check the tx sender here as well? The spec says:
Both keys must be present for a batch to be successfully posted.
Currently this only checks the ephemeral signature, not the espresso batcher address.
Something like:
if (activeIsEspresso) {
if (msg.sender != espressoBatcher) {
revert UnauthorizedEspressoBatcher(msg.sender, espressoBatcher);
}
espressoTEEVerifier.verify(_signature, _commitment, IEspressoTEEVerifier.TeeType.NITRO);
There was a problem hiding this comment.
I'm wondering if the check on the espressoBatcher address is actually included by it being part of the enclave hash? I.E. is the PCR0 dependent on the espressoBatcher EOA?
There was a problem hiding this comment.
The PCR0 is not dependent on EOA.
The reasoning behind this was that the batcher's behaviour is attested to by the PCR0, i.e. even if a random batcher started authorizing batches it wouldn't be able to post anything other than what it saw on Espresso anyways, otherwise it couldn't have registered because it wouldn't get the attestation. However, we have decided that a check here wouldn't hurt though since we're doing permissioned batching anyways. @shenkeyao implemented it here; as soon as the PR is merged I will cherry-pick it for this PR.
|
Hey @QuentinI! I have authorized to run CircleCI tests from your fork. May you commit something to trigger execution? Also I run manually the contract tests and found two are failing: |
|
|
||
| /// @notice Append-only history of authorized Espresso batcher addresses | ||
| /// and the L1 block at which each became active. | ||
| EspressoBatcherEntry[] internal _espressoBatcherHistory; |
There was a problem hiding this comment.
What about using open zeppelin's checkpoints library for this? https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/Checkpoints.sol
| { paths = "src/L1/ProtocolVersions.sol", optimizer_runs = 0 }, | ||
| { paths = "src/universal/StorageSetter.sol", optimizer_runs = 0 } | ||
| { paths = "src/universal/StorageSetter.sol", optimizer_runs = 0 }, | ||
| { paths = "src/L1/StandardValidator.sol", optimizer_runs = 5000 }, |
There was a problem hiding this comment.
Good catch, this looks like a rebase artifact, removed in 4f91109.
| @@ -84,6 +108,13 @@ ffi = true | |||
| # you increase the gas limit above this value it must be a string. | |||
| gas_limit = 9223372036854775807 | |||
|
|
|||
| # Disable forge lint during build so 287+ linter warnings (e.g. unsafe-typecast) don't fail the build. | |||
| # Run `forge lint` separately when fixing style. | |||
| # Note: [lint] is a Foundry 1.5+ top-level section. Forge 1.2.x treats it as a deprecated profile | |||
| # notation and emits a harmless warning; lint_on_build has no effect there (feature didn't exist yet). | |||
| [lint] | |||
| lint_on_build = false | |||
|
|
|||
There was a problem hiding this comment.
Hey @QuentinI I'm wondering if we could resolve these workarounds in the way outlined below?
A few of the foundry.toml workarounds in this PR look like symptoms of the same root cause — the deploy scripts are pulling the entire espresso-tee-contracts impl chain into OP's compile groups. Specifically:
ignored_error_codesadds6321,5667,1878— these are coming from compiling submodule mocks (5667unused param), submodule scripts (1878missing SPDX), and other submodule code (6321unnamed return). The suppression is global, so any matching warning in OP's ownsrc/would now compile silently.[lint] lint_on_build = falsesilences 287+ warnings (unsafe-typecastetc.) — again mostly from the submodule, and again globally, so new lint regressions in OP's own code slip through.jobs = 1in four profiles (ci,cicoverage,ciheavy,lite) plusthreads = 4— band-aid for OOM caused by the 211-file espresso compile group running alongside the 530-file OP group on 16 GB runners. Real CI wall-time cost on every PR.
All three could be mitigated by one structural change: switch DeployEspresso.s.sol to use vm.getCode against the submodule's own out/ for the implementation contracts, mirroring the pattern this PR already uses for ProxyAdmin at DeployBatchAuthenticator.s.sol:50-58. Concretely:
- Pre-build the submodule (it has its own
foundry.tomlpinning solc 0.8.25):forge build --root lib/espresso-tee-contractsas a CI/Makefile step before OP's main build. - Drop the impl imports at
DeployEspresso.s.sol:12-13(EspressoTEEVerifier,EspressoNitroTEEVerifier). Keep the interface imports — they're tiny. - Replace the two
new EspressoTEEVerifier(...)/new EspressoNitroTEEVerifier(...)calls with the samevm.getCode+assembly { create(...) }pattern asProxyAdmin, pointing atlib/espresso-tee-contracts/out/...json. - Add
{ access = 'read', path = './lib/espresso-tee-contracts/out/' }tofs_permissions.
That eliminates the impl closure (TEEHelper, JournalValidation, and the chunk of aws-nitro-enclave-attestation reachable from it) from OP's Solc invocations — probably ~150 of the 211 files in the espresso group. What remains is just the inheritance closure from BatchAuthenticator → OwnableWithGuardiansUpgradeable → OZ, which can't be split this way but is ~30 files, not 200+.
Once the group is that small, jobs = 1 and threads = 4 should come off, and the three new ignored_error_codes + lint_on_build = false go with them since OP's build no longer compiles the submodule's mocks/scripts that emit those warnings.
There was a problem hiding this comment.
This sounds very reasonable, I will try it out!
There was a problem hiding this comment.
This does seems to allow us to drop the new error codes and lint_on_build, but the compilation groups don't shrink perceptibly. I'll see if I can do anything to fix that part. Implemented everything else in in 348fed7
Replace the hand-rolled `EspressoBatcherEntry[]` history + binary search with OpenZeppelin's `Checkpoints.Trace160` (`(uint96 key, uint160 value)`). `uint160` is exactly an address with no waste, and `uint96` easily covers L1 block numbers. `upperLookupRecent` replaces the custom binary search and the same-block-overwrite branch is now handled inside `_insert`. Co-authored-by: OpenCode <noreply@opencode.ai>
|
@jcortejoso I have fixed the two tests; thank you for approving the CI - it CI seems to pass except for the workflows that refuse to run altogether |
…trictions entries src/L1/StandardValidator.sol was renamed to OPContractsManagerStandardValidator.sol upstream in ethereum-optimism#16237; the old path was a rebase artifact with no corresponding file. Co-authored-by: OpenCode <noreply@opencode.ai>
…sed warning codes Drop the two impl imports (EspressoTEEVerifier, EspressoNitroTEEVerifier) from DeployEspresso.s.sol and replace direct instantiation with vm.getCode + assembly create, reading bytecode from the submodule's own out/ directory. This removes the impl closure (TEEHelper, JournalValidation, and the aws-nitro-enclave-attestation chain) from OP's solc invocations. The impls are still parsed/ABI-checked by forge via libs=['lib'], but they no longer require bytecode emission or the optimizer backend. Since OP's build no longer compiles the submodule's impl files, the three error codes those files triggered (6321 unnamed return, 5667 unused param, 1878 missing SPDX) can be removed from ignored_error_codes. OP's own code does not trigger any of them. The lint_on_build=false workaround is also removed for the same reason — with the impl closure gone, forge lint reports 283 warnings (all from OP's own code), none of which cause a build failure. Adds fs_permissions read access for lib/espresso-tee-contracts/out/ so vm.getCode can locate the pre-built artifacts. The submodule must be built (forge build --root lib/espresso-tee-contracts) before OP's main build. Co-authored-by: OpenCode <noreply@opencode.ai>
7 participants
This PR pulls in the
BatchAuthenticatorpart of Espresso integration atpackages/contracts-bedrock/src/L1/BatchAuthenticator.sol. All other changes are deployment scripts, tests, and other wiring.BatchAuthenticatoris the contract that the batchers will have to authenticate their batches with; corresponding changes for the derivation pipeline and batchers forthcoming as subsequent PRs.