Skip to content

Conversation

@manh9203
Copy link
Contributor

@manh9203 manh9203 commented Dec 6, 2025

This PR is a WIP.
The current design and known issues are documented in extensions/ecc/circuit/src/weierstrass_chip/mul/README.md.

Resolves INT-5099.

Copilot AI review requested due to automatic review settings December 6, 2025 17:57
@manh9203 manh9203 marked this pull request as draft December 6, 2025 17:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces an EC (Elliptic Curve) multiplication chip to accelerate elliptic curve scalar multiplication operations. The implementation replaces manual IntrinsicCurve trait implementations with a declarative curve_declare! macro that generates the necessary boilerplate code. The PR adds hardware acceleration for EC multiplication while maintaining backward compatibility through a heuristic fallback for small operations.

Key Changes:

  • Introduces openvm-ecc-curve-macros crate with curve_declare! and curve_init! macros
  • Adds EC multiplication adapter, AIR, and circuit implementations
  • Updates IntrinsicCurve::msm signature to include CHECK_SETUP const generic parameter
  • Adds curve_name field to all curve configurations across the codebase

Reviewed changes

Copilot reviewed 48 out of 49 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
extensions/ecc/curve-macros/src/lib.rs New proc macro crate implementing curve_declare! and curve_init! macros for code generation
extensions/rv32-adapters/src/ec_mul.rs New adapter for EC multiplication with memory operations and register handling
extensions/ecc/circuit/src/weierstrass_chip/mul/* EC multiplication chip implementation with execution logic and AIR constraints
extensions/ecc/circuit/src/weierstrass_chip/curves.rs Native EC multiplication implementations for K256, P256, BN254, and BLS12_381 curves
extensions/ecc/guest/src/weierstrass.rs Updated IntrinsicCurve trait with CHECK_SETUP parameter and set_up_once method
guest-libs/{p256,k256,pairing}/src/*.rs Replaced manual IntrinsicCurve implementations with curve_declare! macro usage
extensions/ecc/circuit/src/extension/weierstrass.rs Added EC mul executor, AIR, and chip integration for both 32 and 48 limb primes
crates/vm/src/arch/integration_api.rs Added Rv32EcMulAdapterInterface and conversions for different-sized tuple arrays
**/openvm.toml Added curve_name field to all curve configurations
extensions/ecc/transpiler/src/lib.rs Added EC_MUL and SETUP_EC_MUL opcodes to the transpiler

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 52 out of 53 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +98 to +99
// TODO: Add `pub field_expr_gate: T` here to fix the column layout mismatch.
// This field should always be 0, gating FieldExpr constraints for digest rows.
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

This TODO represents a critical bug that causes STARK verification failures. The AIR evaluates FieldExpr on digest rows, reading is_valid from offset base_width, but this offset contains from_state.pc (non-zero) for digest rows. This causes assert_bool(is_valid) to fail because pc is not boolean. The field pub field_expr_gate: T should be added before from_state to ensure local[base_width] = 0 for digest rows, acting as FieldExpr's is_valid = 0.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants