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

feat: add OOG error handling for CALLCODE, DELEGATECALL and STATICCALL to ErrorOOGCall#368

Merged
lispc merged 5 commits into
privacy-ethereum:masterfrom
scroll-tech:feat/add-callcode-delegatecall-staticcall-oog-errors
Feb 6, 2023
Merged

feat: add OOG error handling for CALLCODE, DELEGATECALL and STATICCALL to ErrorOOGCall#368
lispc merged 5 commits into
privacy-ethereum:masterfrom
scroll-tech:feat/add-callcode-delegatecall-staticcall-oog-errors

Conversation

@silathdiir
Copy link
Copy Markdown
Contributor

@silathdiir silathdiir commented Feb 2, 2023 •
edited
Loading

Close #360

Circuit PR privacy-ethereum/zkevm-circuits#1127

Summary

  1. Merge ExecutionState.ErrorOutOfGasCALLCODE, ErrorOutOfGasDELEGATECALL and ErrorOutOfGasSTATICCALL into ExecutionState.ErrorOutOfGasCall.

  2. Update both spec and test code for OOG call.

This was referenced Feb 2, 2023
Closed
Merged
@silathdiir silathdiir force-pushed the feat/add-callcode-delegatecall-staticcall-oog-errors branch from 7ad9d96 to a39c246 Compare February 2, 2023 10:28
@han0110 han0110 self-requested a review February 6, 2023 05:31
han0110
han0110 reviewed Feb 6, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

Comment thread src/zkevm_specs/evm/execution/oog_call.py Outdated
@silathdiir
Fix to set rw_counter = CONSTANT + callee_reversible_write_counter …
137f447 
…and `reversible_write_counter = caller_reversible_write_counter` in STOP step.
han0110
han0110 approved these changes Feb 6, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM!

@silathdiir
Copy link
Copy Markdown
Contributor Author

silathdiir commented Feb 6, 2023

Fixed the last step (STOP) of internal call test cases as:

# Add `callee_reversible_write_counter` to its rw_counter constant to verify reverted.
rw_counter = CONSTANT + callee_reversible_write_counter

# Set its (`caller`) reversible_write_counter.
reversible_write_counter=caller_reversible_write_counter

I also update these fixes to previous PR of ErrorInvalidOpcode #362 (not confirm which will be merged first).

Hi @han0110, I have a simple question about callee_reversible_write_counter = 2, is it only set for testing? Not the real value of reversible_write_counter. Thanks.

@han0110
Copy link
Copy Markdown
Contributor

han0110 commented Feb 6, 2023

I have a simple question about callee_reversible_write_counter = 2, is it only set for testing?

Yes, just setting it to non-zero value for testing, and 2 is chosen because BeginTx always does 2 reversible write (balance update) for the underlying call (so actually it shouldn't have been 0).

lispc
lispc reviewed Feb 6, 2023
View reviewed changes
Comment thread tests/evm/test_oog_call.py
.tx_access_list_account_read(1, callee.address, is_warm_access) \
.call_context_read(1, CallContextFieldTag.IsSuccess, 0)
# fmt on

Copy link
Copy Markdown
Collaborator

@lispc lispc Feb 6, 2023

Choose a reason for hiding this comment

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

fmt on or fmt: on?

Copy link
Copy Markdown
Contributor Author

@silathdiir silathdiir Feb 6, 2023

Choose a reason for hiding this comment

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

Fixed to fmt: on in both line 169 and 260 in 94fe42d

@silathdiir silathdiir requested a review from lispc February 6, 2023 10:58
@lispc lispc merged commit bdf8823 into privacy-ethereum:master Feb 6, 2023
@lispc lispc deleted the feat/add-callcode-delegatecall-staticcall-oog-errors branch February 6, 2023 10:58
@silathdiir silathdiir mentioned this pull request Feb 6, 2023
Closed
@KimiWu123 KimiWu123 mentioned this pull request Jul 6, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lispc lispc Awaiting requested review from lispc

1 more reviewer

@han0110 han0110 han0110 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Handle OOG errors for CALLCODE, DELEGATECALL and STATICCALL

3 participants

@silathdiir @han0110 @lispc