Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix max_cpu_len_log #714

Merged
merged 4 commits into from
Oct 17, 2024
Merged

Fix max_cpu_len_log #714

merged 4 commits into from
Oct 17, 2024

Conversation

sai-deng
Copy link
Contributor

@sai-deng sai-deng commented Oct 9, 2024

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.

@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Oct 9, 2024
@sai-deng sai-deng marked this pull request as ready for review October 9, 2024 16:28
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sai-deng
Copy link
Contributor Author

sai-deng commented Oct 9, 2024

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
Copy link
Contributor Author

sai-deng commented Oct 9, 2024

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.

@Nashtare Nashtare added this to the x Misc. milestone Oct 9, 2024
@LindaGuiga
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 sai-deng self-assigned this Oct 11, 2024
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@sai-deng
Copy link
Contributor Author

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

@Nashtare Nashtare merged commit ce0ab8c into develop Oct 17, 2024
20 checks passed
@Nashtare Nashtare deleted the sai/fix_max_cpu_len branch October 17, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants