[WIP] `Int` - sign-and-magnitude #694

erik-3milabs · 2024-10-14T15:57:04Z

What is this?
This PR provides basic signed integer support into crypto_bigint. This is a not-yet-implemented feature (see #1).
This basic implementation involves the following operations:

checked_add,
checked_sub,
checked_mul, and
checked_div

Related
Discussion started in #624.

About
This PR puts the first code on the board. The decision was made to implement Int as sign-and-magnitude (over, say, two's complement) since it is easier to scale to flexible-size implementations and, in my opinion, less error-prone. Moreover, multiplications yield hardly any overhead compared to operating on Uints; most of the overhead is incurred in the addition operation.

Open question:
It was mentioned in #624 to use Choice to represent the sign. Sadly, this struct does not seem to have a const constructor. If this is indeed the case, we will be unable to make the constants Int::ZERO, ::MINUS_ONE, ::ONE, ::MIN and ::MAX unless we resolve this.

Area for improvement:
The current addition/subtraction operations involve

one Uint-addition,
one Uint-subtraction,
three CtOption::ct_selects,
one Uint::eq call, and
one Uint::ct_gt call.

As such, this operation is ~7x more expensive than a Uint addition/subtraction. There is at least one clear area for improvement here: add an is_zero flag to the struct. Doing so will get allow us to get rid of the ct_eq call.
In my understanding, converting to and from two's complement will not be an option; the overhead in doing most likely exceeds the 6x performance penalty.

Any feedback is greatly appreciated!

ycscaly · 2024-10-14T19:15:05Z

@erik-3milabs crypto-bigint has their own const_choice, which is what I believe @tarcieri meant for you to use.

lleoha · 2024-10-14T21:26:15Z

The decision was made to implement Int as sign-and-magnitude

My impression was that in discussion it was suggested to rather use U2 not sign + absolute value. Constant operations would be easier in that representation and IMHO it's more suitable for fixed sized integers.
With that you can even make Int to just be a newtype of Uint if for some reason this is desired (provided you have accessor for the MSB).

lleoha · 2024-10-14T21:28:51Z

src/int/add.rs

+
+impl<const LIMBS: usize> Int<LIMBS> {
+    /// Add two [`Int`]s, checking for overflow.
+    fn checked_adc(&self, rhs: &Self) -> CtOption<Self> {


this is unnecessary complex.
it's much easier to do it that way:

positive and negative never overflows

two positives (or two negatives) overflow if MSB of the result is different that MSB of operands (the result and operands of magnitude I mean)
no need for min/max, sub etc.

lleoha · 2024-10-14T21:32:06Z

src/int/div.rs

+use crate::int::Int;
+use crate::{CheckedDiv, Uint};
+
+impl<const LIMBS: usize> CheckedDiv for Int<LIMBS> {


the zero has two representation, I think we should handle this as well, i.e "-0"/whatever should be "-0" not "+0".

lleoha · 2024-10-14T21:33:52Z

src/int/mul.rs

+
+impl<const LIMBS: usize, const RHS_LIMBS: usize> CheckedMul<Int<RHS_LIMBS>> for Int<LIMBS> {
+    #[inline]
+    fn checked_mul(&self, rhs: &Int<RHS_LIMBS>) -> CtOption<Self> {


same as for div, since there are two zero representation the sign should be preserved.
i.e.

-0 * -0 = +0

+0 * +0 = +0

-0 * +0 = -0

+0 * -0 = -0

lleoha · 2024-10-14T21:35:15Z

src/int/neg.rs

+use crate::int::Int;
+use crate::WrappingNeg;
+
+impl<const LIMBS: usize> Int<LIMBS> {


same here, there are two representation of zero so sign should behave correctly i.e.:
-(-0) = +0
-(+0) = -0

tarcieri · 2024-10-14T21:54:56Z

src/int.rs

+    // TODO(erik): replace with Choice once I've figured how to const-construct those.
+    is_negative: bool,


bool anywhere in constant-time code is worrisome because it's an invitation for e.g. LLVM's optimizer to insert branches based on the value, even in cases where the original code deliberately avoids them

lleoha · 2024-10-14T21:55:33Z

There is at least one clear area for improvement here: add an is_zero

I am strongly against that for the following reasons. Currently zero has two representations (-0 and +0) adding is_zero will make it from 2 to ~2^(limbs * limb_bits).

lleoha · 2024-10-14T21:57:21Z

src/int.rs

+
+#[derive(Copy, Clone, Hash, Debug, PartialEq)]
+pub struct Int<const LIMBS: usize> {
+    // TODO(erik): replace with Choice once I've figured how to const-construct those.


no problem here, worst case scenario, just use u8, CtChoice is more or less that.

tarcieri · 2024-10-14T21:58:35Z

The decision was made to implement Int as sign-and-magnitude (over, say, two's complement) since it is easier to scale to flexible-size implementations

I'd like to reiterate my previous comments that the heap-allocated Boxed* types are generally a much lower priority than the const-generic stack-allocated types.

The Boxed* types largely exist to support RSA and DSA, which are algorithms where it might be necessary to support a very high cardinality of potential key sizes.

Pretty much all other algorithms use a very small, fixed number of key sizes.

lleoha · 2024-10-15T08:24:50Z

src/int.rs

+
+#[cfg(target_pointer_width = "64")]
+#[allow(dead_code)]
+type I128 = Int<2>;


this is where it's tricky, if we want to relate this to builtin i128 type, this is actually closer to I129 because of using sign and magnitude instead of two's complements.

erik-3milabs · 2024-10-16T09:47:23Z

@tarcieri @lleoha thank you for your feedback! It became clear that sign-and-magnitude is not as great a solution as I thought it to be. To investigate whether two's complement might be a better approach, I took some time to whip up a quick implementation; you can find it as an open PR at #695. I would greatly appreciate your feedback!

lleoha · 2024-10-16T11:01:47Z

U2 looks much better than sign magnitude and is more consistent.
I'll take a look at it tomorrow evening, and if you willing to accept help, I would be happy to help. I did implement it that before (I even have this implemented for this repo for my internal purposes but on in the state I want to publish it as of yet).

lleoha · 2024-10-16T11:02:25Z

@erik-3milabs forgot to mention you in my response :).

erik-3milabs · 2024-10-16T13:14:03Z

@lleoha I'm definitely open to collab on this! Given that there is still enough to code, let's discuss with @tarcieri how we're going to structure this PR-wise; ideally, we don't make reviewing it a nightmare.

erik-3milabs · 2024-10-17T13:22:31Z

Closing for now, due to "overwhelming" support for U2 #695.

erik-3milabs and others added 13 commits October 9, 2024 15:20

Introduce basic Int struct

7a0780a

Introduce (not fully correct) Int::checked_add

47a0fde

pass add checked tests

2aae18a

minor add_checked improvements

5595c2e

clean up

b6b73b6

implement some add traits

26d1501

Refactor Int::add

3efe421

Introduce Int::eq

e38062a

Introduce Int::neg

43700d6

Impl Int::sub

2d7257d

Impl Int::checked_mul

f97d60c

Impl Int::checked_div

aa5a085

Merge branch 'RustCrypto:master' into int

b318da3

erik-3milabs marked this pull request as draft October 14, 2024 15:58

erik-3milabs changed the title ~~Basic Int Support~~ [WIP] Basic Int Support Oct 14, 2024

erik-3milabs mentioned this pull request Oct 14, 2024

Supporting Signed Integer Arithmetic #624

Closed

erik-3milabs added 7 commits October 14, 2024 18:01

Add allow(dead_code) tags

e85d199

Update add annotations

66ef85f

Fix formatting

12232b3

revert lib.rs

7e379c9

Remove .idea files

5b8c12f

revert lib.rs

7a8e45c

Update checked_sub_overflow tests.

8f6802d

erik-3milabs marked this pull request as ready for review October 14, 2024 16:22

lleoha reviewed Oct 14, 2024

View reviewed changes

tarcieri reviewed Oct 14, 2024

View reviewed changes

lleoha reviewed Oct 14, 2024

View reviewed changes

lleoha reviewed Oct 15, 2024

View reviewed changes

erik-3milabs mentioned this pull request Oct 16, 2024

Int - two's complement #695

Merged

erik-3milabs changed the title ~~[WIP] Basic Int Support~~ [WIP] Int - sign-and-magnitude Oct 16, 2024

erik-3milabs closed this Oct 17, 2024

erik-3milabs mentioned this pull request Oct 29, 2024

Int feature tracking ticket #700

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] `Int` - sign-and-magnitude #694

[WIP] `Int` - sign-and-magnitude #694

erik-3milabs commented Oct 14, 2024

ycscaly commented Oct 14, 2024

lleoha commented Oct 14, 2024 •

edited

Loading

lleoha Oct 14, 2024 •

edited

Loading

lleoha Oct 14, 2024

lleoha Oct 14, 2024

lleoha Oct 14, 2024

tarcieri Oct 14, 2024

lleoha commented Oct 14, 2024 •

edited

Loading

lleoha Oct 14, 2024

tarcieri commented Oct 14, 2024

lleoha Oct 15, 2024

erik-3milabs commented Oct 16, 2024

lleoha commented Oct 16, 2024

lleoha commented Oct 16, 2024

erik-3milabs commented Oct 16, 2024

erik-3milabs commented Oct 17, 2024

		// TODO(erik): replace with Choice once I've figured how to const-construct those.
		is_negative: bool,

[WIP] Int - sign-and-magnitude #694

[WIP] Int - sign-and-magnitude #694

Conversation

erik-3milabs commented Oct 14, 2024

ycscaly commented Oct 14, 2024

lleoha commented Oct 14, 2024 • edited Loading

lleoha Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

lleoha Oct 14, 2024

Choose a reason for hiding this comment

lleoha Oct 14, 2024

Choose a reason for hiding this comment

lleoha Oct 14, 2024

Choose a reason for hiding this comment

tarcieri Oct 14, 2024

Choose a reason for hiding this comment

lleoha commented Oct 14, 2024 • edited Loading

lleoha Oct 14, 2024

Choose a reason for hiding this comment

tarcieri commented Oct 14, 2024

lleoha Oct 15, 2024

Choose a reason for hiding this comment

erik-3milabs commented Oct 16, 2024

lleoha commented Oct 16, 2024

lleoha commented Oct 16, 2024

erik-3milabs commented Oct 16, 2024

erik-3milabs commented Oct 17, 2024

[WIP] `Int` - sign-and-magnitude #694

[WIP] `Int` - sign-and-magnitude #694

lleoha commented Oct 14, 2024 •

edited

Loading

lleoha Oct 14, 2024 •

edited

Loading

lleoha commented Oct 14, 2024 •

edited

Loading