Skip to content

Conversation

@MozirDmitriy
Copy link

This change implements the previously unimplemented MontgomeryPoint::to_edwards() using the 4-isogeny inverse consistent with the existing forward map in EdwardsPoint::to_montgomery(). It solves the quadratic for y^2 from u, checks discriminant and square roots in the field, and uses the provided sign bit to select the x-sign, returning None on non-residues or invalid denominators. This removes a todo!() panic point, enables Montgomery to Edwards conversion needed for interoperability, and follows existing project style and primitives without changing the public API.

@tarcieri
Copy link
Member

The implementation in this PR is very much variable-time. Does it have prior art somewhere?

I'm wondering if a more straightforward constant-time implementation is possible.

#1350 implements the 4-isogeny. I'm wondering if instead you could first convert from Montgomery to twisted Edwards, then leverage that to convert to untwisted Edwards?

cc @daxpedda

@tarcieri
Copy link
Member

tarcieri commented Oct 25, 2025

It seems like at least an initial implementation of this could use a similar method to curve25519-dalek: computing the Edwards y-coordinate from the Montgomery u-coordinate (and sign bit, passed as parameter), and then decompressing.

FWIW, I guess their implementation handles the exceptional cases in variable-time.

@daxpedda
Copy link
Contributor

daxpedda commented Oct 30, 2025

I already added a working and constant-time implementation in #1306, taken from https://www.rfc-editor.org/rfc/rfc7748#section-4.2.

I will take a look into improving it first and then see if I can use some of the strategies proposed here.

Let me know if you want me to spin it out into a separate PR.

@daxpedda
Copy link
Contributor

daxpedda commented Oct 30, 2025

I went ahead and optimized the conversion in 4071bb7, reducing it to a single inversion.

Regarding the proposed solutions: I'm not aware of any way to get the Montgomery point into a twisted form to use the isogeny mapping introduced in #1350. I'm also a bit confused by the strategy used in curve25519-dalek, which seems to be much more expensive to me then just completing the birational map from the RFC (will make a PR to that effect).

EDIT: Just understood why curve25519-dalek does this; because calculating the y-coordinate is actually expensive. So we can indeed re-use the same optimization here.

@daxpedda
Copy link
Contributor

I analyzed the optimization curve25519-dalek used here a bit further.

So we have three different computations involved (only counting expensive operations):

  1. Computing the y-coordinate: 1 square root operation.
  2. Using the Montgomery to Edwards mapping: 1 inversion operation.
  3. Decompression: 1 square root ratio operation.

All three operations turn out to be similarly expensive.
The #1306 implementation: 1. + 2.
The curve25519-dalek implementation: 2. + 3.

In conclusion: both implementation are about equally expensive.

(I also noticed that I did already some optimization in a separate PR: #1308).

@tarcieri
Copy link
Member

tarcieri commented Oct 30, 2025

I'm not aware of any way to get the Montgomery point into a twisted form

See Section 3 of https://eprint.iacr.org/2008/013.pdf

"In this section we show that the set of Montgomery curves over k is equivalent to the set of twisted Edwards curves
over k."

I also noticed that I did already some optimization in a separate PR: #1308

So many PRs! Sorry

@daxpedda
Copy link
Contributor

I'm not aware of any way to get the Montgomery point into a twisted form

See Section 3 of https://eprint.iacr.org/2008/013.pdf

"In this section we show that the set of Montgomery curves over k is equivalent to the set of twisted Edwards curves over k."

Hm, I couldn't get it to work.

In any case, this shouldn't really be an improvement anyway. Because our mapping could also skip inversion if we convert to a projective point, similar to what our 4-isogeny map from #1350 achieves.

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.

3 participants