search

0xPolygonZero / zk_evm

Apache License 2.0
85 stars 37 forks source link

Prune child context in create and call faults #747

Closed LindaGuiga closed 1 week ago

LindaGuiga commented 3 weeks ago

In sys_calls and create_common, create_context is called (and some values are set) before actually entering the new context. But between create_context and enter_new_ctx, some exceptions might arise, and the new context is therefore not pruned when it should. This PR aims at fixing this issue. It also reverts #738, since the changes should no longer be necessary after this fix.

Block 1033 was failing (with a wire set twice) without #738, but passes fine with this PR's changes.

sai-deng commented 3 weeks ago

Do we need a regression test or better test coverage on the pruned context?

LindaGuiga commented 2 weeks ago

@Nashtare mentioned we should keep your fix @sai-deng to make sure things don't actually fail even if we forgot some test cases.

Do we need a regression test or better test coverage on the pruned context? In my opinion, keeping your fix should be enough, since pruning is "just" an optimization, and we can detect any issues by logging a warning if the memory_after length is not 0 when it should be. What do you think @sai-deng?

sai-deng commented 2 weeks ago

@Nashtare mentioned we should keep your fix @sai-deng to make sure things don't actually fail even if we forgot some test cases.

Do we need a regression test or better test coverage on the pruned context? In my opinion, keeping your fix should be enough, since pruning is "just" an optimization, and we can detect any issues by logging a warning if the memory_after length is not 0 when it should be. What do you think @sai-deng?

LGTM. We can log a warning if is_last_segment && final_len != 0.

Nashtare commented 1 week ago

FYI, ran the 6555 test blocks over night and no failure (and no single WARN message of non-empty table, so that's encouraging).