Skip to content
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

WEB3-171: OP Steel #267

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
90c8b6d
add op-steel
Wollac Oct 11, 2024
3b3e10c
add examples
Wollac Oct 11, 2024
0114452
sort dependencies
Wollac Oct 11, 2024
905e836
Merge branch 'main' into feat/op-steel
Wollac Oct 11, 2024
4c04ce6
sort dependencies
Wollac Oct 11, 2024
f8dbe60
add copyright
Wollac Oct 11, 2024
62d9631
add copyright
Wollac Oct 11, 2024
8692fd2
fix clippy warnings
Wollac Oct 11, 2024
d78b501
cleanups
Wollac Oct 16, 2024
0a16a58
Merge branch 'main' into feat/op-steel
Wollac Oct 28, 2024
9149c51
update alloy to v0.5
Wollac Oct 28, 2024
c280ce1
address review comments
Wollac Oct 29, 2024
7cd5fc0
sort dependencies
Wollac Oct 29, 2024
8f125e9
minor cleanups
Wollac Oct 30, 2024
6f6766a
Merge branch 'main' into feat/op-steel
Wollac Nov 8, 2024
dc13905
Merge branch 'main' into feat/op-steel
Wollac Nov 18, 2024
729284a
remove opt-level
Wollac Nov 18, 2024
4a95ac8
run forge test before cargo test
Wollac Nov 21, 2024
983c5b4
Merge remote-tracking branch 'origin/main' into feat/op-steel
Wollac Nov 21, 2024
080d328
run cargo run -F verify in the CI
Wollac Nov 21, 2024
9afa7c4
do not test all features in the examples
Wollac Nov 21, 2024
c92c479
add build script
Wollac Nov 21, 2024
164724a
test all features
Wollac Nov 21, 2024
1c52764
fix build
Wollac Nov 21, 2024
10d3c83
install foundry before clippy
Wollac Nov 21, 2024
153abd4
checkout submodules for clippy
Wollac Nov 21, 2024
8e7c544
fix build script
Wollac Nov 21, 2024
6e3638f
revert cargo test changes
Wollac Nov 21, 2024
e942ccc
print dir
Wollac Nov 21, 2024
dc82d73
fix foundry root
Wollac Nov 21, 2024
5d96a92
fix source path
Wollac Nov 21, 2024
e4bf626
fix beacon API URL
Wollac Nov 21, 2024
5c6b4d0
use alchemy
Wollac Nov 21, 2024
975d3d2
add logging when run-verify
Wollac Nov 21, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .github/scripts/run-verify.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/bash
set -e

run_verify(){
while read path; do
printf "Project: %s\n" "$path"
cargo run -F verify --manifest-path "$path"
done
}

grep -rlz --include "Cargo.toml" 'verify\s*=\s*\[[^\[]*\]' | sort -u | run_verify
12 changes: 12 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ jobs:
# This is a workaround from: https://github.com/actions/checkout/issues/590#issuecomment-970586842
- run: "git checkout -f $(git -c user.name=x -c user.email=x@x commit-tree $(git hash-object -t tree /dev/null) < /dev/null) || :"
- uses: actions/checkout@v4
with:
submodules: recursive
- if: matrix.feature == 'cuda'
uses: risc0/risc0/.github/actions/cuda@main
- uses: risc0/risc0/.github/actions/rustup@main
Expand All @@ -95,6 +97,7 @@ jobs:
ref: ${{ env.RISC0_MONOREPO_REF }}
toolchain-version: ${{ env.RISC0_TOOLCHAIN_VERSION }}
features: ${{ matrix.feature }}
- uses: risc0/foundry-toolchain@2fe7e70b520f62368a0e3c464f997df07ede420f
- name: cargo clippy risc0-ethereum
run: cargo clippy --workspace --all-targets --all-features
env:
Expand Down Expand Up @@ -193,6 +196,15 @@ jobs:
- name: forge test all examples
run: ../.github/scripts/forge-test.sh
working-directory: examples
- name: cargo run -F verify all examples
run: ../.github/scripts/run-verify.sh
working-directory: examples
env:
RUST_LOG: "info,risc0_steel=debug,risc0_op_steel=debug"
RISC0_DEV_MODE: true
L1_RPC_URL: https://eth-mainnet.g.alchemy.com/v2/${{ secrets.ALCHEMY_RISC0_ETH_API_KEY }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we run the CI against the actual live network, an alternative would be be to spin up a local docker devnet containing at least an L1, L2, and Beacon node

L2_RPC_URL: https://opt-mainnet.g.alchemy.com/v2/${{ secrets.ALCHEMY_RISC0_ETH_API_KEY }}
BEACON_API_URL: https://ethereum-beacon-api.publicnode.com
- run: sccache --show-stats

doc:
Expand Down
8 changes: 6 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[workspace]
resolver = "2"
members = ["build", "contracts", "ffi", "ffi/guests", "steel"]
members = ["build", "contracts", "crates/*", "ffi", "ffi/guests"]

[workspace.package]
version = "1.2.0-alpha.1"
Expand All @@ -13,8 +13,9 @@ repository = "https://github.com/risc0/risc0-ethereum/"
# Intra-workspace dependencies
risc0-build-ethereum = { version = "1.2.0-alpha.1", default-features = false, path = "build" }
risc0-ethereum-contracts = { version = "1.2.0-alpha.1", default-features = false, path = "contracts" }
risc0-steel = { version = "0.14.0-alpha.1", default-features = false, path = "steel" }
risc0-steel = { version = "0.14.0-alpha.1", default-features = false, path = "crates/steel" }
risc0-forge-ffi = { version = "1.2.0-alpha.1", default-features = false, path = "ffi" }
risc0-op-steel = { version = "0.1.0-alpha.1", default-features = false, path = "crates/op-steel" }

# risc0 monorepo dependencies.
risc0-build = { git = "https://github.com/risc0/risc0", branch = "main", default-features = false }
Expand All @@ -27,6 +28,9 @@ alloy-rlp = { version = "0.3.8" }
alloy-primitives = { version = "0.8.8" }
alloy-sol-types = { version = "0.8.8" }

# OP Steel
op-alloy-network = { version = "0.6" }

# Alloy host dependencies
alloy = { version = "0.6.1" }
alloy-trie = { version = "0.7" }
Expand Down
95 changes: 95 additions & 0 deletions contracts/src/steel/OpSteel.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Copyright 2024 RISC Zero, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
// SPDX-License-Identifier: Apache-2.0

pragma solidity ^0.8.9;

import {Encoding, Steel} from "./Steel.sol";

abstract contract OpCommitmentValidator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to be clear, this is the contract deployed on L1 to validate queries against OP L2 data, ya? It's always hard to keep these things straight (versus verifying on L2 querying L1). Adding a docstring contract itself to state this inline might be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really unsure what the best Solidity design would be:
Ideally, I wanted a wrapper (providing the same validateCommitment function) around the Steel library that can also validate the new OP commitment (to verify an L2 proof on L1) unfortunately this requires the address of the OptimismPortal2 contract.

Alternatively, OpSteel could be a library only providing validateOpCommitment(IOptimismPortal2 optimismPortal, Steel.Commitment memory commitment) which fails if commitment does not have the correct version.

Or have this an (abstract) contract to store the address but only provide the above method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this an abstract contract? If concrete, it would still be possible to inherit from, while also being possible to deploy and call as an external dependency. Although they are internal now, the validation functions seem like they'd be reasonable to make public.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of encouraging people to inherit the OpCommitmentValidator, as it would be closer to the library we already have for steel.
I am not sure if there is a real demand to make this contract publicly available, as the actual validation code is rather small and simple, and calling an external contract would increase the cost of gas.

/// @notice Address of the OptimismPortal2 contract.
IOptimismPortal2 public immutable optimismPortal;

constructor(address _optimismPortal) {
optimismPortal = IOptimismPortal2(_optimismPortal);
}

/// @notice Validates a Steel commitment.
/// @param commitment The commitment to validate.
/// @return True if the commitment is valid, false otherwise.
function validateCommitment(Steel.Commitment memory commitment) internal view returns (bool) {
nategraf marked this conversation as resolved.
Show resolved Hide resolved
(uint240 blockID, uint16 version) = Encoding.decodeVersionedID(commitment.id);
if (version == 0x100) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What can be do to reduce the chance of version number collisions causing issues as we expand support? Should we perhaps do something more like having a registry, or perhaps making is a "selector" that is resistant to accidental collisions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I like this idea. We could use sha256("Beacon") or something like that for the selector. However, I see two complications:

  • The version is only a uint16, with only two bytes there could be accidental collisions. (However, it might be possible to change this to a uint32 without breaking anything.)
  • Currently we have 0x00, 0x01 for block and beacon. So I guess it would be a breaking change if we changed this to a selector as well, and using a selector only for other types is also weird.

return validateDisputeGameCommitment(blockID, commitment.digest);
} else {
return Steel.validateCommitment(commitment);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! So this contract will validate L1 queries as well. This seems surprising to me, and potentially could cause issues if the caller thinks they are verifying a query against the L2. In particular, someone might use L1 state when and produce a proof when they should have used L2 state, and perhaps bypass some kind of authentication as a result. (e.g. maybe the query is to confirm tokens have been bridged from L1 to L2, and the prover chooses not to actually bridge the tokens, but produce a proof using the L1 balance).

This raises an interesting point about how we bind the chain we are querying in the interface. My intuition here would be to have a distinct contract deployment for each chain that can be queried, but another way to do this might be to have a function that additionally takes the chain ID of the chain we are expecting to query. Of course, the guest could also bind this, but that doesn't quite seem like the right pattern to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One solution to this would be to check the configID against an immutable value on this contract. We might want to apply this to the functionality in Steel.sol as well. I understand that it slightly more difficult because that is currently a library. We might discuss what the right solution is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree 100%, especially with different chains and configID, we should rethink our Steel Solidity design. Unfortunately one library is not enough anymore and we should discuss what the right solution could be.

}
}

/// @notice Validates a Dispute Game commitment.
/// @param gameIndex The index of the game in the DisputeGameFactory.
/// @param rootClaim The root claim of the dispute game.
/// @return True if the commitment is valid, false otherwise.
function validateDisputeGameCommitment(uint256 gameIndex, bytes32 rootClaim) internal view returns (bool) {
IDisputeGameFactory factory = optimismPortal.disputeGameFactory();

// Retrieve game information from the factory.
(uint32 gameType, uint64 createdAt, IDisputeGame game) = factory.gameAtIndex(gameIndex);

// The game type of the dispute game must be the respected game type.
if (gameType != optimismPortal.respectedGameType()) return false;
// The game must have been created after `respectedGameTypeUpdatedAt`.
if (createdAt < optimismPortal.respectedGameTypeUpdatedAt()) return false;
// The game must be resolved in favor of the root claim (the output proposal).
if (game.status() != GameStatus.DEFENDER_WINS) return false;
// The game must have been resolved for at least `proofMaturityDelaySeconds`.
if (block.timestamp - game.resolvedAt() <= optimismPortal.proofMaturityDelaySeconds()) return false;
// The game must not be blacklisted.
if (optimismPortal.disputeGameBlacklist(game)) return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the OP design here because it makes validateDisputeGameCommitment non-monotonic:

  • disputeGameBlacklist only returns a boolean, not a timestamp
  • If the guardian blacklists a game long after its maturity delay at time $ts$, validateDisputeGameCommitment will succeed before $ts$, but then suddenly fail when rerunning on the same input after $ts$

I don't see a good fix for this. I guess removing the disputeGameBlacklist check would even lead to more inconsistencies.


// Finally, verify that the provided root claim matches the game's root claim.
return game.rootClaim() == rootClaim;
}
}

// https://github.com/ethereum-optimism/optimism/blob/v1.9.3/packages/contracts-bedrock/src/L1/interfaces/IOptimismPortal2.sol
interface IOptimismPortal2 {
function disputeGameBlacklist(IDisputeGame) external view returns (bool);
function disputeGameFactory() external view returns (IDisputeGameFactory);
function proofMaturityDelaySeconds() external view returns (uint256);
function respectedGameType() external view returns (uint32);
function respectedGameTypeUpdatedAt() external view returns (uint64);
function version() external pure returns (string memory);
}

// https://github.com/ethereum-optimism/optimism/blob/v1.9.3/packages/contracts-bedrock/src/dispute/interfaces/IDisputeGameFactory.sol
interface IDisputeGameFactory {
function gameCount() external view returns (uint256);
function gameAtIndex(uint256 index) external view returns (uint32 gameType, uint64 createdAt, IDisputeGame game);
}

// https://github.com/ethereum-optimism/optimism/blob/v1.9.3/packages/contracts-bedrock/src/dispute/interfaces/IDisputeGame.sol
interface IDisputeGame {
function status() external view returns (GameStatus);
function resolvedAt() external view returns (uint64);
function rootClaim() external pure returns (bytes32);
}

// https://github.com/ethereum-optimism/optimism/blob/v1.9.3/packages/contracts-bedrock/src/dispute/lib/Types.sol
enum GameStatus {
IN_PROGRESS,
CHALLENGER_WINS,
DEFENDER_WINS
}
35 changes: 35 additions & 0 deletions crates/op-steel/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
[package]
name = "risc0-op-steel"
description = "Optimism abstraction for Steel."
version = "0.1.0-alpha.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a good versioning for op-steel?
Should we also mark it as unstable or something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think marking it as it's own 0.x version like this makes sense to me. Calling it 0.x signals that it's unstable.

rust-version = "1.81"
edition = { workspace = true }
license = { workspace = true }
homepage = { workspace = true }
repository = { workspace = true }

[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]

[dependencies]
alloy = { workspace = true, optional = true, features = ["contract", "providers"] }
alloy-primitives = { workspace = true, features = ["rlp", "serde"] }
alloy-sol-types = { workspace = true }
anyhow = { workspace = true }
log = { workspace = true }
op-alloy-network = { workspace = true }
revm = { workspace = true, features = ["std", "optimism"] }
risc0-steel = { workspace = true }
serde = { workspace = true }
test-log = { workspace = true }
thiserror = { workspace = true }
tokio = { workspace = true, optional = true }
url = { workspace = true, optional = true }

[dev-dependencies]
risc0-op-steel = { workspace = true, features = ["host"] }

[features]
default = []
host = ["dep:alloy", "dep:tokio", "dep:url", "risc0-steel/host", "alloy-primitives/rand"]
Loading
Loading