refacor/CALL and OOG_CALL

[KimiWu123]

It's for CALL & CALL OOG refactor #940

[KimiWu123]

@ChihChengLiang , could you help to have a quick review to see if I do it in right direction? Any advice are welcome.

[ChihChengLiang]

Great work. I added a suggestion on organizing the code in a function.

[KimiWu123]

Update
After dicussed with @ChihChengLiang , we decided to refactor zkEVM-circuit first and then back here to see if we need more modification. The reason behind is that we would like to make the code in zkEVM-spec is more consistent with zkEVM-circuit

[ChihChengLiang]

First pass

[ChihChengLiang]

LGTM

[ed255]

I've added a few small notes related to the latest circuit implementation, but otherwise looks good! Please take a look at the two comments :)

[ed255]

Recently the case of *CALL* without code execution has been updated due to how non-existing accounts are handled.
Non-existing accounts are now identified by having code_hash = 0, which is not the empty hash value. But those cases should still be handled as no code execution.
So I think it would be better to define:

no_callee_code = call.is_empty_code_hash + call.callee_not_exists

And then in this if replace the first check by no_callee_code == FQ(1)

I notice that call.callee_not_exists is missing from the CallGadget in this PR. I introduced that in a very recent PR in the circuits. See privacy-ethereum/zkevm-circuits@aaaf13e#diff-08d58ebc59ab6c11bf7c049e859441eed2319a03feb3fdffaddaba73f8a8c829

[KimiWu123]

fixed in a7d329a

[ed255]

I noticed that the fields callee_code_hash and is_empty_code_hash are not set in the __init__. They are set in the gas_cost method. Why is that? Isn't it better to initialize all the Class fields in __init__? The circuit does it like this;
https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/b77613e9b42eacb03ccc894f0b353c374da3f283/zkevm-circuits/src/evm_circuit/util/common_gadget.rs#L523-L525

[KimiWu123]

yes, I totally agree that. However, I don't know how to hadle it properly because I need to get callee_code_hash first as following,

        self.callee_code_hash = instruction.account_read(
            self.callee_address, AccountFieldTag.CodeHash
        )

If I moved above code to __init__, it breaks the order of rw table. (reading the value of AccountFieldTag.CodeHash must be after reading the value of AccountFieldTag.Balance, see here). Either I change the order in the test or assign an arbitrary value in __init__(It will fail if anyone uses these vars before calling gas_cost, but I don't think it's a good idea). Do you have any better ideas for this?

[ed255]

Ah I see!
After this change was applied #354 it will become impossible to read a AccountFieldTag.Balance for non-existing accounts. This means that the order of lookups for possibly non-existing accounts should always be

1. Read CodeHash
2. If CodeHash != 0, then read other fields as necessary

By impossible what I mean is that it won't be possible to generate a list of MPT updates + proofs that can verify it. Since we don't have the MPT circuit connected yet, tests will not fail (although we could add some checks to make sure we never read non-codeHash account fields of non-existing accounts).

So I think you should move the callee CodeHash lookup to happen first, and if account exists, then do the callee Balance lookup. I think this affects the circuits as well. I see in error_oog_call.rs first the callee Balance is read, and then call_gadget.gas_cost_expr is called, which causes the callee CodeHash read. Actually, I'm not sure why in error_oog_call the callee balance is read, there doesn't seem to be any constraint using that value: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/ea6d8e38bcaedbf2d69d0661ad14485c6b02b9cb/zkevm-circuits/src/evm_circuit/execution/error_oog_call.rs#L66-L71

So I think you should change the order of reads in the test for the callee: first read the CodeHash, then if its != 0, read the Balance.

After a second look at the test code you mentioned https://github.com/privacy-scaling-explorations/zkevm-specs/blob/master/tests/evm/test_callop.py#L345-L355 I see that the Balance read in there is for the caller which is independent from the callee CodeHash read; which means that the order doesn't matter, but it's still OK to read the callee CodeHash before the caller Balance.

So in summary: for possibly non-existing accounts, first read codeHash, and only if its !=0 read other fields. For accounts known to be existing in the context (like the caller in the call context), the order of reads is not important.

[KimiWu123]

the order of CodeHash read was fixed in 7e0fc6e
.

Actually, I'm not sure why in error_oog_call the callee balance is read, there doesn't seem to be any constraint using that value:

yep, balance is not necessary in error_oog_call, I have removed it.
.

So I think you should change the order of reads in the test for the callee: first read the CodeHash, then if its != 0, read the Balance.

For then if its != 0, read the Balance, I'm not sure I understand it corretly. Do you mean add a condiftion if callee_bytecode_hash != 0 at test_callop.py#L349 and test_callop.py#L353? From my understanding, in evm, user's balance is still able to be read even it's a non-existing account (not initialized in MPT). If we add if callee_bytecode_hash != 0, I guess it'll panic(or just fail) if we read the balance of a non-existing account. Not sure my understanding is correct and not sure I understand your meaning correctly.

[ed255]

For then if its != 0, read the Balance, I'm not sure I understand it corretly. Do you mean add a condiftion if callee_bytecode_hash != 0 at test_callop.py#L349 and test_callop.py#L353?

I was mistaken on my interpretation of the callop logic, so the then if its != 0, read the Balance doesn't apply here, because the Balance is read on the caller, which at this point is guaranteed to exist, so there's no need to make it conditional. I think there's an edge case on the transfer thought, but more on it later.

From my understanding, in evm, user's balance is still able to be read even it's a non-existing account (not initialized in MPT).

From the EVM spec, whenever an opcode that reads account fields encounters a non-existing account particular conditions are applied. Most of the time the behavior is the same as considering the non-existing account as a default account (with nonce=0, balance=0, code_hash=EMPTY_HASH), but that's not always the case. For example, EXTCODEHASH returns 0 for non-existing accounts (instead of EMPTY_HASH). This also affects writes, as opcodes that write to accounts may have different gas cost when they create the account (because it was non-existing before).

In our circuits, account read/writes are triggered by the EVM circuit. Those read/writes are then verified for consistency in the State Circuit, and finally they are collected and matched against MPT proofs to match the state trie transitions. Our current design of the MPT Circuit supports the proofs listed in this table: https://github.com/privacy-scaling-explorations/zkevm-specs/blob/master/specs/tables.md#mpt_table

In particular, the NonceMod, BalanceMod and CodeHashMod don't support a read (valuePrev == value) of non-existing accounts. They only support reads to existing accounts, and writes to existing accounts or to non-existing accounts (which triggers account creation).

So a Nonce, Balance, CodeHash read proof for non-existing accounts is not supported by the MPT circuit.

The MPT circuit supports NonExistingAccountProof which acts like a read and proves that an account doesn't exist.

And in the StateCircuit we map CodeHash read of 0 to NonExistingAccountProof in the MPT. We can do this because code_hash will never be 0 in an existing account, so wenever in the EVM/State circuit a CodeHash is read as 0, it means that the account doesn't exist. And if the account doesn't exist, a following Nonce or Balance read (triggered by the EVM) can't be proved by the MPT Circuit.

This is why every time a Nonce/Balance is read from the EVM circuit, we must make sure that the account exists, otherwise it may be translated to a NonceMod or BalanceMod proof in the MPT that is impossible to succeed. So the idea is, in those cases, have a boolean expression that encodes the account existence, and use it to enable/disable the field lookups.

The way we disable a lookup is by:
1- In all tables always have an all 0 row
2- Multiply all expressions of the lookup by the "selector" expression, so that when it's zero, the lookup is a list of 0s and always suceceds, bypassing the other possibly non-zero values in the original lookup expression.

If we add if callee_bytecode_hash != 0, I guess it'll panic(or just fail) if we read the balance of a non-existing account. Not sure my understanding is correct and not sure I understand your meaning correctly.

I'm not sure I understand this part

---

Edit: I forgot to mention the edge case: If there's a CALL to a non-existing account with value=0, I believe the account will not be created. The current circuit will perform a value transfer which generates a lookup for the calle: in the case I described, this write lookup will target a non-existing account and will change the balance from 0 to 0. Since the account is not created, we should avoid this balance lookup.

[ed255]

BTW I'm working on a simple check on zkevm-circuits that will guarantee that we don't have invalid account field read/writes: privacy-ethereum/zkevm-circuits#1084

[ed255]

I consider this conversation resolved! But we can continue a discussion to resolve any doubts, etc :)

[ed255]

LGTM! Nice work!

As I mentioned in #348 (comment) the logic of if code_hash == 0, skip balance read lookup doesn't apply in this opcode because the possibley non-existing account is the callee, whereas the balance read lookup is done on the caller. So the PR looks good to me in the current form :)