Add skip_while condition for scalar mul

[tcoratger]

During the multiplication of of G1/G2 by a scalar, the leading zeros can be skipped in the loop, because they induce unnecessary doubling calculations, giving always the identity value back before the first non zero element is reached in the rhs boolean array.

Skipping these useless calculations can reduce drastically the calculation costs, especially for small numbers.

[Nashtare]

I believe such logic, while relevant to this library, should be in its own function, with the suffix var_time, as it goes against the expected constant-time implementation of the current scalar multiplication (which will naturally induce overhead).

Also such a bump in rust-toolchain should probably be made in a separate, distinct PR requiring a version bump.

[tcoratger]

I believe such logic, while relevant to this library, should be in its own function, with the suffix var_time, as it goes against the expected constant-time implementation of the current scalar multiplication (which will naturally induce overhead).

Also such a bump in rust-toolchain should probably be made in a separate, distinct PR requiring a version bump.

@Nashtare

• Yes sorry for the bump, already submitted here: bump msrv to 1.64.0 #116, just cleaned up
• About the logic I understand, but then you propose to add another separate function called multiply_vartime?
  • In my opinion that would be a bit of an implementation overhead, wouldn't it? especially since all the rest of the logic is exactly the same.
  • On sqrt or inv implementations, it's not constant time either, so is that really the problem here?

[Nashtare]

In my opinion that would be a bit of an implementation overhead, wouldn't it? especially since all the rest of the logic is exactly the same.

Having a distinction between constant-time VS variable-time for operations to be used in critical cryptographic scenarios is worth the overhead, trust me 🙂 Side-channels attacks do exist and can be devastating for a reason.

On sqrt or inv implementations, it's not constant time either, so is that really the problem here?

They are constant-time. The use of pow_vartime is fine in this context, because the exponent is hardcoded in both, effectively keeping them constant-time (see the note on pow_vartime method). Note that the README is emphasizing the constant-time operations support:

All operations are constant time unless explicitly noted.

[tcoratger]

In my opinion that would be a bit of an implementation overhead, wouldn't it? especially since all the rest of the logic is exactly the same.

Having a distinction between constant-time VS variable-time for operations to be used in critical cryptographic scenarios is worth the overhead, trust me 🙂 Side-channels attacks do exist and can be devastating for a reason.

On sqrt or inv implementations, it's not constant time either, so is that really the problem here?

They are constant-time. The use of pow_vartime is fine in this context, because the exponent is hardcoded in both, effectively keeping them constant-time (see the note on pow_vartime method). Note that the README is emphasizing the constant-time operations support:

All operations are constant time unless explicitly noted.

@Nashtare Ok I get your point. Just modified, tell me what you think about this, I can even add unit tests to cover this in parallel to const time implementation.

[Nashtare]

Yeah unit tests to check consistency between the impls could be a nice add-on.
Note that I am not a maintainer of this library though, so I can't approve your PR.

[tcoratger]

@Nashtare Thanks for the review, unit tests are added where needed, waiting for a review/approval :)

[randombit]

If you're going to make it variable time why not also remove the constant time conditional add? That will save, on average, half of the point addition steps.

[tarcieri]

It seems like you could just use a branch here, rather than conditional_select, since this is variable-time code?

Likewise you could use a bool instead of Choice, which will avoid subtle's memory barriers and should make the code easier for rustc to optimize.