Possible inconsistency in compressed point format for Bls12-381

[kevaundray]

Summary

Following the doc here: https://docs.rs/bls12_381/0.4.0/bls12_381/notes/serialization/index.html

The encoding used seems to be inconsistent with the doc. This would make communicating with other libraries a bit more difficult.

Compressed point in arkworks

#[test]
fn test_serialise() {
    let mut result = [0u8; 48];
    ark_bls12_381::G1Affine::prime_subgroup_generator().serialize(&mut result[..]);
    println!("{}", hex::encode(result))
}

The above code prints out: bbc622db0af03afbef1a7af93fe8556c58ac1b173f3a4ea105b974974f8c68c30faca94f8c63952694d79731a7d3f117

Compressed point in zkcrypto

#[test]
fn test_serialise() {
    let a = G1Affine::generator();
    println!("{}", hex::encode(a.to_compressed()))
}

The above code prints out: 97f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac586c55e83ff97a1aeffb3af00adb22c6bb

Compressed point in go-kzg

// bls_test.go
func TestSerialiase(t *testing.T) {
        compressed := ToCompressedG1(&GenG1)
	fmt.Print(hex.EncodeToString(compressed))
}

The above code prints out: 97f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac586c55e83ff97a1aeffb3af00adb22c6bb

supranational is also consistent with zkcrypto.

Possible solution

I think the endian is probably not much of a problem as you can always reverse the array. I don't know if it is possible to configure this within CanonicalSerialize.

It seems that you may need a new Bls12Flag since SWFlags overlap with the flags in the documentation above. For example here is the condition for the point at infinity for Bls12.

If the above is a valid problem, then I think something like this might work:

// Assuming that this is only used for compressed points
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum Bls12Flags {
    Infinity,
    PositiveY,
    NegativeY,
}
impl Bls12Flags {
    #[inline]
    fn u8_bitmask(&self) -> u8 {
        const COMPRESSED: u8 = 1 << 7;
        let mask = match self {
            Bls12Flags::Infinity => 1 << 6,
            Bls12Flags::PositiveY => 1 << 5,
            Bls12Flags::NegativeY => 0,
        } | COMPRESSED;

        mask
    }
}

[weikengchen]

I am thinking about which direction to go---since changing the encoding is breaking, not just on the code level, but probably on the data level. I am worrying if there have been active users of arkworks that have been using the serialization a lot.

The current CanonicalSerialize and CanonicalDeserialize have been trying to derive the rules of serialization in an almost automatic manner, and adding an exceptional one may make things slightly complicated (e.g., the masking here).

One possibility is to have a separate function (which may go beyond CanonicalSerialize and CanonicalDeserialize) to encode and decode the points in this other format.

[kevaundray]

I think that's a good idea. So you would have a standard encoding that arkworks uses to encode points with, using CanonicalSerialize.Then a way to implement a different, maybe niche, encoding for interoperability with other libraries.

Will help give some more thought to API.

[daira]

Given that the format that was documented is the format that is used by Zcash and other libraries, this can only be considered a bug. I suggest biting the bullet and making an incompatible change, maybe with a feature flag to get the arkworks-legacy encoding.

[Pratyush]

My initial reluctance to switch stemmed from the fact that, because we don't have specialization, doing this changes the encoding for all curves in the ecosystem, and not just bls12-381.

But I think we can work around this by adding some metadata for serialization to SWParameters, and changing SWFlags to SWFlags<P: SWParameters>, so that serialization can take advantage of this.

(I wish specialization would stabilize already... =( )

[weikengchen]

One thing about the alternative encoding is that it assumes three available most significant bits.

If we change all the curves' encodings into this new one, we need to assure that this is the case (at least three reserved bits), but bn254 is an exception (though, people likely would not use it).

If we only change BLS12-381, then it becomes a specialization, for a specific curve, and may affect the automatic serialization/deserialization framework.

[weikengchen]

Note that the alternative encoding has the benefit that it will be always smaller by about one byte.

[Pratyush]

If we only change BLS12-381, then it becomes a specialization, for a specific curve, and may affect the automatic serialization/deserialization framework.

That's what I was trying to avoid by making SWFlags generic over SWParameters (or maybe even making it an associated type of SWParameters?): the specialization would be opaque to downstream layers, and so shouldn't affect those.

[daira]

I'm not sure why we would assume that the encoding method is going to be the same for all curves. In practice, there's a most-commonly-used encoding for each curve, and it's important to use that one in order to prevent incompatibilities and confusion. It doesn't matter that it is different across curves, since interoperability is not an issue in that case.

[weikengchen]

I think the only reason is simplicity. As Pratyush mentioned, I think we do need to specialize in the encoding for BLS12-381 to be compatible with the mainstream format.

[Pratyush]

This can be solved via #308

[mmagician]

Closed in https://github.com/arkworks-rs/curves/pull/129 with initial code provided by @kevaundray.