Skip to content

Conversation

@yelhousni
Copy link
Contributor

@yelhousni yelhousni commented Feb 4, 2025

Description

This PR implements arithmetic over the Grumpkin elliptic curve as native operations in a BN254 SNARK. This is similar to sw_bls12377 and sw_bls24315 packages, except there is no (efficient) pairing on Grumpkin. This needs Consensys/gnark-crypto#625 to be merged first.

Grumpkin forms a 2-cycle with BN254 and the combination is useful for many protocols.

TODO:

  • implement endomorphism-based optimizations.

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

Arithmetic tests against gnark-crypto are implemented + edge cases.

How has this been benchmarked?

r1cs scs
scalar mul 1,724 4,899
joint scalar mul 3,100 8,500

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Note

Introduce native Grumpkin curve support (affine ops, scalar/joint/multi-scalar mul with GLV) and wire it into defaults and emulation params, with hints and tests.

  • Algebra (native):
    • Add std/algebra/native/sw_grumpkin implementing Grumpkin G1Affine ops (Add, Double, unified add), scalar mul (const/variable, GLV endomorphism), joint and multi-scalar mul (incl. folding), base mul, marshalling, and equality.
    • Include solver hints for scalar decomposition and limb splitting; precompute inner-curve config and tables.
  • Integration:
    • Extend std/algebra/defaults.go to return Curve[sw_grumpkin.ScalarField, sw_grumpkin.G1Affine] via sw_grumpkin.NewCurve.
    • Add emulation params emparams.GrumpkinFp and emparams.GrumpkinFr.
  • Tests/Docs:
    • Comprehensive tests for add/double, (joint) scalar mul, MSM (incl. edge cases), and marshalling.
    • Package docs for sw_grumpkin.

Written by Cursor Bugbot for commit 82bd788. This will update automatically on new commits. Configure here.

@yelhousni yelhousni self-assigned this Feb 4, 2025
@yelhousni yelhousni added this to the v0.11.N milestone Feb 4, 2025
@yelhousni yelhousni requested a review from ivokub February 4, 2025 23:00
@ivokub ivokub modified the milestones: v0.11.N, v0.15.0 Oct 7, 2025
@cursor
Copy link

cursor bot commented Oct 23, 2025

Bug: Incorrect Validation Logic in Decompose Function

The decompose function's input validation uses && instead of ||. This causes validation to pass when only one of the input or output slice lengths is incorrect, allowing invalid calls to proceed silently and potentially leading to out-of-bounds access or incorrect behavior.

Additional Locations (1)

Fix in Cursor Fix in Web

@cursor
Copy link

cursor bot commented Oct 23, 2025

Bug: Input/Output Validation Logic Error

The input/output length validation uses && (AND) instead of || (OR). This allows invalid input combinations to pass silently, such as 2 inputs with 4 outputs, or 1 input with 3 outputs. The condition should be if len(inputs) != 1 || len(outputs) != 4 to properly validate that exactly 1 input and 4 outputs are provided.

Fix in Cursor Fix in Web

@Consensys Consensys deleted a comment from Saw-mon-and-Natalie Oct 23, 2025
@cursor
Copy link

cursor bot commented Oct 23, 2025

Bug: Invalid Length Handling Causes Out-of-Bounds Access

The decompose hint's input validation uses && instead of ||. This means it only catches errors when both the input and output lengths are incorrect, allowing invalid lengths to pass. This can lead to out-of-bounds access and panics when the function attempts to use the inputs or outputs.

Fix in Cursor Fix in Web

cursor[bot]

This comment was marked as outdated.

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Looks good to me. I updated a bit:

  • package documentation style
  • implemented algebra.Curve interface by adding marshaling
  • tests for point marshaling
  • added explicit check that the native field is BN254 when initializing Curve
  • decomposition hint input validation

With that I saw similar issues for other 2-chain curves. I'll submit them in a separate PR.

@yelhousni yelhousni merged commit dfaa2ad into master Oct 27, 2025
7 checks passed
@yelhousni yelhousni deleted the feat/grumpkin-curve branch October 27, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants