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

Feat/#1519 implement ErrorOutOfGasDynamicMemoryExpansion error state #1567

Merged
merged 7 commits into from
Sep 7, 2023
Merged

Feat/#1519 implement ErrorOutOfGasDynamicMemoryExpansion error state #1567

merged 7 commits into from
Sep 7, 2023

Conversation

KimiWu123
Copy link
Contributor

@KimiWu123 KimiWu123 commented Aug 16, 2023 •
edited
Loading

Description

This PR was pulled from Scroll team (scroll-tech#351) and

  1. moved CREATE case to a sparated gadget, see implement ErrorOutOfGasCREATE error gadget #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)
  • 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

@lightsing @KimiWu123
add oog dynamic mem expand (#351)
a05e4f4 
* add base impl

* fix stack read

* fix merge

* apply review
@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 labels Aug 16, 2023
@KimiWu123 KimiWu123 force-pushed the feat/#1519-Implement-ErrorOutOfGasDynamicMemoryExpansion branch from 3d23a0e to 01e626b Compare August 16, 2023 03:33
@KimiWu123 KimiWu123 requested a review from miha-stopar August 17, 2023 01:56
KimiWu123
KimiWu123 commented Aug 17, 2023
View reviewed changes
zkevm-circuits/src/evm_circuit/execution/error_code_store.rs Outdated
memory_address.offset(),
memory_address.length(),
);
let common_error_gadget = CommonErrorGadget::construct(cb, opcode.expr(), 5.expr());
Copy link
Contributor Author

@KimiWu123 KimiWu123 Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could see the implementation of geth, it only assigns returnData when this call reverts. Otherwise it clear the returnData. So, here we shouldn't assign offset and length.

KimiWu123
KimiWu123 commented Aug 17, 2023
View reviewed changes
zkevm-circuits/src/evm_circuit/execution/error_invalid_creation_code.rs Outdated
memory_address.offset(),
memory_address.length(),
);
let common_error_gadget = CommonErrorGadget::construct(cb, opcode.expr(), 5.expr());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, we shouldn't assign offset and length.

miha-stopar
miha-stopar approved these changes Aug 17, 2023
View reviewed changes
Copy link
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

LGTM!

@KimiWu123 KimiWu123 force-pushed the feat/#1519-Implement-ErrorOutOfGasDynamicMemoryExpansion branch from 0b4f128 to ddb55fd Compare August 22, 2023 02:50
@KimiWu123 KimiWu123 requested a review from miha-stopar August 22, 2023 03:23
@KimiWu123
Copy link
Contributor Author

KimiWu123 commented Aug 22, 2023 •
edited
Loading

LGTM!

@miha-stopar , could you help review this PR again. I made some changes,

  1. support uint64 overflow, using MemoryExpandedAddressGadget instead of MemoryAddressGadget
  2. remove error_oog_dynamic_memory.rs under bus-mapping and using stackonlyop.rs to replace with

and there was only one commit, ddb55fd.

@ChihChengLiang ChihChengLiang added benchmarks: Super Triggers a Super Circuit benchmark and removed benchmarks: Super Triggers a Super Circuit benchmark labels Aug 23, 2023
@davidnevadoc davidnevadoc self-requested a review August 23, 2023 11:35
@KimiWu123 KimiWu123 force-pushed the feat/#1519-Implement-ErrorOutOfGasDynamicMemoryExpansion branch from 9e4e182 to a2a7b34 Compare August 31, 2023 03:36
davidnevadoc
davidnevadoc approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@davidnevadoc davidnevadoc left a comment

Choose a reason for hiding this comment

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

LGTM!

@KimiWu123 KimiWu123 added this pull request to the merge queue Sep 7, 2023
Merged via the queue into main with commit 1d482c0 Sep 7, 2023
@KimiWu123 KimiWu123 deleted the feat/#1519-Implement-ErrorOutOfGasDynamicMemoryExpansion branch September 7, 2023 01:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@miha-stopar miha-stopar miha-stopar approved these changes

@davidnevadoc davidnevadoc davidnevadoc approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping 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 ErrorOutOfGasDynamicMemoryExpansion error state
5 participants
@KimiWu123 @miha-stopar @davidnevadoc @ChihChengLiang @lightsing