Spec for ErrorInvalidOpcode state#362
Conversation
silathdiir
changed the title
ErrorInvalidOpcode stateErrorInvalidOpcode state
| opcode = instruction.opcode_lookup(True) | ||
| instruction.responsible_opcode_lookup(opcode) | ||
|
|
||
| op_gt_20, _ = instruction.compare(FQ(0x20), opcode, 1) |
There was a problem hiding this comment.
I'm wondering if is not cheaper just to do a lookup that 5 compares + one constrain per SEPARATE_INVALID_OPCODES
There was a problem hiding this comment.
After some investigation, I found that FixedTableTag.InvalidOpcode was deleted in PR #110 and @han0110 commented with #110 (comment).
I am sorry that I could not confirm if this 5 compares + one set constrain is correct. Or I should use fourth column auxiliary of ResponsibleOpcode to specify it's an invalid opcode. WDYT?
Also @han0110 may have some suggestions. Thanks.
There was a problem hiding this comment.
I'm wondering if is not cheaper just to do a lookup that 5 compares + one constrain per
SEPARATE_INVALID_OPCODES
if I remember correct, one lookup generate 4 gates 😄
There was a problem hiding this comment.
Sorry I have missed this, so I think the responsible_opcode_lookup already does the opcode check, since only the invalid opcode could pass the lookup, or perhaps I'm missing something?
The invalid opcode responsible opcodes are generated here:
There was a problem hiding this comment.
Yes. I see. I am sorry for my previous misunderstanding. It seems that responsible_opcode_lookup is enough, it has already lookup for the invalid opcode with ExecutionState.ErrorInvalidOpcode.
And it seems that I should also map the invalid_opcodes function to ExecutionState::ErrorInvalidOpcode in responsible_opcodes of zkevm-circuit. So I could do the same lookup in circuit. Thanks.
There was a problem hiding this comment.
Ah you are right! That's currently missed in circuit side.
There was a problem hiding this comment.
Thanks. Will update the code.
There was a problem hiding this comment.
Fixed to only do fixed lookup for ResponsibleOpcode in 721a1ca.
And also updated the circuit PR privacy-ethereum/zkevm-circuits#1089
| is_success = instruction.call_context_lookup(CallContextFieldTag.IsSuccess) | ||
| instruction.constrain_equal(is_success, FQ(0)) | ||
|
|
||
| # Go to EndTx only when is_root. | ||
| is_to_end_tx = instruction.is_equal(instruction.next.execution_state, ExecutionState.EndTx) | ||
| instruction.constrain_equal(FQ(instruction.curr.is_root), is_to_end_tx) | ||
|
|
||
| # When it's a root call. | ||
| if instruction.curr.is_root: | ||
| # Do step state transition. | ||
| instruction.constrain_step_state_transition( | ||
| rw_counter=Transition.delta(1 + instruction.curr.reversible_write_counter), | ||
| call_id=Transition.same(), | ||
| ) | ||
| else: | ||
| # When it is internal call, need to restore caller's state as finishing this call. | ||
| # Restore caller state to next StepState. | ||
| instruction.step_state_transition_to_restored_context( | ||
| rw_counter_delta=1 + instruction.curr.reversible_write_counter.n, | ||
| return_data_offset=FQ(0), | ||
| return_data_length=FQ(0), | ||
| gas_left=instruction.curr.gas_left, | ||
| ) |
There was a problem hiding this comment.
AFAIS this fragment of code repeats at:
- https://github.com/privacy-scaling-explorations/zkevm-specs/blob/a86dcdaee1c38720be2f71fcb71f81627de157b1/src/zkevm_specs/evm/execution/oog_constant.py#L22
- https://github.com/privacy-scaling-explorations/zkevm-specs/blob/a86dcdaee1c38720be2f71fcb71f81627de157b1/src/zkevm_specs/evm/execution/error_Invalid_jump.py#L33
and probally is going to repeat more times in the errors. I think that is a good idea to make a function in instruction like failed_opcode(rw_count) or something like
There was a problem hiding this comment.
Yes. I see. I found a similar circuit issue for common error gadget as privacy-ethereum/zkevm-circuits#1059
Will discuss with @DreamWuGit if we could update for both spec and circuit.
There was a problem hiding this comment.
Fixed to add a constrain_error_state function to Instruction, and update code of related error states in 9b2a262.
…repeated code for error states.
|
Hi @adria0, @DreamWuGit, @han0110 and @icemelon, I updated the code according to previous review, please help review again when you have time. Thanks. |
silathdiir
requested review from
adria0 and
han0110
and removed request for
adria0,
han0110 and
icemelon
…the hidden logic in `step_state_transition_to_restored_context`.
…ter.n` in OOG call. And updated test cases to set `reversible_write_counter = 0` for root call, and `reversible_write_counter = 2` (parent Call and Stop) for internal call.
… and `reversible_write_counter = caller_reversible_write_counter` in STOP step.
|
file names not consistent? error_invalid_opcode.py and error_Invalid_jump Invalid / invalid upper case small case |
Renamed |
5 participants
Close #350
Circuit PR privacy-ethereum/zkevm-circuits#1089
Reference https://www.evm.codes and verifies invalid opcode if any below condition is met:opcode > 0x20 && opcode < 0x30opcode > 0x48 && opcode < 0x50opcode > 0xA4 && opcode < 0xF0one of[0x0C, 0x0D, 0x0E, 0x0F, 0x1E, 0x1F, 0x5C, 0x5D, 0x5E, 0x5F, 0xF6, 0xF7, 0xF8, 0xF9, 0xFB, 0xFC, 0xFE]