Implement Elligator2 hash-to-curve for Twisted Edwards curves

[drskalman]

Description

The IETF standard requires/recommond Elligator 2 hash-to-curve for Twisted Edwards Curve. This PR implement the Elligator2 map-to-curve so such hashing is possible in Arkworks.

• 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

[burdges]

"The IETF standard requires/recommends Elligator 2 hash-to-curve for [any curve with a] twisted Edwards form", so ed_on_*, bls12-377, etc.

It won't be easily usable without #643 or similar of course, given the current interface is basically exclusive to bls12-381.

[weikengchen]

Jeff, I would likely take a look at #643 separately, for the reason that that one is a considerably larger PR...

[burdges]

Sure. Also, there are a few small parts of #643 like 15fc1a5 which permit all of this to be moved out of algebra. That's not ideal long-term, but it'd simplify using hash-to-curve besides bls12-381 now.

[drskalman]

given the current interface is basically exclusive to bls12-381.

Hmm I wonder why is that. I have implemented a complete example for a TE curves in the test example. no?

[drskalman]

@mmagician you are familiar with the code, it is mostly borrowed from the SWU map. You should be able to review it quite easily.

[burdges]

Hmm I wonder why is that. I have implemented a complete example for a TE curves in the test example. no?

What you have here is fine. It's not possible to make it the default for a curve, well the current interface does not handle any defaults correctly. It's also not possible to generate the base field elements using the correct field hasher either (shake). #643 already solves those issues. I've already slurped too much up into #643 though, so it's fine to do this part separately.

[Pratyush]

What's the status of this?

[mmagician]

I think it's ready, I will review it against the spec.

[mmagician]

@drskalman It seems that in the end we are not taking advantage of the new function?

Maybe I had misunderstood the intentions here - but it seems that the current version might as well have done away with using the previously available check_parameters no? Since the precomputed parameters are stored as trait const anyway.

[drskalman]

@drskalman It seems that in the end we are not taking advantage of the new function?

I thought in the case of Elligator2 the precomputation is quite simple (computing 1/B^2 and A/B in the field) so I thought I could expect it as a const.

Nonetheless, I consider it an improvement, because the object itself is responsible to check if the precomputation provided to it is correct rather than relying on other actors to call the test.

Having said that, I don't have a strong opinion one way or another. I''l implement some benches comparing the two scenarios and let you decide which way to go specifically with Elligator precomputations based on the performance.

[drskalman]

It seems that when you drop your hasher you get similar performance hit when the precomputation is in const or in new (around 2% in 100K hashes). So the Elligtor precomputation does not seems to be significant one way or another:

Elligator - 10^5 hashes - Precomputation in const - Hasher Kept alive
                        time:   [228.04 ms 229.60 ms 231.35 ms]
                        time:   [244.07 ms 246.17 ms 248.38 ms]
                        time:   [246.38 ms 247.63 ms 248.95 ms]
Elligator - 10^5 hashes - Precomputation in const - Hasher dropped after each hash
                        time:   [255.06 ms 256.78 ms 258.58 ms]
                        time:   [251.67 ms 253.45 ms 255.37 ms]
                        time:   [252.91 ms 253.76 ms 254.72 ms]
Elligator - 10^5 hashes - Precomputation in new - Hasher Kept alive
                        time:   [243.74 ms 245.21 ms 246.75 ms]
                        time:   [243.57 ms 244.58 ms 245.67 ms]
                        time:   [237.13 ms 239.50 ms 241.92 ms]
Elligator - 10^5 hashes - Precomputation in new - Hasher dropped after each hash
                        time:   [248.11 ms 249.08 ms 250.12 ms]
                        time:   [242.66 ms 244.14 ms 245.90 ms]
                        time:   [244.98 ms 246.46 ms 248.09 ms]

Let me know if you prefer to use const or precompute them in new.

[mmagician]

When you say Hasher dropped - what are you referring to exactly, that we have new() which returns the object and we're not re-using that?
Just to double check - do you have release mode on? If not, then the debug assert will execute and that might be causing the performance difference between dropping the hasher and not, since then the debug check will run for each hash.

I think it only makes sense to either have the precomputation in const (and remove new) or have it done once and for all as part of new. The latter is more user-friendly, as defining a new implementation doesn't require the user to precompute the parameters offline - but as we've discussed with @Pratyush today, this is not something that users would implement very often anyway. As such my vote would be to remove new (again...sorry for the back and forth), keep the precomputed parameters in const and, if my hypotheses about the release mode is correct, that should still achieve the best performance.

[drskalman]

This is the benchmarking code:

    fn hash_arbitary_string_to_curve_elligator2_keep_alive() {
        let test_elligator2_to_curve_hasher = MapToCurveBasedHasher::<
            Projective<TestElligator2MapToCurveConfig>,
            DefaultFieldHasher<Sha256, 128>,
            Elligator2Map<TestElligator2MapToCurveConfig>,
        >::new(&[1])
        .unwrap();

        for i in 0..100000 {
            let to_be_hashed = format!("{}",i);
            let hash_result = test_elligator2_to_curve_hasher.hash(to_be_hashed.as_bytes()).expect("hash should work for all values");
              assert!(
                 hash_result.is_on_curve(),
                 "hash results into a point off the curve"
             );

        }
    }

    fn hash_arbitary_string_to_curve_elligator2_drop_hasher_after_hash() {

        for i in 0..100000 {
            let test_elligator2_to_curve_hasher = MapToCurveBasedHasher::<
                    Projective<TestElligator2MapToCurveConfig>,
                DefaultFieldHasher<Sha256, 128>,
                Elligator2Map<TestElligator2MapToCurveConfig>,
                >::new(&[1])
                .unwrap();
            let to_be_hashed = format!("{}",i);
            let hash_result = test_elligator2_to_curve_hasher.hash(to_be_hashed.as_bytes()).expect("hash should work for all values");
              assert!(
                 hash_result.is_on_curve(),
                 "hash results into a point off the curve"
             );

        }
    }

cargo bench automatically compiles in release so no debug assert. it could be that computing in stack is faster than computing with const somehow.

Sure go ahead and revert the removal of the new and I would revert to 74e07bc.

[mmagician]

@drskalman I've re-reverted the necessary commit, so that we end up without new parameters. Once that's merged, let's go ahead and adapt this PR to use const, we can then merge this.

[mmagician]

@drskalman, the revert PR was just merged. We can now go ahead with the changes here, after adapting to the new API (removed new)

[drskalman]

@mmagician I removed new and changed the interface. It should be ready now.

[mmagician]

Thanks @drskalman for quickly handling the changes, green light from my side!

[Pratyush]

Looks mostly good to me, just a few questions about errors and assertions.

[Pratyush]

What was the motivation of changing this to debug_assert?

[mmagician]

This one is a little more tricky, @drskalman maybe can confirm, but it should never happen at this point that the assert fails right?

[mmagician]

So, technically under the current code, it could happen that div = 0. Here we apparently get a non-zero div, but that's working off the assumption that P::ZETA is non-zero.
The current check on ZETA here should actually be more strict - currently it allows a zero, whereas according to RFC9380 Sec. 6.6.2: "Z is non-square in F".
With the stricter condition, this debug_assert here is fine instead of assert.

[Pratyush]

I guess my higher-level question was that earlier these checks would happen even in release mode; is it safe for them to happen only in debug mode now?

[drskalman]

So it should not fail if the map is correct. So, it is safe if the map is correct. However if an implementor choose a wrong constant, I thought it is a good idea that they get the error that they are mapping off the curve.

However, once they got the paramaters right, then we could forgo the check in the interest of performance assuming that they have at least run it once in the debug mode.
Given that it never fails if the parameters are right, we could remove the check completely if you don't like debug_assert rather than slowing done every hashing with assert. But I thought we can give a hand to the curve developers in debug time so they can fix their maps.

Your call.

[Pratyush]

Makes sense to me, thanks for the explanation!