Allow addition and multiplication for references to secret shared values #787

akoshelev · 2023-09-26T20:56:38Z

This change brings an extra trait bound for linear secret sharing trait, to allow addition and multiplication on references, rather than owned types. Currently it is only possible to do a + b or a + &b, but not &a + b or &a + &b. This change makes it possible.

As usual, it is not all rosy here, because of the compiler issue: rust-lang/rust#20671. There is a need currently to specify the additional trait bound on every function, but I think it is worth it because not every .clone() call can be eliminated by the compiler, as it can be seen here. We're better off helping optimizer here. So it cost us additional trait bound, but gives a much nicer addition/multiplication w/o extra clones and makes us not worry about compiler not being able to optimize it.

Once (if?) that gnarly issue is fixed, we could probably just add for <'a, 'b> &'a Self: Add<'b Self>bound to ArithmeticOps trait and eliminate all extra helper traits along with extra bounds on functions.

Implementation notes

I could've probably used a macro to synthesize all extra Add/Sub/Mul implementations, but decided it is not worth it for just two types. Maybe if we had more secret sharing types ....
I tried hard to mimic num trait definition (Add new traits for reference and assignment operators rust-num/num#283), but ran into an error complaining that Add operation is not general enough:

error: implementation of `Add` is not general enough
  --> src/secret_sharing/mod.rs:97:49
   |
97 |         assert_eq!(AdditiveShare::<Fp31>::ZERO, sum_ref_ref(&AdditiveShare::<Fp31>::ZERO, &AdditiveShare::ZERO));
   |                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `Add` is not general enough
   |
   = note: `Add<replicated::semi_honest::additive_share::AdditiveShare<fp31::Fp31>>` would have to be implemented for the type `&'0 replicated::semi_honest::additive_share::AdditiveShare<fp31::Fp31>`, for any lifetime `'0`...
   = note: ...but `Add<replicated::semi_honest::additive_share::AdditiveShare<fp31::Fp31>>` is actually implemented for the type `&'1 replicated::semi_honest::additive_share::AdditiveShare<fp31::Fp31>`, for some specific lifetime `'1`

it remains a mystery to me, why I was getting it. The same function worked just fine for u32 and primitive types had the same 4 Add overloads with reference ones defined exactly as ours for additive secret shares.

New trait bounds for arithmetic but getting the following error overflow evaluating the requirement `&ordered_float::OrderedFloat<_>: Neg` I guess I need to constrain that type to `Linear`

If this plane did not have a WiFi, I'd never be able to do it. It seems that `Neg` was tripping me off - adding it as a constraint to trait <A, B> and only constraining it to B, makes the compiler go to infinite loop trying to evaluate all possible types that can fit. At the end, I was getting the gnarly "overflow evaluating ..." error So structure needs to be as follows: * Traits that have Rhs and Output * Traits that only have Output * Traits that are valid for secret sharing (add, sub, neg) * Traits that are valid for basic arithmetic (mul)

Add new trait bound to fix it

…erywhere

martinthomson

Some clarity about naming is probably worth trying to achieve.

src/secret_sharing/replicated/semi_honest/additive_share.rs

martinthomson · 2023-09-27T01:21:35Z

src/ff/mod.rs

+///
+/// [`RefNum`]: https://docs.rs/num/0.4.1/num/traits/trait.RefNum.html
+/// [`rust-issue`]: https://github.com/rust-lang/rust/issues/20671
+pub trait RefOps<'a, Base: 'a, R: 'a>:


I think that ArithmeticOps is the better choice of name here, with the trait above being for ArithmeticAssignOps or something like that.

I'd love to keep Ref somewhere in the name to indicate that it only makes sense to implement/bound if you need l-value references

ArithmeticRefOps then, I guess.

martinthomson · 2023-09-27T01:22:32Z

src/ff/mod.rs

+pub trait LocalAssignOps<Rhs = Self>: AddAssign<Rhs> + SubAssign<Rhs> {}
+impl<T, Rhs> LocalAssignOps<Rhs> for T where T: AddAssign<Rhs> + SubAssign<Rhs> {}
+
+/// Arithmetic operations that may or may not require communication.


Can these really involve communication? None of these traits are async, so this can't do that. That suggests that these are still local operations, but only those that involve value-based l-values.

I tried to distinguish operations that can be safely performed locally and confusing ones like Mul that someone can accidentally implement for secret shares as (a.0 * b.0, a.1 * b.1). Lets chat later today about it - I'd love to have better names for all of those traits

martinthomson · 2023-09-27T22:55:08Z

src/protocol/basics/if_else.rs

@@ -30,6 +31,6 @@ where
    //   = false_value
    Ok(false_value.clone()


Missed a clone opportunity?

akoshelev · 2023-09-27T22:57:13Z

src/ff/mod.rs

+/// Note: Neg operation is also local, but is causing issues when added as a bound to this trait.
+/// The problem is that it does not use `Rhs` generic and rustc overflows trying to compile functions
+/// that use HRTB generics bounded by `LocalArithmeticOps`.
+pub trait LocalArithmeticOps<Rhs = Self, Output = Self>:


thanks to mt@'s eagle eye vision.

It is too specific to be in the finite fields, basically nothing except `Linear` can conform to it

and rename RefOps to `LinearRefOps`

akoshelev · 2023-09-28T23:21:43Z

@martinthomson I think it is ready for review now

akoshelev added 14 commits September 26, 2023 11:05

Checkpoint

a37c12d

New trait bounds for arithmetic but getting the following error overflow evaluating the requirement `&ordered_float::OrderedFloat<_>: Neg` I guess I need to constrain that type to `Linear`

Make all 4 use cases work

fcf3737

Tests for both malicious and semi-honest

2143a0a

Make Add for malicious share mimic semi honest definition

8b3ac2a

Remove clones from add and sub operations

53c6883

Add new trait bound to fix it

Checkpoint for multiplication

b6446b2

Multiplication is ready

a7c00df

Remove clone calls on multiplications

c9fff40

Clippy + formatting

91e5708

Rename some traits for brevity

1c960fa

Fix benches

990dd68

Add a reference to the rust issue that causes HRTB bound to appear ev…

efe1261

…erywhere

Clean up the code

664b86e

martinthomson reviewed Sep 27, 2023

View reviewed changes

Address feedback

c38f16b

martinthomson reviewed Sep 27, 2023

View reviewed changes

akoshelev commented Sep 27, 2023

View reviewed changes

akoshelev added 5 commits September 28, 2023 09:34

Remove one more clone

b43905c

thanks to mt@'s eagle eye vision.

Move RefOps trait to secret sharing module

d0af8f8

It is too specific to be in the finite fields, basically nothing except `Linear` can conform to it

Move ArithmeticOps to shared value module

bc6690b

and rename RefOps to `LinearRefOps`

Merge from main

fa7688c

Fix clones in saturating sum module

16e8768

martinthomson approved these changes Sep 29, 2023

View reviewed changes

martinthomson merged commit 0599ba6 into main Sep 29, 2023
6 checks passed

martinthomson deleted the ref-refs branch September 29, 2023 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow addition and multiplication for references to secret shared values #787

Allow addition and multiplication for references to secret shared values #787

akoshelev commented Sep 26, 2023

martinthomson left a comment

martinthomson Sep 27, 2023

akoshelev Sep 27, 2023

martinthomson Sep 27, 2023

martinthomson Sep 27, 2023

akoshelev Sep 27, 2023

martinthomson Sep 27, 2023

akoshelev Sep 28, 2023

akoshelev Sep 27, 2023

akoshelev commented Sep 28, 2023

		@@ -30,6 +31,6 @@ where
		// = false_value
		Ok(false_value.clone()

Allow addition and multiplication for references to secret shared values #787

Allow addition and multiplication for references to secret shared values #787

Conversation

akoshelev commented Sep 26, 2023

Implementation notes

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoshelev commented Sep 28, 2023