Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

implement ErrorOutOfGasCREATE error gadget #1562

Merged
merged 14 commits into from
Sep 8, 2023
Merged

implement ErrorOutOfGasCREATE error gadget #1562

merged 14 commits into from
Sep 8, 2023

Conversation

DreamWuGit
Copy link
Collaborator

@DreamWuGit DreamWuGit commented Aug 14, 2023 •
edited by KimiWu123
Loading

Description

implement ErrorOutOfGasCREATE error gadget, relies on PR #1558, only can be ready after that pr merged.

Issue Link

closed #1522

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

This PR contains:

  • oog create/create2 both use OOG::Create
  • circuit for oog create
  • tests for create/tx deploy cases

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member crate-mock Issues related to the mock workspace member crate-eth-types Issues related to the eth-types workspace member labels Aug 14, 2023
@github-actions github-actions bot removed the crate-mock Issues related to the mock workspace member label Aug 15, 2023
@DreamWuGit DreamWuGit marked this pull request as ready for review August 15, 2023 06:02
@KimiWu123 KimiWu123 mentioned this pull request Aug 16, 2023
Closed
KimiWu123
KimiWu123 reviewed Aug 16, 2023
View reviewed changes
Copy link
Contributor

@KimiWu123 KimiWu123 left a comment

Choose a reason for hiding this comment

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

First round pass.
However, I didn't see the oog case when is_root==true. Could you help complete is_root==true case?

zkevm-circuits/src/evm_circuit/execution/error_oog_create.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_create.rs Outdated Show resolved Hide resolved
@KimiWu123 KimiWu123 mentioned this pull request Aug 17, 2023
Merged
4 tasks
@KimiWu123
Copy link
Contributor

First round pass. However, I didn't see the oog case when is_root==true. Could you help complete is_root==true case?

Hi @DreamWuGit , after discussed with C.C., we'll have another PR to take care is_root==true case. You could keep working on the current work. Thanks.

ChihChengLiang
ChihChengLiang approved these changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for completing this! Please address Kimi's comment.

@ChihChengLiang ChihChengLiang added this pull request to the merge queue Sep 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 4, 2023
@ChihChengLiang
Copy link
Collaborator

@DreamWuGit It looks like the merge failed because it failed some tests in error_oog_create. Let us know if you'd like to fix this. We can also ignore the failing tests and create another issue to fix them.

github-merge-queue bot pushed a commit that referenced this pull request Sep 7, 2023
@KimiWu123 @lightsing
Feat/#1519 implement ErrorOutOfGasDynamicMemoryExpansion error state (#…
1d482c0 
…1567)

### Description

This PR was pulled from Scroll team
(scroll-tech#351) and
1. moved CREATE case to a sparated gadget, see
#1562.
2. supported uint64 overflow
3. updated the rlc part to fit our word lo/hi design.

### Issue Link

closed #1519 

### Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

---------

Co-authored-by: Akase Cho <lightsing@users.noreply.github.com>
@DreamWuGit
Copy link
Collaborator Author

@DreamWuGit It looks like the merge failed because it failed some tests in error_oog_create. Let us know if you'd like to fix this. We can also ignore the failing tests and create another issue to fix them.

@ChihChengLiang I 'll have a look today !

@DreamWuGit DreamWuGit added this pull request to the merge queue Sep 8, 2023
Merged via the queue into privacy-scaling-explorations:main with commit 887278c Sep 8, 2023
11 checks passed
@DreamWuGit DreamWuGit deleted the oog_create branch September 8, 2023 08:54
@KimiWu123 KimiWu123 mentioned this pull request Oct 12, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KimiWu123 KimiWu123 KimiWu123 approved these changes

@ChihChengLiang ChihChengLiang ChihChengLiang approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement ErrorOutOfGasCREATE2 error state
4 participants
@DreamWuGit @KimiWu123 @ChihChengLiang @lispc