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

handle gas uint overflow (ErrorGasUintOverflow) #1564

Merged

Conversation

DreamWuGit
Copy link
Collaborator

@DreamWuGit DreamWuGit commented Aug 15, 2023

Description

currently ErrorGasUintOverflow is handled within normal OOG cases, so distribute ErrorGasUintOverflow checking within each OOG gadgets (oog sha3, oog static/dynamic memory expansion, oog log etc.)
the MemoryExpandedAddressGadget is responsible for overflow status.

Issue Link

[link issue here]

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

How Has This Been Tested?

all tests pass

This PR contains:

  • Cleanup of xxxx, yyyy
  • Changed xxxx to yyyy in order to bla bla
  • Added xxxx function to ...
  • Refactored ....

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Aug 15, 2023
@KimiWu123
Copy link
Contributor

Hi @DreamWuGit , we decided to handle is_root == true in another PR and you could keep working on this if you're available. Thanks.

@DreamWuGit DreamWuGit marked this pull request as ready for review September 12, 2023 08:08
@DreamWuGit
Copy link
Collaborator Author

@ChihChengLiang @KimiWu123 can have a look now.

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 noticed some refactored code got reversed.
Feel free to fix them or not. If you are hurry we can merge it and we can have someone from our side fix them in the subsequent PRs.
Thank you for upstreaming the refactor and adding tests.

let common_error_gadget = CommonErrorGadget::construct(
cb,
opcode.expr(),
11.expr() + is_call.expr() + is_callcode.expr(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed we fall back to manual offset counting here. Is it because cb.rw_counter_offset() doesn't work here?

Copy link
Collaborator Author

@DreamWuGit DreamWuGit Sep 13, 2023

Choose a reason for hiding this comment

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

no, it works, but I feel cb.rw_counter_offset() is indirect way for calculating the rw offset. we can not know how the rwc change by reading the code directly, it is always right. I prefer old way of it. so keep it when i touch it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

issue #1599 for discussion .

zkevm-circuits/src/evm_circuit/execution/error_oog_call.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_call.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_log.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_log.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_log.rs Outdated Show resolved Hide resolved
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

@DreamWuGit DreamWuGit added this pull request to the merge queue Sep 13, 2023
Merged via the queue into privacy-scaling-explorations:main with commit c67b4e8 Sep 13, 2023
13 checks passed
@DreamWuGit DreamWuGit deleted the gas_uint_overflow branch September 13, 2023 04:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants