-
Notifications
You must be signed in to change notification settings - Fork 857
handle gas uint overflow (ErrorGasUintOverflow) #1564
handle gas uint overflow (ErrorGasUintOverflow) #1564
Conversation
Hi @DreamWuGit , we decided to handle |
1db3850
to
1d267b6
Compare
…evm-circuits into gas_uint_overflow
@ChihChengLiang @KimiWu123 can have a look now. |
There was a problem hiding this 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(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_memory_copy.rs
Outdated
Show resolved
Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_memory_copy.rs
Outdated
Show resolved
Hide resolved
zkevm-circuits/src/evm_circuit/execution/error_oog_memory_copy.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
How Has This Been Tested?
all tests pass
This PR contains: