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

Hash2Curve uniform interface #188

Merged
merged 22 commits into from
May 25, 2022
Merged

Hash2Curve uniform interface #188

merged 22 commits into from
May 25, 2022

Conversation

Tabaie
Copy link
Contributor

@Tabaie Tabaie commented May 16, 2022

No description provided.

@Tabaie Tabaie marked this pull request as ready for review May 16, 2022 16:57
@Tabaie Tabaie changed the title refactor!: four functions for bls12-377. mapToCurve has subgroup error Hash2Curve uniform interface May 17, 2022
@Tabaie
Copy link
Contributor Author

Tabaie commented May 17, 2022

@yelhousni There are errors with bw6-633 G1 and bw6-761 G2. The combination of the SSWU and the isogeny is not on the curve. It could either be that the SSWU parameters are incorrect, or the isogeny. Would you take a look at the isogenies? In the meantime I'll develop a test for the map itself to see if the output is actually on E'.

@Tabaie Tabaie requested a review from yelhousni May 17, 2022 22:39
@Tabaie
Copy link
Contributor Author

Tabaie commented May 18, 2022

It seems that all the SSWU are "correct" (in the sense that the output is always on E')

@yelhousni
Copy link
Collaborator

@Tabaie So there were some errors in ordering the isogeny denominator coefficients for bw6-761 G2 and a duplicate coefficient in bw6-633 G1. Now everything is corrected and tests pass.

@Tabaie Tabaie requested a review from gbotrel May 18, 2022 20:53
Copy link
Collaborator

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

LGTM!
It would be nice though to have the hash_to_curve.go file for Svdw map separated into two files hash_to_g1.go and hash_to_g2.go as it is for SSWU and with corresponding test files (ideally generified but this isn't a priority now).

@gbotrel
Copy link
Collaborator

gbotrel commented May 19, 2022

@Tabaie so API wise, we have

  • HashToG1
  • EncodeToG1
  • MapToG1

In the signatures, (msg, dst []byte) (G1Affine, error) What is dst for ? It is a bit confusing for an API to have different outputs both as params and returns, at least deserve a bit more doc 👍

@yelhousni
Copy link
Collaborator

In the signatures, (msg, dst []byte) (G1Affine, error) What is dst for ? It is a bit confusing for an API to have different outputs both as params and returns, at least deserve a bit more doc 👍

dst is a domain separation tag to guarantee independent outputs in case of multiple hashes/encodings (see discussion here).

@gbotrel
Copy link
Collaborator

gbotrel commented May 21, 2022

I see I thought dst == destination

}

// returns false if u>-u when seen as a bigInt
func sign0(u fp.Element) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Tabaie can we use fp.Element.LexicographicallyLargest here?

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 think so.

Copy link
Contributor Author

@Tabaie Tabaie May 24, 2022

Choose a reason for hiding this comment

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

@gbotrel The result is the same but the change seems to have caused multiple race conditions, no idea why.


EDIT: The result is the opposite actually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you investigate a bit? Are you sure it's coming from that specific commit? (I also don't obviously see why it happened, maybe it uncovered a bug somewhere we need to be aware of 👍 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Tabaie so when I set this e3 to false or true, the tests always pass (bn254):

e3 := false // sign0(u) && sign0(y)
	if !e3 {
		y.Neg(&y)
	}

}

// EncodeToG2 maps an fp.Element to a point on the curve using the Shallue and van de Woestijne map
// https://tools.ietf.org/html/draft-irtf-cfrg-hash-to-curve-06#section-2.2.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Tabaie can we update the doc of the public functions (EncodeXXX, MapXX, ...) to explain the parameters?
For example, DST is not clear if you don't jump on the specs. Some function says "map an fp.Element" but takes as param a []byte.

@Tabaie
Copy link
Contributor Author

Tabaie commented May 23, 2022

@gbotrel dst is domain separation tag actually.

@gbotrel gbotrel merged commit 7196550 into develop May 25, 2022
@gbotrel gbotrel deleted the refac/h2c-cleanup branch May 25, 2022 17:54
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.

3 participants