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

refactor #17951

Merged
merged 50 commits into from
Aug 20, 2024
Merged

refactor #17951

merged 50 commits into from
Aug 20, 2024

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Aug 19, 2024

No description provided.

@dantaik dantaik marked this pull request as ready for review August 19, 2024 04:34
Copy link

openzeppelin-code bot commented Aug 19, 2024

refactor

Generated at commit: 12e77f173178c0eedb3af7fbd1d2a5ec77f3b42f

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
8
42
54
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

/// @notice This contract is a verifier for the Mainnet ZkVM that composes RiscZero and SP1
/// Verifiers.
/// @custom:security-contact security@taiko.xyz
contract ZkVMVerifier is ComposeVerifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just think maybe also impl TEEVerifier in this PR since the framework code is exactly the same.

@dantaik dantaik marked this pull request as draft August 20, 2024 02:51
@dantaik dantaik marked this pull request as ready for review August 20, 2024 04:04
@dantaik dantaik marked this pull request as draft August 20, 2024 04:06
@dantaik dantaik requested a review from smtmfft August 20, 2024 05:32
@dantaik dantaik marked this pull request as draft August 20, 2024 07:16
Comment on lines +43 to +44
for (uint256 i; i < verifiers.length; ++i) {
// Store the value 1 in the temporary storage slot using inline assembly
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not we have a safeguard to not possibly reuse verifiers ? 🤔

Something like:

address prevVerifier;
for (uint256 i; i < verifiers.length; ++i) {
    if (uint160(prevVerifier) >= uint160(verifiers[i])) {
                    revert L1_INVALID_VERIFIER();
    }
    ...
    prevVerifier = verifiers[i];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The verifier is coded by us and is not dynamically configured by proposers/provers.

This reverts commit e6aa39d.
This reverts commit 1ed0b70.
This reverts commit 392cc42.
This reverts commit 40f6ea6.
This reverts commit 666697b.
@dantaik dantaik changed the base branch from main to refactor_tiers August 20, 2024 10:44
@dantaik dantaik changed the title feat(protocol): make tier verifier composable from sub verifeirs refactor Aug 20, 2024
@dantaik dantaik marked this pull request as ready for review August 20, 2024 10:45
@dantaik dantaik merged commit abc509f into refactor_tiers Aug 20, 2024
4 of 5 checks passed
@dantaik dantaik deleted the compose_verifiers branch August 20, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants