chore(l2): use SP1 patch for ecpairing precompile #4809
Conversation
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
There was a problem hiding this comment.
Pull Request Overview
This PR integrates SP1 patches for the ecpairing precompile by adding substrate-bn as an optional dependency and refactoring the BN254 pairing implementation to support both lambdaworks and substrate-bn backends.
Key changes:
- Adds substrate-bn dependency with SP1 patches for BN254 pairing operations
- Refactors ecpairing precompile to use a feature-flag based backend selection
- Simplifies coordinate parsing and validation logic
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/vm/levm/src/precompiles.rs | Refactors ecpairing precompile with simplified parsing and dual backend support |
| crates/vm/levm/Cargo.toml | Adds substrate-bn dependency with SP1 patch and sp1 feature flag |
| crates/vm/Cargo.toml | Adds sp1 feature flag propagation |
| crates/l2/prover/src/guest_program/src/sp1/Cargo.toml | Enables sp1 feature for ethrex-vm dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
crates/vm/levm/src/precompiles.rs
Outdated
|
|
||
| let (second_point_x, second_point_y) = parse_second_point_coordinates(&input)?; | ||
| #[allow(unreachable_code)] | ||
| let pairing_check = 'inner: { |
There was a problem hiding this comment.
Using a label is not a recommended practice.
I suggest to use a construction that is less error prone. For example, something that refuses to compile when using incompatible features.
There was a problem hiding this comment.
Suggested approach: let pairing_check be a function with the appropriate implementation for each config, e.g.:
#[cfg(feature = "sp1")]
fn pairing_check(batch: &[(G1, G2)]) -> Result<bool, VMError> {
if batch.is_empty() {
return Ok(true);
}
// Code currently in `pairing_substrate`
}
#[cfg(not(feature = "sp1")]
fn pairing_check(batch: &[(G1, G2)]) -> Result<bool, VMError> {
if batch.is_empty() {
return Ok(true);
}
// Code currently in `pairing_lambdaworks`
}The calling code would then not need to add special cases, it would just be:
let pairing_check = pairing_check(&batch)?;
There was a problem hiding this comment.
addressed! thank you :)
crates/vm/levm/src/precompiles.rs
Outdated
| let result = if batch.is_empty() { | ||
| substrate_bn::Gt::one() |
There was a problem hiding this comment.
I would prefer to do an early return here to avoid the huge else.
crates/vm/levm/src/precompiles.rs
Outdated
| let g1 = if g1.0.is_zero() && g1.1.is_zero() { | ||
| ProjectiveG1::neutral_element() | ||
| } else { | ||
| let (g1_x, g1_y) = ( | ||
| Fq::from_bytes_be(&g1.0.to_big_endian()) | ||
| .map_err(|_| InternalError::msg("failed to parse g1 x"))?, | ||
| Fq::from_bytes_be(&g1.1.to_big_endian()) | ||
| .map_err(|_| InternalError::msg("failed to parse g1 y"))?, | ||
| ); | ||
| LambdaworksG1::create_point_from_affine(g1_x, g1_y) | ||
| .map_err(|_| PrecompileError::InvalidPoint)? | ||
| }; | ||
| let g2 = if g2.0.is_zero() && g2.1.is_zero() && g2.2.is_zero() && g2.3.is_zero() { | ||
| ProjectiveG2::neutral_element() |
There was a problem hiding this comment.
this for loop body it's better to check the condition of equality with neutral element (is_neutral_element) within the top of the loop
There was a problem hiding this comment.
an early continue; there is tricky because we would be skipping the creation of one of the points (create_point_from_affinity), which contains implicit validation checks.
e.g. if G1 is zero and G2 is an invalid point, the precompile should revert but instead it would continue.
There was a problem hiding this comment.
we talked internally and decided to leave it as it is, but communicate to the Lambdaworks team the wish to implement the ecpairing and other operations in Lambdaworks to avoid having too much cryptography code in LEVM
Motivation
Elliptic Curve pairing is one of the most expensive operations in a zkVM .We are using lambdaworks for implementing the ecpairing precompile, which is not optimized for zkVMs. We must use each zkVM pairing patch/precompile instead.
This is true for other operations as well, like ecmul or ecadd, which can be optimized the same way in future PRs
Flamegraphs for block 23385900 Mainnet
ecpairing cycles are reduced from 135k to 6k, which is a 10% improvement in the total cycles.
before:
after: