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

Add dummy segment to the left #185

Merged
merged 14 commits into from
Apr 25, 2024
Merged

Conversation

LindaGuiga
Copy link
Contributor

@LindaGuiga LindaGuiga commented Apr 24, 2024

This PR adds dummy segments to the left in generate_all_data_segments when necessary. Note that for this, we need to allow values that are not powers of two for max_cpu_len.

@github-actions github-actions bot added crate: proof_gen Anything related to the proof_gen crate. crate: evm_arithmetization Anything related to the evm_arithmetization crate. labels Apr 24, 2024
@LindaGuiga LindaGuiga changed the title Add dummy segment left Add dummy segment to the left Apr 24, 2024
@Nashtare
Copy link
Collaborator

Not that for this, we need to allow values that are not powers of two for max_cpu_len.

Could you elaborate on this? As the max_cpu_len_log is purely a user argument, I don't get why it would need to be changed.

@LindaGuiga
Copy link
Contributor Author

Not that for this, we need to allow values that are not powers of two for max_cpu_len.

Could you elaborate on this? As the max_cpu_len_log is purely a user argument, I don't get why it would need to be changed.

For dummy segments, we need to set the max number of CPU cycles to exactly the number of cycles in init, so that the dummy segments can be aggregated with the next segments without regenerating their GenerationSegmentData.

@Nashtare
Copy link
Collaborator

My point was that dummy segment generation is handled by the system, not the user, who only cares about actual segments lengths. So this notion should be handled arbitrarily of what the user sets as max cpu length (which should be a power of two).

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.

overall 90% of the PR changes are just max_cpu_len_log -> max_cpu_len and corresponding changes, which I believe should not even happen in the first place out of safety. We should probably just handle the dummy segment specifically so that we can maintain the existing logic.

evm_arithmetization/src/generation/mod.rs Show resolved Hide resolved
evm_arithmetization/src/memory/memory_stark.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/memory/memory_stark.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/prover.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/prover.rs Outdated Show resolved Hide resolved
@LindaGuiga
Copy link
Contributor Author

overall 90% of the PR changes are just max_cpu_len_log -> max_cpu_len and corresponding changes, which I believe should not even happen in the first place out of safety. We should probably just handle the dummy segment specifically so that we can maintain the existing logic.

I reverted those changes and instead added an is_dummy field in GenerationSegmentData so that at_end_segment can differentiate between dummy and real segments.

@github-actions github-actions bot removed the crate: proof_gen Anything related to the proof_gen crate. label Apr 25, 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.

LGTM!

@Nashtare Nashtare merged commit dc23696 into feat/continuations Apr 25, 2024
6 checks passed
@Nashtare Nashtare deleted the add_dummy_segment_left branch April 25, 2024 13:59
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