From 469ce9106baae6797ced424b11a8eff15f38386e Mon Sep 17 00:00:00 2001
From: Luke Parker <lukeparker5132@gmail.com>
Date: Fri, 27 May 2022 02:01:01 -0400
Subject: [PATCH] Implement a binary search for BatchVerifier blame

Adds helper functions to verify and, on failure, blame, which move an
unwrap from callers into multiexp where it's guaranteed to be safe and
easily verified to be proper.

Closes https://github.com/serai-dex/serai/issues/10.
---
 crypto/frost/src/key_gen.rs       |  5 +---
 crypto/frost/src/schnorr.rs       | 15 +++++------
 crypto/frost/src/tests/schnorr.rs | 21 ++++++++--------
 crypto/multiexp/src/lib.rs        | 42 ++++++++++++++++++++++++-------
 4 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/crypto/frost/src/key_gen.rs b/crypto/frost/src/key_gen.rs
index 6108f858..b63c5d22 100644
--- a/crypto/frost/src/key_gen.rs
+++ b/crypto/frost/src/key_gen.rs
@@ -225,10 +225,7 @@ fn complete_r2<R: RngCore + CryptoRng, C: Curve>(
 
     batch.queue(rng, *l, values);
   }
-
-  if !batch.verify() {
-    Err(FrostError::InvalidCommitment(batch.blame_vartime().unwrap()))?;
-  }
+  batch.verify_with_vartime_blame().map_err(|l| FrostError::InvalidCommitment(l))?;
 
   // TODO: Clear the original share
 
diff --git a/crypto/frost/src/schnorr.rs b/crypto/frost/src/schnorr.rs
index 8c4ce401..31cc6065 100644
--- a/crypto/frost/src/schnorr.rs
+++ b/crypto/frost/src/schnorr.rs
@@ -47,8 +47,12 @@ pub(crate) fn batch_verify<C: Curve, R: RngCore + CryptoRng>(
   triplets: &[(u16, C::G, C::F, SchnorrSignature<C>)]
 ) -> Result<(), u16> {
   let mut values = [(C::F::one(), C::G::generator()); 3];
-  let mut batch = BatchVerifier::new(triplets.len() * 3, C::little_endian());
+  let mut batch = BatchVerifier::new(triplets.len(), C::little_endian());
   for triple in triplets {
+    // s = r + ca
+    // sG == R + cA
+    // R + cA - sG == 0
+
     // R
     values[0].1 = triple.3.R;
     // cA
@@ -59,12 +63,5 @@ pub(crate) fn batch_verify<C: Curve, R: RngCore + CryptoRng>(
     batch.queue(rng, triple.0, values);
   }
 
-  // s = r + ca
-  // sG == R + cA
-  // R + cA - sG == 0
-  if batch.verify_vartime() {
-    Ok(())
-  } else {
-    Err(batch.blame_vartime().unwrap())
-  }
+  batch.verify_vartime_with_vartime_blame()
 }
diff --git a/crypto/frost/src/tests/schnorr.rs b/crypto/frost/src/tests/schnorr.rs
index 617cd549..5f64c303 100644
--- a/crypto/frost/src/tests/schnorr.rs
+++ b/crypto/frost/src/tests/schnorr.rs
@@ -31,18 +31,18 @@ pub(crate) fn verify<R: RngCore + CryptoRng, C: Curve>(rng: &mut R) {
 }
 
 pub(crate) fn batch_verify<R: RngCore + CryptoRng, C: Curve>(rng: &mut R) {
-  // Create 3 signatures
+  // Create 5 signatures
   let mut keys = vec![];
   let mut challenges = vec![];
   let mut sigs = vec![];
-  for i in 0 .. 3 {
+  for i in 0 .. 5 {
     keys.push(C::F::random(&mut *rng));
     challenges.push(C::F::random(&mut *rng));
     sigs.push(schnorr::sign::<C>(keys[i], C::F::random(&mut *rng), challenges[i]));
   }
 
   // Batch verify
-  let mut triplets = (0 .. 3).map(
+  let triplets = (0 .. 5).map(
     |i| (u16::try_from(i + 1).unwrap(), C::generator_table() * keys[i], challenges[i], sigs[i])
   ).collect::<Vec<_>>();
   schnorr::batch_verify(rng, &triplets).unwrap();
@@ -59,14 +59,15 @@ pub(crate) fn batch_verify<R: RngCore + CryptoRng, C: Curve>(rng: &mut R) {
       assert!(false);
     }
   }
-  // Sanity
-  schnorr::batch_verify(rng, &triplets).unwrap();
 
   // Make sure a completely invalid signature fails when included
-  triplets[0].3.s = C::F::random(&mut *rng);
-  if let Err(blame) = schnorr::batch_verify(rng, &triplets) {
-    assert_eq!(blame, 1);
-  } else {
-    assert!(false);
+  for i in 0 .. 5 {
+    let mut triplets = triplets.clone();
+    triplets[i].3.s = C::F::random(&mut *rng);
+    if let Err(blame) = schnorr::batch_verify(rng, &triplets) {
+      assert_eq!(blame, u16::try_from(i + 1).unwrap());
+    } else {
+      assert!(false);
+    }
   }
 }
diff --git a/crypto/multiexp/src/lib.rs b/crypto/multiexp/src/lib.rs
index e935bb95..a89a8125 100644
--- a/crypto/multiexp/src/lib.rs
+++ b/crypto/multiexp/src/lib.rs
@@ -106,27 +106,51 @@ impl<Id: Copy, G: Group> BatchVerifier<Id, G> {
 
   pub fn verify(&self) -> bool {
     multiexp(
-      self.0.iter().flat_map(|sets| sets.1.iter()).cloned(),
+      self.0.iter().flat_map(|pairs| pairs.1.iter()).cloned(),
       self.1
     ).is_identity().into()
   }
 
   pub fn verify_vartime(&self) -> bool {
     multiexp_vartime(
-      self.0.iter().flat_map(|sets| sets.1.iter()).cloned(),
+      self.0.iter().flat_map(|pairs| pairs.1.iter()).cloned(),
       self.1
     ).is_identity().into()
   }
 
-  // Solely has a vartime variant as there shouldn't be any reason for this to not be vartime, yet
-  // we should explicitly label vartime software as vartime
-  // TODO: Binary search, or at least randomly sort
+  // A constant time variant may be beneficial for robust protocols
   pub fn blame_vartime(&self) -> Option<Id> {
-    for value in &self.0 {
-      if !bool::from(multiexp_vartime(value.1.clone(), self.1).is_identity()) {
-        return Some(value.0);
+    let mut slice = self.0.as_slice();
+    while slice.len() > 1 {
+      let split = slice.len() / 2;
+      if multiexp_vartime(
+        slice[.. split].iter().flat_map(|pairs| pairs.1.iter()).cloned(),
+        self.1
+      ).is_identity().into() {
+        slice = &slice[split ..];
+      } else {
+        slice = &slice[.. split];
       }
     }
-    None
+
+    slice.get(0).filter(
+      |(_, value)| !bool::from(multiexp_vartime(value.clone(), self.1).is_identity())
+    ).map(|(id, _)| *id)
+  }
+
+  pub fn verify_with_vartime_blame(&self) -> Result<(), Id> {
+    if self.verify() {
+      Ok(())
+    } else {
+      Err(self.blame_vartime().unwrap())
+    }
+  }
+
+  pub fn verify_vartime_with_vartime_blame(&self) -> Result<(), Id> {
+    if self.verify_vartime() {
+      Ok(())
+    } else {
+      Err(self.blame_vartime().unwrap())
+    }
   }
 }