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

Encapsulate isogeny mapping in a new struct #517

Merged
merged 9 commits into from
Nov 29, 2022
Merged

Conversation

drskalman
Copy link
Contributor

@drskalman drskalman commented Nov 16, 2022

Description

This PR moves the definition of the coefficients of the isogeny map of WB hashing to a separate struct rather than defining it in the WBParams itself. This is done to declutter the definition of the curve and make it explicit that the four polynomials are related and defining a single map according to these suggestions from @mmagician
and @Pratyush


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

@drskalman drskalman requested review from a team as code owners November 16, 2022 10:41
@drskalman drskalman requested review from Pratyush, mmagician and weikengchen and removed request for a team November 16, 2022 10:41
@drskalman
Copy link
Contributor Author

drskalman commented Nov 16, 2022

@mmagician @Pratyush I moved the definition of the coefficient to g1_swu_iso.rs instead of making a new file. I thought is less visited compared to g1.rs so it is not so critical if it is filled with the coefficients of the isogeny map (perhaps we can rename it as g1_iso.rs as it now that it contains both the definitions of the isogenous curve and of the isogeny map). If you still think it should be rather a separate file containing only the const struct instantiation, I could do so as well..

Copy link
Member

@Pratyush Pratyush left a comment

Choose a reason for hiding this comment

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

This looks great to me, modulo some small changes.

CHANGELOG.md Outdated Show resolved Hide resolved
ec/src/hashing/curve_maps/wb/mod.rs Outdated Show resolved Hide resolved
ec/src/hashing/curve_maps/wb/mod.rs Outdated Show resolved Hide resolved
drskalman and others added 3 commits November 18, 2022 03:37
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
- Move `isogeny_map` function from `WBParams` to `IsogenyMap`
- Update BLS12_381 test-curve accordingly.
@drskalman drskalman requested review from Pratyush and removed request for mmagician and weikengchen November 21, 2022 16:15
Comment on lines 26 to 38
pub struct IsogenyMap<
'a,
DOMAINE: SWCurveConfig,
CODOMAINE: SWCurveConfig<BaseField = BaseField<DOMAINE>>,
> {
pub x_map_numerator: &'a [BaseField<CODOMAINE>],
pub x_map_denominator: &'a [BaseField<CODOMAINE>],

pub y_map_numerator: &'a [BaseField<CODOMAINE>],
pub y_map_denominator: &'a [BaseField<CODOMAINE>],

pub _phantom_domain: PhantomData<DOMAINE>,
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub struct IsogenyMap<
'a,
DOMAINE: SWCurveConfig,
CODOMAINE: SWCurveConfig<BaseField = BaseField<DOMAINE>>,
> {
pub x_map_numerator: &'a [BaseField<CODOMAINE>],
pub x_map_denominator: &'a [BaseField<CODOMAINE>],
pub y_map_numerator: &'a [BaseField<CODOMAINE>],
pub y_map_denominator: &'a [BaseField<CODOMAINE>],
pub _phantom_domain: PhantomData<DOMAINE>,
}
pub struct IsogenyMap<
'a,
DOMAIN: SWCurveConfig,
CODOMAIN: SWCurveConfig<BaseField = BaseField<DOMAIN>>,
> {
pub x_map_numerator: &'a [BaseField<CODOMAIN>],
pub x_map_denominator: &'a [BaseField<CODOMAIN>],
pub y_map_numerator: &'a [BaseField<CODOMAIN>],
pub y_map_denominator: &'a [BaseField<CODOMAIN>],
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pratyush: As you see, it says:

error[E0392]: parameter `DOMAIN` is never used                                                                                                                                                                                                                                            
  --> ec/src/hashing/curve_maps/wb/mod.rs:28:5                                                                                                                                                                                                                                            
   |                                                                                                                                                                                                                                                                                      
28 |     DOMAIN: SWCurveConfig,                                                                                                                                                                                                                                                           
   |     ^^^^^^ unused parameter                                                                                                                                                                                                                                                          
   |                                                                                                                                                                                                                                                                                      
   = help: consider removing `DOMAIN`, referring to it in a field, or using a marker such as `PhantomData`                                                                                                                                                                                

What should we do beside using PhantomData?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, didn't realize that we we weren't using DOMAIN. There's two options:

  • Reinstate the PhantomData<CODOMAIN> field, but make it private. To enable constructing an IsogenyMap, let's add a new method which does the construction.
  • Change the *_map_denominator type signatures to be &'a [BaseField<DOMAIN>]. Admittedly this is a bit of a hack, but this avoids adding back the field.

Copy link
Contributor Author

@drskalman drskalman Nov 21, 2022

Choose a reason for hiding this comment

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

  • To enable constructing an IsogenyMap, let's add a new method which does the construction.

Hmm, then new need to be a const function which means the arguments to new should be const. so I have to define constants PHI_X_NOM, PHI_X_DEN,..., before calling new. Then we should put them in a struct so we know they are related perhaps. Then the IsogenyMap struct will have some existential crisis. (because it is just there now to hold the apply function).

Copy link
Member

Choose a reason for hiding this comment

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

That should be possible, since it's a simple method, no?

Copy link
Contributor Author

@drskalman drskalman Nov 21, 2022

Choose a reason for hiding this comment

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

hmm but they need to have static lifetime no? perhaps I don't know the syntax of sending an argument on the fly witth static lifetime. IsogenyMap::new(&[1,23]) complains about lifetime and it doesn't let me do IsogenyMap::new(&'static[1,23]) where my creativing reachs its limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I try sending them as array instead of refrences...

Copy link
Contributor Author

@drskalman drskalman Nov 21, 2022

Choose a reason for hiding this comment

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

I can't because they don't have the same size across the curves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pratyush here is the code with new constructor. If you tell me how to send the slices with static lifetime to new function so it compiles, then I'll update this PR accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

I think the following should work:

impl</* bounds */> IsogenyMap<'a, DOMAIN, CODOMAIN> {
	const fn new(
	    x_numerator: &'a [BaseField<CODOMAIN>],
	    x_denominator: &'a [BaseField<CODOMAIN>],
	    ...
	)Self {
	    Self {
	        x_numerator,
	        x_denominator,
	        ...
	    }
	}
}

@Pratyush Pratyush changed the title Move the definition of the isogeny map of WB hashing to a separate struct Encapsulate isogeny mapping in a new struct Nov 29, 2022
@Pratyush Pratyush merged commit 596a7af into arkworks-rs:master Nov 29, 2022
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.

None yet

2 participants