Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Draft] wNAF multiplication for BN256 curve #36

Closed

Conversation

ZamDimon
Copy link

@ZamDimon ZamDimon commented Mar 12, 2024

What ❔

This pull request implements multiplication by a scalar for the BN256 curve. This PR adds:

  • WNAF-based implementation, mostly based on this implementation.
  • .sage files for finding:
    • $\beta$ and $\lambda$ parameters for useful endomorphism;
    • short vectors $(a_1,b_1)$ and $(a_2,b_2)$ for effective scalar decomposition

Why ❔

Currently, multiplication by a scalar is not implemented for BN256 and therefore one needs to add support for such an operation.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.

@ZamDimon ZamDimon changed the title wNAF multiplication for BN256 curve [Draft] wNAF multiplication for BN256 curve Mar 12, 2024
@ZamDimon ZamDimon marked this pull request as draft March 12, 2024 20:16
Copy link
Member

@jules jules left a comment

Choose a reason for hiding this comment

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

Everything looks great - as discussed, let's move to the cheaper mul circuit and we should be done here :)

Copy link
Member

Choose a reason for hiding this comment

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

As discussed, this can be deleted

Copy link
Member

Choose a reason for hiding this comment

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

Can also be deleted I believe


/// Converts the specified `BN256ScalarNNField` element to a
/// WNAF representation using an array of `UInt8<F>` elements.
fn convert_to_wnaf<F: SmallField, CS: ConstraintSystem<F>>(
Copy link
Member

Choose a reason for hiding this comment

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

All wNAF functionality can be removed. We keep the GLV decomposition but instead use Alex's fixed-width 4-bit mul circuit found here. Note that the code here deals with potential 129 bit decomposed scalars, whereas bn254 will only occupy max 127 bits. Thus, there should be some extra constraints we can drop as the scalars fit in a smaller size.

@ZamDimon
Copy link
Author

ZamDimon commented Apr 4, 2024

Hey @jules, enormous thanks for the review! Updated the wnaf-based scalar multiplication to a fixed-width 4-bit multiplication, as requested, according to implementation here.

After your comment, I am now aware of the extra circuit complexity due to dealing with the potential 129-bit decomposed scalar. Still, I suggest staying with the current implementation since it is well-tested. Then, after we successfully integrate the ecMul in the more high-level repositories and make sure it works, we will surely cut off the constraints.

@ZamDimon
Copy link
Author

BN254-specific functions will be transferred to era-zkevm_circuits. For helper gadgets, see #38. Thus, we close this pull request.

@ZamDimon ZamDimon closed this May 22, 2024
@NikitaMasych NikitaMasych deleted the feature/wnaf-scalar-mul branch May 23, 2024 13:42
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.

None yet

2 participants