Improvements from review

Signed-off-by: maurges <nikita@dfns.co>

Author: maurges

Cargo.lock

@@ -26,7 +26,6 @@ digest = { workspace = true }
 sha2 = { workspace = true }
 rand_core = { workspace = true }
 rand_hash = { workspace = true }
-rand = { workspace = true, optional = true, features = ["alloc"] }
 
 futures = { workspace = true }
 
@@ -54,7 +53,7 @@ curve-stark = ["generic-ec/curve-stark", "hd-wallet?/curve-stark"]
 hd-wallet = ["dep:hd-wallet", "cggmp21-keygen/hd-wallet"]
 hd-slip10 = ["hd-wallet/slip10"]
 hd-stark = ["hd-wallet/stark"]
-spof = ["key-share/spof", "dep:rand"]
+spof = ["key-share/spof"]
 
 state-machine = ["cggmp21-keygen/state-machine"]
 

cggmp21/Cargo.toml

@@ -147,6 +147,8 @@ impl<E: Curve, L: SecurityLevel> TrustedDealerBuilder<E, L> {
 
     /// Generates [`IncompleteKeyShare`]s
     ///
+    /// For Shamir secret sharing, it's shared at points `1` to `n`
+    ///
     /// Returns error if provided inputs are invalid, or if internal
     /// error has occurred.
     pub fn generate_core_shares(
@@ -169,11 +171,10 @@ impl<E: Curve, L: SecurityLevel> TrustedDealerBuilder<E, L> {
         self,
         rng: &mut (impl RngCore + CryptoRng),
     ) -> Result<Vec<KeyShare<E, L>>, TrustedDealerError> {
-        let preimages = (1..=self.n)
-            .map(|i| generic_ec::NonZero::from_scalar(generic_ec::Scalar::from(i)))
-            .collect::<Option<Vec<_>>>()
-            .ok_or(Reason::DeriveKeyShareIndex)?;
-        self.generate_shares_at(preimages, rng)
+        self.generate_shares_at_internal(
+            key_share::trusted_dealer::TrustedDealerBuilder::generate_shares,
+            rng,
+        )
     }
 
     /// Generates [`KeyShare`]s shared at preimages provided. Each share is
@@ -184,16 +185,58 @@ impl<E: Curve, L: SecurityLevel> TrustedDealerBuilder<E, L> {
     /// Returns error if provided inputs are invalid, or if internal
     /// error has occurred.
     pub fn generate_shares_at(
-        mut self,
+        self,
         preimages: Vec<NonZero<generic_ec::Scalar<E>>>,
         rng: &mut (impl RngCore + CryptoRng),
     ) -> Result<Vec<KeyShare<E, L>>, TrustedDealerError> {
+        self.generate_shares_at_internal(
+            |builder, rng| builder.generate_shares_at(preimages, rng),
+            rng,
+        )
+    }
+
+    /// Generates [`CoreKeyShare`]s shared at random points
+    ///
+    /// Returns error if provided inputs are invalid, or if internal
+    /// error has occurred.
+    ///
+    /// For Shamir secret sharing, the points at which the value is shared at
+    /// are chosen at random between `1` and `u16::MAX`. For additive shares,
+    /// this is the same as [`TrustedDealerBuilder::generate_shares`]
+    ///
+    /// Returns error if provided inputs are invalid, or if internal
+    /// error has occurred.
+    pub fn generate_shares_at_random(
+        self,
+        rng: &mut (impl rand_core::RngCore + rand_core::CryptoRng),
+    ) -> Result<Vec<KeyShare<E, L>>, TrustedDealerError> {
+        self.generate_shares_at_internal(
+            key_share::trusted_dealer::TrustedDealerBuilder::generate_shares_at_random,
+            rng,
+        )
+    }
+
+    fn generate_shares_at_internal<R, F>(
+        mut self,
+        inner_generate: F,
+        rng: &mut R,
+    ) -> Result<Vec<KeyShare<E, L>>, TrustedDealerError>
+    where
+        F: FnOnce(
+            CoreBuilder<E>,
+            &mut R,
+        ) -> Result<
+            Vec<IncompleteKeyShare<E>>,
+            key_share::trusted_dealer::TrustedDealerError,
+        >,
+        R: rand_core::RngCore + rand_core::CryptoRng,
+    {
         let n = self.n;
         let enable_multiexp = self.enable_mulitexp;
         let enable_crt = self.enable_crt;
 
         let primes = self.pregenerated_primes.take();
-        let core_key_shares = self.inner.generate_shares_at(preimages, rng).map_err(Reason::CoreError)?;
+        let core_key_shares = inner_generate(self.inner, rng).map_err(Reason::CoreError)?;
         let aux_data = if let Some(primes) = primes {
             generate_aux_data_with_primes(rng, primes, enable_multiexp, enable_crt)?
         } else {
@@ -209,30 +252,6 @@ impl<E: Curve, L: SecurityLevel> TrustedDealerBuilder<E, L> {
 
         Ok(key_shares)
     }
-
-    /// Generates [`CoreKeyShare`]s shared at random points
-    ///
-    /// Returns error if provided inputs are invalid, or if internal
-    /// error has occurred.
-    ///
-    /// For Shamir secret sharing, the points at which the value is shared at
-    /// are chosen at random between `1` and `u16::MAX`. For additive shares,
-    /// this is the same as [`TrustedDealerBuilder::generate_shares`]
-    ///
-    /// Returns error if provided inputs are invalid, or if internal
-    /// error has occurred.
-    pub fn generate_shares_at_random(
-        self,
-        rng: &mut (impl rand_core::RngCore + rand_core::CryptoRng),
-    ) -> Result<Vec<KeyShare<E, L>>, TrustedDealerError> {
-        let key_shares_indexes =
-            rand::seq::index::sample(rng, usize::from(u16::MAX - 1), usize::from(self.n))
-                .iter()
-                .map(|i| generic_ec::NonZero::from_scalar(generic_ec::Scalar::from(i + 1)))
-                .collect::<Option<Vec<_>>>()
-                .ok_or(Reason::DeriveKeyShareIndex)?;
-        self.generate_shares_at(key_shares_indexes, rng)
-    }
 }
 
 /// Generates auxiliary data for `n` signers
@@ -334,8 +353,6 @@ enum Reason {
     BuildCrt(#[source] InvalidKeyShare),
     #[error("couldn't build multiexp tables")]
     BuildMultiexp(#[source] InvalidKeyShare),
-    #[error("deriving key share index failed")]
-    DeriveKeyShareIndex,
     #[error(transparent)]
     CoreError(#[from] key_share::trusted_dealer::TrustedDealerError),
 }

cggmp21/src/trusted_dealer.rs

@@ -14,7 +14,6 @@ keywords = ["mpc", "threshold-signatures", "tss"]
 generic-ec = { workspace = true, features = ["alloc"] }
 generic-ec-zkp = { workspace = true, features = ["alloc"] }
 rand_core = { workspace = true, optional = true }
-rand = { workspace = true, optional = true, features = ["alloc"] }
 
 hd-wallet = { workspace = true, optional = true }
 udigest = { workspace = true, features = ["alloc", "derive"], optional = true }
@@ -34,7 +33,7 @@ default = ["std"]
 
 serde = ["dep:serde", "serde_with", "hex", "generic-ec/serde"]
 hd-wallet = ["dep:hd-wallet"]
-spof = ["dep:rand_core", "dep:rand"]
+spof = ["dep:rand_core"]
 udigest = ["dep:udigest", "generic-ec/udigest"]
 
 std = ["dep:thiserror"]

key-share/Cargo.toml

@@ -128,13 +128,20 @@ impl<E: Curve> TrustedDealerBuilder<E> {
         self,
         rng: &mut (impl rand_core::RngCore + rand_core::CryptoRng),
     ) -> Result<Vec<CoreKeyShare<E>>, TrustedDealerError> {
-        let key_shares_indexes =
-            rand::seq::index::sample(rng, usize::from(u16::MAX - 1), usize::from(self.n))
-                .iter()
-                .map(|i| generic_ec::NonZero::from_scalar(Scalar::from(i + 1)))
-                .collect::<Option<Vec<_>>>()
-                .ok_or(Reason::DeriveKeyShareIndex)?;
-        self.generate_shares_at(key_shares_indexes, rng)
+        let mut points = Vec::with_capacity(self.n.into());
+        'each_point: for _ in 0..self.n {
+            for _ in 0..u16::MAX {
+                let point = generic_ec::NonZero::<Scalar<E>>::random(rng);
+                if !points.contains(&point) {
+                    points.push(point);
+                    continue 'each_point;
+                }
+            }
+            // if we did not continue in inner loop, it means we couldn't
+            // generate a distinct scalar
+            return Err(Reason::BadRandom.into());
+        }
+        self.generate_shares_at(points, rng)
     }
 
     /// Generates [`CoreKeyShare`]s shared at preimages provided. Each share is
@@ -149,6 +156,10 @@ impl<E: Curve> TrustedDealerBuilder<E> {
         preimages: Vec<NonZero<Scalar<E>>>,
         rng: &mut (impl rand_core::RngCore + rand_core::CryptoRng),
     ) -> Result<Vec<CoreKeyShare<E>>, TrustedDealerError> {
+        if preimages.len() != usize::from(self.n) {
+            return Err(Reason::InvalidPreimages.into());
+        }
+
         let shared_secret_key = self
             .shared_secret_key
             .unwrap_or_else(|| NonZero::<SecretScalar<_>>::random(rng));
@@ -242,6 +253,10 @@ enum Reason {
     DeriveKeyShareIndex,
     #[displaydoc("randomly generated share is zero - probability of that is negligible")]
     ZeroShare,
+    #[displaydoc("invalid share preimages given")]
+    InvalidPreimages,
+    #[displaydoc("randomness source doesn't have enough entropy")]
+    BadRandom,
 }
 
 impl From<Reason> for TrustedDealerError {