implement insufficient balance state #313
Conversation
There was a problem hiding this comment.
Thanks for the work and the PR!
I have a couple of concerns regarding the incompleteness of the opcodes to support (which might cause significant changes later in the spec and possibly even bigger in the implementation) and also in the tests.
We should find a common ground where we have enough things speced and at the same time doesn't imply a titanic work now and we can leaverage some stuff to the future.
What I guess I want to say is that is "OK" to not include CREATE and CREATE2 maybe (although I'll do it). But at least the design of the specs should already consider all of them. As for example, we will need a selection method for the OPCODE such a lagrange polynomial that only allows 1 of the OPCODES to be selected.
|
|
||
| ### circuit behavior | ||
| 1. pop 7 stack elements, even though the other six elements not closely relevant to this error | ||
| state constraints, but in order to be accordance with evm trace, also need to handle them here for stack pointer |
There was a problem hiding this comment.
| state constraints, but in order to be accordance with evm trace, also need to handle them here for stack pointer | |
| state constraints, but in order to be in accordance with evm trace, we also need to handle them here for stack pointer |
| ### circuit behavior | ||
| 1. pop 7 stack elements, even though the other six elements not closely relevant to this error | ||
| state constraints, but in order to be accordance with evm trace, also need to handle them here for stack pointer | ||
| transition, which impact next step's stack status. |
There was a problem hiding this comment.
| transition, which impact next step's stack status. | |
| transition, which impacts the next step's stack status. |
| # TODO: add Create / Create2 in the future | ||
| instruction.constrain_in(opcode, [FQ(Opcode.CALL), FQ(Opcode.CALLCODE)]) | ||
| # TODO: for create/create2 have different stack from Call, will handle it in the future | ||
| # Lookup values from stack |
There was a problem hiding this comment.
IMO, we should already build the lagrange polynomial now that we will use as a selector for the different possible opcodes.
Also, I think we should also add the switch between the opcode executions and leave with todo!() the ones that don't matter for now.
This would be the bear minimum to me. Because otherwise when we implement stuff into zkevm-circuits the code will be significantly worse as there are a lot of things we could abstract and simplify for the 4 cases that we're probably not be able to do with the 2 cases now and the 2 later in the future.
| # if is_success_rlc value is zero then decode RLC should also be zero | ||
| instruction.constrain_zero(is_success_rlc) | ||
|
|
||
| value = instruction.rlc_to_fq(value_rlc, 31) |
There was a problem hiding this comment.
Instead of 31, please use
| value = instruction.rlc_to_fq(value_rlc, 31) | ||
| current_address = instruction.call_context_lookup(CallContextFieldTag.CalleeAddress) | ||
| caller_balance_rlc = instruction.account_read(current_address, AccountFieldTag.Balance) | ||
| caller_balance = instruction.rlc_to_fq(caller_balance_rlc, 31) |
| caller_balance_rlc = instruction.account_read(current_address, AccountFieldTag.Balance) | ||
| caller_balance = instruction.rlc_to_fq(caller_balance_rlc, 31) | ||
| # compare value and balance | ||
| insufficient_balance, _ = instruction.compare(caller_balance, value, 31) |
| rw_counter=Transition.delta(10), | ||
| program_counter=Transition.delta(1), | ||
| stack_pointer=Transition.delta(6), | ||
| # TODO: handle gas_left |
There was a problem hiding this comment.
I think we should add the gas_left at least for the Opcodes we're implementing in this PR. Otherwise, this is a slightly incomplete PR, considering we don't support the 4 cases neither.
|
|
||
|
|
||
| @pytest.mark.parametrize("balance, transfer_value", TESTING_DATA) | ||
| def test_insufficient_balance(balance: int, transfer_value: int): |
There was a problem hiding this comment.
We should have test cases for both CALL and CALLCODE.
| rw_counter=Transition.delta(10), | ||
| program_counter=Transition.delta(1), | ||
| stack_pointer=Transition.delta(6), | ||
| # TODO: handle gas_left |
There was a problem hiding this comment.
We should at least handle CALL and CALLCODE use cases for the gas_left. And consider the others for what the design refers.
There was a problem hiding this comment.
@CPerezz thanks for reviewing, yes, I am considering refactoring this merging into CallOp as it shares more codes with it , will update spec after circuit refactoring done.
|
Hey @DreamWuGit just looking at older PRs. Are you still working on this one? |
|
close this , as it is merged into call op code from circuit side, will open new spec PR to match it. |
circuit PR https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/919， this error execution is different from others(OOG, stack error etc.) we have done, when this error happens, the result was pushed into stack and execution will consume instead of restore to caller's context.