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

SW <-> TE map for Bandersnatch #804

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

drskalman
Copy link
Contributor

Description

Implement the birational map between the Short Weierstrass and Twisted Edwards forms of the Bandersnatch curve.

It is important because it is significantly cheaper to verifiy SNARK using the incomplete addition formula of SW form while Elligator hash-to-curve is significantly cheaper than WB or SWU hash-to-curve.

Specifically for Bandersnatch serializing in TE form saves a byte from 33 to 32 which is more machine friendly.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@drskalman drskalman requested review from a team as code owners March 8, 2024 14:32
@drskalman drskalman requested review from z-tech, Pratyush and weikengchen and removed request for a team March 8, 2024 14:32
@@ -4,6 +4,7 @@

- [\#772](https://github.com/arkworks-rs/algebra/pull/772) (`ark-ff`) Implementation of `mul` method for `BigInteger`.
- [\#794](https://github.com/arkworks-rs/algebra/pull/794) (`ark-ff`) Fix `wasm` compilation.
- [\#XXX](https://github.com/arkworks-rs/algebra/pull/XXX) (`ark-curves`) Implement TE <-> SW map for Bandersnatch.
Copy link
Member

@weikengchen weikengchen Mar 8, 2024

Choose a reason for hiding this comment

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

Suggested change
- [\#XXX](https://github.com/arkworks-rs/algebra/pull/XXX) (`ark-curves`) Implement TE <-> SW map for Bandersnatch.
- [\#804](https://github.com/arkworks-rs/algebra/pull/XXX) (`ark-curves`) Implement TE <-> SW map for Bandersnatch.

(since the PR is from an organization, I cannot edit directly)

@weikengchen
Copy link
Member

Ok, I need to say that we generally want to have uniformity in the curve repository, so if we want to add, likely we will structure a trait and facilitates it from there. I.e., implementing it for all the curves if possible.

What do you think? It would be preferred, of course, if the implementation can be done for general curves.

But I want to ask a question about the motivation: so, if I remember correctly, the incomplete addition formula for SW should have similar performance with the one in twisted Edwards curve, nope?

@burdges
Copy link
Contributor

burdges commented Mar 9, 2024

I'd think one or more flavors of this map should typically exist whenever the both the TE and SW forms exist, although some exceptions exist. Among other reasons, serialization should be in Edwards, even if the circuts use SW, if only because the serialized form is one byte shorter, but often because advanced compression techniques are only researched for TE.

In particular, Bandersnatch should be serialized as ganderwagon, which reuqires passing through TE. I suppose decaf377 would similarlly be the preferable serialization for BLS12-377.

As a rule, you'll have worse interfaces if you rush into trait design than if you let the requirement emerge organically, with the hash-to-curve shit show being one example. We do not know if multiple isogenies were alredy deployed for bandersnatch, bls12-377, or ed25519.

@mmagician
Copy link
Member

I agree with @weikengchen here.
Also, contrary to what @burdges says, having an extra trait (say SW_TE_Mapping) doesn't mean the interface will necessarily get bloated. Instead, people can then choose to implement the trait for whatever curves they want.

@burdges
Copy link
Contributor

burdges commented Apr 6, 2024

It's quite general if you do roughly:

pub trait IsogenySW2TE {
    type SW: SWCurveConfig;
    type TE: TECurveConfig;
    ...
}

You can even still do impl IsogenySW2TE for Config { .. } in bls12_377 there, but doing this does not forbid other mappings since the curves are given by associated types.

I'm warning against assuming that particular impl exists, aka ignoring the associated types. I'm especially warning against doing this:

pub trait CannonicalSW2TE: SWCurveConfig + TECurveConfig {
    ...
}

I'm also saying there is no real reason to block providing the mapping upon selecting the trait, but if the general trait exists then whatever.

MontFp!("41180284393978236561320365279764246793818536543197771097409483252169927600582");

impl BandersnatchConfig {
/// Map an point in TE form to its corresponding point on SW curve
Copy link

Choose a reason for hiding this comment

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

Suggested change
/// Map an point in TE form to its corresponding point on SW curve
/// Map a point in TE form to its corresponding point on SW curve

let v = v_w_num * v_denom_inv;
let w = v_w_num * w_denom_inv;

//now we are mappyng the montgamory point to SW.
Copy link

@dvdplm dvdplm Apr 17, 2024

Choose a reason for hiding this comment

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

Suggested change
//now we are mappyng the montgamory point to SW.
//now we are mapping the montgomery point to SW.

Comment on lines +203 to +206
debug_assert!(
point_on_sw_curve.is_on_curve(),
"TE point mapped off the SW curve"
);
Copy link

Choose a reason for hiding this comment

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

I'd probably play it safe here and use SWAffine::new here and remove the debug asserts. That way you'd run both checks. Then consider adding a te_to_sw_unchecked() if/when there's proof that it's too slow to run the checks.

}

pub fn map_sw_to_te(point: SWAffine) -> Option<EdwardsAffine> {
//first we are mappyng the sw point to montgamory point
Copy link

Choose a reason for hiding this comment

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

Suggested change
//first we are mappyng the sw point to montgamory point
//first we are mappng the sw point to its montgomery point

Comment on lines +226 to +230
let point_on_te_curve = EdwardsAffine::new_unchecked(v, w);
debug_assert!(
point_on_te_curve.is_on_curve(),
"TE point mapped off the SW curve"
);
Copy link

Choose a reason for hiding this comment

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

Same comment as above: consider using EdwartdsAffine::new instead and ditch the debug assert.

impl BandersnatchConfig {
/// Map an point in TE form to its corresponding point on SW curve
pub fn map_te_to_sw(point: EdwardsAffine) -> Option<SWAffine> {
//First we map the point from the TE to Montgamory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//First we map the point from the TE to Montgamory
if point.is_zero() {
return SWAffine::identity()
}
//First we map the point from the TE to Montgamory

}

pub fn map_sw_to_te(point: SWAffine) -> Option<EdwardsAffine> {
//first we are mappyng the sw point to montgamory point
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//first we are mappyng the sw point to montgamory point
if point.is_zero() {
return EdwardsAffine::zero()
}
// First we are mappyng the sw point to montgamory point

@moven0831
Copy link

moven0831 commented Jun 16, 2024

Hi, I'm working on the SW <-> TE map for BN254 and came across this PR. Nice work! One question: is it possible to apply the same technique "Short Weierstrass <-> Montgomery <-> Twisted Edwards" for BN254 curve? Thank you.

Update: Since BN254 is a prime order curve, there is no TE model for it. Thanks for the clarification from Pratyush.

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.

8 participants