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

Add hashing into the curve. #30

Closed
wants to merge 16 commits into from
Closed

Add hashing into the curve. #30

wants to merge 16 commits into from

Conversation

mmaker
Copy link
Contributor

@mmaker mmaker commented Aug 10, 2017

Hello.
A few days ago @zmanian said it'd be nice if somebody added hashing to the curve to pairing, and I found myself with a few spare days in bed, soo… I'd like to propose the folllowing changes:

  • Add Legendre symbol for Fq and Fq2, with test cases
  • Implement Foque-Ticouchi's hashing to the curve algorithm

The algorithm is pretty much along the lines of https://github.com/herumi/mcl/blob/9fbc54305d01b984e39d83e96bfa94bb17648a86/include/mcl/bn.hpp#L65-L127 , as suggested by @zmanian.

@ebfull
Copy link
Collaborator

ebfull commented Aug 10, 2017

(Awesome work!)

I don't have time to review this today, but one comment: why can't SqrtField have the Legendre symbol defined over it instead of making a new trait?

@ebfull
Copy link
Collaborator

ebfull commented Aug 10, 2017

Two additional comments:

  1. The implementations of SqrtField internally perform residue tests that are perhaps redundant with legendre().
  2. I think the implementation of rand in G1/G2 should probably just use this hashing, if it works.

@ebfull
Copy link
Collaborator

ebfull commented Aug 10, 2017

As far as I can tell this implementation does not currently guarantee hashing into the correct subgroup. BLS12-381 has nontrivial cofactors for G1/G2, whereas BN curves have h=1 for G1.

@daira
Copy link
Collaborator

daira commented Aug 10, 2017

If we add this then I'd like to make sure it is consistent with the hashing we specify for Sapling (if we decide to use diversified addresses and/or the generalized-Pedersen approach to UITs), so bear with me until I have time to review it and the Fouque–Joux–Tibouchi paper properly. Yes we'll need to hash into the subgroup.

[Edit: I had the wrong paper here.]

At first sight, +1 on moving legendre() into SqrtField and factoring out any redundancy there.

@@ -561,8 +562,7 @@ impl SqrtField for Fr {
return Some(*self);
}

// if self^((r - 1) // 2) != 1
if self.pow([0x7fffffff80000000, 0xa9ded2017fff2dff, 0x199cec0404d0ec02, 0x39f6d3a994cebea4]) != Self::one() {
if let QNonResidue = self.legendre() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this if let QNonResidue =? (Maybe I'm just missing something about Rust pattern matching, but I would have expected if self.legendre() != QResidue for similarity to the existing code. I know the zero case is handled above.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is idomatic Rust. In order to do if self.legendre() != QResidue you need to implement PartialEq (equality) for LegendreSymbol, but absent a good reason to do this, it's preferable to use if let.

So, speaking of which, this code should use a match instead of if let and handle the Zero case of LegendreSymbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I'm a n00b.
I've polished that function a bit here mmaker@e27801d and it looks a bit better.

}

}
Self {x: Self::Base::zero(), y: Self::Base::zero(), infinity: true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the hash be defined never to output the point at infinity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is just Rust not understanding that the function it will return before for sure. That line is never executed.

I wasn't really sure what would have been conventional to do there, whether panic! or return a silly point like infinity - bn.h returns an ERR_POINT as far as I remember.
Plus, from ocaml I had this feeling that exceptions are bad and that I should keep my functions pure, so …

In any case, I'm happy to change it if needed!
As a side note, making it constant-time will probably get rid of that for loop and the silly return
(as described on the paper). For now I am aiming at a state that allows me to generate test-vectors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's true that the algorithm never can produce the point at infinity, then this should return unreachable!().

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ebfull, what's the policy for "cannot happen" cases? (Without having thought about it very hard, a panic! seems fine to me.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

unreachable!() is idiomatic rust. It just panics with "unreachable code".

@mmaker
Copy link
Contributor Author

mmaker commented Aug 10, 2017

The implementations of SqrtField internally perform residue tests that are perhaps redundant with legendre().

At first sight, +1 on moving legendre() into SqrtField and factoring out any redundancy there.

Initially I created a separate trait because I wasn't sure I really needed to implement the legendre symbol for all fields implementing sqrt. Turns out this is the case.

I merged SqrtField and LegendreField at mmaker@e6014d1 as a separate commit. (can squash if needed!)

@mmaker
Copy link
Contributor Author

mmaker commented Aug 10, 2017

The implementations of SqrtField internally perform residue tests that are perhaps redundant with legendre().

Right. So, fr.rsuses it; fq.rs and fq2.rs do not, and I don't really see how they would use it - the algorithms for sqrt mod 4 already understand if it is a quadratic residue or not.

@mmaker
Copy link
Contributor Author

mmaker commented Aug 10, 2017

As far as I can tell this implementation does not currently guarantee hashing into the correct subgroup. BLS12-381 has nontrivial cofactors for G1/G2, whereas BN curves have h=1 for G1.

If we add this then I'd like to make sure it is consistent with the hashing we specify for Sapling (if we decide to use diversified addreses and/or the generalized-Pedersen approach to UITs), so bear with me until I have time to review it and the Foque-Ticouchi paper properly. Yes we'll need to hash into the subgroup.

oh silly silly me I didn't check at all this. Okay mm so in the meantime I'll dive into that loooong issue of yours

Thanks for poining this out!

@zmanian
Copy link

zmanian commented Aug 10, 2017

I'm unsure if Fouques and Tabouchi works with cofactor != 1.

This implies that we should potentially use some variant of Eligator -Squared (https://eprint.iacr.org/2014/043.pdf & https://hal.inria.fr/hal-01275711/document)

@hdevalence has argued that providing a mapping to curve point should be considered an essential part any curve library.

@mmaker
Copy link
Contributor Author

mmaker commented Aug 10, 2017

what happens if I multiply by the cofactor at the end of the function?
The point is going to be in the subgroup no?

Edit: indeed the paper mentions BLS curves and suggests to multiply by the cofactor:

Our results apply almost without change to any elliptic curve of the form […]: this includes in particular the curves constructed by Barreto, Lynn and Scott in [2, §3.1] and […]
The elliptic curve group in those cases is not usually of prime order, […], so hashing to the prime order subgroup requires multiplying the point obtained with the technique described herein by the cofactor. This does not affect indifferentiability, as was shown in [12, §6.1].

In an attempt to see this with my own eyes I tried to add a patch that multiplies by the cofactor at the end, together with some test cases that verify "some images" are in the desidered subgroup.
Tests suceeds for G1, still failing for G2. The reason they fail for G2 (I think) is that the cofactor is "too big" for our type:

sage: cofactor_g2 = Integer(x**8 - 4*x**7 + 5*x**6 - 4*x**4 + 6*x**3 - 4*x**2 - 4*x + 13) / 9                   
sage: cofactor_g2           
305502333931268344200999753193121504214466019254188142667664032982267604182971884026507427359259977847832272839041616661285803823378372096355777062779109                                                     
sage: Fr(cofactor_g2)             
52435875175126190477137643957470197624944050669916493059025303477134612409005     

and I need to do arithmetic outside Fr to do so. How can I do it?
Note: for G1 tests also properly fail if I comment out multiplication by the cofactor.

@ebfull
Copy link
Collaborator

ebfull commented Aug 11, 2017

Yes, the G2 cofactor is very large.

I'll have to think about a clean way to allow us to perform scalar multiplications of such magnitude. I don't want that capability to be generally available to downstream users because I never want to encourage them to multiply by non-Fr scalars.

In some of my tests I just manually perform double-and-add, like this:

// Cofactor of G2 is 305502333931268344200999753193121504214466019254188142667664032982267604182971884026507427359259977847832272839041616661285803823378372096355777062779109.
// Calculated by: ((x**8) - (4 * (x**7)) + (5 * (x**6)) - (4 * (x**4)) + (6 * (x**3)) - (4 * (x**2)) - (4*x) + 13) // 9
// where x is the BLS parameter.
for b in "101110101010100001110101001010101000001010011100111111100010000100100011101010100000111100100101000011101101010001000000010110011011001000111011110010001010100011100001000010110101011101010100110100010100010000001011011001011100101101001111101110111111010011000101000111100011100101101001101100111101000001011101111001000010101001101111110001010010011101001100110100100011010111000010110000101101110110001101110011110000110111100001100011100001100111100011100001110001110001100011100011100100011100011100101"
         .chars()
         .map(|c| c == '1')
{
    g2.double();

    if b {
        g2.add_assign_mixed(&p);
    }
}

You can just use this until we figure it out.

Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

Lots of comments.


while t != Self::one() {
match self.legendre() {
Zero => Some(*self),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this return an object that stays zero if self is mutated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it dereferences self which copies it.

@@ -557,45 +557,42 @@ impl SqrtField for Fr {
fn sqrt(&self) -> Option<Self> {
// Tonelli-Shank's algorithm for q mod 16 = 1
// https://eprint.iacr.org/2012/685.pdf (page 12, algorithm 5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe assert somewhere that q mod 16 = 1.

(Nonblocking; I know this hasn't changed in this PR.)

break;
}
{
let mut t2i = t;
t2i.square();
Copy link
Collaborator

Choose a reason for hiding this comment

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

wishes that diff would not try so hard to find accidentally matching lines within modified blocks

else if s == Fq::one() { QResidue }
else { QNonResidue }
}

fn sqrt(&self) -> Option<Self> {
// Shank's algorithm for q mod 4 = 3
// https://eprint.iacr.org/2012/685.pdf (page 9, algorithm 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe assert somewhere that q mod 4 = 3. (Nonblocking.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, add constants for (q - 3)/2 and -1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be left to another PR btw.


// C2 = (C1 - 1) / 2 mod q = 793479390729215512621379701633421447060886740281060493010456487427281649075476305620758731620350
pub const C2: Fq = Fq(FqRepr([0x30f1361b798a64e8, 0xf3b8ddab7ece5a2a, 0x16a8ca3ac61577f7, 0xc26a2ff874fd029b, 0x3636b76660701c6e, 0x51ba4ab241b6160]));

// GENERATOR = 2 (multiplicative generator of q-1 order, that is also quadratic nonresidue)
const GENERATOR: FqRepr = FqRepr([0x321300000006554f, 0xb93c0018d6c40005, 0x57605e0db0ddbb51, 0x8b256521ed1f9bcb, 0x6cf28d7901622c03, 0x11ebab9dbb81e28c]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe assert that it is a quadratic nonresidue, now that we have legendre(). (Nonblocking.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does it say "= 2", btw, @ebfull? (Nonblocking.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's 2 in the field. (Represented in Montgomery form, which is why it looks random.)

pub const G1_COFACTOR: Fr = Fr(FrRepr([0x31207728cdb90779, 0xffea90400320679a, 0xa479130bcc248091, 0x6d41ecdf98f9d8cc]));

// G2 has cofactor (x^8 - 4x^7 + 5x^6 - 4x^4 + 6x^3 - 4x^2 - 4x + 13) / 9.
pub const G2_COFACTOR: Fr = Fr(FrRepr([0x755c12a6851eb90e, 0xe117af10228fcb8c, 0xd67937ea756c9ced, 0x6dabbfa8c0fd834b]));
Copy link
Collaborator

Choose a reason for hiding this comment

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

For each group, assert that the cofactor times the group order is the curve order.

let mut c = Fr(ROOT_OF_UNITY);
// r = self^((t + 1) // 2)
let mut r = self.pow([0x7fff2dff80000000, 0x4d0ec02a9ded201, 0x94cebea4199cec04, 0x39f6d3a9]);
// t = self^t
Copy link
Collaborator

Choose a reason for hiding this comment

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

These comments don't make sense to me; what is t initially? Is this a naming clash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// r = self^((t + 1) // 2)
let mut r = self.pow([0x7fff2dff80000000, 0x4d0ec02a9ded201, 0x94cebea4199cec04, 0x39f6d3a9]);
// t = self^t
let mut t = self.pow([0xfffe5bfeffffffff, 0x9a1d80553bda402, 0x299d7d483339d808, 0x73eda753]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's explained in the algorithm I think.

src/lib.rs Outdated
@@ -214,6 +214,9 @@ pub trait CurveAffine: Copy +
fn into_uncompressed(&self) -> Self::Uncompressed {
<Self::Uncompressed as EncodedPoint>::from_affine(*self)
}

/// Hash into this curve
Copy link
Collaborator

Choose a reason for hiding this comment

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

Be clearer about the properties of the mapping.

Copy link
Contributor Author

@mmaker mmaker Aug 11, 2017

Choose a reason for hiding this comment

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

done, I tried to elaborate a bit more.

src/lib.rs Outdated
@@ -325,11 +328,15 @@ pub trait Field: Sized +
/// This trait represents an element of a field that has a square root operation described for it.
pub trait SqrtField: Field
{
/// Returns the legendre symbol of the field element.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Legendre". (The method name is fine as lowercase.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@mmaker mmaker force-pushed the master branch 3 times, most recently from 2b1e33e to 84c9a83 Compare August 11, 2017 14:58
@ebfull
Copy link
Collaborator

ebfull commented Aug 11, 2017

Our review is going to be really exhaustive, I hope you can bear with us! 😅

@bmerge try

@bmerge
Copy link
Collaborator

bmerge commented Aug 11, 2017

⌛ Trying commit 84c9a83 with merge 80a963e...

bmerge added a commit that referenced this pull request Aug 11, 2017
Add hashing into the curve.

Hello.
A few days ago @zmanian said it'd be nice if somebody added hashing to the curve to `pairing`, and I found myself with a few spare days in bed, soo… I'd like to propose the folllowing changes:

- Add Legendre symbol for Fq and Fq2, with test cases
- Implement Foque-Ticouchi's hashing to the curve algorithm

The algorithm is pretty much along the lines of https://github.com/herumi/mcl/blob/9fbc54305d01b984e39d83e96bfa94bb17648a86/include/mcl/bn.hpp#L65-L127 , as suggested by @zmanian.
0 => { x.mul_assign(&t); x.negate(); x.add_assign(&Self::get_c2()) },
1 => { x.negate(); x.sub_assign(&Self::Base::one()) },
2 => { x=w; x.square(); x = x.inverse().unwrap(); x.add_assign(&Self::Base::one()) },
_ => {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

unreachable!()

let mut x = w;
for i in 0..3 {
match i {
0 => { x.mul_assign(&t); x.negate(); x.add_assign(&Self::get_c2()) },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Separate these statements into individual lines.

src/lib.rs Outdated
#[derive(Debug, PartialEq)]
pub enum LegendreSymbol {
Zero = 0,
QResidue = 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

QuadraticResidue and QuadraticNonResidue

Copy link
Collaborator

Choose a reason for hiding this comment

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

This blocks for me, I prefer explicit names like this. 😸

src/lib.rs Outdated
pub trait LegendreField: Field
{
/// Returns the legendre symbol of the field element.
fn legendre(&self) -> LegendreSymbol;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of making a trait let's add this to SqrtField (which will require us to additionally implement it for Fr.)

@@ -561,8 +562,7 @@ impl SqrtField for Fr {
return Some(*self);
}

// if self^((r - 1) // 2) != 1
if self.pow([0x7fffffff80000000, 0xa9ded2017fff2dff, 0x199cec0404d0ec02, 0x39f6d3a994cebea4]) != Self::one() {
if let QNonResidue = self.legendre() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is idomatic Rust. In order to do if self.legendre() != QResidue you need to implement PartialEq (equality) for LegendreSymbol, but absent a good reason to do this, it's preferable to use if let.

So, speaking of which, this code should use a match instead of if let and handle the Zero case of LegendreSymbol.


while t != Self::one() {
match self.legendre() {
Zero => Some(*self),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it dereferences self which copies it.

// r = self^((t + 1) // 2)
let mut r = self.pow([0x7fff2dff80000000, 0x4d0ec02a9ded201, 0x94cebea4199cec04, 0x39f6d3a9]);
// t = self^t
let mut t = self.pow([0xfffe5bfeffffffff, 0x9a1d80553bda402, 0x299d7d483339d808, 0x73eda753]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's explained in the algorithm I think.

for _ in 0 .. 20 {
let t = Fq::rand(&mut rng);
let p = G1Affine::hash(t);
assert!(G1Affine::is_on_curve(&p));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be just p.is_on_curve().

}
}

/// Maps any elliptic curve point to the subgroup G2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, it cannot map points from small subgroups into the G2 subgroup. I would rather call this scale_by_cofactor.

else if s == Fq::one() { QResidue }
else { QNonResidue }
}

fn sqrt(&self) -> Option<Self> {
// Shank's algorithm for q mod 4 = 3
// https://eprint.iacr.org/2012/685.pdf (page 9, algorithm 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be left to another PR btw.

@bmerge
Copy link
Collaborator

bmerge commented Aug 11, 2017

💔 Test failed - pairing-linux64-try

@ebfull
Copy link
Collaborator

ebfull commented Aug 11, 2017

Don't worry; that test failure has nothing to do with you. It's our clippy lints failing to compile on the latest nightly Rust. If you want, change Cargo.toml to use clippy version 0.0.150 and I can try again.

edit: actually, this would be a rabbit hole. Don't do it yet.

@ebfull
Copy link
Collaborator

ebfull commented Aug 11, 2017

(By the way, all of the other tests passed just fine.)

@ebfull
Copy link
Collaborator

ebfull commented Aug 11, 2017

There has to be something in the literature (like this paper) which we can apply here to avoid the huge G2 cofactor scaling.

@ebfull
Copy link
Collaborator

ebfull commented Aug 12, 2017

As @mmaker mentioned, the paper says:

We also assume that q ≡ 7 (mod 12) and that g(1) = 1 + b is a nonzero square in Fq.

I cannot tell if these assumptions are important or relied on strongly by the paper, but they aren't satisfied by BLS12-381. Fq(5) is nonsquare in Fq, and for G2, q^2 is 1 mod 12. (Perhaps the paper made a typo and meant for p to be 7 mod 12?)

One of the advantages of BLS12-381 is that all of the curve constants can be determined uniquely by a single parameter, which happens also to be part of a subfamily of parameters that are good candidates for standardization.

However, if we're willing to forgo this, we could get this hashing to work for G1 by using a different isomorphism. Barreto pointed out to me many months ago, with regards to BLS12-381:

there's a nicer isomorphic curve, E(F_p): y^2 = x^3 + 24, that admits G = h*(1, 5) as the generator for group G_1.

And of course, with Fq(25) being a nonzero square, it satisfies the assumptions of this paper. But since you still don't get G2 hashing with this... it's probably not worth it.

I would much rather implement hashing via a "try-and-increment" algorithm as suggested in the BLS signatures paper. This also avoids problems with points of small order.

@mmaker
Copy link
Contributor Author

mmaker commented Aug 13, 2017

I cannot tell if these assumptions are important or relied on strongly by the paper, but they aren't satisfied by BLS12-381.

Going after the introduction to section 2:

Later works such as [31] use a different point as the generator, and the corresponding
construction does no longer ensure that 1 + b is a square. This only causes a minor inconvenience for our purposes, namely two extra elements of Fq that have to be treated separately in the encoding given in Section 3.

What happens in section 3 is that we have to handle the cases t = 0 and t^2+b+1 = 0 separately (see page 8, when giving a rational parametrization of the conic), mapping them to arbitrary points.

In fact, they already do this in Definition 2 for t = 0: they construct a nice map from Fq* to the curve, and they "program" the encoding for the case t = 0 reducing it to one of the cases already handled:

The encoding can be extended to all of Fq by sending 0 to some arbitrary point in E(Fq). Since
x1 is well-defined […]

I added a commit that does exately that (mapping to arbitrary points), in case you were interested in checking it out anyway.

I would much rather implement hashing via a "try-and-increment" algorithm as suggested in the BLS signatures paper. This also avoids problems with points of small order.

Alright, I can give it a shot and benchmark the two. Could you please pretty please bear with me and just take a look at both? This way we have some comparative terms.

@ebfull
Copy link
Collaborator

ebfull commented Aug 13, 2017

Hm, the way we would implement "try-and-increment" could vary. @daira might have some good ideas.

We'd like to be able to "hash to" Fr and Fq elements for the purpose of both hashing to BLS12-381 and hashing to constants/embedded curves for zk-SNARKs. I suppose we can build "hashing to" Fq2 and the curve on top of this.

I'm prepared to bite the bullet and let BLAKE2b be a dependency of pairing. :)


// w = (t^2 + b + 1)^(-1) * sqrt(-3) * t
let mut w = t;
w.square();
w.add_assign(&Self::get_coeff_b());
w.add_assign(&Self::Base::one());
// handle the case t^2 + b + 1 == 0
if w.is_zero() { return infty };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just return Self::zero()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@ebfull
Copy link
Collaborator

ebfull commented Mar 31, 2018

@bmerge try

@bmerge
Copy link
Collaborator

bmerge commented Mar 31, 2018

⌛ Trying commit 3ce634a with merge 54b48e1...

bmerge added a commit that referenced this pull request Mar 31, 2018
Add hashing into the curve.

Hello.
A few days ago @zmanian said it'd be nice if somebody added hashing to the curve to `pairing`, and I found myself with a few spare days in bed, soo… I'd like to propose the folllowing changes:

- Add Legendre symbol for Fq and Fq2, with test cases
- Implement Foque-Ticouchi's hashing to the curve algorithm

The algorithm is pretty much along the lines of https://github.com/herumi/mcl/blob/9fbc54305d01b984e39d83e96bfa94bb17648a86/include/mcl/bn.hpp#L65-L127 , as suggested by @zmanian.
@bmerge
Copy link
Collaborator

bmerge commented Mar 31, 2018

☀️ Test successful - pairing-linux32-try, pairing-linux64-try, pairing-windows32msvc-try, pairing-windows64msvc-try
State: approved= try=True

@ebfull ebfull requested review from daira and ebfull March 31, 2018 06:25
@@ -10,6 +10,15 @@ macro_rules! curve_impl {
$compressed:ident,
$pairing:ident
) => {

fn y2_from_x(x: $basefield) -> $basefield {
Copy link
Collaborator

@ebfull ebfull Mar 31, 2018

Choose a reason for hiding this comment

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

I don't know if this change still needs to be in this PR anymore, it could be separate. I don't think y2_from_x is used directly in sw_encode etc. anymore. But, it's not a bad refactoring.

(Does not block this PR.)

// x3 = 1/w^2 + 1
let mut x3 = w;
x3.square();
x3 = x3.inverse().unwrap();
Copy link
Collaborator

@ebfull ebfull Mar 31, 2018

Choose a reason for hiding this comment

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

I suspect this inverse could be optimized away. (Does not block this PR.)

Copy link

Choose a reason for hiding this comment

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

1/w^2 is w^(q-4). Trouble is, this is much slower than egcd inverse.

Unless you have completely different formula for x3 in mind.

Copy link
Collaborator

@ebfull ebfull Apr 2, 2018

Choose a reason for hiding this comment

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

  • A = t^2
  • B = A + b + 1
  • C = 3 * A
  • D = B * C
  • E = D^(-1)
  • F = E * B makes for F = 1/(3t^2)
  • G = E * C makes for G = 1/(t^2 + b + 1)

I don't have time to write the rest of the formulas out, but I think that's one inversion that gets you everything you need.

let p = G1Affine::sw_encode(t);
assert!(p.is_on_curve());
assert!(!p.is_zero());
assert_eq!(p.y.parity(), t.parity());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, we should check assert_eq!(p, G1::one()) since these are well-defined edge cases.

@ebfull
Copy link
Collaborator

ebfull commented Mar 31, 2018

@bmerge try

@bmerge
Copy link
Collaborator

bmerge commented Mar 31, 2018

⌛ Trying commit a3bbece with merge d9f9b95...

bmerge added a commit that referenced this pull request Mar 31, 2018
Add hashing into the curve.

Hello.
A few days ago @zmanian said it'd be nice if somebody added hashing to the curve to `pairing`, and I found myself with a few spare days in bed, soo… I'd like to propose the folllowing changes:

- Add Legendre symbol for Fq and Fq2, with test cases
- Implement Foque-Ticouchi's hashing to the curve algorithm

The algorithm is pretty much along the lines of https://github.com/herumi/mcl/blob/9fbc54305d01b984e39d83e96bfa94bb17648a86/include/mcl/bn.hpp#L65-L127 , as suggested by @zmanian.
@bmerge
Copy link
Collaborator

bmerge commented Mar 31, 2018

☀️ Test successful - pairing-linux32-try, pairing-linux64-try, pairing-windows32msvc-try, pairing-windows64msvc-try
State: approved= try=True

@burdges
Copy link

burdges commented Aug 19, 2018

What is the status of this? As written, ff wants to take over much of this, but not sure if that's optimal.

@dignifiedquire
Copy link
Contributor

@ebfull any update on the status of this and the intent of moving forward with it? Looks like I could really use this. Is it only a matter of using this as a baseline and making a PR on ff?

@ebfull
Copy link
Collaborator

ebfull commented Apr 9, 2019

I'm sorry that I haven't revisited this. This PR was in excellent shape prior to some modifications to master, and we've begun an effort to refactor the implementations which has put this on the sideline. (Zcash itself doesn't need hashing to the curve, but it's an absolute requirement for BLS signatures.)

I will try to revisit this in our new implementation of BLS12-381 which is an ongoing WIP and I'll update this thread with more info as soon as I have it.

@paberr
Copy link

paberr commented Apr 27, 2019

While looking for an implementation of BLS signatures using BLS12-381, I stumbled across this PR after reading through the code of the bls crate.

There seems to be some pretty new work by Wahby and Boneh on "Fast and simple constant-time hashing to the BLS12-381 elliptic curve".
Are you already aware of this work and how does their Shallue–van de Woestijne map differ from your adaption?

@mmaker
Copy link
Contributor Author

mmaker commented Apr 28, 2019

hey, yes I'm aware though I still have to look into that - I'm really sorry it's taking me so long.
Anyhoo SWU map aside, I already did some work on the fast cofactor multiplication here #102 which is what Boneh also is suggesting

str4d pushed a commit that referenced this pull request Apr 30, 2020
str4d pushed a commit that referenced this pull request Apr 30, 2020
Sapling proving and verifying API
@ebfull ebfull closed this Sep 8, 2020
@tuxxy
Copy link

tuxxy commented Oct 12, 2020

So what's the deal with this being closed? Are there any plans to add hash_to_curve support in this lib?

mmaker added a commit to mmaker/bls12_381 that referenced this pull request Oct 27, 2020
mmaker added a commit to mmaker/bls12_381 that referenced this pull request Oct 27, 2020
@str4d
Copy link
Member

str4d commented Feb 22, 2021

@tuxxy no; this PR was closed because this crate no longer contains an implementation of BLS12-381. Hash-to-curve would need to be added to the bls12_381 crate instead.

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.