Construct isogeny map using new function #529

drskalman · 2022-11-30T17:52:47Z

Description

This PR is trying to open discussion around issue #525 and hopefully closes it.

closes: #525

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.

[x ] 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

Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>

- Move `isogeny_map` function from `WBParams` to `IsogenyMap` - Update BLS12_381 test-curve accordingly.

…G.md` Cargo fmt on test-curves

of domain field to avoid phantomData.

…TWBF127_NEW`

error[E0716]: temporary value dropped while borrowed

…eny-map-with-new

drskalman · 2022-11-30T17:57:03Z

@lamafab has done some tests and thinks this might be a rust bug. Quoting them here:

this works

trait Trait {
    type AssocType;
}

struct Test<
    A: Trait,
    B: Trait<AssocType = A>,
> {
    some: B,
}

this does NOT:

trait Trait {
    type AssocType;
}

struct Test<
    A: Trait,
    // HERE:
    B: Trait<AssocType = A::AssocType>,
    // Or:
    // B: Trait<AssocType = <A as Trait>::AssocType>,
> {
    some: B,
}

I'm really not sure if this is expected behavior or a bug? 🤔

Pratyush · 2022-11-30T19:17:45Z

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

+    pub const fn new(
+        x_map_numerator: &'a [BaseField<CODOMAIN>],
+        x_map_denominator: &'a [BaseField<CODOMAIN>],
+        y_map_numerator: &'a [BaseField<CODOMAIN>],
+        y_map_denominator: &'a [BaseField<CODOMAIN>],
+    ) -> Self {
+        Self {
+            x_map_numerator,
+            x_map_denominator,
+            y_map_numerator,
+            y_map_denominator,
+            _phantom_domain: PhantomData::<DOMAIN>,
+        }
+    }


We can also limit this to 'static lifetimes. That might help with the "temporary dropped while borrowed" issue.

do you mean to hard code static life time? I think I did that and the error still persists.

I tried it too, couldn't find a way around it...

Pratyush · 2022-12-09T02:11:49Z

I think this is a Rust bug. What seems to be the equivalent code does compile: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=51633acd5ecaf330b62a44216c1b7e41

@drskalman do you want to file a bug against rust-lang/rust?

drskalman · 2022-12-12T11:41:29Z

@drskalman do you want to file a bug against rust-lang/rust?

Sure, I'll do it but I need find a minimal example that it doesn't work beside arkworks :-) I'll try.

Pratyush · 2022-12-12T19:57:07Z

I think you can just make an issue linking the playground and your branch. Re: minimization tips, the main differences that come to mind are

we're using macros in the constant
we're doing some relatively heavy computation at compile time, which might be exhausting the const execution time limit.

mmagician · 2023-09-18T10:51:09Z

@drskalman Have you filed a bug report in the end?

drskalman and others added 12 commits November 16, 2022 05:13

define the isogeny map of WB hashing in a separate struct.

4bb133c

Merge branch 'arkworks-rs:master' into master

351ec56

Add the description of the PR to the CHANGELOG.md

5b67949

moving the PR to from Breaking to Pending section of the CHANGELOG.md

f2ec1d8

Improve the documentation for Isogeny Rational Map

41c02d9

Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>

- Rename RationalMap struct to IsogenyMap

796e8fa

- Move `isogeny_map` function from `WBParams` to `IsogenyMap` - Update BLS12_381 test-curve accordingly.

Move PR-arkworks-rs#517 from Pending to Breaking section of 'CHANGELO…

6744179

…G.md` Cargo fmt on test-curves

Small cleanups from code review

18c9bbc

make the denominators of the isogeny map to be defined as base field

b3ee27b

of domain field to avoid phantomData.

Add a new map to IsogenyMap and use it to instatiate `ISOGENY_MAP_TES…

bc8e962

…TWBF127_NEW`

implement IsogenyMap::new as suggested by @Pratyush to reproduce

e2479fe

error[E0716]: temporary value dropped while borrowed

Merge remote-tracking branch 'upstream/master' into skalman-make-isog…

f819ee1

…eny-map-with-new

drskalman requested review from a team as code owners November 30, 2022 17:52

drskalman requested review from Pratyush, mmagician and weikengchen and removed request for a team November 30, 2022 17:52

drskalman changed the title ~~Skalman make isogeny map with new~~ Construct isogeny map using new function Nov 30, 2022

drskalman marked this pull request as draft November 30, 2022 18:04

Pratyush reviewed Nov 30, 2022

View reviewed changes

hard code static lifetime for isogeny map coeffcient slices

e00b7be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Construct isogeny map using new function #529

Construct isogeny map using new function #529

drskalman commented Nov 30, 2022

drskalman commented Nov 30, 2022

Pratyush Nov 30, 2022

drskalman Dec 1, 2022 •

edited

mmagician Dec 1, 2022

Pratyush commented Dec 9, 2022

drskalman commented Dec 12, 2022

Pratyush commented Dec 12, 2022

mmagician commented Sep 18, 2023

Construct isogeny map using new function #529

Are you sure you want to change the base?

Construct isogeny map using new function #529

Conversation

drskalman commented Nov 30, 2022

Description

closes: #525

drskalman commented Nov 30, 2022

Pratyush Nov 30, 2022

Choose a reason for hiding this comment

drskalman Dec 1, 2022 • edited

Choose a reason for hiding this comment

mmagician Dec 1, 2022

Choose a reason for hiding this comment

Pratyush commented Dec 9, 2022

drskalman commented Dec 12, 2022

Pratyush commented Dec 12, 2022

mmagician commented Sep 18, 2023

drskalman Dec 1, 2022 •

edited