Skip to content

Conversation

daxpedda
Copy link
Contributor

A bunch of functionality implemented on EdwardsPoint internally just converts to AffinePoint. This functionality should rather live in AffinePoint.

This PR also implement BatchNormalize for EdwardsPoint, which wouldn't have much of a point without these changes to begin with.

@tarcieri
Copy link
Member

tarcieri commented Aug 3, 2025

This is definitely pushing my knowledge of both Edwards curves and Ed448-Goldilocks, and sent me on a rabbit hole reading the paper.

I think both versions of edwards_isogeny are naively implemented, and we would ideally be converting directly from EdwardsPoint to TwistedExtendedPoint (sidebar: which should probably be called TwistedEdwardsPoint for consistency, I guess the former was renamed from ExtendedPoint at some point) without first converting to an intermediate AffinePoint with its associated inversions. There's even a comment to that effect:

So what we can do is optimise each respective isogeny method for a=1 or a = -1 (currently, I just made it really slow and simple)

Edit: opened an issue about this #1349, and it seems to be the reason you weren't seeing speedups using twisted scalar multiplication in #1337

@tarcieri
Copy link
Member

tarcieri commented Aug 3, 2025

To expound on that:

  • EdwardsPoint should probably remain the primary user-facing type with the majority of the functionality
  • AffinePoint and TwistedExtendedPoint are implementation details

Conversions look like:

CompressedEdwardsY <-> AffinePoint <-> EdwardsPoint <-> TwistedExtendedPoint

...but I think it's fine to preserve methods like compress and decompress directly on EdwardsPoint, but with the actual point compression/decompression implementation factored onto AffinePoint, and the methods on EdwardsPoint delegating to AffinePoint and doing affine conversions as needed.

Likewise, scalar multiplication would ideally do a more direct conversion from EdwardsPoint to TwistedExtendedPoint, and delegate to the implementation there.

@daxpedda
Copy link
Contributor Author

daxpedda commented Aug 3, 2025

compress() and decompress() remain available on EdwardsPoint through GroupEncoding. Eventually we should remove these functions entirely and implement GroupEncoding on AffinePoint as well (tracking that in #1348).

I hope I'm on the right track here? I'm kinda trying to align things with the other RustCrypto crates, where most types have very few methods and its all mostly implemented through traits. It also makes sense to me, because otherwise we have all those duplicate methods.

@daxpedda daxpedda marked this pull request as draft August 3, 2025 14:38
@daxpedda
Copy link
Contributor Author

daxpedda commented Aug 3, 2025

Putting on draft until I rebase. Still working on those optimized implementations from #1316.

@daxpedda daxpedda marked this pull request as ready for review August 3, 2025 16:57
@daxpedda daxpedda requested a review from tarcieri August 3, 2025 17:00
@@ -61,7 +64,7 @@ impl ExpandedSecretKey {

let point = EdwardsPoint::GENERATOR * scalar;
let public_key = VerifyingKey {
compressed: point.compress(),
compressed: point.to_affine().compress(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't remove this because we are storing CompressedEdwardsY in VerifyingKey and GroupEncoding doesn't return that.

I'm happy to change the type.
I'm planning on removing the type entirely anyway to align with the other curves in this repo, so we can wait for that as well.

@daxpedda daxpedda requested a review from tarcieri August 3, 2025 20:56
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