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-213: add SteelVerifier to verify Steel commitments in the guest #319

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Wollac
Copy link
Contributor

@Wollac Wollac commented Nov 4, 2024

This PR introduces the SteelVerifier, which acts as a built-in Steel Contract to verify Steel commitments. It is used like any other Contract, during the preflight step and in the guest. This functionality is currently marked unstable and must be enabled using the unstable-verifier feature.

The `TokenStats' example has been updated to use the verifier to sample the stats over two datapoints at different times.

@github-actions github-actions bot changed the title add Verifier to verify Steel commitments in the guest WEB3-213: add Verifier to verify Steel commitments in the guest Nov 4, 2024
@Wollac Wollac changed the title WEB3-213: add Verifier to verify Steel commitments in the guest WEB3-213: add SteelVerifier to verify Steel commitments in the guest Nov 7, 2024
@nategraf
Copy link
Contributor

Need to clean-up the way preflight is factored on the host

@Wollac Wollac marked this pull request as ready for review December 16, 2024 15:55
@Wollac Wollac requested review from capossele and a team as code owners December 16, 2024 15:55
/// Implement [BlockHeaderCommit] for the unit type.
/// This makes it possible to treat an `HostEvmEnv<D, H, ()>`, which is used for the [BlockInput]
/// in the same way as any other `HostEvmEnv<D, H, BlockHeaderCommit>`.
impl<H: EvmBlockHeader> BlockHeaderCommit<H> for () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels very strange to implement a trait for an empty tuple, but here it could actually be kind of intuitive...


impl<D, H: EvmBlockHeader, C: Clone + BlockHeaderCommit<H>> HostEvmEnv<D, H, C> {
/// Returns the [Commitment] used to validate the environment.
pub fn commitment(&self) -> Commitment {
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 that the corresponding commitment(&self) method on a GuestEvmEnv returns &Commitment, but I don't know how to fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think this is alright. In most cases, I don't think devs will even notice.

@Wollac Wollac requested a review from nategraf December 16, 2024 16:38
Comment on lines +32 to +35
/// Implement [BlockHeaderCommit] for the unit type.
/// This makes it possible to treat an `HostEvmEnv<D, H, ()>`, which is used for the [BlockInput]
/// in the same way as any other `HostEvmEnv<D, H, BlockHeaderCommit>`.
impl<H: EvmBlockHeader> BlockHeaderCommit<H> for () {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the following empty-type instead of unit-type. This is more clear and explicit, but I think implementing it on () also makes sense. Up to you.

Suggested change
/// Implement [BlockHeaderCommit] for the unit type.
/// This makes it possible to treat an `HostEvmEnv<D, H, ()>`, which is used for the [BlockInput]
/// in the same way as any other `HostEvmEnv<D, H, BlockHeaderCommit>`.
impl<H: EvmBlockHeader> BlockHeaderCommit<H> for () {
struct DirectCommit;
/// Implement [BlockHeaderCommit] for the unit type.
/// This makes it possible to treat an `HostEvmEnv<D, H, DirectCommit>`, which is used for the [BlockInput]
/// in the same way as any other `HostEvmEnv<D, H, BlockHeaderCommit>`.
impl<H: EvmBlockHeader> BlockHeaderCommit<H> for DirectCommit {

steel/src/history/beacon_roots.rs Show resolved Hide resolved
steel/src/history/beacon_roots.rs Outdated Show resolved Hide resolved
steel/src/history/beacon_roots.rs Outdated Show resolved Hide resolved

impl<D, H: EvmBlockHeader, C: Clone + BlockHeaderCommit<H>> HostEvmEnv<D, H, C> {
/// Returns the [Commitment] used to validate the environment.
pub fn commitment(&self) -> Commitment {
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think this is alright. In most cases, I don't think devs will even notice.

Comment on lines +145 to +146
let block_number: u64 = block_number.saturating_to();
let diff = header.number().saturating_sub(block_number);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be try_into and checked_sub. I suppose these cases would be caught be the subsequent checks (e.g. there is no block hash in the DB for a future block number) but it seems prudent to exist early here IMO.

Suggested change
let block_number: u64 = block_number.saturating_to();
let diff = header.number().saturating_sub(block_number);
let block_number: u64 = block_number.try_into().context("invalid block number >= 2^64")?;
let diff = header.number().checked_sub(block_number).context("block number is greater than header block number")?;

Co-authored-by: Victor Snyder-Graf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants