Boxed uintlike coverage by xuganyu96 · Pull Request #436 · RustCrypto/crypto-bigint

xuganyu96 · 2023-12-15T21:19:32Z

This is an attempt to implement for BoxedUint the missing arithmetic needed for crypto-primes (see entropyxyz/crypto-primes#37 (comment)).

This is the continuation of #428.

xuganyu96 · 2023-12-17T02:07:13Z

@tarcieri @fjarri this PR it ready for review. Thank you!

tarcieri · 2023-12-17T02:14:21Z

src/uint/boxed.rs

+    pub const fn const_new(n: BoxedUint) -> (Self, ConstChoice) {
+        let is_nonzero = n.is_nonzero();
+        (Self(n), is_nonzero)
+    }


BoxedUint can't be instantiated in a const fn context because it's heap-allocated

When should a function return (Value, Choice) instead of CtChoice(Value)?

In boxed/sqrt.rs the implementation of sqrt uses a NonZero::<BoxedUint>::new(x) whose value needs to be used whether x is zero or not, but BoxedUint is not ConditionallySelectable so I can't use CtChoice's map and unwrap_or. It seems like having a NonZero::<BoxedUint>::new that returns (NonZero<BoxedUint>, Choice) would be necessary, but the current new's signature doesn't work this way, and such a new is definitely not const_new.

The (hopefully temporary) workaround for that sort of thing is BoxedUint::conditional_map.

I'm trying to find an upstream solution in subtle but it may be necessary to define traits shaped like the ones in that PR in crypto-bigint as a stopgap (and a CtOption-alike type, possibly ConstCtOption) /cc @fjarri

I saw the ConstantTimeSelect trait and will use ct_select and ct_assign where appropriate. Thank you for the implementation!

tarcieri · 2023-12-17T02:16:16Z

src/uint/boxed/cmp.rs

+    pub(crate) fn select(a: &Self, b: &Self, c: ConstChoice) -> Self {
+        debug_assert_eq!(a.limbs.len(), b.limbs.len());
+        let mut limbs = vec![Limb::ZERO; a.limbs.len()];
+
+        let mut i = 0;
+        while i < limbs.len() {
+            limbs[i] = Limb::select(a.limbs[i], b.limbs[i], c);
+            i += 1;
+        }
+
+        Self {
+            limbs: limbs.into(),
+        }
+    }


This is already impl'd as BoxedUint::conditional_select. You can use Into to convert from ConstChoice to Choice.

tarcieri · 2023-12-17T02:18:14Z

src/uint/boxed/cmp.rs

+    /// Returns the truthy value if `self >= rhs` and the falsy value otherwise.
+    #[inline]
+    pub(crate) fn gt(lhs: &Self, rhs: &Self) -> ConstChoice {
+        let (_res, borrow) = rhs.sbb(lhs, Limb::ZERO);
+        ConstChoice::from_word_mask(borrow.0)
+    }


This is already impl'd as BoxedUint::ct_gt. Really there's not a lot of point in supporting ConstChoice with BoxedUint, and the least-common-denominator trait API should use Choice instead (which provides an optimization barrier in non-const fn contexts, which is the only place BoxedUint can be used)

tarcieri · 2023-12-17T02:20:31Z

src/uint/boxed/sqrt.rs

@@ -0,0 +1,295 @@
+//! [`BoxedUint`] square root operations.


This is fine to get this implemented, but in a followup PR (which I'm happy to do) it should probably get a similar macro treatment to the multiply functions so Uint and BoxedUint can share a common implementation.

xuganyu96 · 2023-12-17T04:33:42Z

Note to myself:

use conditional_map in BoxedUint::sqrt

Edit: this is no longer needed; we can now use <BoxedUint as ConstantTimeSelect>::ct_select

tarcieri · 2023-12-17T15:21:56Z

src/modular/boxed_residue.rs

+    pub fn div_by_2(&self) -> Self {
+        Self {
+            montgomery_form: div_by_2(&self.montgomery_form, &self.residue_params.modulus),


This is a little confusing due to the name clash which makes it almost look like it's going to recurse, although it's hard to suggest something better

What about using fully qualified namespace like div_by_2::boxed::div_by_2?

Sure, sounds good

xuganyu96 · 2023-12-18T18:15:05Z

Newest commits:

Used ConstantTimeSelect::{ct_assign, ct_select} where appropriate (see boxed/sqrt.rs and div_by_2)
Added ConstCtOption<NonZero<Limb>>::expect (for use in crypto-primes)

tarcieri · 2023-12-20T01:49:40Z

src/uint/boxed.rs

+        let mut b = 0;
+        let mut i = 0;
+        while i < self.limbs.len() {
+            b |= self.limbs[i].0;
+            i += 1;
+        }
+        Limb(b).is_nonzero().into()


Suggested change

let mut b = 0;

let mut i = 0;

while i < self.limbs.len() {

b |= self.limbs[i].0;

i += 1;

}

Limb(b).is_nonzero().into()

let choice = Choice::from(1u8);

for limb in &self.limbs {

choice &= limb.ct_eq(&Limb::ZERO);

}

choice

How about just !self.is_zero()? The logic suggested above is basically what is_zero uses but backwards.

I mean, we can do that, but other instances can be replaced with !x.is_zero() too. It makes less sense when const fn isn't required

tarcieri · 2023-12-20T01:50:42Z

src/uint/boxed.rs

+    /// TODO: this is not really "const", but I need a way to return (value, choice) since
+    /// BoxedUint is not [`ConditionallySelectable`] so `CtChoice::map` and such does not work
+    pub fn const_new(n: BoxedUint) -> (Self, Choice) {
+        let nonzero = n.is_nonzero();
+        (Self(n), nonzero)
+    }


I factored these into methods on e.g. in this case BoxedUint: #484

You can use ConstCtChoice now.

tarcieri · 2023-12-20T01:52:17Z

src/uint/boxed/encoding.rs

    }
+
+    /// Create a new [`BoxedUint`] from the provided big endian hex string.
+    pub fn from_be_hex(hex: &str, bits_precision: u32) -> Self {


Since these can't be constructed at compile-time, it would probably be good to make it fallible

tarcieri · 2023-12-20T01:52:45Z

src/uint/boxed/shl.rs

-    /// Computes `self << shift`.
-    /// Returns `None` if `shift >= self.bits_precision()`.
-    ///
-    /// NOTE: this operation is variable time with respect to `shift` *ONLY*.
-    ///
-    /// When used with a fixed `shift`, this function is constant-time with respect to `self`.
-    #[inline(always)]
-    pub fn shl_vartime(&self, shift: u32) -> Option<Self> {
-        let mut result = Self::zero_with_precision(self.bits_precision());
-        let success = self.shl_vartime_into(&mut result, shift);
-        success.map(|_| result)
-    }


I originally made them to accommodate the requirement from crypto-primes, but now I realized I can use overflowing_shl/overflowing_shr to achieve the same thing, so I felt no need to add shl_vartime/shr_vartime for BoxedUint.

Err, I added shl_vartime in #330. The reason we have *_vartime equivalents of the sh* methods is because they can be quite a bit faster.

Oh now I see the reason for all the replaced _vartime methods previously. Yes, we should keep these.

tarcieri · 2023-12-20T01:53:00Z

src/uint/boxed/shr.rs

-    /// Computes `self >> shift`.
-    /// Returns `None` if `shift >= self.bits_precision()`.
-    ///
-    /// NOTE: this operation is variable time with respect to `shift` *ONLY*.
-    ///
-    /// When used with a fixed `shift`, this function is constant-time with respect to `self`.
-    #[inline(always)]
-    pub fn shr_vartime(&self, shift: u32) -> Option<Self> {
-        let mut result = Self::zero_with_precision(self.bits_precision());
-        let success = self.shr_vartime_into(&mut result, shift);
-        success.map(|_| result)
-    }


Same as above.

tarcieri · 2023-12-20T14:30:33Z

@xuganyu96 sorry about the rebase you're gonna have to do after #485, but we'll try to get this merged ASAP once you do!

xuganyu96 · 2023-12-20T15:58:56Z

@xuganyu96 sorry about the rebase you're gonna have to do after #485, but we'll try to get this merged ASAP once you do!

Totally not an issue. Thank you for being so thorough with my PR!

…lySelectable in BoxedUint

fjarri · 2023-12-20T18:26:26Z

benches/boxed_uint.rs

        b.iter_batched(
            || BoxedUint::random(&mut OsRng, UINT_BITS),
-            |x| black_box(x.shl_vartime(UINT_BITS / 2 + 10)),
+            |x| black_box(x.overflowing_shl(UINT_BITS / 2 + 10).0),


Why the change here? And if you're benchmarking overflowing_shl now, the string label should be changed too.

Yeah sorry I think I got really confused between the various shl/shr functions.

fjarri · 2023-12-20T18:28:02Z

src/modular/boxed_monty_form.rs

+    pub fn div_by_2(&self) -> Self {
+        Self {
+            montgomery_form: div_by_2::boxed::div_by_2(&self.montgomery_form, &self.params.modulus),
+            params: self.params.clone(), // TODO: avoid clone?


There would be no need for clone if it was an inplace method, but if it creates a new number, I think cloning is unavoidable.

When std is available they're stored as an Arc, so cloning is cheap (incrementing an atomic reference counter)

fjarri · 2023-12-20T18:31:23Z

src/modular/div_by_2.rs

 }
+
+#[cfg(feature = "alloc")]
+pub(crate) mod boxed {


I think putting different variants of this function in modules is a bit of an overcomplication, perhaps just different suffixes would suffice.

fjarri · 2023-12-20T18:34:07Z

src/uint/boxed/bits.rs

        let mut result = BoxedUint::zero_with_precision(256);
        for &pos in positions {
-            result |= BoxedUint::one_with_precision(256).shl_vartime(pos).unwrap();
+            result |= BoxedUint::one_with_precision(256).overflowing_shl(pos).0;


Any specific reason for the change here?

Another instance of me being confused. Will restore back to its original state.

fjarri · 2023-12-20T18:36:47Z

src/uint/boxed/cmp.rs


+impl BoxedUint {
+    /// Returns the Ordering between `self` and `rhs` in variable time.
+    pub fn cmp_vartime(&self, rhs: &Self) -> Ordering {


I have a feeling that using comparisons between limbs instead of subtraction may be faster. Put a TODO perhaps?

fjarri · 2023-12-20T18:38:23Z

src/uint/boxed/div.rs

        let mut rem = self.clone();
        // Will not overflow since `bd < bits_precision`
-        let mut c = rhs.shl_vartime(bd).expect("shift within range");
+        let mut c = rhs.overflowing_shl(bd).0;


It's a vartime method, no need to use a constant-time overflowing_shl here

fjarri · 2023-12-20T18:38:57Z

src/uint/boxed/div.rs

        let mut quotient = Self::zero_with_precision(self.bits_precision());
        // Will not overflow since `bd < bits_precision`
-        let mut c = rhs.shl_vartime(bd).expect("shift within range");
+        let mut c = rhs.overflowing_shl(bd).0;


Same as above, no need for a constant-time shift here.

fjarri · 2023-12-20T18:40:41Z

src/uint/reciprocal.rs

@@ -0,0 +1,208 @@
+//! Reciprocal, shared across Uint and BoxedUint


This should reference the paper the algorithm was taken from.

fjarri · 2023-12-20T18:41:57Z

src/uint/boxed/div_limb.rs

+    use super::{div2by1, Reciprocal};
+    use crate::{Limb, NonZero, Word};
+    #[test]
+    fn div2by1_overflow() {


Since this test tests a function from uint::reciprocal, perhaps it should be located there.

This test is actually an exact copy of the same test in uint::div_limb. I will delete this one in boxed/div_limb.rs.

fjarri · 2023-12-20T18:47:51Z

src/uint/reciprocal.rs

+
+/// Returns `u32::MAX` if `a < b` and `0` otherwise.
+#[inline]
+const fn lt(a: u32, b: u32) -> u32 {


lt and select should really be moved to ConstChoice - forgot to do that in #458

I've added them as TODO's.

xuganyu96 · 2023-12-21T07:36:37Z

@tarcieri @fjarri sorry but I felt I lost track of where I made changes that overrode your work (e.g. shl_vartime and shr_vartime). I've opened another PR #489.

See also: #436 and entropyxyz/crypto-primes#37

xuganyu96 marked this pull request as ready for review December 16, 2023 06:46

xuganyu96 force-pushed the boxed-uintlike-coverage branch from 9d730ab to f433d38 Compare December 16, 2023 23:07

xuganyu96 mentioned this pull request Dec 17, 2023

Further coverage of UintLike entropyxyz/crypto-primes#37

Closed

4 tasks

tarcieri reviewed Dec 17, 2023

View reviewed changes

xuganyu96 force-pushed the boxed-uintlike-coverage branch from a14399a to 2eba0e7 Compare December 18, 2023 17:32

xuganyu96 force-pushed the boxed-uintlike-coverage branch from 2f31a4f to 1b65d01 Compare December 20, 2023 01:26

xuganyu96 requested a review from tarcieri December 20, 2023 01:43

tarcieri reviewed Dec 20, 2023

View reviewed changes

xuganyu96 added 9 commits December 20, 2023 10:59

Implemented and tested BoxedUint::bit_vartime

13ca4be

Ipmlemented trailing_ones and trailing_ones_vartime for BoxedUint

f8ecad3

Implemented div and sqrt for BoxedUint

ef2c7bb

Refactored shr_vartime and shl_vartime to return (Self, ConstChoice)

24792c3

div_rem_limb_with_reciprocal and its dependencies

97f2111

Boxed div by 2

83e065e

Gate boxed div_by_2 by alloc

1d100c1

Share Reciprocal across Uint and BoxedUint

9666ad5

Fixed clippy and rust docs problem

d10f9fd

xuganyu96 added 12 commits December 20, 2023 11:00

BoxedUint::is_nonzero returns Choice

4aedbec

Refactored const_new to return (NonZero<BoxedUint>, Choice)

4db5f76

replace select with conditional_select

8c839eb

Use ct_gt instead of gt

ce00645

Use crate::ConstantTimeSelect as a substitute for subtle::Conditional…

a4f8125

…lySelectable in BoxedUint

Use fully qualified namespace for div_by_2 to avoid confusion

1e6cb45

Use ct_assign to avoid unnecessary cloning

89171b5

ConstCtOption<NonZero<Limb>>::expect

233b9e5

use overflowing_shl instead of shl_vartime; same for shr

97209b3

Moved mulhilo and addhilo to crate::primitives

abbfd1f

Fixed import pathing for 32-bit targets

49d72c5

Migrated from Residue to Monty

d2d0e0f

xuganyu96 force-pushed the boxed-uintlike-coverage branch from cc2efd0 to d2d0e0f Compare December 20, 2023 16:05

fjarri reviewed Dec 20, 2023

View reviewed changes

fjarri mentioned this pull request Dec 20, 2023

Add traits to connect integers and residue represenations #431

Merged

xuganyu96 mentioned this pull request Dec 21, 2023

Expand integer trait #489

Merged

xuganyu96 closed this Dec 21, 2023

tarcieri pushed a commit that referenced this pull request Dec 21, 2023

Expand integer trait (#489)

f9ef5aa

See also: #436 and entropyxyz/crypto-primes#37

		@@ -0,0 +1,208 @@
		//! Reciprocal, shared across Uint and BoxedUint

Conversation

xuganyu96 commented Dec 15, 2023

Uh oh!

xuganyu96 commented Dec 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarcieri Dec 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuganyu96 commented Dec 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuganyu96 commented Dec 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuganyu96 Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjarri Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Dec 20, 2023

Uh oh!

xuganyu96 commented Dec 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarcieri Dec 17, 2023 •

edited

Loading

xuganyu96 commented Dec 17, 2023 •

edited

Loading

xuganyu96 Dec 20, 2023 •

edited

Loading

fjarri Dec 20, 2023 •

edited

Loading