`Signed` improvements #166

fjarri · 2024-12-10T08:04:42Z

Split Signed and Bounded into SecretSigned, PublicSigned, and SecretUnsigned.
More careful handling of secret values in SecretSigned/Unsigned (fixes Zeroizing follow-up: Signed #165)
Simplify Exponentiable trait (part of Multiplication/exponentiation speed-ups #34)
Simplify ring-Pedersen methods (kind of Group similar methods using traits #60)

Remaining work (for other PRs):

Fix Zeroize MontyParams of RSA primes #162 (have to wait until the next release of crypto-bigint)
Make Scalar always-secret too? Do we ever use non-secret scalars?

codecov · 2024-12-10T08:07:33Z

Codecov Report

Attention: Patch coverage is 97.85592% with 25 lines in your changes missing coverage. Please review.

Project coverage is 93.59%. Comparing base (4c04dcf) to head (2c242d0).

Files with missing lines	Patch %	Lines
synedrion/src/uint/secret_signed.rs	96.70%	14 Missing ⚠️
synedrion/src/uint/public_signed.rs	93.44%	8 Missing ⚠️
synedrion/src/cggmp21/interactive_signing.rs	94.73%	1 Missing ⚠️
synedrion/src/paillier/encryption.rs	99.15%	1 Missing ⚠️
synedrion/src/uint/secret_unsigned.rs	98.21%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #166      +/-   ##
==========================================
+ Coverage   93.44%   93.59%   +0.14%     
==========================================
  Files          34       36       +2     
  Lines        6849     6727     -122     
==========================================
- Hits         6400     6296     -104     
+ Misses        449      431      -18

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

dvdplm

Pheew, a big one!

Overall LGTM; left some nitpicks, suggestions and questions.

There's some inconsistency in what is documented and what isn't, and I think we should avoid asserts in favour of Result wherever possible, although I don't consider it a blocker for this PR.

synedrion/src/cggmp21/conversion.rs

dvdplm · 2024-12-13T08:18:47Z

synedrion/src/paillier/encryption.rs

        }
    }

-    fn homomorphic_mul_unsigned(self, rhs: &Bounded<P::Uint>) -> Self {
-        let rhs_wide = rhs.to_wide();
+    pub fn homomorphic_mul_wide_public(&self, rhs: &PublicSigned<P::WideUint>) -> Self {


Missing docs

synedrion/src/tools/secret.rs

dvdplm · 2024-12-13T08:28:35Z

synedrion/src/uint/secret_signed.rs

+/// A wrapper over secret unsigned integers that treats two's complement numbers as negative.
+#[derive(Debug, Clone)]
+pub(crate) struct SecretSigned<T: Zeroize> {
+    /// bound on the bit size of the absolute value (that is, `abs(value) < 2^bound`)


Suggested change

/// bound on the bit size of the absolute value (that is, `abs(value) < 2^bound`)

// bound on the bit size of the absolute value (that is, `abs(value) < 2^bound`)

(or move this info to the docs)

Not sure what you mean, isn't it in the docs already? It's a doc comment

You are correct, my bad. Maybe capitalize to "Bound" though?

dvdplm · 2024-12-13T08:34:28Z

synedrion/src/uint/secret_signed.rs

+    /// Asserts that the value is within the interval the paper denotes as $\pm 2^exp$.
+    /// Panics if it is not the case.
+    ///
+    /// That is, the value must be within $[-2^{exp}, 2^{exp}]$
+    /// (See Section 2).
+    pub fn assert_exp_range(&self, exp: u32) {
+        let in_bound = self.in_bound(exp);
+        // Have to check for the ends of the range too
+        let is_end = self.abs().expose_secret().ct_eq(&(T::one() << exp));
+        assert!(bool::from(in_bound | is_end), "out of bounds $\\pm 2^{exp}$",)
+    }


I think that we should strive to avoid using asserts in non-test code, and instead prefer Result or Option. Doing this consistently is a lot of work and I am fine doing it bit by bit, but perhaps we can avoid adding new asserting code and instead return Result here?

Yes, I thought of that too, we can make all the proof constructors (where this method is used) return Result<..., LocalError>, so they can fail.

Not sure how often this is called or how important it is for it to be CT, but I tinkered a bit with making it faster (made it return bool for convenience):

pub fn assert_exp_range_fast(&self, exp: u32) -> bool { let abs = self.abs(); let mask = T::one().wrapping_neg().wrapping_shl_vartime(exp); if abs.expose_secret() == &(mask.not() + T::one()) { return true; } let masked = abs & mask; masked.expose_secret() == &T::zero() }

Benchmark timings for U2048:

Bounds check/assert_exp_range, small exponent time: [420.31 ns 425.26 ns 431.98 ns] change: [-1.3407% +0.3532% +2.1482%] (p = 0.72 > 0.05) No change in performance detected. Found 6 outliers among 100 measurements (6.00%) 2 (2.00%) high mild 4 (4.00%) high severe Bounds check/assert_exp_range_fast, small exponent time: [266.69 ns 271.08 ns 277.42 ns] change: [-0.9114% +0.2448% +1.4652%] (p = 0.71 > 0.05) No change in performance detected. Found 7 outliers among 100 measurements (7.00%) 1 (1.00%) high mild 6 (6.00%) high severe Bounds check/assert_exp_range, big exponent time: [713.33 ns 769.78 ns 821.11 ns] change: [-8.6114% +0.4042% +10.744%] (p = 0.94 > 0.05) No change in performance detected. Bounds check/assert_exp_range_fast, big exponent time: [383.40 ns 421.51 ns 462.13 ns] change: [-6.4458% +2.8628% +12.866%] (p = 0.55 > 0.05) No change in performance detected.

Not sure how often this is called or how important it is for it to be CT

A bunch of times during the protocol, but on the order of dozens maybe. So given that it takes <1us, I don't think its performance is too important. It does handle secret data, so I think constant-timeness is appropriate.

Fair enough. Do you happen to know if & is CT?

synedrion/src/uint/secret_signed.rs

fjarri self-assigned this Dec 10, 2024

fjarri marked this pull request as ready for review December 13, 2024 03:01

fjarri requested a review from dvdplm December 13, 2024 03:09

fjarri force-pushed the better-signed branch 2 times, most recently from 47fd19e to 0b9eba1 Compare December 13, 2024 06:26

dvdplm reviewed Dec 13, 2024

View reviewed changes

fjarri added 4 commits December 13, 2024 17:37

Split Signed/Bounded into SecretSigned/PublicSigned/SecretUnsigned

a731a62

Move conversions to their own module

64b7866

Add comments to conversions

3117306

Some of RFCs

283cc7e

fjarri force-pushed the better-signed branch from 2c242d0 to 283cc7e Compare December 14, 2024 02:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Signed` improvements #166

`Signed` improvements #166

fjarri commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

dvdplm left a comment

dvdplm Dec 13, 2024

dvdplm Dec 13, 2024

fjarri Dec 14, 2024

dvdplm Dec 14, 2024

dvdplm Dec 13, 2024

fjarri Dec 13, 2024

dvdplm Dec 13, 2024

fjarri Dec 14, 2024

dvdplm Dec 14, 2024

	/// bound on the bit size of the absolute value (that is, `abs(value) < 2^bound`)
	// bound on the bit size of the absolute value (that is, `abs(value) < 2^bound`)

Signed improvements #166

Are you sure you want to change the base?

Signed improvements #166

Conversation

fjarri commented Dec 10, 2024 • edited Loading

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

dvdplm 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

Choose a reason for hiding this comment

`Signed` improvements #166

`Signed` improvements #166

fjarri commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading