search

0xPolygonZero / zk_evm

Apache License 2.0
86 stars 38 forks source link

Kernel should not read target account of `sys_call` if gas is insufficient #194

Closed frisitano closed 7 months ago

frisitano commented 7 months ago

Whilst working on https://github.com/0xPolygonZero/zero-bin/pull/51 I have run into a number of cases in which the kernel panics. In these cases it appears that a call operation is invoked against some target account, however this call operation fails due to there being insufficient gas to start the call. As such, when I observe the output of debug_traceTransaction using both the callTracer and prestateTracer the call target account is not included. I believe this is because the call context is never actually created due to there being insufficient gas. As a consequence of this, when we create the block trace we do not include this account in the state trie to send to the prover. However, when I observe the logs from evm arithmetization I can see that it is trying to read the call target account from the state trie. It panics here because the account is not included in the trie for reasons described above.

I suspect that we need to add a check in the kernel context to evaluate if there is sufficient gas to enter into the call context before it is created. At this point this is somewhat of a "hunch" as I'm not an expert of the kernel. This problem may be relevant for other opcodes which also create child contexts, not just sys_call.

I have added specific instances of transactions of this nature to this comment and those following it: https://github.com/0xPolygonZero/zero-bin/pull/51#issuecomment-2075441515

cc: @Nashtare

wborgeaud commented 7 months ago

We encountered a similar issue before and https://github.com/0xPolygonZero/zk_evm/pull/124 fixed it for us. It ensures that we don't needlessly query state to compute the gas cost in call operations. Are you using the latest main? Otherwise, could you provide a tx where this issue occurs?

frisitano commented 7 months ago

I branched from develop not main - this is the branch I'm working from https://github.com/0xPolygonZero/zk_evm/pull/157.

Yes an example of this transaction is included in this comment here:

https://github.com/0xPolygonZero/zero-bin/pull/51#issuecomment-2075441515

frisitano commented 7 months ago

I wonder if it's because there is some value associated with the call and as such an additional gas cost which should be charged prior to the call operation. once again, just a hunch.

If call_value > 0 (sending value with call):
    base_gas += 9000
wborgeaud commented 7 months ago

Yeah I think that's it. Currently we charge these 9000 gas after querying the state trie. In this tx for example, the call is made with 4323 gas left, which is enough for the cold access gas so we end up querying the state trie even though there's not enough gas to pay for the 9000 non-zero callvalue gas. To fix this, we'd just have to charge the 9000 gas before querying the state.