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

Optimize zkVM Proving by Skipping Unused Keccak Tables #690

Merged
merged 122 commits into from
Oct 15, 2024

Conversation

sai-deng
Copy link
Contributor

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

This PR introduces optional Keccak table proving, which optimizes performance by skipping the proving process when the Keccak tables are empty. This change reduces unnecessary computation and requires an update to Plonky2 for full integration.

Copy link
Contributor Author

@sai-deng sai-deng left a comment

Choose a reason for hiding this comment

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

Thanks for the reviews!

zero/src/prover_state/mod.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/arithmetic/mod.rs Outdated Show resolved Hide resolved
@Nashtare Nashtare added this to the Performance Tuning milestone Oct 9, 2024
@sai-deng sai-deng force-pushed the sai/refactor_optional_table_proving branch from 5892a8c to 27590f8 Compare October 9, 2024 21:59
@sai-deng
Copy link
Contributor Author

sai-deng commented Oct 9, 2024

@Nashtare I’ve refactored the code and merged two tests into one integration test. PTAL.

Comment on lines 34 to 35
// Default degree of the recursive circuit when a proof is missing from AllProof
const DEFAULT_CIRCUIT_DEGREE: usize = 8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would we want to load anything if a proof is missing? Also this may crash unexpectedly in load_table_circuits if we specify a config with a range (for Keccak for instance) that excludes this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need a recursive circuit to generate a dummy proof for the root circuit. Let me refactor the code to optimize this while also addressing the other comment.

evm_arithmetization/src/fixed_recursive_verifier.rs Outdated Show resolved Hide resolved
@sai-deng sai-deng force-pushed the sai/refactor_optional_table_proving branch from 3fd9032 to 9021124 Compare October 11, 2024 16:53
@sai-deng sai-deng force-pushed the sai/refactor_optional_table_proving branch from 667c462 to 557d221 Compare October 11, 2024 17:21
@sai-deng
Copy link
Contributor Author

I have changed the code to generate dummy proofs during the initial setup. PTAL again.

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 LGTM, would just appreciate the extension of the empty_table test to include the segment aggregation logic.
Also would wait on others to comment on the usage of unstable feature.

evm_arithmetization/src/fixed_recursive_verifier.rs Outdated Show resolved Hide resolved
zero/src/lib.rs Outdated Show resolved Hide resolved
.as_ref()
.expect("Missing ctl_zs")
.clone()
} else if i == *Table::Keccak {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this will get refactored in the generalization to other tables as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It got refactored in #724

evm_arithmetization/tests/empty_tables.rs Show resolved Hide resolved
@sai-deng sai-deng merged commit f0a7ab2 into develop Oct 15, 2024
20 checks passed
@sai-deng sai-deng deleted the sai/refactor_optional_table_proving branch October 15, 2024 22:20
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. crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants