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

implement oog sha3 error state #1558

Merged
merged 8 commits into from
Aug 15, 2023
Merged

implement oog sha3 error state #1558

merged 8 commits into from
Aug 15, 2023

Conversation

DreamWuGit
Copy link
Collaborator

@DreamWuGit DreamWuGit commented Aug 9, 2023 •
edited
Loading

Description

implement oog sha3 error state

Issue Link

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:

  • refactor memory address gadget to handle overflow condition
  • add oog sha3 gadget
  • add tests

@github-actions github-actions bot added crate-zkevm-circuits Issues related to the zkevm-circuits workspace member crate-eth-types Issues related to the eth-types workspace member labels Aug 9, 2023
@ChihChengLiang ChihChengLiang linked an issue Aug 9, 2023 that may be closed by this pull request
Implement ErrorOutOfGasSHA3 error state #1520
Closed
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-mock Issues related to the mock workspace member labels Aug 10, 2023
@DreamWuGit DreamWuGit marked this pull request as ready for review August 11, 2023 10:39
ChihChengLiang
ChihChengLiang reviewed Aug 11, 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.

This is the first round of review. I added some quick feedback on nitpicks.

I noticed that we have MemoryExpandedAddressGadget, but no checks are performed.

zkevm-circuits/src/evm_circuit/util/memory_gadget.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/util/memory_gadget.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/util/memory_gadget.rs Outdated Show resolved Hide resolved
mock/src/lib.rs Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_sha3.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_sha3.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/util/memory_gadget.rs Outdated Show resolved Hide resolved
@ChihChengLiang ChihChengLiang self-requested a review August 11, 2023 16:41
KimiWu123
KimiWu123 approved these changes Aug 14, 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.

LGTM

zkevm-circuits/src/evm_circuit/util/memory_gadget.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/util/memory_gadget.rs Outdated Show resolved Hide resolved
@DreamWuGit DreamWuGit mentioned this pull request Aug 14, 2023
Merged
4 tasks
ChihChengLiang
ChihChengLiang approved these changes Aug 14, 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. I have a question for the trait CommonMemoryAddressGadget

zkevm-circuits/src/evm_circuit/util/memory_gadget.rs
@@ -61,6 +69,32 @@ pub(crate) mod address_high {
}
}

/// Memory address trait to adapt for right and Uint overflow cases.
pub(crate) trait CommonMemoryAddressGadget<F: Field> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the CommonMemoryAddessGadget trait necessary?
We only use MemoryAddressGadget for the no overflow case, and we only use MemoryExpandedAddressGadget for Sha3 oog when the overflow is possible.
In the current PR it seems the traits are not super useful as we already know the concrete struct in the function body that consumes it.

I don't plan to block the PR longer, but it would be great if you could share some of the rationale for adding the trait.

Copy link
Collaborator Author

@DreamWuGit DreamWuGit Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the rationale is to design common methods for normal and error memory address gadget use , they both have offset, length etc. we can see successful opcode gadgets also use it. @ChihChengLiang does it make sense to you?

Copy link
Collaborator Author

@DreamWuGit DreamWuGit Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it is also used in oog static/dynamic memory(Kimi working on it) error cases etc.cases

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for sharing the rationale. I think it makes sense to me.

@DreamWuGit DreamWuGit added this pull request to the merge queue Aug 15, 2023
Merged via the queue into privacy-scaling-explorations:main with commit 5d01150 Aug 15, 2023
11 checks passed
@DreamWuGit DreamWuGit deleted the oog_sha3 branch August 15, 2023 05:45
@DreamWuGit DreamWuGit mentioned this pull request Aug 15, 2023
Merged
4 tasks
github-merge-queue bot pushed a commit that referenced this pull request Sep 8, 2023
@DreamWuGit @lispc
implement ErrorOutOfGasCREATE error gadget (#1562)
887278c 
### Description

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

### Issue Link

closed
[#1522](#1522)

### 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




This PR contains:
- oog create/create2 both use OOG::Create
- circuit for oog create
- tests for create/tx deploy cases

---------

Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ChihChengLiang ChihChengLiang ChihChengLiang approved these changes

@KimiWu123 KimiWu123 KimiWu123 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-mock Issues related to the mock 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 ErrorOutOfGasSHA3 error state
3 participants
@DreamWuGit @ChihChengLiang @KimiWu123