Skip to content

Conversation

@rheitjoh
Copy link
Member

@rheitjoh rheitjoh commented May 10, 2021

New hash functions will be admissible in the sense that they can be used to replace a random oracle in any scheme in the ROM.

Done

  • Added hash function to G1, G2. I integrated it into our BN implementation and tested that it actually produces points on G1, respectively G2. Both are combined in a single class BarretoNaehrigHashToSourceGroupImpl. Cofactor multiplication is used from PairingSourceGroupImpl. To enable that, I had to make the cofactor multiplication method public so the hash class can access it.
  • Some Javadocs
  • Added to changelog

TODO

  • Could implement faster cofactor multiplication for G2. Right now we just seem to always use a simple binary square and multiply algorithm.

@rheitjoh rheitjoh added the enhancement New feature or request label May 10, 2021
@rheitjoh rheitjoh self-assigned this May 10, 2021
@rheitjoh rheitjoh linked an issue May 10, 2021 that may be closed by this pull request
@rheitjoh
Copy link
Member Author

One current problem is that the indifferentiability requires two independent random oracles to F_q. I am currently just using two VariableOutputLengthHashFunction (which are basically the same SHA function underneath). @JanBobolz Do you know whether it is sufficient to just add some constant to the input to make them independent? I am not sure what independence means exactly.

@JanBobolz
Copy link
Member

JanBobolz commented May 11, 2021

Do you know whether it is sufficient to just add some constant to the input to make them independent? I am not sure what independence means exactly.

Prefixing every input to the hash function does do what you need, e.g. H_1(x) = H(1||x) and H_2(x) = H(2||x) are two independent random oracles (if H is modeled as a random oracle).
("Independent" I guess it's just as if you have two ROM oracles that function independently, i.e. their outputs are uncorrelated)

@JanBobolz
Copy link
Member

Hey, maybe it would be cool if one could just pass a prefix to the VariableOutputLengthHashFunction's constructor so that one could use the "two independent hash functions" functionality without prefixing every input manually.

@rheitjoh
Copy link
Member Author

Hey, maybe it would be cool if one could just pass a prefix to the VariableOutputLengthHashFunction's constructor so that one could use the "two independent hash functions" functionality without prefixing every input manually.

Problem is that I also might use any other hash function that is passed to the class, not just VariableOutputLengthHashFunction. Happens for example for the BN specs, where the hash function is defined in the spec.

Considering that, it seems simpler to keep the input prefixing to the hash class.

@rheitjoh rheitjoh requested a review from JanBobolz May 12, 2021 13:34
@rheitjoh rheitjoh marked this pull request as ready for review May 12, 2021 13:34
@JanBobolz
Copy link
Member

Hey. I'll review this later. Just at a quick glance: I'm not a huge fan of having a public multiplyWithCofactor method for GroupElements. The abstraction for a specific GroupElement should be that whatever value it holds is actually a valid element of the group. So it's weird to add a method that is only useful to call on "malformed" GroupElements - those shouldn't exist!

I'd prefer a constructor or a static method to map given x-y coordinates into the group using cofactor multiplication. Anything that keeps the "an instance of this is a valid group element (in the right subgroup)" abstraction :)

@rheitjoh
Copy link
Member Author

The original method did only accept coordinate arguments, I just added the GroupElement version for convenience.
So would just making the coordinate-argument method public be ok? I can remove the version that takes in a GroupElement.

Perhaps we should then also change the getElement method of PairingSourceGroupImpl such that is always checks first for membership of the element, and does not just create the element. Currently it does allow for creating group elements that are on the curve but not in the group.

@JanBobolz
Copy link
Member

Coordinate argument version can be public, no issue.

ad PairingSourceGroupImpl::getElement: I feel like there should be two methods for this. One that is public and that ensures that input is valid. The other should be protected and should not perform any such checks (for performance). The latter method would be used internally where it is clear that the input is necessarily valid (e.g., they are the result of adding two points).

@rheitjoh
Copy link
Member Author

getElementUnsafe

Ok, so I started by adding a protected getElementUnsafe method to PairingSourceGroupImpl and changing getElement to first check for membership.
The issue with this approach is that e.g. the operations implemented in AffineEllipticCurvePoint make use of getElement of the WeierstrassCurve interface. This means that they are wasting runtime on unnecessarily checking for membership. getElementUnsafe is meant exactly for these situations for where we already know that the point will be on the curve, but we cannot add getElementUnsafe to WeierstrassCurve since the latter is an interface and therefore only allows for public methods (in Java 8). We obviously don't want to make that method public.

An option would be to make WeierstrassCurve an abstract class. Then we can add a protected getElementUnsafe method to it. This change is not immediately problematic since none of the implementing classes (PairingSourceGroupImpl and Secp256k1) extend any other classes.
We can then also add the isOnCurve method from PairingSourceGroupImpl method to it.

Making WeierstrassCurve a proper class could also be an option. Subclasses such as PairingSourceGroupImpl can then still be abstract.
After all, WeierstrassCurve in itself is already functional together with an element class such as AffineEllipticCurvePoint.
Then we would have a generic elliptic curve class that can be used to for example simplify the Secp256k1 class and other elliptic curves not associated with a bilinear group.
If we add the isOnCurve method, we can even implement getElement properly. Subgroups can then replace the implementation using an isMember function instead of isOnCurve.

Cofactor multiplication

An annoyance with making the cofactor multiplication coordinate-only is that I now need two cofactor multiplications for the hashing, and not just one. I used to be able to map to the curve, but not to the group, and then execute the op() that is part of the hashing before doing the actual mapping to the group via cofactor multiplication. Now I need to first map both points to the group before I can execute the op().

Some options for keeping the single cofactor multiplication:

  1. A workaround for this would be to make the cofactor multiplication method that takes in a group element protected and then override it in BarretoNaehrigSourceGroupImpl. Then the hashing function would be able to access it. Would mean having group elements that are not actually part of the group, but this time only using non-public methods.

  2. We can potentially completely avoid the "group element that is not actually in the subgroup"- thing: We could switch to an object representing the curve and not the subgroup G2. So for the part where the elements are not in the subgroup, we instantiate them as points on the curve that underlies G_X, execute the group operation, and then map the resulting curve point to the subgroup via cofactor multiplication. This seems like the mathematically most correct approach (we do the curve stuff on the curve and the subgroup stuff in the subgroup).
    Here having WeierstrassCurve be a concrete class would be helpful so we can use that to instantiate the BN curve that G_X is based on (the getAX() methods can be used to obtain the parameters).
    To map from the curve to the group, a cofactor multiplication method (or mapFromCurveToGroup might be more intuitive since cofactor multiplication is a more technical term) that takes in a curve element would make sense. This would be similar to before, but now we give it an element of the curve and not a group element that is not actually a group element. That method could be implemented by taking the curve point and performing the cofactor multiplication on the curve. Then we take the resulting point (who is now guaranteed to be in the subgroup) and instantiate a subgroup element from it.
    To do the latter, we can add a protected constructor to PairingSourceGroupElement that instantiates such an element from a curve point that is guaranteed to already be in the subgroup. For a potential group-specific cofactor multiplication implementation later, we can then add a similar constructor to the constructors of that group element. We can therefore keep the x, y, and z-coordinates package-private.

@JanBobolz
Copy link
Member

Give me some time to comment on this 🙈. It’s a complicated discussion. Will do later.

@rheitjoh rheitjoh merged commit e61cd65 into develop Aug 19, 2021
@rheitjoh rheitjoh deleted the bn-hash branch August 19, 2021 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Admissible hash for BarretoNaehrigPointEncoding

3 participants