Implement modular arithmetic

[jellevos]

Hi @tarcieri and the other contributors! For the scicrypt crate I was working on constant-time big integers by wrapping GMP, but the interface was extremely clumsy. I decided to switch to this crate (which looks awesome!) and added the necessary methods for modular arithmetic.

I implemented the following functionality:

• Shifting left and right by 1 and returning the overflow as a Choice
• Neg for Wrapping<Uint<LIMBS>>
• Modular multiplicative inverse
• Computing parameters for the Montgomery form (MF)
• Going to and from MF
• Modular addition (in MF)
• Modular multiplication (in MF)
• Modular exponentiation (in MF)

In my opinion, there are at least two large issues right now that I would like to hear your opinion on.

• The Modular interface is not satisfying to me because it copies the MontgomeryParameters and does not assert that two Modular integers indeed share the same modulus.
• None of the functions are currently const fn.

Still, I thought it was good to already make a PR to bring this to your attention. I am happy to put in the effort to bring this to a point where it is mergeable! :)

Edit:
The modulus is now a constant and the struct is called Residue rather than Modular.

[jellevos]

I now see issues #70 and #28. I think adt_const_params will take quite a while before it is stabilized, but in the meantime one option is not to use a constant modulus and instead use debug asserts to check that e.g. the two moduli in a multiplication are the same. Based on these two issues I will change Modular to Residue and change as many functions as possible to be const.

[tarcieri]

@jellevos the workaround that @haslersn suggested which uses a ZST + trait with an associated constant seems OK as a workaround until adt_const_params is stabilized:

#70 (comment)

pub trait ResidueParameters<const L: usize>: PartialEq + Debug {
    const MODULUS: Option<UInt<L>>;
}

#[derive(PartialEq)]
pub struct Residue<P, const L: usize>
where
    P: ResidueParameters<L>,
{
    repr: UInt<L>,
    phantom: PhantomData<P>,
}

It could be wrapped up in a macro which defines a ZST and impls the trait for it like so to reduce the boilerplate:

impl_modulus!(Modulus, U256, "ffffffff00000001...");

[tarcieri]

Also thanks for submitting this! I'll try to review it in depth when I have some time.

[jellevos]

It could be wrapped up in a macro which defines a ZST and impls the trait for it like so to reduce the boilerplate:

impl_modulus!(Modulus, "ffffffff00000001...");

This is a really good idea! One thing that is not immediately clear to me is how ergonomic this will be for non-hardcoded moduli, but I will investigate.

Also thanks for submitting this! I'll try to review it in depth when I have some time.

Feel free to wait a bit until I have incorporated the changes above. :) If you like, I can ping you when they are ready.

[tarcieri]

This is a really good idea! One thing that is not immediately clear to me is how ergonomic this will be for non-hardcoded moduli, but I will investigate.

The philosophy of this crate is to largely focus on compile-time optimizations, and perhaps do a secondary pass for things that are dependent on runtime values (e.g. #58).

Having a const MODULUS unlocks some potential interesting future optimizations when used in conjunction with other const fn methods of UInt.

It would enable checking special properties of the modulus, and using those properties to select particular implementations in a way this could all be known at compile time. This would allow the compiler to optimize away the branches, so you could provide several implementations of e.g. modular sqrt, picking the best one for a particular modulus at compile time.

[jellevos]

@tarcieri The implementation is mostly finished, but I am running into a seemingly fundamental issue. Specifically, I get the following error when running the tests in uint/modular/pow.rs on stable:

error: any use of this value will cause an error
  --> src/uint/shr.rs:67:20
   |
67 |                   if i < (LIMBS - 1) - full_shifts {
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |                      |
   |                      exceeded interpreter step limit (see `#[const_eval_limit]`)
   |                      inside `uint::shr::<impl uint::UInt<16_usize>>::shr_vartime` at src/uint/shr.rs:67:20
   |                      inside `uint::shr::<impl uint::UInt<16_usize>>::shr_vartime_wide` at src/uint/shr.rs:89:17
   |                      inside `uint::div::<impl uint::UInt<16_usize>>::ct_reduce_wide` at src/uint/div.rs:100:17
   |                      inside `<modular::pow::tests::Modulus as ResidueParams<{ nlimbs!(<$uint_type>::BIT_SIZE) }>>::R2` at src/uint/modular/macros.rs:24:17
   |
  ::: src/uint/modular/macros.rs:23:13
   |
23 | /             const R2: $crate::UInt<{ nlimbs!(<$uint_type>::BIT_SIZE) }> =
24 | |                 $crate::UInt::ct_reduce_wide(Self::R.square_wide(), &Self::MODULUS).0;
   | |______________________________________________________________________________________-
   |
   = note: `#[deny(const_err)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>

error[E0080]: evaluation of `uint::modular::Residue::<uint::modular::pow::tests::Modulus, 16_usize>::new` failed
  --> src/uint/modular/mod.rs:48:41
   |
48 |         let product = integer.mul_wide(&MOD::R2);
   |                                         ^^^^^^^ referenced constant has errors

note: the above error was encountered while instantiating `fn uint::modular::Residue::<uint::modular::pow::tests::Modulus, 16_usize>::new`
  --> src/uint/modular/macros.rs:36:9
   |
36 |         $crate::uint::modular::Residue::<$modulus, { $modulus::LIMBS }>::new($variable)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
  ::: src/uint/modular/pow.rs:56:24
   |
56 |         let base_mod = residue!(base, Modulus);
   |                        ----------------------- in this macro invocation
   |
   = note: this note originates in the macro `residue` (in Nightly builds, run with -Z macro-backtrace for more info)

In other words, we are hitting the compiler's const_eval_limit, and I do not immediately see a way to fix this. ☹️ On nightly, the fix is simple: I have added the following snippet in lib.rs, which basically turns off this check (we could also just put in a larger number than the default, rather than completely omitting the check):

#![feature(const_eval_limit)]
#![const_eval_limit = "0"]

What do you think? Do you have any clever ideas on how to get around this? Apart from this, I only need to add a few docstrings before the PR should be ready for review.

[jellevos]

I suppose we could also shrink the modulus in those tests, which would make the tests and library work on stable but would force dependencies that use somewhat large moduli to be nightly.

[haslersn]

the workaround that @haslersn suggested which uses a ZST + trait with an associated constant seems OK as a workaround

@tarcieri The initial attempt at this was very cumbersome, because one has to drag the const L parameter through everything. So instead I ended up creating a trait GenericUInt that abstracts over UInt<L> and using it like this:

pub trait ResidueParameters: PartialEq + Debug + Send + 'static {
    type UInt: GenericUInt;
    const MODULUS: Option<Self::UInt>;
}

#[derive(PartialEq)]
pub struct Residue<P>
where
    P: ResidueParameters,
{
    repr: P::UInt,
    phantom: PhantomData<P>,
}

[tarcieri]

@jellevos wow, that is really unfortunate. I was unaware const_eval_limit was set so low as to preclude these sorts of computations from working.

I see there's a tracking issue for the low limit precluding other real-world usages: rust-lang/rust#93481 (The main tracking issue for const_eval_limit is rust-lang/rust#67217).

Tough to say what to do about this. It would be good to determine that the caller can change this limit, or otherwise if crypto-bigint needs to set it, we'd need to add something like a nightly crate feature which bumps the limit.

[tarcieri]

So instead I ended up creating a trait GenericUInt that abstracts over UInt<L>

@haslersn FWIW the existing Integer trait largely provides that.

[haslersn]

@tarcieri yes, but the Integer trait lacks access to modular arithmetic (add_mod_special, ...), direct access to limbs, and conversion from primitive types such as u32.

[tarcieri]

@haslersn you can add those bounds to the existing Integer trait, rather than making a new one

[haslersn]

@tarcieri yes, when adding the functionality directly in crypto-bigint, that would be the way to go.

[jellevos]

Tough to say what to do about this. It would be good to determine that the caller can change this limit, or otherwise if crypto-bigint needs to set it, we'd need to add something like a nightly crate feature which bumps the limit.

I think the best solution in the mean time would be to make sure the tests in this library pass on stable and then leave the problem with the user (i.e. a user must switch to nightly if the const computations exceed the current limit).

I can finish these changes tomorrow, then I would say this is ready for review. I will add a clear statement in the docs that one may need to up const_eval_limit.

[tarcieri]

@jellevos sounds good!

FWIW I left a comment here: rust-lang/rust#93481 (comment)

[jellevos]

the workaround that @haslersn suggested which uses a ZST + trait with an associated constant seems OK as a workaround

@tarcieri The initial attempt at this was very cumbersome, because one has to drag the const L parameter through everything.

Currently, I use a second macro residue! that at least prevents the end user from being exposed to this (i.e. having to set const LIMBS). It takes a UInt and such a modulus trait and returns a Residue.

[jellevos]

@tarcieri Ready for review! The efficient squaring method is in #133.

Edit: I have an almost ready version of the runtime-selected moduli, and to make the code much nicer, this PR also requires some refactoring. This does mean that things get shuffled around a bit. I will still keep this PR only for constant moduli. I'll push this code in an hour or so and also create a draft PR for the runtime moduli.

[jellevos]

Summary of new changes:

• Changed folder structure to accommodate runtime-based moduli neatly.
• I forgot to implement the inverse for residues before; that's done now.
• Renamed Residue to ConstResidue, I reserved Residue for residues with a modulus decided at runtime.
• Added traits to provide a consistent API between ConstResidues and Residues.

I want to raise an issue about the multiplicative inverses as they are currently very unsafe to use. Currently, I only check that an inverse exists using debug_assert!. Release builds, however, do not panic when an inverse does not exist. Instead, they actually continue on with computations on a residue with an undefined value.

@tarcieri Do you have any clever ideas on how to handle this? One idea is to return a CtOption, but then the inverse cannot be a const fn. Thanks for your help!

[tarcieri]

Renamed Residue to ConstResidue, I reserved Residue for residues with a modulus decided at runtime.

I'd personally prefer (and it would be more consistent with the other RustCrypto projects) if it were the reverse of that, with Residue being const-friendly, and something like DynResidue with a runtime-dependent modulus.

[jellevos]

I agree, I reverted it. 😄 I am still thinking about how to best handle the problem with multiplicative inverses:

I want to raise an issue about the multiplicative inverses as they are currently very unsafe to use. Currently, I only check that an inverse exists using debug_assert!. Release builds, however, do not panic when an inverse does not exist. Instead, they actually continue on with computations on a residue with an undefined value.

@tarcieri Do you have any clever ideas on how to handle this? One idea is to return a CtOption, but then the inverse cannot be a const fn. Thanks for your help!

[tarcieri]

@jellevos yeah, that's a tricky and annoying tradeoff.

You could have a const fn version that panics, and a non-const fn version that returns a CtOption, which are both wrappers around a third internal const fn version that does something like return the Choice selector as a u8 or Word, but that's messy.

[jellevos]

It should be ready! @tarcieri

[tarcieri]

Cool, will take another look this weekend

[haslersn]

When using this much appreciated feature in my project, I needed to do some changes to the API surface around Residue. Some changes were necessary to make it usable at all (from different crates) and some of them were meant to simplify usage of Residues, especially in generic contexts.

I'd like to PR these changes, discuss them, and reach a consensus. However, I'll only get to that after the paper deadline, which is in about a week.

So it would be nice if you could wait a bit with the next release.

[tarcieri]

No worries there, it's going to be at least a few months before we're ready to cut another release