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

Twisted Edwards curves operations #1949

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

querolita
Copy link
Member

This PR is the twisted counterpart of the operations found in /src/lib/provable/gadgets/elliptic-curve.ts. It is a necessary step to support EdDSA, which uses the twisted curve Ed25519. Related to o1-labs/o1js-bindings#317.

@querolita querolita marked this pull request as ready for review January 10, 2025 22:36
@querolita querolita requested review from a team as code owners January 10, 2025 22:36
@querolita querolita requested review from 45930 and ymekuria January 10, 2025 22:36
@querolita querolita force-pushed the feature/eddsa/twisted branch from b741643 to 576725d Compare January 15, 2025 18:56
Copy link
Member Author

@querolita querolita left a comment

Choose a reason for hiding this comment

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

left some comments for reviewers

toBigint({ x, y }: Point) {
let x_ = Field3.toBigint(x);
let y_ = Field3.toBigint(y);
return { x: x_, y: y_, infinity: x_ === 0n && y_ === 1n };
Copy link
Member Author

Choose a reason for hiding this comment

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

I created an equivalent Point type (elliptic-curve also has it) with the difference that the infinity is not always set to false as it is the case in the other gadgets.

let witnesses = exists(12, () => {
let [x1_, x2_, y1_, y2_] = Field3.toBigints(x1, x2, y1, y2);

// TODO: reuse code in twistedAdd to avoid recomputing these
Copy link
Member Author

Choose a reason for hiding this comment

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

This would be a nice feature and much cleaner code, but apparently this wouldn't be a trivial refactor at all. Ideas for the future: inspiration from the interpreter structure in o1vm?

* TODO: could use lookups for picking precomputed multiples, instead of O(2^c) provable switch
* TODO: custom bit representation for the scalar that avoids 0, to get rid of the degenerate addition case
*/
function multiScalarMul(
Copy link
Member Author

Choose a reason for hiding this comment

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

basically the difference between this function and the counterpart in elliptic-curve.ts is that the Curve is a CurveTwisted instead of an affine curve. I don't know if making this more generic would be a good idea, maybe we want to have a specific implementation for each curve instead of parameterizing it. Open to suggestions (similar concerns arise in other functions of this interface as well).

@mitschabaude
Copy link
Collaborator

Since twisted curve addition is complete, there's a lot of complexity in the scalar multiplication code around handling zeros that could be removed. E.g. you don't need the initial aggregator point, and you can just return the result instead of asserting it to be zero or non-zero

@querolita
Copy link
Member Author

@mitschabaude Oh that makes sense, I will look into it and clean the algorithm for twisted msm.

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.

4 participants