test(DON'T MERGE): develop-v2.1.0 by shuklaayush · Pull Request #2646 · openvm-org/openvm

shuklaayush · 2026-04-01T17:09:51Z

No description provided.

github-actions · 2026-04-01T17:10:06Z

Claude finished @shuklaayush's task in 2m 44s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand changes
Review ECC circuit changes (add/double chips)
Review guest library and macro changes
Review transpiler and test changes
Review docs and miscellaneous changes
Post final review

This PR migrates Short Weierstrass elliptic curve operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This is a significant architectural change that improves performance by eliminating field inversions and simplifies the code by handling all edge cases (identity, doubling, etc.) uniformly.

Overall Assessment

The migration is well-structured and thorough. The complete projective formulas are correctly implemented with good optimization of AIR column counts. The PR touches the full stack: circuit constraints, execution, guest libraries, macros, transpiler, and tests.

Correctness

1. Projective addition/doubling formulas look correct

The implementations in add/mod.rs and double/mod.rs correctly follow Algorithms 1, 3, 7, 9 from ePrint 2015/1060. The a == 0 specialization (for secp256k1, BN254, BLS12-381) and general a case (for P256) are properly handled. The column count optimizations (combined mul-sub, inlined intermediates, degree-2 outputs) are well-documented and sound.

2. Native field implementations match

The curves.rs file provides native field arithmetic implementations (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) that follow the same algorithms. The k256-specific implementations correctly handle the normalize_weak() calls needed for k256's magnitude-tracking field representation.

3. Identity representation is consistent

Identity is (0, 1, 0) throughout — in impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, and from_xy which maps (0, 0) to identity. This is correct for projective coordinates.

4. Setup instruction correctly validates projective layout

In add/execution.rs:282-319, the setup instruction validates that P1 = (modulus, a, b) by splitting BLOCKS into three coordinate regions. This matches the guest-side set_up_once() in sw-macros/src/lib.rs:253-261 which sends [modulus, curve_a, curve_b].

Potential Issues

1. (Medium) PartialEq in sw-macros uses unprotected field multiplications

extensions/ecc/sw-macros/src/lib.rs:288-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

This is mathematically correct for projective equivalence, but when running inside zkVM, these multiplications emit unconstrained modular arithmetic instructions. If setup hasn't been called yet for the modular type, this could be problematic. The add_impl and double_impl have a CHECK_SETUP guard — PartialEq does not. This may be acceptable if equality checks always happen after setup in practice, but it's worth noting.

2. (Low) b3 recomputed on every impl_sw_proj! add/double call

extensions/ecc/guest/src/weierstrass.rs:387-390:

fn add_impl<const CHECK_SETUP: bool>(&self, p2: &Self) -> Self {
    let b3 = &(&<Self as WeierstrassPoint>::CURVE_B + &<Self as WeierstrassPoint>::CURVE_B) + &<Self as WeierstrassPoint>::CURVE_B;
    Self::add_a0(&self.x, &self.y, &self.z, &p2.x, &p2.y, &p2.z, &b3)
}

3*b is recomputed on every call. For the zkVM target (where sw_declare! is used), this is handled by the chip directly. But for non-zkVM targets using impl_sw_proj!, this adds two extra field additions per EC operation. Consider caching b3 as a const or lazy_static. This is minor since it's only non-zkVM host execution.

3. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is actually on the curve (e.g., y² = x³ + b when b = 0), this would incorrectly map a valid affine point to identity. In practice none of the supported curves have b = 0, so this is fine, but it's a latent footgun for future curve additions.

4. (Info) The dispatch! macro in add/execution.rs and double/execution.rs is quite verbose

The macro at add/execution.rs:109-157 expands to 10 match arms. This pattern is duplicated in double/execution.rs. If more curves are added in the future, this will grow quadratically. Not a problem now, but worth noting.

Code Quality

Well done:

Excellent comments explaining optimization strategies and column count reductions (e.g., "34.0% reduction from naive 2668")
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs::tests that verify points land on the curve
The impl_sw_proj! macro is clean and well-documented
Setup validation properly checks modulus, a, and b coefficients

Minor style notes:

crates/vm/src/arch/execution_mode/pure.rs — the unwrap_or simplification is good cleanup
crates/vm/src/arch/interpreter.rs — the #[allow(clippy::unnecessary_unwrap)] with explaining comment is the right approach

Breaking Changes

This PR changes the opcode names (SW_EC_ADD_NE → SW_EC_ADD_PROJ, etc.), the memory layout (2 coordinates → 3 coordinates per point), and the WeierstrassPoint trait (adds z coordinate, into_coords now returns 3-tuple). These are all breaking changes to the verifying key and guest ABI. The pairing crates (bn254, bls12_381) are updated to add z fields to GENERATOR/NEG_GENERATOR constants. This all looks correctly handled.

Summary

Solid PR implementing a well-known optimization. The projective formulas are correctly implemented, the AIR constraints are well-optimized, and the migration touches all necessary layers consistently. The potential issues noted above are minor/informational. The test coverage is good, though integration tests (in extensions/ecc/tests/) would provide additional confidence that the full prove/verify pipeline works end-to-end with the new coordinate system.

github-actions · 2026-04-01T17:15:37Z

Claude finished @shuklaayush's task in 1m 56s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review core circuit changes (add/double AIR constraints)
Review execution and trace generation
Review guest library and macro changes
Review transpiler, tests, and docs
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (with DivUnsafe field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates field inversions and handles all edge cases (identity, doubling) uniformly.

Overall Assessment

Solid, well-executed migration. The projective formulas are correctly implemented across all layers (AIR constraints, execution, guest libraries, macros, transpiler). The AIR column count optimizations are well-documented with specific reduction percentages. The code is clean and the test coverage is comprehensive.

Correctness

AIR constraint formulas are correct. I verified the ec_add_proj_a0_expr (Algorithm 7), ec_add_proj_general_expr (Algorithm 1), ec_double_proj_a0_expr (Algorithm 9), and ec_double_proj_general_expr (Algorithm 3) against ePrint 2015/1060. The optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are sound and well-commented.

The b3 modular reduction is correct. add/mod.rs:38 and double/mod.rs:38 compute b3 = (b * 3) % modulus, ensuring correctness before passing to the expression builder.

Native implementations match. The curves.rs implementations (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) follow the same algorithms. The k256-specific implementations correctly handle normalize_weak() calls needed for k256's magnitude-tracking field representation.

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, and the pairing crate constants.

Setup validation is correct. Both add/execution.rs:282-319 and double/execution.rs:278-316 validate P1 = (modulus, a, b) by splitting BLOCKS into three coordinate regions, matching the guest-side set_up_once() in sw-macros/src/lib.rs:253-261.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic

extensions/ecc/sw-macros/src/lib.rs:287-292:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

When running inside zkVM, these multiplications emit modular arithmetic instructions. Unlike add_impl and double_impl which have a CHECK_SETUP guard, PartialEq does not call set_up_once(). If eq is called before any EC operation triggers setup, this could be problematic. In practice this is likely safe since equality checks happen after setup, but it's a latent footgun.

2. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (when b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but this is worth a comment for future curve additions.

3. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:387-390:

fn add_impl<const CHECK_SETUP: bool>(&self, p2: &Self) -> Self {
    let b3 = &(&<Self as WeierstrassPoint>::CURVE_B + &<Self as WeierstrassPoint>::CURVE_B)
        + &<Self as WeierstrassPoint>::CURVE_B;
    Self::add_a0(&self.x, &self.y, &self.z, &p2.x, &p2.y, &p2.z, &b3)
}

3*b is recomputed every call. For zkVM targets this is irrelevant (the chip handles it), but for non-zkVM targets using impl_sw_proj! this adds two extra field additions per EC operation. Minor since it's only host execution — could be cached via LazyLock or similar if performance matters.

4. (Low) Doubling in sw_declare! non-zkVM path: Z3 computation is expensive

extensions/ecc/sw-macros/src/lib.rs:226:

let z3 = &(&(&t2_yz * &t1) + &(&t2_yz * &t1) + &(&t2_yz * &t1)) + &(&t2_yz * &t1); // 4*t2_yz*t1

This computes 4*t2_yz*t1 using 4 multiplications and 3 additions, when let tmp = &t2_yz * &t1; let tmp2 = &tmp + &tmp; let z3 = &tmp2 + &tmp2; would use 1 multiplication and 2 additions. Similarly, line 189 computes 8*t0 with 7 additions instead of 3 doublings. Minor since this is the non-zkVM host fallback path.

Code Quality

Well done:

Excellent optimization documentation (column count reduction percentages, technique descriptions)
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs::tests covering k256, BN254, P256 with on-curve verification
The dispatch! macro pattern cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b)

Style notes:

crates/vm/src/arch/execution_mode/pure.rs — the unwrap_or simplification is a nice cleanup
crates/vm/src/arch/interpreter.rs — the #[allow(clippy::unnecessary_unwrap)] with explaining comment is the right approach

Breaking Changes

This PR changes:

Opcode names (SW_EC_ADD_NE → SW_EC_ADD_PROJ, etc.)
Memory layout (2 coordinates per point → 3 coordinates)
WeierstrassPoint trait (adds z coordinate, into_coords returns 3-tuple)
SwBaseFunct7 variants renamed to SwAddProj, SwDoubleProj
Pairing crate constants updated with z fields

All breaking changes appear correctly handled across the full stack (circuit, transpiler, guest, macros, tests, docs).

Summary

Well-structured migration implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reduction percentages, and the change propagates consistently through all layers. The issues noted above are minor/informational. The existing unit tests are solid; the integration tests in extensions/ecc/tests/ (which compile guest programs and run them through the full pipeline) provide additional confidence.

github-actions · 2026-04-01T17:30:24Z

Claude finished @shuklaayush's task in 5m 23s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review core circuit changes (add/double AIR constraints and expressions)
Review execution, preflight, and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling) uniformly.

Overall Assessment

Well-executed migration. The projective formulas are correctly implemented across all layers (AIR constraints, execution, guest libraries, macros, transpiler). The AIR column count optimizations are well-documented and the code is clean. I verified the formulas in add/mod.rs and double/mod.rs against both the ePrint reference and the native implementations in curves.rs — they are consistent.

Correctness Verification

AIR constraint formulas are correct. I manually traced the a=0 addition (Algorithm 7) and general addition (Algorithm 1) against ePrint 2015/1060:

ec_add_proj_a0_expr: Steps 1-33 correctly mapped. The intermediate variable named y3 (line 104 of add/mod.rs) corresponds to the spec's Y3 at step 18, not the final output — this matches the reference in curves.rs:83. Final outputs at lines 128-143 are correct.
ec_add_proj_general_expr: Steps 1-40 correctly mapped with the optimization of absorbing step 23 into z3 (line 214) and recovering x3 via 2*t1 - z3 (line 219). This is algebraically sound.
ec_double_proj_a0_expr and ec_double_proj_general_expr: Both correctly follow Algorithms 9 and 3 respectively. The algebraic rearrangement in the a=0 doubling (lines 108-116 of double/mod.rs) using t0² + t2·(4·t0 + z3) is a clever optimization that avoids an extra save.

Native implementations match. The curves.rs functions (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) follow the same algorithms. k256-specific implementations correctly handle normalize_weak().

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b coefficients by splitting BLOCKS into three coordinate regions.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

fn eq(&self, other: &Self) -> bool {
    (&self.x * &other.z) == (&other.x * &self.z)
        && (&self.y * &other.z) == (&other.y * &self.z)
}

The projective equivalence formula itself is mathematically correct for all cases (including identity — when both Z=0, both checks reduce to 0==0; when only one Z=0, the Y check fails). However, unlike add_impl/double_impl which have a CHECK_SETUP guard, PartialEq does not call set_up_once(). If eq() is called before any EC operation triggers setup for the underlying modular type, this would emit unconstrained modular arithmetic instructions in zkVM. Safe in practice if equality always follows setup, but a latent footgun.

2. (Medium) Missing identity/edge-case tests

The test suite in tests.rs covers standard point operations well, but lacks explicit tests for:

Identity + Point = Point
Point + (-Point) = Identity
Identity + Identity = Identity
Double(Identity) = Identity

These are the key cases that the complete projective formulas are supposed to handle correctly. Adding them would provide confidence that the complete formulas work end-to-end through the AIR.

3. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but worth a comment for future curve additions.

4. (Low) Non-zkVM doubling path computes 8*t0 with 7 additions

extensions/ecc/sw-macros/src/lib.rs:189:

let z3 = &(&(&(&t0 + &t0) + &t0 + &t0) + &t0 + &t0 + &t0) + &t0; // 8*t0

This uses 7 additions when 3 doublings suffice: let z3 = &t0_2 + &t0_2; let z3 = &z3 + &z3;. Similarly, 4*t2_yz*t1 at line 226 uses 4 multiplications when 1 mul + 2 additions would work. Minor since this is only the non-zkVM host fallback path.

5. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:387-390 — 3*b is recomputed every add/double call via two field additions. For zkVM targets this is irrelevant (the chip handles it directly), but for non-zkVM targets using impl_sw_proj! it's a small overhead per operation. Could be cached via LazyLock if host performance matters.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:68)
Clean separation between AIR expressions, execution, native computation, and CUDA
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b)

Minor style notes:

The unwrap_or simplification in pure.rs is a nice cleanup
The #[allow(clippy::unnecessary_unwrap)] with explaining comment in interpreter.rs is the right approach

Breaking Changes

All correctly handled across the full stack:

Opcode names: SW_EC_ADD_NE → SW_EC_ADD_PROJ, SW_EC_DOUBLE → SW_EC_DOUBLE_PROJ
Memory layout: 2 coordinates per point → 3 coordinates (projective)
WeierstrassPoint trait: adds z() accessor, into_coords() returns 3-tuple
SwBaseFunct7 variants renamed
Pairing crate constants updated with z fields
Docs and spec updated consistently

Summary

Solid PR implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reduction percentages, and the change propagates consistently through all layers. The most actionable suggestion is adding identity/edge-case tests (issue #2) to exercise the complete formula's key advantage over the old affine approach. The other issues are minor or informational.

github-actions · 2026-04-02T11:56:21Z

Claude finished @shuklaayush's task in 3m 23s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review core circuit changes (add/double AIR constraints)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across the full stack: AIR constraints, execution, guest libraries, macros, transpiler, and docs. The AIR column count optimizations are well-documented and sound. Identity representation (0, 1, 0) is consistent throughout.

Correctness

AIR constraint formulas are correct. Verified against ePrint 2015/1060:

ec_add_proj_a0_expr (Algorithm 7): 1761 columns for secp256k1 (34% reduction from naive)
ec_add_proj_general_expr (Algorithm 1): 2081 columns for secp256r1 (38.3% reduction)
ec_double_proj_a0_expr (Algorithm 9): 1359 columns for secp256k1 (22.3% reduction)
ec_double_proj_general_expr (Algorithm 3): 2034 columns for secp256r1 (30.7% reduction)

The algebraic rearrangements (combined mul-sub, inlined intermediates, degree-2 outputs) are mathematically sound and well-commented.

Native implementations match. The curves.rs functions follow the same algorithms. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field.

Setup validation is correct. Both add/execution.rs:282-319 and double/execution.rs:278-316 validate modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 integer division is safe because ECC_BLOCKS_32 and ECC_BLOCKS_48 are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

Breaking changes handled consistently. Opcode renames, memory layout (2→3 coords), trait changes, pairing crate constants — all correctly propagated.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

fn eq(&self, other: &Self) -> bool {
    (&self.x * &other.z) == (&other.x * &self.z)
        && (&self.y * &other.z) == (&other.y * &self.z)
}

The projective equivalence formula is mathematically correct, but unlike add_impl/double_impl which have a CHECK_SETUP guard, PartialEq does not call set_up_once(). If eq() is called before any EC operation triggers setup for the underlying modular type, this would emit unconstrained modular arithmetic instructions in zkVM. Safe in practice if equality always follows setup, but a latent footgun.

2. (Medium) Missing identity/edge-case tests

The test suite covers standard point operations well, but lacks explicit tests for the key cases that complete projective formulas are supposed to handle:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity

These are the primary advantage of complete formulas over the old affine approach. Adding them would provide confidence that the complete formulas work end-to-end through the AIR. Add identity tests →

3. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would help prevent future footguns.

4. (Low) Redundant multiplications in non-zkVM doubling path

extensions/ecc/sw-macros/src/lib.rs:189:

let z3 = &(&(&(&t0 + &t0) + &t0 + &t0) + &t0 + &t0 + &t0) + &t0; // 8*t0

Uses 7 additions when 3 doublings suffice: let z3 = &t0 + &t0; let z3 = &z3 + &z3; let z3 = &z3 + &z3;

Line 201:

let x3 = &(&t0 * &t1) + &(&t0 * &t1); // 2 * t0 * t1

Computes t0*t1 twice instead of computing once and adding to itself.

Line 226:

let z3 = &(&(&t2_yz * &t1) + &(&t2_yz * &t1) + &(&t2_yz * &t1)) + &(&t2_yz * &t1); // 4*t2_yz*t1

Computes t2_yz*t1 four times instead of 1 mul + 2 additions.

These are only in the non-zkVM host fallback path so the impact is minor, but they're easy wins. Fix redundant operations →

5. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:387-390 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages and named techniques
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b)
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) updated for projective API

Minor style notes:

crates/vm/src/arch/execution_mode/pure.rs — the unwrap_or simplification is a nice cleanup
crates/vm/src/arch/interpreter.rs — the #[allow(clippy::unnecessary_unwrap)] with explaining comment is the right approach

Summary

Solid PR implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reductions, and the change propagates consistently through all layers. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) against pre-setup use, matching the pattern in add_impl/double_impl

The remaining issues are minor/informational.

github-actions · 2026-04-02T12:09:58Z

Claude finished @shuklaayush's task in 4m 25s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review core circuit changes (add/double AIR constraints and expressions)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across the full stack: AIR constraints, execution, guest libraries, macros, transpiler, and docs. I verified the formulas against the ePrint reference — they match. The AIR column count optimizations are well-documented and sound. Identity representation (0, 1, 0) is consistent throughout.

Correctness

AIR formulas verified correct:

ec_add_proj_a0_expr (add/mod.rs): Algorithm 7 — steps traced correctly. The variable named y3 at line 104 holds the intermediate X1·Z2 + X2·Z1 (matching the spec's reuse pattern). The degree-2 output optimization at lines 127-143 correctly combines spec steps 25-33.
ec_add_proj_general_expr (add/mod.rs:149-257): Algorithm 1 — the z3/x3 fusion at lines 212-219 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct.
ec_double_proj_a0_expr (double/mod.rs:46-124): Algorithm 9 — verified. Note: Step 5 is t1 = Y·Z (not 2·Y·Z — that's Algorithm 3). The Y3 algebraic rearrangement at lines 108-116 is sound: Y3 = t0² + t2·(4·t0 + z3).
ec_double_proj_general_expr (double/mod.rs:126-217): Algorithm 3 — verified. The difference-of-squares optimization at lines 186-187 is clean.

Native implementations match in curves.rs — all four functions follow the same algorithms. k256-specific implementations correctly call normalize_weak().

Setup validation correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE).

FieldExpr field access at add/execution.rs:289 (pre_compute.expr.prime) is correct — FieldExpr implements Deref<Target = ExprBuilder> which has pub prime: BigUint.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

fn eq(&self, other: &Self) -> bool {
    (&self.x * &other.z) == (&other.x * &self.z)
        && (&self.y * &other.z) == (&other.y * &self.z)
}

The projective equivalence formula is mathematically correct (including for identity — when both Z=0, both checks reduce to 0==0). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun.

2. (Medium) Missing identity/edge-case tests

The test suite covers standard point operations well, but lacks explicit tests for the key cases that complete projective formulas are supposed to handle:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity

These are the primary advantage of complete formulas over the old affine approach. Adding them would confirm the complete formulas work end-to-end through the AIR. Add identity tests →

3. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would help prevent future footguns.

4. (Low) Redundant multiplications in non-zkVM doubling path

extensions/ecc/sw-macros/src/lib.rs:201:

let x3 = &(&t0 * &t1) + &(&t0 * &t1); // 2 * t0 * t1

Computes t0*t1 twice. Should be: let tmp = &t0 * &t1; let x3 = &tmp + &tmp;

Line 226:

let z3 = &(&(&t2_yz * &t1) + &(&t2_yz * &t1) + &(&t2_yz * &t1)) + &(&t2_yz * &t1); // 4*t2_yz*t1

Computes t2_yz*t1 four times. Should be 1 mul + 2 additions.

Similarly line 209 computes x*y and x*z twice each instead of once and doubling.

These are only in the non-zkVM host fallback path so the impact is minor, but they're easy wins. Fix redundant operations →

5. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:387-390 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "22.3% reduction from naive 1748" in double/mod.rs:65)
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied
Clean separation between AIR expressions, execution, and native computation
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) updated for projective API

Minor style notes:

crates/vm/src/arch/execution_mode/pure.rs — the unwrap_or simplification is a nice cleanup
crates/vm/src/arch/interpreter.rs — the #[allow(clippy::unnecessary_unwrap)] with explaining comment is the right approach

Breaking Changes

All correctly handled across the full stack:

Opcode names: SW_EC_ADD_NE → SW_EC_ADD_PROJ, SW_EC_DOUBLE → SW_EC_DOUBLE_PROJ
Memory layout: 2 coordinates per point → 3 coordinates (projective)
WeierstrassPoint trait: adds z() accessor, into_coords() returns 3-tuple
SwBaseFunct7 variants renamed to SwAddProj, SwDoubleProj
Pairing crate constants updated with z fields
Docs and spec updated consistently

Summary

Solid PR implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reductions, and the change propagates consistently through all layers. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) against pre-setup use, matching the pattern in add_impl/double_impl

The remaining issues are minor/informational.

github-actions · 2026-04-02T12:27:49Z

Claude finished @shuklaayush's task in 3m 10s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand changes
Review core circuit changes (add/double AIR constraints)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across the full stack: AIR constraints, execution, guest libraries, macros, transpiler, and docs. The AIR column count optimizations are well-documented and sound. Identity representation (0, 1, 0) is consistent throughout.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (Algorithm 7): Correct step mapping. Combined mul-sub and degree-2 output optimizations are algebraically sound.
ec_add_proj_general_expr (Algorithm 1): The z3/x3 fusion (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct.
ec_double_proj_a0_expr (Algorithm 9): The Y3 algebraic rearrangement (t0² + t2·(4·t0 + z3)) is sound.
ec_double_proj_general_expr (Algorithm 3): Difference-of-squares optimization is clean.

Native implementations match. The curves.rs functions follow the same algorithms. k256-specific implementations correctly handle normalize_weak().

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs and double/execution.rs — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE).

Breaking changes handled consistently — opcode renames, memory layout (2→3 coords), trait changes, pairing constants all correctly propagated.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

fn eq(&self, other: &Self) -> bool {
    (&self.x * &other.z) == (&other.x * &self.z)
        && (&self.y * &other.z) == (&other.y * &self.z)
}

The projective equivalence formula is mathematically correct (including identity cases). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Add setup guard →

2. (Medium) Missing identity/edge-case tests

The test suite covers standard point operations well but all circuit tests use Z=1 input (affine in projective form). The key cases that complete projective formulas are supposed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)
Verification that add(P, P) = double(P)

These are the primary advantage of complete formulas over the old affine approach. Adding them would confirm the formulas work end-to-end through the AIR. Add identity tests →

3. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would help prevent future footguns.

4. (Low) Redundant multiplications in non-zkVM doubling path

extensions/ecc/sw-macros/src/lib.rs:201:

let x3 = &(&t0 * &t1) + &(&t0 * &t1); // 2 * t0 * t1

Computes t0*t1 twice. Should be: let tmp = &t0 * &t1; let x3 = &tmp + &tmp;

Line 226:

let z3 = &(&(&t2_yz * &t1) + &(&t2_yz * &t1) + &(&t2_yz * &t1)) + &(&t2_yz * &t1); // 4*t2_yz*t1

Computes t2_yz*t1 four times instead of 1 mul + 2 additions. Similarly line 189 computes 8*t0 with 7 additions instead of 3 doublings. These are only in the non-zkVM host fallback path so the impact is minor, but they're easy wins. Fix redundant operations →

5. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:387-390 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs)
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b)
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) updated for projective API
crates/vm/src/arch/execution_mode/pure.rs — the unwrap_or simplification is a nice cleanup

Summary

Solid PR implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reductions, and the change propagates consistently through all layers. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) against pre-setup use, matching the pattern in add_impl/double_impl

The remaining issues are minor/informational.

github-actions · 2026-04-02T12:36:41Z

Claude finished @shuklaayush's task in 4m 14s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review core circuit changes (add/double AIR constraints and expressions)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across the full stack: AIR constraints, execution, guest libraries, macros, transpiler, and docs. The AIR column count optimizations are well-documented and sound. Identity representation (0, 1, 0) is consistent throughout all layers.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (Algorithm 7): 1761 columns for secp256k1 — 34.0% reduction from naive 2668
ec_add_proj_general_expr (Algorithm 1): 2081 columns for secp256r1 — 38.3% reduction from naive 3371. The z3/x3 fusion (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct.
ec_double_proj_a0_expr (Algorithm 9): 1359 columns — 22.3% reduction. The Y3 rearrangement t0² + t2·(4·t0 + z3) is sound.
ec_double_proj_general_expr (Algorithm 3): 2034 columns — 30.7% reduction. Difference-of-squares optimization is clean.

Native implementations in curves.rs match the AIR formulas. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field representation.

Setup validation correct in both add/execution.rs:282-319 and double/execution.rs — validates modulus, a, and b by interpreting the 3-coordinate layout as (prime, a_coeff, b_coeff). The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE).

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constant updates all correctly propagated.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

fn eq(&self, other: &Self) -> bool {
    (&self.x * &other.z) == (&other.x * &self.z)
        && (&self.y * &other.z) == (&other.y * &self.z)
}

The projective equivalence formula is mathematically correct (including identity cases — when both Z=0, both checks reduce to 0==0). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Add setup guard →

2. (Medium) Missing identity/edge-case tests

The test suite covers standard point operations across multiple curves and field sizes, but all circuit-level tests use Z=1 inputs (affine in projective form). The key cases that complete projective formulas are supposed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. Adding them would confirm the formulas work end-to-end through the AIR. Add identity tests →

3. (Low) Redundant multiplications in non-zkVM doubling path

extensions/ecc/sw-macros/src/lib.rs:189:

let z3 = &(&(&(&t0 + &t0) + &t0 + &t0) + &t0 + &t0 + &t0) + &t0; // 8*t0

Uses 7 additions when 3 doublings suffice: let z3 = &t0 + &t0; let z3 = &z3 + &z3; let z3 = &z3 + &z3;

Line 201:

let x3 = &(&t0 * &t1) + &(&t0 * &t1); // 2 * t0 * t1

Computes t0*t1 twice. Should be: let tmp = &t0 * &t1; let x3 = &tmp + &tmp;

Line 209:

let t3 = &(&p.x * &p.y) + &(&p.x * &p.y); // 2*x*y

Same pattern — p.x * p.y computed twice. Repeated on lines 210 (x*z), 223 (y*z).

Line 226:

let z3 = &(&(&t2_yz * &t1) + &(&t2_yz * &t1) + &(&t2_yz * &t1)) + &(&t2_yz * &t1); // 4*t2_yz*t1

Computes t2_yz*t1 four times instead of 1 mul + 2 additions.

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so the impact is minor, but they're easy wins for host-side execution performance. Fix redundant operations →

4. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would help prevent future footguns.

5. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:387-390 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it directly), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs)
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b)
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) properly updated for projective API with normalize() calls
crates/vm/src/arch/execution_mode/pure.rs — the unwrap_or simplification is a nice cleanup
README and docs updated consistently

Summary

Solid PR implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reductions (22-38%), and the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs). The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) — these exercise the key advantage of complete formulas and are currently missing from circuit-level tests
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) against pre-setup use in zkVM, matching the pattern in add_impl/double_impl

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-02T12:44:28Z

Claude finished @shuklaayush's task in 2m 35s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review core circuit changes (add/double AIR constraints and expressions)
Review execution and trace generation changes
Review guest library, macros, and transpiler changes
Review tests and documentation
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across the full stack: AIR constraints, execution, guest libraries, macros, transpiler, and docs. The AIR column count optimizations are well-documented and algebraically sound. Identity representation (0, 1, 0) is consistent throughout all layers.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. The combined mul-sub pattern (lines 82-100) saves 3 variables vs naive. The degree-2 output optimization at lines 122-138 combines two muls per output, saving 3 more variables. Total: 1761 columns (34% reduction).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented.
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — the Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save; the derivation is clearly documented.
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — the difference-of-squares optimization at lines 178-182 (t1² - y3² instead of (t1+y3)·(t1-y3)) avoids two separate saves.

Native implementations match. The curves.rs functions (ec_add_proj_impl_a0, ec_double_proj_impl_a0, etc.) follow the same algorithms. k256-specific implementations correctly handle normalize_weak().

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated through the full stack.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

fn eq(&self, other: &Self) -> bool {
    (&self.x * &other.z) == (&other.x * &self.z)
        && (&self.y * &other.z) == (&other.y * &self.z)
}

The projective equivalence formula is mathematically correct (including identity cases). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun.

2. (Medium) Missing identity/edge-case tests in circuit-level tests

The test suite covers standard point operations across curves and field sizes, but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. Adding them to the circuit tests (not just the native curves.rs unit tests) would confirm the AIR constraints work end-to-end for these edge cases.

3. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM fallback path compute the same product multiple times:

sw-macros/src/lib.rs:201:

let x3 = &(&t0 * &t1) + &(&t0 * &t1); // 2 * t0 * t1

Computes t0*t1 twice. Should be: let tmp = &t0 * &t1; let x3 = &tmp + &tmp;

Line 226:

let z3 = &(&(&t2_yz * &t1) + &(&t2_yz * &t1) + &(&t2_yz * &t1)) + &(&t2_yz * &t1); // 4*t2_yz*t1

Computes t2_yz*t1 four times instead of 1 mul + 2 additions. Similarly line 189 computes 8*t0 with 7 additions instead of 3 doublings, and lines 209-210, 223 duplicate multiplications.

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so runtime impact is minor, but they're easy wins.

4. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would help prevent future footguns.

5. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:387-390 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:63)
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b)
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) updated for projective API with normalize() calls
Sanity tests with proj_to_affine helper provide good confidence in output correctness

Summary

Solid PR implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reductions (22-38%), and the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs). The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) against pre-setup use in zkVM, matching the pattern in add_impl/double_impl

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-02T13:03:55Z

Claude finished @shuklaayush's task in 5m 52s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand changes
Review AIR circuit constraints (add/double)
Review execution and trace generation
Review guest library, macros, and transpiler
Review tests, curves, and recent commits
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. It also adapts to v2's DEFAULT_BLOCK_SIZE=4 memory constraint (commit 9c08ad7) and cleans up dead CUDA files (commit d96449e).

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across all layers: AIR constraints, execution, guest libraries, macros, transpiler, and docs. The AIR column count optimizations are well-documented with specific reduction percentages. Identity representation (0, 1, 0) is consistent throughout. The recent rebase fixes (removing dead CUDA files, fixing pre-push.sh array expansion) are clean.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (Algorithm 7): 1761 columns — 34% reduction from naive 2668
ec_add_proj_general_expr (Algorithm 1): 2081 columns — 38.3% reduction. The z3/x3 fusion (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically sound.
ec_double_proj_a0_expr (Algorithm 9): 1359 columns — 22.3% reduction. Y3 rearrangement t0² + t2·(4·t0 + z3) avoids an extra save.
ec_double_proj_general_expr (Algorithm 3): 2034 columns — 30.7% reduction. Difference-of-squares optimization is clean.

Native implementations match. The curves.rs functions follow the same algorithms. k256-specific implementations correctly handle normalize_weak().

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE).

Block size migration (9c08ad7) is correct. ECC_BLOCKS_32 = 24 and ECC_BLOCKS_48 = 36 with DEFAULT_BLOCK_SIZE = 4 properly replaces the old <3, 32> / <9, 16> patterns. All extension, hybrid, and test code uses these constants consistently.

Note: add/execution.rs:289 uses pre_compute.expr.prime while double/execution.rs:285 uses pre_compute.expr.builder.prime — both are correct since FieldExpr implements Deref<Target = ExprBuilder>, so .prime resolves through the deref. No bug here, just a stylistic inconsistency.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

fn eq(&self, other: &Self) -> bool {
    (&self.x * &other.z) == (&other.x * &self.z)
        && (&self.y * &other.z) == (&other.y * &self.z)
}

The projective equivalence formula is mathematically correct (including identity cases). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun.

Suggestion: Add a set_up_once() call at the start of eq() on the #[cfg(target_os = "zkvm")] path, matching the pattern in add_impl/double_impl. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes, but all circuit-level tests use Z=1 inputs only. The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity

These are the primary advantage of complete formulas over the old affine approach. Adding them to circuit tests would confirm the AIR constraints work end-to-end for edge cases. Add identity tests →

3. (Low) Redundant multiplications in non-zkVM doubling path

extensions/ecc/sw-macros/src/lib.rs:201:

let x3 = &(&t0 * &t1) + &(&t0 * &t1); // 2 * t0 * t1

Computes t0*t1 twice. Should be: let tmp = &t0 * &t1; let x3 = &tmp + &tmp;

Same pattern at lines 209, 210, 223 (products computed twice instead of once-and-double) and line 226 (4 multiplications instead of 1 mul + 2 additions). Also line 189 computes 8*t0 with 7 additions instead of 3 doublings. These are only in the non-zkVM host fallback path so the impact is minor, but they're easy wins. Fix redundant operations →

4. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68 — For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would help prevent future footguns.

5. (Low) Stylistic inconsistency in prime access

add/execution.rs:289 uses pre_compute.expr.prime (via Deref) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically, but picking one style would be cleaner. Fix consistency →

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668")
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied
Clean separation between AIR expressions, execution, and native computation
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b)
Integration test programs updated for projective API with normalize() calls
Recent cleanup: dead CUDA files removed, pre-push.sh fixed for set -u, unnecessary #![allow(deprecated)] removed
parallel feature correctly enabled on openvm-circuit-primitives dependency (70aaf4d)

Breaking Changes

All correctly handled across the full stack:

Opcode names: SW_EC_ADD_NE → SW_EC_ADD_PROJ, SW_EC_DOUBLE → SW_EC_DOUBLE_PROJ
Memory layout: 2 coordinates per point → 3 coordinates (projective), BLOCK_SIZE → DEFAULT_BLOCK_SIZE=4
WeierstrassPoint trait: adds z() accessor, into_coords() returns 3-tuple
SwBaseFunct7 variants renamed to SwAddProj, SwDoubleProj
Pairing crate constants updated with z fields
Docs and spec updated consistently

Summary

Solid PR implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reductions (22-38%), and the change propagates consistently through all layers. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-02T13:57:50Z

Claude finished @shuklaayush's task in 2m 27s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand changes
Review core circuit changes (add/double AIR constraints)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across the full stack: AIR constraints, execution, guest libraries, macros, transpiler, and docs. The AIR column count optimizations are well-documented and algebraically sound. Identity representation (0, 1, 0) is consistent throughout all layers.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. Combined mul-sub pattern saves 3 variables. Degree-2 output optimization at lines 122-138 is sound. 1761 columns (34% reduction from naive 2668).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented. 2081 columns (38.3% reduction).
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save. 1359 columns (22.3% reduction).
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — difference-of-squares optimization at lines 178-182 is clean. 2034 columns (30.7% reduction).

Native implementations match. The curves.rs functions follow the same algorithms. k256-specific implementations correctly handle normalize_weak().

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

The projective equivalence formula is mathematically correct. However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes, but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity

These are the primary advantage of complete formulas over the old affine approach. Adding them to circuit tests would confirm the AIR constraints work end-to-end for edge cases. Add identity tests →

3. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM fallback path compute the same product multiple times:

sw-macros/src/lib.rs:201: let x3 = &(&t0 * &t1) + &(&t0 * &t1) — computes t0*t1 twice. Should be let tmp = &t0 * &t1; let x3 = &tmp + &tmp;

sw-macros/src/lib.rs:209-210: p.x * p.y and p.x * p.z each computed twice instead of once and doubled.

sw-macros/src/lib.rs:226: t2_yz*t1 computed 4 times instead of 1 mul + 2 additions.

sw-macros/src/lib.rs:189: 8*t0 with 7 additions instead of 3 doublings.

These are only in the non-zkVM host fallback path so runtime impact is minor, but they're easy wins. Fix redundant operations →

4. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68 — For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would prevent future footguns.

5. (Low) Stylistic inconsistency in prime access

add/execution.rs:289 uses pre_compute.expr.prime (via Deref) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically, but picking one style would be cleaner.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:63)
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b)
The impl_sw_proj! macro provides a clean non-zkVM implementation with well-documented algorithm references
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) updated for projective API with normalize() calls

Summary

Solid PR implementing a well-known optimization. The complete projective formulas are correctly implemented, AIR constraints are well-optimized with documented reductions (22-38%), and the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs). The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-08T21:44:48Z

Claude finished @shuklaayush's task in 3m 10s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review AIR circuit constraints (add/double expression builders)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching. The PR spans the full stack: AIR constraints, execution, guest libraries, macros, transpiler, docs, and tests.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. The combined mul-sub pattern (lines 82-100) saves 3 variables. Degree-2 output optimization at lines 122-138 is algebraically sound. 1761 columns (34% reduction from naive 2668).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented. 2081 columns (38.3% reduction).
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — the Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save. 1359 columns (22.3% reduction).
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — difference-of-squares optimization at lines 178-182 is clean. 2034 columns (30.7% reduction).

Native implementations match. The curves.rs functions (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) follow the same algorithms. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field representation.

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated through the full stack.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

The projective equivalence formula is mathematically correct (including identity cases — when both Z=0, both checks reduce to 0==0). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Note: the impl_sw_proj! macro (commit c00c20c) does have the same issue — its PartialEq also lacks a setup guard, though impl_sw_proj! is only used in non-zkVM contexts where setup is a no-op. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes, but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. The curves.rs unit tests cover native implementations well, but circuit-level tests (which exercise the AIR constraints end-to-end) should also cover these edge cases. Add identity tests →

3. (Low) Stylistic inconsistency in prime access

add/execution.rs:289 uses pre_compute.expr.prime (via Deref<Target=ExprBuilder>) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically — FieldExpr derefs to ExprBuilder — but picking one style consistently would be cleaner.

4. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM fallback path compute the same product multiple times:

sw-macros/src/lib.rs:189: 8*t0 uses 7 additions instead of 3 doublings
sw-macros/src/lib.rs:201: t0*t1 computed twice instead of once + double
sw-macros/src/lib.rs:209-210: p.x * p.y and p.x * p.z each computed twice
sw-macros/src/lib.rs:226: t2_yz*t1 computed 4 times instead of 1 mul + 2 additions

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so runtime impact is minor, but they're easy wins for host-side execution performance. Fix redundant operations →

5. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would help prevent future footguns.

6. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:396-399 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:63) and named techniques
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b) in the projective layout
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) properly updated for projective API with normalize() calls
The impl_sw_proj! macro PartialEq fix (commit c00c20c) was a good follow-up
b3 modular reduction at add/mod.rs:33 and double/mod.rs:33 (b3 = (b * 3) % modulus) is correct
The WeierstrassPoint trait cleanly extends with z(), from_xyz_unchecked(), and normalize()/is_identity() methods
Docs and ISA spec updated consistently with the projective coordinate semantics

Summary

Solid PR implementing a well-known optimization with 22-38% AIR column count reductions. The complete projective formulas are correctly implemented, the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs), and the benchmarks show good results. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) in sw_declare! against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-08T22:15:01Z

Claude finished @shuklaayush's task in 3m 11s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review AIR circuit constraints (add/double expression builders)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching. The change spans the full stack: AIR constraints, execution, guest libraries, macros, transpiler, docs, and tests.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. The combined mul-sub pattern (lines 82-100) saves 3 variables vs naive. Degree-2 output optimization at lines 122-138 is algebraically sound. 1761 columns (34% reduction from naive 2668).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented. 2081 columns (38.3% reduction).
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — the Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save. 1359 columns (22.3% reduction).
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — difference-of-squares optimization at lines 178-182 is clean. 2034 columns (30.7% reduction).

Native implementations match. The curves.rs functions (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) follow the same algorithms. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field representation.

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

b3 modular reduction at add/mod.rs:33 and double/mod.rs:33 (b3 = (b * 3) % modulus) is correct — ensures the constant fits within the field before passing to the expression builder.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated through the full stack.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

The projective equivalence formula is mathematically correct (including identity cases — when both Z=0, both checks reduce to 0==0). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Note that impl_sw_proj! at weierstrass.rs:258-263 has the same pattern but is only used in non-zkVM contexts where setup is a no-op.

Suggestion: Add a set_up_once() call at the start of eq() on the #[cfg(target_os = "zkvm")] path, matching the pattern in add_impl/double_impl. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes, but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. The curves.rs unit tests cover native implementations, but circuit-level tests (which exercise the AIR constraints end-to-end) should also cover these edge cases. Add identity tests →

3. (Low) Stylistic inconsistency in prime access between add and double execution

add/execution.rs:289 accesses the modulus via pre_compute.expr.prime (through Deref<Target=ExprBuilder>) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically — FieldExpr derefs to ExprBuilder — but picking one style consistently would be cleaner. Fix consistency →

4. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM host fallback path compute the same product multiple times:

sw-macros/src/lib.rs:189: 8*t0 uses 7 additions instead of 3 doublings
sw-macros/src/lib.rs:201: t0*t1 computed twice instead of once + double
sw-macros/src/lib.rs:209-210: p.x * p.y and p.x * p.z each computed twice
sw-macros/src/lib.rs:226: t2_yz*t1 computed 4 times instead of 1 mul + 2 additions

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so runtime impact is minor, but they're easy wins. Fix redundant operations →

5. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would prevent future footguns.

6. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:396-399 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages and named techniques throughout add/mod.rs and double/mod.rs
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b) in the projective layout
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) properly updated for projective API with normalize() calls
ECC_BLOCKS_32 = 24 and ECC_BLOCKS_48 = 36 with DEFAULT_BLOCK_SIZE = 4 correctly replaces the old block size patterns
parallel feature correctly enabled on openvm-circuit-primitives dependency in sha2-air
Docs and ISA spec updated consistently with the projective coordinate semantics

Summary

Solid PR implementing a well-known optimization with 22-38% AIR column count reductions. The complete projective formulas are correctly implemented, the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs), and the benchmarks look good. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) in sw_declare! against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-09T10:11:30Z

Claude finished @shuklaayush's task in 3m 23s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review AIR circuit constraints (add/double expression builders)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching. The change spans the full stack: AIR constraints, execution, guest libraries, macros, transpiler, docs, and tests.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. The combined mul-sub pattern (lines 82-100) saves 3 variables vs naive. Degree-2 output optimization at lines 122-138 combines two muls per output. 1761 columns (34% reduction from naive 2668).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented. 2081 columns (38.3% reduction).
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — the Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save. 1359 columns (22.3% reduction).
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — difference-of-squares optimization at lines 178-182 is clean. 2034 columns (30.7% reduction).

Native implementations match. The curves.rs functions (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) follow the same algorithms. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field representation.

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

b3 modular reduction at add/mod.rs:33 and double/mod.rs:33 (b3 = (b * 3) % modulus) is correct.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated through the full stack.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

The projective equivalence formula is mathematically correct (including identity cases — when both Z=0, both checks reduce to 0==0). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Note that impl_sw_proj! at weierstrass.rs:258-263 has the same pattern but is only used in non-zkVM contexts where setup is a no-op.

Suggestion: Add a set_up_once() call at the start of eq() on the #[cfg(target_os = "zkvm")] path, matching the pattern in add_impl/double_impl. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes, but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. The curves.rs unit tests cover native implementations, but circuit-level tests (which exercise the AIR constraints end-to-end) should also cover these edge cases. Add identity tests →

3. (Low) Stylistic inconsistency in prime access between add and double execution

add/execution.rs:289 accesses the modulus via pre_compute.expr.prime (through Deref<Target=ExprBuilder>) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically — FieldExpr derefs to ExprBuilder — but picking one style consistently would be cleaner.

4. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM host fallback path compute the same product multiple times:

sw-macros/src/lib.rs:189: 8*t0 uses 7 additions instead of 3 doublings
sw-macros/src/lib.rs:201: t0*t1 computed twice instead of once + double
sw-macros/src/lib.rs:209-210: p.x * p.y and p.x * p.z each computed twice
sw-macros/src/lib.rs:226: t2_yz*t1 computed 4 times instead of 1 mul + 2 additions

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so runtime impact is minor, but they're easy wins for host-side execution performance. Fix redundant operations →

5. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would prevent future footguns.

6. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:396-399 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:63) and named techniques
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b) in the projective layout
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) properly updated for projective API with normalize() calls
ECC_BLOCKS_32 = 24 and ECC_BLOCKS_48 = 36 with DEFAULT_BLOCK_SIZE = 4 correctly replaces the old block size patterns
parallel feature correctly enabled on openvm-circuit-primitives dependency in sha2-air
README and docs updated consistently with the projective coordinate semantics

Summary

Solid PR implementing a well-known optimization with 22-38% AIR column count reductions. The complete projective formulas are correctly implemented, the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs), and the benchmarks look good. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) in sw_declare! against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-09T21:45:06Z

Claude finished @shuklaayush's task in 3m 43s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes
Review AIR circuit constraints (add/double expression builders)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching. The change spans the full stack: AIR constraints, execution, guest libraries, macros, transpiler, docs, and tests.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across the full stack. The AIR column count optimizations are well-documented with specific reduction percentages, and the algebraic rearrangements are sound. Identity representation (0, 1, 0) is consistent throughout all layers. Breaking changes are handled consistently.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. The combined mul-sub pattern (lines 82-100) saves 3 variables. Degree-2 output optimization at lines 122-138 is algebraically sound. 1761 columns (34% reduction from naive 2668).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented. 2081 columns (38.3% reduction).
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — the Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save. 1359 columns (22.3% reduction).
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — difference-of-squares optimization at lines 178-182 (t1² - y3² instead of (t1+y3)·(t1-y3)) avoids saving two factors. 2034 columns (30.7% reduction).

Native implementations match. The curves.rs functions follow the same algorithms. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field representation.

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

b3 modular reduction at add/mod.rs:33 and double/mod.rs:33 (b3 = (b * 3) % modulus) is correct.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated through the full stack.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

The projective equivalence formula is mathematically correct (including identity cases — when both Z=0, both checks reduce to 0==0). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Note that impl_sw_proj! at weierstrass.rs:258-263 has the same pattern but is only used in non-zkVM contexts where setup is a no-op.

Suggestion: Add a set_up_once() call at the start of eq() on the #[cfg(target_os = "zkvm")] path, matching the pattern in add_impl/double_impl. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes (secp256k1, BLS12-381, secp256r1), but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. The curves.rs unit tests cover native implementations, but circuit-level tests (which exercise the AIR constraints end-to-end) should also cover these edge cases to confirm the AIR handles them correctly. Add identity tests →

3. (Low) Stylistic inconsistency in prime access between add and double execution

add/execution.rs:289 accesses the modulus via pre_compute.expr.prime (through Deref<Target=ExprBuilder>) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically — FieldExpr derefs to ExprBuilder — but picking one style consistently would be cleaner.

4. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM host fallback path compute the same product multiple times:

sw-macros/src/lib.rs:189: 8*t0 uses 7 additions instead of 3 doublings
sw-macros/src/lib.rs:201: t0*t1 computed twice instead of once + double
sw-macros/src/lib.rs:209-210: p.x * p.y and p.x * p.z each computed twice
sw-macros/src/lib.rs:226: t2_yz*t1 computed 4 times instead of 1 mul + 2 additions

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so runtime impact is minor, but they're easy wins for host-side execution performance. Fix redundant operations →

5. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would prevent future footguns.

6. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:396-399 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:63) and named techniques
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b) in the projective layout
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) properly updated for projective API with normalize() calls
ECC_BLOCKS_32 = 24 and ECC_BLOCKS_48 = 36 with DEFAULT_BLOCK_SIZE = 4 correctly replaces the old block size patterns
parallel feature correctly enabled on openvm-circuit-primitives dependency in sha2-air
Docs, ISA spec, and README updated consistently with the projective coordinate semantics
Pairing crate GENERATOR/NEG_GENERATOR constants correctly updated with z: from_const_u8(1) fields
The WeierstrassPoint trait cleanly extends with z(), from_xyz_unchecked(), normalize(), and is_identity() methods

Summary

Solid PR implementing a well-known optimization with 22-38% AIR column count reductions. The complete projective formulas are correctly implemented, the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs), and the benchmarks look good. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) in sw_declare! against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-10T13:17:48Z

Claude finished @shuklaayush's task in 3m 8s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand PR changes (diff, new commits)
Review AIR circuit constraints (add/double expression builders)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching. The change spans the full stack: AIR constraints, execution, guest libraries, macros, transpiler, docs, and tests.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across the full stack. The AIR column count optimizations are well-documented with specific reduction percentages, and the algebraic rearrangements are sound. Identity representation (0, 1, 0) is consistent throughout all layers. Breaking changes are handled consistently.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. The combined mul-sub pattern (lines 82-100) saves 3 variables. Degree-2 output optimization at lines 122-138 is algebraically sound. 1761 columns (34% reduction from naive 2668).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented. 2081 columns (38.3% reduction).
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — the Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save. 1359 columns (22.3% reduction).
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — difference-of-squares optimization at lines 178-182 (t1² - y3² instead of (t1+y3)·(t1-y3)) avoids saving two factors. 2034 columns (30.7% reduction).

Native implementations match. The curves.rs functions (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) follow the same algorithms. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field representation.

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

b3 modular reduction at add/mod.rs:33 and double/mod.rs:33 (b3 = (b * 3) % modulus) is correct.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated through the full stack.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

The projective equivalence formula is mathematically correct (including identity cases — when both Z=0, both checks reduce to 0==0). However, unlike add_impl/double_impl which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Note that impl_sw_proj! at weierstrass.rs:258-263 has the same pattern but is only used in non-zkVM contexts where setup is a no-op.

Suggestion: Add a set_up_once() call at the start of eq() on the #[cfg(target_os = "zkvm")] path. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes (secp256k1, BLS12-381, secp256r1), but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. The curves.rs unit tests cover native implementations, but circuit-level tests (which exercise the AIR constraints end-to-end) should also cover these edge cases to confirm the AIR handles them correctly. Add identity tests →

3. (Low) Stylistic inconsistency in prime access between add and double execution

add/execution.rs:289 accesses the modulus via pre_compute.expr.prime (through Deref<Target=ExprBuilder>) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically — FieldExpr derefs to ExprBuilder — but picking one style consistently would be cleaner.

4. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM host fallback path compute the same product multiple times:

sw-macros/src/lib.rs:189: 8*t0 uses 7 additions instead of 3 doublings
sw-macros/src/lib.rs:201: t0*t1 computed twice instead of once + double
sw-macros/src/lib.rs:209-210: p.x * p.y and p.x * p.z each computed twice
sw-macros/src/lib.rs:226: t2_yz*t1 computed 4 times instead of 1 mul + 2 additions

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so runtime impact is minor, but they're easy wins for host-side execution performance. Fix redundant operations →

5. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would prevent future footguns.

6. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:396-399 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:63) and named techniques
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b) in the projective layout
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) properly updated for projective API with normalize() calls
ECC_BLOCKS_32 = 24 and ECC_BLOCKS_48 = 36 with DEFAULT_BLOCK_SIZE = 4 correctly replaces the old block size patterns
parallel feature correctly enabled on openvm-circuit-primitives dependency in sha2-air
Docs, ISA spec, and README updated consistently with the projective coordinate semantics
Pairing crate GENERATOR/NEG_GENERATOR constants correctly updated with z fields
The WeierstrassPoint trait cleanly extends with z(), from_xyz_unchecked(), normalize(), and is_identity() methods

Summary

Solid PR implementing a well-known optimization with 22-38% AIR column count reductions. The complete projective formulas are correctly implemented, the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs), and benchmarks confirm the performance gains. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) in sw_declare! against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-10T18:10:35Z

Claude finished @shuklaayush's task in 2m 35s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and examine diff
Review AIR circuit constraints (add/double expression builders)
Review execution and trace generation changes
Review guest library, macros, and transpiler changes
Review tests and documentation
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching. The change spans the full stack: AIR constraints, execution, guest libraries, macros, transpiler, docs, and tests.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across all layers. The AIR column count optimizations are well-documented and algebraically sound. Identity representation (0, 1, 0) is consistent throughout. Breaking changes are handled consistently.

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. The combined mul-sub pattern (lines 82-100) saves 3 variables. Degree-2 output optimization at lines 122-138 is algebraically sound. 1761 columns (34% reduction from naive 2668).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented. 2081 columns (38.3% reduction).
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — the Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save. 1359 columns (22.3% reduction).
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — difference-of-squares optimization at lines 178-182 (t1² - y3² instead of (t1+y3)·(t1-y3)) avoids saving two factors. 2034 columns (30.7% reduction).

Native implementations match. The curves.rs functions (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) follow the same algorithms. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field representation.

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

b3 modular reduction at add/mod.rs:33 and double/mod.rs:33 (b3 = (b * 3) % modulus) is correct.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

The projective equivalence formula is mathematically correct (including identity cases — when both Z=0, both checks reduce to 0==0). However, unlike add_proj/double_proj which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun.

Suggestion: Add a Self::set_up_once() call at the start of eq(), matching the pattern in add_proj/double_proj. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes (secp256k1, BLS12-381, secp256r1), but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. The curves.rs unit tests cover native implementations, but circuit-level tests (which exercise the AIR constraints end-to-end) should also cover these edge cases. Add identity tests →

3. (Low) Stylistic inconsistency in prime access between add and double execution

add/execution.rs:289 accesses the modulus via pre_compute.expr.prime (through Deref<Target=ExprBuilder>) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically — FieldExpr derefs to ExprBuilder — but picking one style consistently would be cleaner.

4. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM host fallback path compute the same product multiple times:

sw-macros/src/lib.rs:189: 8*t0 uses 7 additions instead of 3 doublings
sw-macros/src/lib.rs:201: t0*t1 computed twice instead of once + double
sw-macros/src/lib.rs:209-210: p.x * p.y and p.x * p.z each computed twice
sw-macros/src/lib.rs:226: t2_yz*t1 computed 4 times instead of 1 mul + 2 additions

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so runtime impact is minor, but they're easy wins. Fix redundant operations →

5. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would prevent future footguns.

6. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:396-399 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:63) and named techniques
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b) in the projective layout
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) properly updated for projective API with normalize() calls
ECC_BLOCKS_32 = 24 and ECC_BLOCKS_48 = 36 with DEFAULT_BLOCK_SIZE = 4 correctly replaces the old block size patterns
parallel feature correctly enabled on openvm-circuit-primitives dependency in sha2-air
Docs, ISA spec, and README updated consistently with the projective coordinate semantics
Pairing crate GENERATOR/NEG_GENERATOR constants correctly updated with z fields
The WeierstrassPoint trait cleanly extends with z(), from_xyz_unchecked(), normalize(), and is_identity() methods
Sanity tests with proj_to_affine helper provide good confidence in output correctness for standard cases

Summary

Solid PR implementing a well-known optimization with 22-38% AIR column count reductions. The complete projective formulas are correctly implemented, the change propagates consistently through all layers (circuit → execution → guest → macros → transpiler → docs), and benchmarks confirm the performance gains. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) in sw_declare! against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

A description of the `HintNonQr` and `HintSqrt` phantom instructions in the algebra extension was missing from instruction reference. This PR adds it.

Resolves INT-6133. - Rename SW_EC_ADD_NE -> SW_EC_ADD_PROJ, SW_EC_DOUBLE -> SW_EC_DOUBLE_PROJ - Update memory layout from 2*COORD_SIZE to 3*COORD_SIZE (projective) - Add r32_projective_point notation with X, Y, Z coordinates - Update setup validation to check X=modulus, Y=a, Z=b - Reference ePrint 2015/1060 complete formulas - Update ISA spec, instruction reference, transpiler, RISC-V custom code - Update circuit README and user documentation

…te (#2404) Resolves INT-6134 - Update ec_add and ec_double to use projective coordinates (X:Y:Z) instead of affine coordinates (x,y), eliminating field inversions - Implement algorithms from ePrint 2015/1060: - Algorithm 7 (add, a=0) and Algorithm 1 (add, general) - Algorithm 9 (double, a=0) and Algorithm 3 (double, general) - Add k256-specific implementations with normalize_weak() for magnitude tracking - Update BLOCKS constants from affine (2,6) to projective (3,9) - Rename opcodes: SW_EC_ADD_NE -> SW_EC_ADD_PROJ, SW_EC_DOUBLE -> SW_EC_DOUBLE_PROJ - Rename add_ne directory and all identifiers to add (EcAddNeExecutor -> EcAddExecutor, etc.)

Resolves INT-6135. Update guest-side Weierstrass EC from affine to projective coordinates: - WeierstrassPoint trait: projective interface with (X, Y, Z), identity (0, 1, 0), complete addition/doubling formulas, normalize(), is_identity() - impl_sw_proj! macro: replaces impl_sw_affine!, a=0 complete formulas from ePrint 2015/1060 - impl_sw_group_ops!: simplified with no branching (complete formulas handle all edge cases) - sw_declare!/sw_init! proc macros: projective struct, SwAddProj/SwDoubleProj opcodes, setup sends (modulus, a, b), projective PartialEq via cross-multiplication - ECDSA: normalize before extracting affine coordinates - Test programs: normalize results before comparing to known affine values

Resolves INT-6096. - CachedMulTable: updated to use projective add/double --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Adapt projective EC extension to v2's `DEFAULT_BLOCK_SIZE=4` memory constraint. The projective coordinates code used `BLOCK_SIZE=32` (secp256k1) and `BLOCK_SIZE=16` (BLS12-381) for memory accesses, but v2's `TracingMemory` enforces 4-cell accesses. This updates: - `ECC_BLOCKS` constants from `2 * NUM_LIMBS / 4` (affine, 2 coords) to `3 * NUM_LIMBS / 4` (projective, 3 coords) - All hardcoded `<3, 32>` / `<9, 16>` const generics to `<ECC_BLOCKS_N, DEFAULT_BLOCK_SIZE>` in extension, hybrid, and test code - Test helpers to write memory in 4-byte chunks resolves int-6644

github-actions · 2026-04-10T21:52:47Z

Claude finished @shuklaayush's task in 2m 29s —— View job

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Gather context and understand changes
Review AIR circuit constraints (add/double expression builders)
Review execution and trace generation
Review guest library, macros, and transpiler changes
Review tests, docs, and miscellaneous changes
Post final review

This PR migrates Short Weierstrass EC operations from affine coordinates (using DivUnsafe for field inversions) to projective coordinates using complete addition/doubling formulas from ePrint 2015/1060. This eliminates expensive field inversions and handles all edge cases (identity, doubling, P + (-P)) uniformly without branching. The change spans the full stack: AIR constraints, execution, guest libraries, macros, transpiler, docs, and tests.

Overall Assessment

Well-executed migration. The complete projective formulas are correctly implemented across all layers. The AIR column count optimizations are well-documented and algebraically sound. Identity representation (0, 1, 0) is consistent throughout. Breaking changes are handled consistently. Benchmarks show good results (ecrecover 746ms, pairing 923ms).

Correctness

AIR constraint formulas verified correct against ePrint 2015/1060:

ec_add_proj_a0_expr (add/mod.rs:41-142): Algorithm 7 — steps correctly mapped. The combined mul-sub pattern (lines 82-100) saves 3 variables. Degree-2 output optimization at lines 122-138 is algebraically sound. 1761 columns (34% reduction from naive 2668).
ec_add_proj_general_expr (add/mod.rs:144-252): Algorithm 1 — the z3/x3 fusion at lines 207-215 (absorbing step 23 into z3, recovering x3 via 2·t1 - z3) is algebraically correct and well-commented. 2081 columns (38.3% reduction).
ec_double_proj_a0_expr (double/mod.rs:41-119): Algorithm 9 — the Y3 algebraic rearrangement at lines 103-111 (t0² + t2·(4·t0 + z3)) avoids an extra save. 1359 columns (22.3% reduction).
ec_double_proj_general_expr (double/mod.rs:121-212): Algorithm 3 — difference-of-squares optimization at lines 178-182 (t1² - y3² instead of (t1+y3)·(t1-y3)) avoids saving two factors. 2034 columns (30.7% reduction).

Native implementations match. The curves.rs functions (ec_add_proj_impl_a0, ec_double_proj_impl_a0, ec_add_proj_impl_general, ec_double_proj_impl_general) follow the same algorithms. k256-specific implementations correctly handle normalize_weak() for k256's magnitude-tracking field representation.

Identity representation (0, 1, 0) is consistent across impl_sw_proj!, sw_declare!, WeierstrassPoint::IDENTITY, Group::IDENTITY, generator constants in pairing crates, and from_xy_unchecked (which sets z = ONE).

Setup validation is correct in both add/execution.rs:282-319 and double/execution.rs:278-316 — validates modulus, a, and b by splitting BLOCKS into three coordinate regions. The BLOCKS / 3 division is safe because ECC_BLOCKS_* constants are defined as 3 * (NUM_LIMBS / DEFAULT_BLOCK_SIZE) in lib.rs:22-24.

b3 modular reduction at add/mod.rs:33 and double/mod.rs:33 (b3 = (b * 3) % modulus) is correct.

Breaking changes handled consistently — opcode renames (SW_EC_ADD_NE → SW_EC_ADD_PROJ), memory layout (2→3 coords), trait changes, SwBaseFunct7 variant renames, pairing crate constants all correctly propagated through the full stack.

Potential Issues

1. (Medium) PartialEq in sw_declare! emits unguarded modular arithmetic in zkVM

extensions/ecc/sw-macros/src/lib.rs:287-291:

impl core::cmp::PartialEq for #struct_name {
    fn eq(&self, other: &Self) -> bool {
        (&self.x * &other.z) == (&other.x * &self.z)
            && (&self.y * &other.z) == (&other.y * &self.z)
    }
}

The projective equivalence formula is mathematically correct (including identity cases — when both Z=0, both checks reduce to 0==0). However, unlike add_proj/double_proj which have a CHECK_SETUP guard calling set_up_once(), PartialEq does not. If eq() is called before any EC operation triggers setup for the underlying modular type, the modular arithmetic instructions in zkVM would be unconstrained. Safe in practice if equality always follows setup, but a latent footgun. Note that impl_sw_proj! at weierstrass.rs:258-263 has the same pattern but is only used in non-zkVM contexts where setup is a no-op.

Suggestion: Add a Self::set_up_once() call at the start of eq() on the #[cfg(target_os = "zkvm")] path. Add setup guard →

2. (Medium) Missing identity/edge-case tests at circuit level

The test suite covers standard point operations across curves and field sizes (secp256k1, BLS12-381, secp256r1), but all circuit-level tests use Z=1 inputs only (affine in projective form). The key cases that complete projective formulas are designed to handle are missing:

P + Identity = P
Identity + P = P
P + (-P) = Identity
Identity + Identity = Identity
Double(Identity) = Identity
Points with arbitrary Z values (not 1)

These are the primary advantage of complete formulas over the old affine approach. The curves.rs unit tests cover native implementations, but circuit-level tests (which exercise the AIR constraints end-to-end) should also cover these edge cases to confirm the AIR handles them correctly. Add identity tests →

3. (Low) Stylistic inconsistency in prime access between add and double execution

add/execution.rs:289 accesses the modulus via pre_compute.expr.prime (through Deref<Target=ExprBuilder>) while double/execution.rs:285 uses pre_compute.expr.builder.prime (direct field access). Both work identically — FieldExpr derefs to ExprBuilder — but picking one style consistently would be cleaner.

4. (Low) Redundant multiplications in non-zkVM doubling path (sw_declare!)

Several lines in the non-zkVM host fallback path compute the same product multiple times:

sw-macros/src/lib.rs:189: 8*t0 uses 7 additions instead of 3 doublings
sw-macros/src/lib.rs:201: t0*t1 computed twice instead of once + double
sw-macros/src/lib.rs:209-210: p.x * p.y and p.x * p.z each computed twice
sw-macros/src/lib.rs:226: t2_yz*t1 computed 4 times instead of 1 mul + 2 additions

These are only in the non-zkVM host fallback path (the zkVM path calls the chip directly), so runtime impact is minor, but they're easy wins for host-side execution performance. Fix redundant operations →

5. (Low) from_xy maps (0, 0) to identity unconditionally

extensions/ecc/guest/src/weierstrass.rs:67-68:

if x == Self::Coordinate::ZERO && y == Self::Coordinate::ZERO {
    Some(Self::IDENTITY)
}

For curves where (0, 0) is on the curve (b = 0), this would incorrectly map a valid affine point to identity. None of the currently supported curves have b = 0, but a comment documenting this assumption would prevent future footguns.

6. (Low) b3 recomputed on every call in impl_sw_proj!

extensions/ecc/guest/src/weierstrass.rs:396-399 — 3*b is recomputed every add/double call via two field additions. Irrelevant for zkVM targets (the chip handles it), but adds small overhead for non-zkVM targets using impl_sw_proj!.

Code Quality

Well done:

Excellent optimization documentation with specific column count reduction percentages (e.g., "34.0% reduction from naive 2668" in add/mod.rs:63) and named techniques
The three optimization techniques (combined mul-sub, inlined intermediates, degree-2 outputs) are clearly explained and consistently applied across both add and double
Clean separation between AIR expressions (add/mod.rs, double/mod.rs), execution (execution.rs), and native computation (curves.rs)
Comprehensive unit tests in curves.rs covering k256, P256, BN254, BLS12-381 with on-curve verification
The dispatch! macro cleanly handles curve-type specialization with const generics for monomorphized fast paths
Setup validation properly checks all three curve parameters (modulus, a, b) in the projective layout
Integration test programs (ec.rs, ec_nonzero_a.rs, ec_two_curves.rs) properly updated for projective API with normalize() calls
ECC_BLOCKS_32 = 24 and ECC_BLOCKS_48 = 36 with DEFAULT_BLOCK_SIZE = 4 correctly replaces the old block size patterns
parallel feature correctly enabled on openvm-circuit-primitives dependency in sha2-air
Docs, ISA spec, and README updated consistently with the projective coordinate semantics
Pairing crate GENERATOR/NEG_GENERATOR constants correctly updated with z fields
The WeierstrassPoint trait cleanly extends with z(), from_xyz_unchecked(), normalize(), and is_identity() methods
Sanity tests with proj_to_affine helper provide good confidence in output correctness for standard cases

Summary

Solid PR implementing a well-known optimization with 22-38% AIR column count reductions. The complete projective formulas are correctly implemented, the change propagates consistently through all layers (circuit -> execution -> guest -> macros -> transpiler -> docs), and benchmarks confirm the performance gains. The most actionable suggestions are:

Add identity/edge-case tests (issue feat: add preprocessing support #2) to exercise the complete formula's key advantage over the old affine approach at the circuit level
Consider guarding PartialEq (issue feat: add interaction permutation support to prover and verifier #1) in sw_declare! against pre-setup use in zkVM

The remaining issues are minor optimizations for the non-zkVM host fallback path.

github-actions · 2026-04-10T21:58:31Z

group	app.proof_time_ms	app.cycles	leaf.proof_time_ms
fibonacci	3,836	12,000,265	955
keccak	18,340	18,655,329	3,277
sha2_bench	9,857	14,793,960	1,396
regex	1,425	4,137,067	376
ecrecover	733	317,792	353
pairing	910	1,745,757	316
kitchen_sink	2,354	2,580,026	770

Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights.

Commit: 0cc0b0b

Benchmark Workflow

shuklaayush force-pushed the develop-v2.1.0 branch from 04fd82b to e561053 Compare April 1, 2026 17:30

branch-rebase-bot bot force-pushed the develop-v2.0.0-rc.1 branch 3 times, most recently from 1ad4372 to 87f486c Compare April 1, 2026 20:50

jpw-axiom force-pushed the develop-v2.0.0-rc.1 branch from 2dc3ff8 to acfcec5 Compare April 2, 2026 00:56

branch-rebase-bot bot force-pushed the develop-v2.0.0-rc.1 branch from acfcec5 to 45182cf Compare April 2, 2026 01:03

shuklaayush force-pushed the develop-v2.0.0-rc.1 branch from 45182cf to 5fc92b8 Compare April 2, 2026 11:23

shuklaayush force-pushed the develop-v2.1.0 branch from 64261c0 to 19fdb18 Compare April 2, 2026 11:56

shuklaayush force-pushed the develop-v2.1.0 branch from 19fdb18 to 0de0239 Compare April 2, 2026 12:09

shuklaayush force-pushed the develop-v2.1.0 branch from 0de0239 to 9feb9c5 Compare April 2, 2026 12:27

shuklaayush force-pushed the develop-v2.1.0 branch from 9feb9c5 to eb2cbdf Compare April 2, 2026 12:36

This comment has been minimized.

Sign in to view

shuklaayush force-pushed the develop-v2.1.0 branch from eb2cbdf to d96449e Compare April 2, 2026 12:44

This comment has been minimized.

Sign in to view

shuklaayush force-pushed the develop-v2.1.0 branch from 70aaf4d to 58582a0 Compare April 2, 2026 13:57

This comment has been minimized.

Sign in to view

branch-rebase-bot bot force-pushed the develop-v2.0.0-rc.1 branch 3 times, most recently from 51b2eed to 15e3033 Compare April 2, 2026 22:45

shuklaayush force-pushed the develop-v2.1.0 branch from a4b0557 to c00c20c Compare April 8, 2026 21:44

This comment has been minimized.

Sign in to view

shuklaayush force-pushed the develop-v2.1.0 branch from c00c20c to 7e6c07b Compare April 8, 2026 22:14

This comment has been minimized.

Sign in to view

shuklaayush force-pushed the develop-v2.1.0 branch from 7e6c07b to d9dd0f4 Compare April 9, 2026 10:11

This comment has been minimized.

Sign in to view

shuklaayush force-pushed the develop-v2.1.0 branch from d9dd0f4 to 139e261 Compare April 9, 2026 21:44

This comment has been minimized.

Sign in to view

shuklaayush force-pushed the develop-v2.1.0 branch from 139e261 to dc1f2e6 Compare April 10, 2026 13:15

This comment has been minimized.

Sign in to view

branch-rebase-bot bot force-pushed the develop-v2.0.0-rc.1 branch from c627316 to 0d19e9c Compare April 10, 2026 18:01

shuklaayush force-pushed the develop-v2.1.0 branch from dc1f2e6 to 3898ae8 Compare April 10, 2026 18:10

This comment has been minimized.

Sign in to view

branch-rebase-bot bot force-pushed the develop-v2.0.0-rc.1 branch from 5cd1c1c to 71a0da5 Compare April 10, 2026 21:52

Avaneesh-axiom and others added 7 commits April 10, 2026 23:52

chore: Add phantom instructions to algebra specs (#2334)

84de24e

A description of the `HintNonQr` and `HintSqrt` phantom instructions in the algebra extension was missing from instruction reference. This PR adds it.

perf: switch SW_EC to projective coordinates - MSM (#2421)

4d496ad

Resolves INT-6096. - CachedMulTable: updated to use projective add/double --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

fix: enable parallel feature on openvm-circuit-primitives in sha2-air

0cc0b0b

shuklaayush force-pushed the develop-v2.1.0 branch from 3898ae8 to 0cc0b0b Compare April 10, 2026 21:52

Conversation

shuklaayush commented Apr 1, 2026

Uh oh!

github-actions bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Overall Assessment

Correctness

Potential Issues

Code Quality

Breaking Changes

Summary

Uh oh!

github-actions bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Overall Assessment

Correctness

Potential Issues

Code Quality

Breaking Changes

Summary

Uh oh!

github-actions bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Overall Assessment

Correctness Verification

Potential Issues

Code Quality

Breaking Changes

Summary

Uh oh!

github-actions bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Overall Assessment

Correctness

Potential Issues

Code Quality

Summary

Uh oh!

github-actions bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Overall Assessment

Correctness

Potential Issues

Code Quality

Breaking Changes

Summary

Uh oh!

github-actions bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Overall Assessment

Correctness

Potential Issues

Code Quality

Summary

Uh oh!

github-actions bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Overall Assessment

Correctness

Potential Issues

Code Quality

Summary

Uh oh!

This comment has been minimized.

github-actions bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: PR #2646 — SW_EC Projective Coordinates Migration

Overall Assessment

Correctness

Potential Issues

Code Quality

Summary

Uh oh!

github-actions bot commented Apr 1, 2026 •

edited

Loading

github-actions bot commented Apr 1, 2026 •

edited

Loading

github-actions bot commented Apr 1, 2026 •

edited

Loading

github-actions bot commented Apr 2, 2026 •

edited

Loading

github-actions bot commented Apr 2, 2026 •

edited

Loading

github-actions bot commented Apr 2, 2026 •

edited

Loading

github-actions bot commented Apr 2, 2026 •

edited

Loading

github-actions bot commented Apr 2, 2026 •

edited

Loading

github-actions bot commented Apr 2, 2026 •

edited

Loading

github-actions bot commented Apr 2, 2026 •

edited

Loading

github-actions bot commented Apr 8, 2026 •

edited

Loading

github-actions bot commented Apr 8, 2026 •

edited

Loading

github-actions bot commented Apr 9, 2026 •

edited

Loading

github-actions bot commented Apr 9, 2026 •

edited

Loading

github-actions bot commented Apr 10, 2026 •

edited

Loading