Bug: fix to not invoke transfer for call related opcodes (except CALL)#335
Conversation
…GATECALL and STATICCALL) expect CALL.
CPerezz
left a comment
CPerezz
left a comment
There was a problem hiding this comment.
I'd use the lookup for non-existance of accounts
| # TODO: | ||
| # Suppose to fix to use non-existing proofs for account existence after it | ||
| # has been used for one opcode in zkevm-circuit. | ||
| # https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/907 |
There was a problem hiding this comment.
This is already merged here in the specs. So I'm not sure why can't it be done here already. See: #291 which marks this as completed.
I think the specs should always try to be correct. And it's the circuits implementation who needs the "TODO"s, not this repo.
There was a problem hiding this comment.
Deleted the TODO comment. And fixed to pass callee_exists via auxiliary witness data.
| instruction.is_zero(callee_nonce) | ||
| * instruction.is_zero(callee_balance_prev) | ||
| * is_empty_code_hash | ||
| instruction.is_zero(callee_nonce) * instruction.is_zero(callee_balance) * is_empty_code_hash |
There was a problem hiding this comment.
I'd use the non_existance lookup here.
There was a problem hiding this comment.
Has been replaced with callee_exists.
| ) | ||
| else: | ||
| # Get callee balance for CALLCODE, DELEGATECALL and STATICCALL opcodes. | ||
| callee_balance = instruction.account_read(callee_address, AccountFieldTag.Balance) |
There was a problem hiding this comment.
According to the CALLCODE implementation you linked in #334 (comment)
For CALLCODE you should constraint that value <= callee_balance right?
I think I didn't see this constraint.
There was a problem hiding this comment.
Fixed to add a constraint to verify caller balance is greater than or equal to stack value for both CALL and CALLCODE opcodes as below:
if is_call + is_callcode == 1:
value_lt_caller_balance, value_eq_caller_balance = instruction.compare_word(
value, caller_balance
)
instruction.constrain_zero(1 - value_lt_caller_balance - value_eq_caller_balance)
|
@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 privacy-ethereum/zkevm-circuits#975 and privacy-ethereum/zkevm-circuits#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? |
Yes. Will update this PR according to code review during this weekend. Thanks. |
|
Thank you!! |
… constraint yo verify caller balance should be greater than or equal to stack `value` (only for CALL and CALLCODE).
silathdiir
requested review from
CPerezz and
ed255
and removed request for
CPerezz and
ed255
CPerezz
left a comment
CPerezz
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for this work @silathdiir 🥳
Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com>
5 participants
Close #334
Related circuit PR privacy-ethereum/zkevm-circuits#973
As issue described, there should be no
transferinvocation 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.