Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uint traits #39

Closed
wants to merge 5 commits into from
Closed

Uint traits #39

wants to merge 5 commits into from

Conversation

xuganyu96
Copy link
Contributor

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

Copy link

codecov bot commented Dec 25, 2023

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fjarri
Copy link
Member

fjarri commented Dec 25, 2023

Two other possible strategies for dealing with precision:

  • 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()?
  • Automatically widen arguments in arithmetic operations to the larger precision. Too error-prone?

@xuganyu96
Copy link
Contributor Author

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
Copy link

RustCrypto/crypto-bigint#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.

RustCrypto/crypto-bigint#403 includes some additional widening support.

} else {
let abs_q = MontyForm::<L>::new(&Uint::<L>::from(abs_q), params);
let abs_q = <T as Integer>::Monty::new(
T::from(abs_q).widen(candidate.bits_precision()),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When instantiating BoxedMontyForm::new(residue, params), the current behavior is to panic when the residue's precision is not equal to the parameter's precision. Should this be changed to "automatic widening"?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, sounds good

@tarcieri
Copy link

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
Copy link
Member

fjarri commented Dec 27, 2023

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

@tarcieri
Copy link

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
Copy link
Member

fjarri commented Dec 27, 2023

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

@tarcieri
Copy link

Aah, yeah, makes sense

@fjarri
Copy link
Member

fjarri commented Dec 28, 2023

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

@tarcieri
Copy link

v0.6.0-pre.6 is out: RustCrypto/crypto-bigint#527

@xuganyu96
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants