entropyxyz / crypto-primes

Random prime generation and primality testing library based on `crypto-bigint`.
https://docs.rs/crypto-primes
Apache License 2.0
17 stars 4 forks source link

Uint traits #39

Closed xuganyu96 closed 7 months ago

xuganyu96 commented 7 months ago

This PR builds upon #36 and adds the following items:

  1. Two tests in presets.rs: prime_generation_boxed and safe_prime_generation_boxed, both of which pass
  2. Some fixes to overflowing_shl_vartime and overflowing_shr_vartime, as well as random_bits for BoxedUint
  3. Added one_with_precision, zero_with_precision, and widen to UintLike so that all BoxedUint sizes can be kept identical
  4. Modified miller_rabin.rs and lucas.rs to maintain equal BoxedUint sizes across the entire module.

The most awkward part of these proposed changes is the need for one_with_precision, zero_with_precision, and widen, which makes sense in the context of BoxedUint but feels redundant when working with Uint. Again, I think some clear documentation will suffice, but maybe there are some better ways.

cc: @fjarri

codecov[bot] commented 7 months ago

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (45152ad) 99.21% compared to head (b59be9c) 98.48%.

Files Patch % Lines
src/uint_traits.rs 86.95% 12 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #39 +/- ## ========================================== - Coverage 99.21% 98.48% -0.73% ========================================== Files 9 10 +1 Lines 1402 1521 +119 ========================================== + Hits 1391 1498 +107 - Misses 11 23 +12 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fjarri commented 7 months ago

Two other possible strategies for dealing with precision:

xuganyu96 commented 7 months ago

Personally I prefer explicit over implicit. Having "automatic" widening thus doesn't sound too appealing. Having a one/zero as an instance method also sound awkward.

Some of the places where "one" is used are for "minus by one". For that I think having a "wrapping/overflowing decrement/increment" method would make a lot of sense.

In other places, I think having an explicit "*_with_precision" and "widen" functions is the least awkward of all the awkward solutions.

tarcieri commented 7 months ago

https://github.com/RustCrypto/crypto-bigint/issues/312 is the tracking issue for where it might sense to implement automatic widening. Several operations do already support it.

It's actually fairly important for performance as it can cut down on the total number of operations that need to be performed versus explicitly widening the inputs to be the same size. So long as those widths are always fixed (for e.g. a given key size) the result is still constant-time.

https://github.com/RustCrypto/crypto-bigint/pull/403 includes some additional widening support.

tarcieri commented 7 months ago

Have one() and zero methods on UintLike objects that will take the object's precision. The problem is the naming. zero_with_the_same_precision()?

@fjarri I've kind of wanted something like that but have also been stuck on the name

fjarri commented 7 months ago

If it's a "static" method, we could follow numpy and call it Integer::zero_like() and Integer::one_like().

tarcieri commented 7 months ago

For that particular application, I was thinking something which accepts &self as an argument and returns zero/one with the same precision as the original value, versus the current BoxedUint::zero_with_precision(n.bits_precision())

fjarri commented 7 months ago

I just think that in case of the static method the naming is easier. BoxedUint::zero_like(n)

tarcieri commented 7 months ago

Aah, yeah, makes sense

fjarri commented 7 months ago

I finally dissolved the UintLike trait in #36 (waiting for crypto-bigint to have another pre release to merge).

tarcieri commented 7 months ago

v0.6.0-pre.6 is out: https://github.com/RustCrypto/crypto-bigint/pull/527

xuganyu96 commented 7 months ago

I'm on vacation at this moment so apologies in advance for slow progress.