Pseduo-remove MapToCurve::new, renamed to check_parameters #679

burdges · 2023-09-08T23:31:26Z

Description

MapToCurve::new seemingly originates from a more runtime oriented elliptic curve crate: https://github.com/armfazh/redox-ecc/blob/master/src/ellipticcurve.rs#L36

Arguably test_parameters should be inherent methods, invoked by whoever defined the parameters, but maybe not? I left the trait method for now. I suspect the hash-to-curve code would benefit form a wider more idiomatic rewrite btw.

Rebase of #627 but also moved to personal github, so maintainers could make changes. See previous discussion: #627 (comment)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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

MapToCurve::new seemingly originates from a more runtime oriented elliptic curve crate: https://github.com/armfazh/redox-ecc/blob/master/src/ellipticcurve.rs#L36 Arguably test_parameters should be inherent methods, invoked by whoever defined the parameters, but maybe not? I left the trait method for now.

Co-authored-by: Pratyush Mishra <[email protected]>

rustfmt is wrong that f()? should be preferred over f() ?, as errors paths should often be highlighted, but nobody merged that fix yet, so whatever. rust-lang/rustfmt#5595

mmagician

I left two comments to potentially simplify the interface further.

mmagician · 2023-09-11T11:55:49Z

ec/src/hashing/map_to_curve_hasher.rs

@@ -24,7 +24,7 @@ where
    M2C: MapToCurve<T>,
 {
    field_hasher: H2F,
-    curve_mapper: M2C,
+    _curve_mapper: PhantomData<M2C>,


how about bundling the two phantoms, then:

_phantom: PhantomData<(T, M2C)>,

I forget if this goes away later or not, but maybe it stays, so sure let's merge them.

mmagician · 2023-09-11T12:00:37Z

ec/src/hashing/curve_maps/wb.rs

@@ -79,13 +79,13 @@ pub trait WBConfig: SWCurveConfig + Sized {
 }

 pub struct WBMap<P: WBConfig> {
-    swu_field_curve_hasher: SWUMap<P::IsogenousCurve>,
+    swu_field_curve_hasher: PhantomData<SWUMap<P::IsogenousCurve>>,


That's not needed anymore IIUC?
I think we can remove this field, and change curve_params to:

phantom: PhantomData<P>,

Almost:

fn() -> P is covariant in P https://doc.rust-lang.org/nomicon/subtyping.html

SWUMap<P::IsogenousCurve> is fn() -> P::IsogenousCurve which is covariant in P::IsogenousCurve.

We know the story here of course, but in principle you cannot say the associated type P::IsogenousCurve is covariant in P. It's be fine to do this in practice, given it's all static in practice, but maybe leave it for now?

Sure, it also doesn't affect the user's end.

…rkworks-rs#679)" This reverts commit 85546e4.

Co-authored-by: Jeff Burdges <[email protected]>

…rs#679) Co-authored-by: Pratyush Mishra <[email protected]> Co-authored-by: ¨Jeff <¨[email protected]¨>

Co-authored-by: Jeff Burdges <[email protected]>

This reverts commit bd13d1e.

…rkworks-rs#679)" This reverts commit 40e8b67.

This reverts commit 5dfeedf.

of arkworks-rs#679

* Implement Elligator2 hash to curve for Twisted Edward curves * - Update the pull request number for Elligator2 map in CHANGELOG.md - Fix fmt errors in other parts of the repo. * Remove diagnostic `println`s * Remove irrelevant comment as elligator is not based on pasta Co-authored-by: mmagician <[email protected]> * Referencing RFC9380 for hash-to-curve instead of the draft, plus comment clean up. Co-authored-by: mmagician <[email protected]> * Cite new reference for hash-to-curve Co-authored-by: mmagician <[email protected]> * Make sig0 function of elligator2 map boolean instead of 0u8, 1u8 Co-authored-by: mmagician <[email protected]> * Move parity method from `curve_maps::swu` to `curve_maps` as it is used by both swu and elligator. * Remove map-to-curve sanity checks from release build. * cargo fmt * - apply new naming convention for map2curves - rename `new` to `check_parameters` * fix the documentation for `Elligator2Map::check_parameters` * Call elements of the field `element` not `point` in SWU hash-to-curve map * Mention moving of `parity` function in breaking changes. * fmt * move \#659 from pending bto features in `CHANGELOG.md` * bring back `new` and MapToCurve Object. * `cargo fmt` * Move Elligator2 pre-computatable values to `Elligator2Config` * Pre-computatable Elligator2 test example * Move Elligator2 `MapToCurve` implementation to its own folder. * Fix reference to the Elligator paper * Make elligator curve map a static object following revert of revert of #679 * Remove whitespace * Stricter check on SWU parameters Co-authored-by: Marcin <[email protected]> --------- Co-authored-by: mmagician <[email protected]> Co-authored-by: Pratyush Mishra <[email protected]> Co-authored-by: Pratyush Mishra <[email protected]>

¨Jeff and others added 9 commits September 9, 2023 01:24

Avoid unused subdirectories

bb39f36

Update ec/src/hashing/curve_maps/wb.rs

2f966d6

Co-authored-by: Pratyush Mishra <[email protected]>

Update ec/src/hashing/curve_maps/swu.rs

682feca

Co-authored-by: Pratyush Mishra <[email protected]>

Update ec/src/hashing/map_to_curve_hasher.rs

784f3ef

Co-authored-by: Pratyush Mishra <[email protected]>

Update ec/src/hashing/curve_maps/swu.rs

472bd01

Co-authored-by: Pratyush Mishra <[email protected]>

Two more renames

201940b

make rustfmt happier

69812fb

rustfmt is wrong on this one but whatever

0afbdfc

rustfmt is wrong that f()? should be preferred over f() ?, as errors paths should often be highlighted, but nobody merged that fix yet, so whatever. rust-lang/rustfmt#5595

burdges requested review from a team as code owners September 8, 2023 23:31

burdges requested review from Pratyush, mmagician and weikengchen and removed request for a team September 8, 2023 23:31

burdges mentioned this pull request Sep 8, 2023

Pseduo-remove MapToCurve::new, renamed to check_parameters #627

Closed

6 tasks

mmagician reviewed Sep 11, 2023

View reviewed changes

Request by mmarcin

ee43eea

Pratyush approved these changes Sep 11, 2023

View reviewed changes

mmagician approved these changes Sep 12, 2023

View reviewed changes

Pratyush merged commit 85546e4 into arkworks-rs:master Sep 12, 2023
15 checks passed

mmagician pushed a commit to mmagician/algebra that referenced this pull request Oct 4, 2023

Revert "Pseduo-remove MapToCurve::new, renamed to check_parameters (a…

65277e5

…rkworks-rs#679)" This reverts commit 85546e4.

mmagician mentioned this pull request Oct 4, 2023

Revert #679 #687

Merged

6 tasks

Pratyush pushed a commit that referenced this pull request Oct 4, 2023

Revert #679 (#687)

5dfeedf

Co-authored-by: Jeff Burdges <[email protected]>

aleasims pushed a commit to NilFoundation/arkworks-algebra that referenced this pull request Oct 18, 2023

Revert arkworks-rs#679 (arkworks-rs#687)

bd13d1e

Co-authored-by: Jeff Burdges <[email protected]>

aleasims added a commit to NilFoundation/arkworks-algebra that referenced this pull request Oct 18, 2023

Revert "Revert arkworks-rs#679 (arkworks-rs#687)"

de86bce

This reverts commit bd13d1e.

aleasims added a commit to NilFoundation/arkworks-algebra that referenced this pull request Oct 18, 2023

Revert "Pseduo-remove MapToCurve::new, renamed to check_parameters (a…

03af8ef

…rkworks-rs#679)" This reverts commit 40e8b67.

mmagician added a commit to mmagician/algebra that referenced this pull request Oct 31, 2023

Revert "Revert arkworks-rs#679 (arkworks-rs#687)"

1603826

This reverts commit 5dfeedf.

drskalman added a commit to w3f/arkworks-algebra that referenced this pull request Jan 12, 2024

Make elligator curve map a static object following revert of revert

4b770e1

of arkworks-rs#679

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pseduo-remove MapToCurve::new, renamed to check_parameters #679

Pseduo-remove MapToCurve::new, renamed to check_parameters #679

burdges commented Sep 8, 2023 •

edited

Loading

mmagician left a comment

mmagician Sep 11, 2023

burdges Sep 11, 2023

mmagician Sep 11, 2023

burdges Sep 11, 2023

mmagician Sep 12, 2023

Pseduo-remove MapToCurve::new, renamed to check_parameters #679

Pseduo-remove MapToCurve::new, renamed to check_parameters #679

Conversation

burdges commented Sep 8, 2023 • edited Loading

Description

mmagician left a comment

Choose a reason for hiding this comment

mmagician Sep 11, 2023

Choose a reason for hiding this comment

burdges Sep 11, 2023

Choose a reason for hiding this comment

mmagician Sep 11, 2023

Choose a reason for hiding this comment

burdges Sep 11, 2023

Choose a reason for hiding this comment

mmagician Sep 12, 2023

Choose a reason for hiding this comment

burdges commented Sep 8, 2023 •

edited

Loading