zkevm-specs
zkevm-specs copied to clipboard
privacy-scaling-explorations
Bug: fix to not invoke `transfer` for call related opcodes (except `CALL`)
Close https://github.com/privacy-scaling-explorations/zkevm-specs/issues/334
Related circuit PR https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/973
As issue described, there should be no transfer invocation for call related opcodes (except CALL) - CALLCODE, DELEGATECALL and STATICCALL.
~~And callee balance is also needed for checking account existence below (for now). Suppose if it could be fixed to replace with non-existing proofs after using in the BALANCE circuit.~~
@CPerezz don't need to be so strict...
Sometimes we prefer perfect PR, sometimes we prefer urgent PR. For the latter, as long as a PR is better than before, it can be accepted.
This issue is a bit urgent both for pse/zkevm-chain and scroll circuits, related to https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/975 and https://github.com/privacy-scaling-explorations/zkevm-circuits/issues/969
Hey @lispc .
I'm not sure if I'm interpreting your message correctly. But let me be clear. I think that for a codebase this big and complex, being "strict" if you want to call it like this, is not crazy and rather what we all should be when we review.
I don't think I have asked anything crazy considering that we just need to remove 3 lines of code to include the existance lookup swapping it by the balance, codehash and value checks..
In any case, @ed255 I think is in the same line as I am. We cannot merge anything just for the purpose of being fast. And, instead, we should try to fix things propperly (without of course affecting the overall progress of the project).
Summarizing. Using here the non-existance lookup is a task we need to do in the close future and takes 1 day of work MAXIMUM. So why leaving more tech-debt behind when we can already fix it quickly here and avoid more TODO's??
After some discussion we choose to let non-existing proofs be a TODO (future PR). @silathdiir
Anyway 1 day dev time does not mean 1 day diff in wall clock delivery time. Developers have other tasks to do, reviewers ditto.
Hi @silathdiir ! Just checking in, are you good to apply the changes to merge this PR?
Hi @silathdiir ! Just checking in, are you good to apply the changes to merge this PR?
Yes. Will update this PR according to code review during this weekend. Thanks.
Hi @CPerezz and @ed255 , I updated this PR according to above comments. Could you help review again when you have time? Thanks.