Correctly handle serialization of curves where BaseField::MODULUS_SIZE = m * 64 - 1

[Pratyush]

Handle this as follows:

1. Change the ConstantSerializedSize trait to use methods instead of constants:

pub trait ConstantSerializedSize {
	fn serialized_size<F: Flags>() -> usize;
	fn uncompressed_size<F: Flags>() -> usize;
}

2. Change the implementation of ConstantSerializedSize for all fields to additionally use F::len() when calculating the number of required bytes, and similarly for elliptic curves

cc @therealyingtong; I believe this is the reason why your tests are failing in arkworks-rs/curves#8

also cc @paberr @kobigurk since y'all were involved in the original serialization code; does this strategy seems reasonable?

[paberr]

If I understand correctly, the reason for first change is that you want to make the size dependent on the flags?
Presumably to increase the size in case there's not enough space to fit the flags?
Also, what's m in BaseField::MODULUS_SIZE = m * 64 - 1?

But yes, I think this is a reasonable way to deal with this additional dependency. 👍

[kobigurk]

In general it sounds like a good idea to me!
Having the sizes as consts was useful when you wanted to rely on it in constant contexts. Will we lose it now?

How about the following?
Add a FLAGS_SIZE in bits, since this is different for every curve (SW/TE) anyway, and calculate the const size by (<P::BaseField as ConstantSerializedSize>::SERIALIZED_SIZE*8 + FLAGS_SIZE - 1)/8? Will this work?

[paberr]

Having the sizes as consts was useful when you wanted to rely on it in constant contexts. Will we lose it now?

Yes, the proposed method would mean you cannot use it in constant contexts anymore.
It's a pity that const fn doesn't work with traits.

How about the following?
Add a FLAGS_SIZE in bits, since this is different for every curve (SW/TE) anyway, and calculate the const size by (<P::BaseField as ConstantSerializedSize>::SERIALIZED_SIZE*8 + FLAGS_SIZE - 1)/8? Will this work?

This would probably work as well. Even a bit more complicated calculations could be made as long as they rely on const fn.

[Pratyush]

Add a FLAGS_SIZE in bits, since this is different for every curve (SW/TE) anyway, and calculate the const size by (<P::BaseField as ConstantSerializedSize>::SERIALIZED_SIZE*8 + FLAGS_SIZE - 1)/8? Will this work?

We can make this work if we change the trait to be ConstantSerializedSize<F: Flags = EmptyFlags>. But separately, that calculation isn't correct, because it misses empty bits. For example, for BLS12-381 G1, SERIALIZED_SIZE = 48 bytes, but there are enough bits for the all values of SWFlags

A correct impl for prime fields would be:

impl<P: $params, F: Flags> ark_serialize::ConstantSerializedSize<F> for $field<P> {
	const SERIALIZED_SIZE: usize = ark_serialize::buffer_byte_size(
		P::MODULUS_BITS as usize + F::BIT_SIZE,
	);
	const UNCOMPRESSED_SIZE: usize = Self::SERIALIZED_SIZE;
}

A correct impl for curves would be

impl<P: Parameters, F: Flags> ConstantSerializedSize<F> for GroupAffine<P> {
    const SERIALIZED_SIZE: usize = <P::BaseField as ConstantSerializedSize<F>>::SERIALIZED_SIZE;
    const UNCOMPRESSED_SIZE: usize = <P::BaseField as ConstantSerializedSize>::SERIALIZED_SIZE
		+ <P::BaseField as ConstantSerializedSize<EmptyFlags>>::SERIALIZED_SIZE;
}

[kobigurk]

Add a FLAGS_SIZE in bits, since this is different for every curve (SW/TE) anyway, and calculate the const size by (<P::BaseField as ConstantSerializedSize>::SERIALIZED_SIZE*8 + FLAGS_SIZE - 1)/8? Will this work?

We can make this work if we change the trait to be ConstantSerializedSize<F: Flags = EmptyFlags>. But separately, that calculation isn't correct, because it misses empty bits. For example, for BLS12-381 G1, SERIALIZED_SIZE = 48 bytes, but there are enough bits for the all values of SWFlags

A correct impl for prime fields would be:

impl<P: $params, F: Flags> ark_serialize::ConstantSerializedSize<F> for $field<P> {
	const SERIALIZED_SIZE: usize = ark_serialize::buffer_byte_size(
		P::MODULUS_BITS as usize + F::BIT_SIZE,
	);
	const UNCOMPRESSED_SIZE: usize = Self::SERIALIZED_SIZE;
}

A correct impl for curves would be

impl<P: Parameters, F: Flags> ConstantSerializedSize<F> for GroupAffine<P> {
    const SERIALIZED_SIZE: usize = <P::BaseField as ConstantSerializedSize<F>>::SERIALIZED_SIZE;
    const UNCOMPRESSED_SIZE: usize = <P::BaseField as ConstantSerializedSize>::SERIALIZED_SIZE
		+ <P::BaseField as ConstantSerializedSize<EmptyFlags>>::SERIALIZED_SIZE;
}

You're right, thanks for the correction!

[Pratyush]

Hm so changing ConstantSerializedSize to be the following doesn't really work:

trait ConstantSerializedSize<F: Flags = EmptyFlags> {
	const SERIALIZED_SIZE: usize;
	const UNCOMPRESSED_SIZE: usize;
}

This is because adding the type parameter (even a defaulted one) makes the trait unusable in trait bounds: either we just keep it the trait bound to be ConstantSerializedSize, which means that we're only constrained to serializing with EmptyFlags, or we change the trait bound to ConstantSerializedSize<F>, but this requires us to add the F parameter everywhere: Field<F: Flags>: ConstantSerializedSize<F>.

I think we're stuck with using the non-const route; @kobigurk is there a specific usecase that requires const?

[kobigurk]

I think we've used it in a constant case, but I'll check and let you know.

EDIT: I've checked my uses, and while they could be const, they are used in non-const contexts. So no objection from me at this point.

[Pratyush]

Ok thanks! I'll go ahead and implement the non-const version.