search

0xPolygonZero / zk_evm

Apache License 2.0
85 stars 37 forks source link

Fix max_cpu_len_log #714

Closed sai-deng closed 1 month ago

sai-deng commented 1 month ago

Address the ‘attempt to subtract with overflow’ issue that occurs when max_cpu_len_log is set to a small value (less than 7).

This PR will also help with the empty segment test, where we aim to use the minimal number of CPU cycles in the segment.

sai-deng commented 1 month ago

I think we'd rather want to have a warn / error msg about the CPU target length being too small, and set it to NUM_EXTRA_CYCLES_AFTER.next_power_of_two() or simply abort early.

We need small CPU target length for empty segment tests, where we aim to use the minimal number of CPU cycles in the first segment.

sai-deng commented 1 month ago

I think we'd rather want to have a warn / error msg about the CPU target length being too small, and set it to NUM_EXTRA_CYCLES_AFTER.next_power_of_two() or simply abort early.

We need small CPU target length for empty segment tests, where we aim to use the minimal number of CPU cycles in the first segment.

As mentioned, if you are happy with the current empty table proving test, where we use max_cpu_len_log = 7 or 46 cycles + 82 (NUM_EXTRA_CYCLES_AFTER), we can abort early here.

LindaGuiga commented 1 month ago

I'm a bit confused because max_cpu_len_log is supposed to represent the full length of the CPU segment, including the final exc_stop cycles (which takes NUM_CYCLES_AFTER cycles). So why would we want to allow it to be less than that?

sai-deng commented 1 month ago

I'm a bit confused because max_cpu_len_log is supposed to represent the full length of the CPU segment, including the final exc_stop cycles (which takes NUM_CYCLES_AFTER cycles). So why would we want to allow it to be less than that?

This change only affects the behavior when max_cpu_len_log is less than 7, where we cannot represent the full length of the CPU segment. I planed to modify it so that it specifies the maximum actual CPU cycles we want in the CPU table. It would help with the empty segment test, where we aim to use the minimal number of CPU cycles in the segment.

sai-deng commented 1 month ago

Anyways, I think we are good with 46 cycles + 82 (NUM_EXTRA_CYCLES_AFTER) in the first segment for empty table testing, so I have made the changes to abort early.

sai-deng commented 1 month ago

I think it'd be a tad cleaner to have it as a hard assert! in the Interpreter initialization (method new()) which is used for both simulation and actual proving (mentioning assert! because it's probably not worth changing the method signatures to return a Result instead of Self given that this failure case is a hard no-go anyway).

Done