-
Notifications
You must be signed in to change notification settings - Fork 110
fix(l1,l2): add feature flag to decide precompute number #4893
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a feature flag system to conditionally set the KZG precompute parameter based on the target environment. It addresses Risc0 compatibility issues by setting the precompute value to 0 for Risc0 builds while maintaining the optimized value of 8 for other environments.
- Added a
risc0
feature flag across the common crates - Introduced conditional compilation for KZG precompute values
- Updated all KZG function calls to use the new configurable constant
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
crates/l2/prover/src/guest_program/src/risc0/Cargo.toml | Enables the risc0 feature for ethrex-common dependency |
crates/common/crypto/kzg.rs | Adds conditional KZG_PRECOMPUTE constant and updates function calls |
crates/common/crypto/Cargo.toml | Defines the risc0 feature flag |
crates/common/Cargo.toml | Propagates the risc0 feature to the crypto subcrate |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Lines of code reportTotal lines added: Detailed view
|
crates/common/crypto/Cargo.toml
Outdated
[features] | ||
default = [] | ||
c-kzg = ["dep:c-kzg"] | ||
risc0 = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's include c-kzg
in risc0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 2c5b849
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to update RISC0's guest Cargo.lock
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there is no need. I ran cargo check
inside risc0 guest program and nothing change
8fc9d1e
to
36be0a2
Compare
Motivation
Risc0 is currently failing because precompute must be set to 0.
This changes were made by @gianbelinche
Follow up
Opened this issue to update the code once the change becomes available.