Simplify Monero key image handling

coins/monero/src/clsag/multisig.rs

@@ -16,13 +16,12 @@ use monero::util::ringct::{Key, Clsag};
 use group::Group;
 
 use transcript::Transcript as TranscriptTrait;
-use frost::{Curve, FrostError, algorithm::Algorithm, MultisigView};
+use frost::{FrostError, algorithm::Algorithm, MultisigView};
 use dalek_ff_group as dfg;
 
 use crate::{
   hash_to_point,
-  frost::{Transcript, MultisigError, Ed25519, DLEqProof},
-  key_image,
+  frost::{Transcript, MultisigError, Ed25519, DLEqProof, read_dleq},
   clsag::{Input, sign_core, verify}
 };
 
@@ -80,8 +79,9 @@ struct Interim {
 pub struct Multisig {
   transcript: Transcript,
 
+  H: EdwardsPoint,
+  // Merged here as CLSAG needs it, passing it would be a mess, yet having it beforehand requires a round
   image: EdwardsPoint,
-  commitments_H: Vec<u8>,
   AH: (dfg::EdwardsPoint, dfg::EdwardsPoint),
 
   details: Rc<RefCell<Option<Details>>>,
@@ -100,8 +100,8 @@ impl Multisig {
       Multisig {
         transcript,
 
+        H: EdwardsPoint::identity(),
         image: EdwardsPoint::identity(),
-        commitments_H: vec![],
         AH: (dfg::EdwardsPoint::identity(), dfg::EdwardsPoint::identity()),
 
         details,
@@ -134,24 +134,21 @@ impl Algorithm<Ed25519> for Multisig {
   type Signature = (Clsag, EdwardsPoint);
 
   fn preprocess_addendum<R: RngCore + CryptoRng>(
+    &mut self,
     rng: &mut R,
     view: &MultisigView<Ed25519>,
     nonces: &[dfg::Scalar; 2]
   ) -> Vec<u8> {
-    let (share, proof) = key_image::generate_share(rng, view);
-
-    #[allow(non_snake_case)]
-    let H = hash_to_point(&view.group_key().0);
-    #[allow(non_snake_case)]
-    let nH = (nonces[0].0 * H, nonces[1].0 * H);
+    self.H = hash_to_point(&view.group_key().0);
 
     let mut serialized = Vec::with_capacity(Multisig::serialized_len());
-    serialized.extend(share.compress().to_bytes());
-    serialized.extend(nH.0.compress().to_bytes());
-    serialized.extend(nH.1.compress().to_bytes());
-    serialized.extend(&DLEqProof::prove(rng, &nonces[0].0, &H, &nH.0).serialize());
-    serialized.extend(&DLEqProof::prove(rng, &nonces[1].0, &H, &nH.1).serialize());
-    serialized.extend(proof);
+    serialized.extend((view.secret_share().0 * self.H).compress().to_bytes());
+    serialized.extend(DLEqProof::prove(rng, &view.secret_share().0, &self.H).serialize());
+
+    serialized.extend((nonces[0].0 * self.H).compress().to_bytes());
+    serialized.extend(&DLEqProof::prove(rng, &nonces[0].0, &self.H).serialize());
+    serialized.extend((nonces[1].0 * self.H).compress().to_bytes());
+    serialized.extend(&DLEqProof::prove(rng, &nonces[1].0, &self.H).serialize());
     serialized
   }
 
@@ -167,49 +164,36 @@ impl Algorithm<Ed25519> for Multisig {
       Err(FrostError::InvalidCommitmentQuantity(l, 9, serialized.len() / 32))?;
     }
 
-    if self.commitments_H.len() == 0 {
+    if self.AH.0.is_identity().into() {
       self.transcript.domain_separate(b"CLSAG");
       self.input().transcript(&mut self.transcript);
       self.transcript.append_message(b"mask", &self.mask().to_bytes());
       self.transcript.append_message(b"message", &self.msg());
     }
 
-    let (share, serialized) = key_image::verify_share(view, l, serialized).map_err(|_| FrostError::InvalidShare(l))?;
+    let share = read_dleq(
+      serialized,
+      0,
+      &self.H,
+      l,
+      &view.verification_share(l).0
+    ).map_err(|_| FrostError::InvalidCommitment(l))?.0;
     // Given the fact there's only ever one possible value for this, this may technically not need
     // to be committed to. If signing a TX, it'll be double committed to thanks to the message
     // It doesn't hurt to have though and ensures security boundaries are well formed
     self.transcript.append_message(b"image_share", &share.compress().to_bytes());
     self.image += share;
 
-    let alt = &hash_to_point(&view.group_key().0);
-
     // Uses the same format FROST does for the expected commitments (nonce * G where this is nonce * H)
     // Given this is guaranteed to match commitments, which FROST commits to, this also technically
     // doesn't need to be committed to if a canonical serialization is guaranteed
     // It, again, doesn't hurt to include and ensures security boundaries are well formed
     self.transcript.append_message(b"participant", &u16::try_from(l).unwrap().to_be_bytes());
-    self.transcript.append_message(b"commitments_H", &serialized[0 .. 64]);
 
-    #[allow(non_snake_case)]
-    let H = (
-      <Ed25519 as Curve>::G_from_slice(&serialized[0 .. 32]).map_err(|_| FrostError::InvalidCommitment(l))?,
-      <Ed25519 as Curve>::G_from_slice(&serialized[32 .. 64]).map_err(|_| FrostError::InvalidCommitment(l))?
-    );
-
-    DLEqProof::deserialize(&serialized[64 .. 128]).ok_or(FrostError::InvalidCommitment(l))?.verify(
-      &alt,
-      &commitments[0],
-      &H.0
-    ).map_err(|_| FrostError::InvalidCommitment(l))?;
-
-    DLEqProof::deserialize(&serialized[128 .. 192]).ok_or(FrostError::InvalidCommitment(l))?.verify(
-      &alt,
-      &commitments[1],
-      &H.1
-    ).map_err(|_| FrostError::InvalidCommitment(l))?;
-
-    self.AH.0 += H.0;
-    self.AH.1 += H.1;
+    self.transcript.append_message(b"commitment_D_H", &serialized[0 .. 32]);
+    self.AH.0 += read_dleq(serialized, 96, &self.H, l, &commitments[0]).map_err(|_| FrostError::InvalidCommitment(l))?;
+    self.transcript.append_message(b"commitment_E_H", &serialized[0 .. 32]);
+    self.AH.1 += read_dleq(serialized, 192, &self.H, l, &commitments[1]).map_err(|_| FrostError::InvalidCommitment(l))?;
 
     Ok(())
   }

coins/monero/src/frost.rs

@@ -12,7 +12,6 @@ use curve25519_dalek::{
   edwards::EdwardsPoint as DPoint
 };
 
-
 use ff::PrimeField;
 use group::Group;
 
@@ -29,7 +28,7 @@ pub enum MultisigError {
   #[error("internal error ({0})")]
   InternalError(String),
   #[error("invalid discrete log equality proof")]
-  InvalidDLEqProof,
+  InvalidDLEqProof(usize),
   #[error("invalid key image {0}")]
   InvalidKeyImage(usize)
 }
@@ -88,15 +87,17 @@ impl Curve for Ed25519 {
   }
 
   fn G_from_slice(slice: &[u8]) -> Result<Self::G, CurveError> {
-    let point = dfg::CompressedEdwardsY::new(
-      slice.try_into().map_err(|_| CurveError::InvalidLength(32, slice.len()))?
-    ).decompress();
+    let bytes = slice.try_into().map_err(|_| CurveError::InvalidLength(32, slice.len()))?;
+    let point = dfg::CompressedEdwardsY::new(bytes).decompress();
 
-    if point.is_some() {
-      let point = point.unwrap();
+    if let Some(point) = point {
       // Ban torsioned points
       if !point.is_torsion_free() {
-        Err(CurveError::InvalidPoint)?
+        Err(CurveError::InvalidPoint)?;
+      }
+      // Ban point which weren't canonically encoded
+      if point.compress().to_bytes() != bytes {
+        Err(CurveError::InvalidPoint)?;
       }
       Ok(point)
     } else {
@@ -113,7 +114,7 @@ impl Curve for Ed25519 {
   }
 }
 
-// Used to prove legitimacy in several locations
+// Used to prove legitimacy of key images and nonces which both involve other basepoints
 #[derive(Clone)]
 pub struct DLEqProof {
   s: DScalar,
@@ -125,19 +126,23 @@ impl DLEqProof {
   pub fn prove<R: RngCore + CryptoRng>(
     rng: &mut R,
     secret: &DScalar,
-    H: &DPoint,
-    alt: &DPoint
+    H: &DPoint
   ) -> DLEqProof {
     let r = random_scalar(rng);
-    let R1 =  &DTable * &r;
-    let R2 = r * H;
+    let rG = &DTable * &r;
+    let rH = r * H;
 
+    // TODO: Should this use a transcript?
     let c = dfg::Scalar::from_hash(
       Blake2b512::new()
-        .chain(R1.compress().to_bytes())
-        .chain(R2.compress().to_bytes())
+        // Doesn't include G which is constant, does include H which isn't
+        .chain(H.compress().to_bytes())
         .chain((secret * &DTable).compress().to_bytes())
-        .chain(alt.compress().to_bytes())
+        // We can frequently save a scalar mul if we accept this as an arg, yet it opens room for
+        // ambiguity not worth having
+        .chain((secret * H).compress().to_bytes())
+        .chain(rG.compress().to_bytes())
+        .chain(rH.compress().to_bytes())
     ).0;
     let s = r + (c * secret);
 
@@ -147,26 +152,28 @@ impl DLEqProof {
   pub fn verify(
     &self,
     H: &DPoint,
-    primary: &DPoint,
-    alt: &DPoint
-) -> Result<(), MultisigError> {
+    l: usize,
+    sG: &DPoint,
+    sH: &DPoint
+  ) -> Result<(), MultisigError> {
     let s = self.s;
     let c = self.c;
 
-    let R1 = (&s * &DTable) - (c * primary);
-    let R2 = (s * H) - (c * alt);
+    let rG = (&s * &DTable) - (c * sG);
+    let rH = (s * H) - (c * sH);
 
     let expected_c = dfg::Scalar::from_hash(
       Blake2b512::new()
-        .chain(R1.compress().to_bytes())
-        .chain(R2.compress().to_bytes())
-        .chain(primary.compress().to_bytes())
-        .chain(alt.compress().to_bytes())
+        .chain(H.compress().to_bytes())
+        .chain(sG.compress().to_bytes())
+        .chain(sH.compress().to_bytes())
+        .chain(rG.compress().to_bytes())
+        .chain(rH.compress().to_bytes())
     ).0;
 
-    // Take the opportunity to ensure a lack of torsion in key images/randomness commitments
-    if (!primary.is_torsion_free()) || (!alt.is_torsion_free()) || (c != expected_c) {
-      Err(MultisigError::InvalidDLEqProof)?;
+    // Take the opportunity to ensure a lack of torsion in key images/nonce commitments
+    if c != expected_c {
+      Err(MultisigError::InvalidDLEqProof(l))?;
     }
 
     Ok(())
@@ -196,3 +203,29 @@ impl DLEqProof {
     )
   }
 }
+
+#[allow(non_snake_case)]
+pub fn read_dleq(
+  serialized: &[u8],
+  start: usize,
+  H: &DPoint,
+  l: usize,
+  sG: &DPoint
+) -> Result<dfg::EdwardsPoint, MultisigError> {
+  // Not using G_from_slice here would enable non-canonical points and break blame
+  let other = <Ed25519 as Curve>::G_from_slice(
+    &serialized[(start + 0) .. (start + 32)]
+  ).map_err(|_| MultisigError::InvalidDLEqProof(l))?;
+
+  let proof = DLEqProof::deserialize(
+    &serialized[(start + 32) .. (start + 96)]
+  ).ok_or(MultisigError::InvalidDLEqProof(l))?;
+  proof.verify(
+    H,
+    l,
+    sG,
+    &other
+  ).map_err(|_| MultisigError::InvalidDLEqProof(l))?;
+
+  Ok(other)
+}

coins/monero/src/key_image/mod.rs

@@ -1,16 +0,0 @@
-use curve25519_dalek::{
-  constants::ED25519_BASEPOINT_TABLE,
-  scalar::Scalar,
-  edwards::EdwardsPoint
-};
-
-use crate::hash_to_point;
-
-#[cfg(feature = "multisig")]
-mod multisig;
-#[cfg(feature = "multisig")]
-pub use crate::key_image::multisig::{generate_share, verify_share};
-
-pub fn generate(secret: &Scalar) -> EdwardsPoint {
-  secret * hash_to_point(&(secret * &ED25519_BASEPOINT_TABLE))
-}

coins/monero/src/key_image/multisig.rs

@@ -1,48 +0,0 @@
-use rand_core::{RngCore, CryptoRng};
-
-use curve25519_dalek::edwards::{EdwardsPoint, CompressedEdwardsY};
-
-use frost::MultisigView;
-
-use crate::{hash_to_point, frost::{MultisigError, Ed25519, DLEqProof}};
-
-#[allow(non_snake_case)]
-pub fn generate_share<R: RngCore + CryptoRng>(
-  rng: &mut R,
-  view: &MultisigView<Ed25519>
-) -> (EdwardsPoint, Vec<u8>) {
-  let H = hash_to_point(&view.group_key().0);
-  let image = view.secret_share().0 * H;
-  // Includes a proof. Since:
-  // sum(lagranged_secrets) = group_private
-  // group_private * G = output_key
-  // group_private * H = key_image
-  // Then sum(lagranged_secrets * H) = key_image
-  // lagranged_secret * G is known. lagranged_secret * H is being sent
-  // Any discrete log equality proof confirms the same secret was used,
-  // forming a valid key_image share
-  (image, DLEqProof::prove(rng, &view.secret_share().0, &H, &image).serialize())
-}
-
-pub fn verify_share(
-  view: &MultisigView<Ed25519>,
-  l: usize,
-  share: &[u8]
-) -> Result<(EdwardsPoint, Vec<u8>), MultisigError> {
-  if share.len() < 96 {
-    Err(MultisigError::InvalidDLEqProof)?;
-  }
-  let image = CompressedEdwardsY(
-    share[0 .. 32].try_into().unwrap()
-  ).decompress().ok_or(MultisigError::InvalidKeyImage(l))?;
-  let proof = DLEqProof::deserialize(
-    &share[(share.len() - 64) .. share.len()]
-  ).ok_or(MultisigError::InvalidDLEqProof)?;
-  proof.verify(
-    &hash_to_point(&view.group_key().0),
-    &view.verification_share(l),
-    &image
-  ).map_err(|_| MultisigError::InvalidKeyImage(l))?;
-
-  Ok((image, share[32 .. (share.len() - 64)].to_vec()))
-}

coins/monero/src/lib.rs

@@ -15,7 +15,6 @@ use monero::util::key::H;
 #[cfg(feature = "multisig")]
 pub mod frost;
 
-pub mod key_image;
 pub mod bulletproofs;
 pub mod clsag;
 
@@ -76,3 +75,7 @@ pub fn hash_to_point(point: &EdwardsPoint) -> EdwardsPoint {
   unsafe { c_hash_to_point(bytes.as_mut_ptr()); }
   CompressedEdwardsY::from_slice(&bytes).decompress().unwrap()
 }
+
+pub fn generate_key_image(secret: &Scalar) -> EdwardsPoint {
+  secret * hash_to_point(&(secret * &ED25519_BASEPOINT_TABLE))
+}

coins/monero/src/transaction/mod.rs

@@ -31,7 +31,7 @@ use crate::{
   Commitment,
   random_scalar,
   hash, hash_to_scalar,
-  key_image, bulletproofs, clsag,
+  generate_key_image, bulletproofs, clsag,
   rpc::{Rpc, RpcError}
 };
 #[cfg(feature = "multisig")]
@@ -215,7 +215,7 @@ async fn prepare_inputs<R: RngCore + CryptoRng>(
   for (i, input) in inputs.iter().enumerate() {
     signable.push((
       spend + input.key_offset,
-      key_image::generate(&(spend + input.key_offset)),
+      generate_key_image(&(spend + input.key_offset)),
       clsag::Input::new(
         input.commitment,
         decoys[i].clone()

coins/monero/src/transaction/multisig.rs

@@ -3,7 +3,7 @@ use std::{rc::Rc, cell::RefCell};
 use rand_core::{RngCore, CryptoRng, SeedableRng};
 use rand_chacha::ChaCha12Rng;
 
-use curve25519_dalek::{scalar::Scalar, edwards::{EdwardsPoint, CompressedEdwardsY}};
+use curve25519_dalek::{traits::Identity, scalar::Scalar, edwards::{EdwardsPoint, CompressedEdwardsY}};
 
 use monero::{
   Hash, VarInt,
@@ -17,7 +17,7 @@ use frost::{FrostError, MultisigKeys, MultisigParams, sign::{State, StateMachine
 
 use crate::{
   frost::{Transcript, Ed25519},
-  random_scalar, key_image, bulletproofs, clsag,
+  random_scalar, bulletproofs, clsag,
   rpc::Rpc,
   transaction::{TransactionError, SignableTransaction, decoys::{self, Decoys}}
 };
@@ -49,6 +49,7 @@ impl SignableTransaction {
     included: &[usize]
   ) -> Result<TransactionMachine, TransactionError> {
     let mut our_images = vec![];
+    our_images.resize(self.inputs.len(), EdwardsPoint::identity());
     let mut inputs = vec![];
     inputs.resize(self.inputs.len(), Rc::new(RefCell::new(None)));
     let msg = Rc::new(RefCell::new(None));
@@ -91,13 +92,6 @@ impl SignableTransaction {
     ).await.map_err(|e| TransactionError::RpcError(e))?;
 
     for (i, input) in self.inputs.iter().enumerate() {
-      let keys = keys.offset(dalek_ff_group::Scalar(input.key_offset));
-      let (image, _) = key_image::generate_share(
-        rng,
-        &keys.view(included).map_err(|e| TransactionError::FrostError(e))?
-      );
-      our_images.push(image);
-
       clsags.push(
         AlgorithmMachine::new(
           clsag::Multisig::new(
@@ -105,7 +99,7 @@ impl SignableTransaction {
             inputs[i].clone(),
             msg.clone()
           ).map_err(|e| TransactionError::MultisigError(e))?,
-          Rc::new(keys),
+          Rc::new(keys.offset(dalek_ff_group::Scalar(input.key_offset))),
           included
         ).map_err(|e| TransactionError::FrostError(e))?
       );
@@ -145,8 +139,10 @@ impl StateMachine for TransactionMachine {
 
     // Iterate over each CLSAG calling preprocess
     let mut serialized = vec![];
-    for clsag in self.clsags.iter_mut() {
-      serialized.extend(&clsag.preprocess(rng)?);
+    for (i, clsag) in self.clsags.iter_mut().enumerate() {
+      let preprocess = clsag.preprocess(rng)?;
+      self.our_images[i] += CompressedEdwardsY(preprocess[0 .. 32].try_into().unwrap()).decompress().unwrap();
+      serialized.extend(&preprocess);
     }
 
     if self.leader {

coins/monero/tests/clsag.rs

@@ -7,7 +7,7 @@ use curve25519_dalek::{constants::ED25519_BASEPOINT_TABLE, scalar::Scalar};
 
 use monero::VarInt;
 
-use monero_serai::{random_scalar, Commitment, transaction::decoys::Decoys, key_image, clsag};
+use monero_serai::{Commitment, random_scalar, generate_key_image, transaction::decoys::Decoys, clsag};
 #[cfg(feature = "multisig")]
 use monero_serai::frost::{MultisigError, Transcript};
 
@@ -42,7 +42,7 @@ fn clsag() {
       ring.push([&dest * &ED25519_BASEPOINT_TABLE, Commitment::new(mask, amount).calculate()]);
     }
 
-    let image = key_image::generate(&secrets[0]);
+    let image = generate_key_image(&secrets[0]);
     let (clsag, pseudo_out) = clsag::sign(
       &mut OsRng,
       &vec![(

coins/monero/tests/key_image.rs

@@ -1,51 +0,0 @@
-#![cfg(feature = "multisig")]
-
-use rand::{RngCore, rngs::OsRng};
-
-use curve25519_dalek::{traits::Identity, edwards::EdwardsPoint};
-
-use monero_serai::key_image;
-
-mod frost;
-use crate::frost::{THRESHOLD, PARTICIPANTS, generate_keys};
-
-#[test]
-fn key_image() {
-  let (keys, group_private) = generate_keys();
-  let image = key_image::generate(&group_private);
-
-  let mut included = (1 ..= PARTICIPANTS).into_iter().collect::<Vec<usize>>();
-  while included.len() > THRESHOLD {
-    included.swap_remove((OsRng.next_u64() as usize) % included.len());
-  }
-  included.sort();
-
-  let mut views = vec![];
-  let mut shares = vec![];
-  for i in 1 ..= PARTICIPANTS {
-    if included.contains(&i) {
-      // If they were included, include their view
-      views.push(keys[i - 1].view(&included).unwrap());
-      let share = key_image::generate_share(&mut OsRng, &views[i - 1]);
-      let mut serialized = share.0.compress().to_bytes().to_vec();
-      serialized.extend(b"abc");
-      serialized.extend(&share.1);
-      shares.push(serialized);
-    } else {
-      // If they weren't included, include dummy data to fill the Vec
-      // Uses the view of someone actually included as Params::new verifies inclusion
-      views.push(keys[included[0] - 1].view(&included).unwrap());
-      shares.push(vec![]);
-    }
-  }
-
-  for i in &included {
-    let mut multi_image = EdwardsPoint::identity();
-    for l in &included {
-      let share = key_image::verify_share(&views[i - 1], *l, &shares[l - 1]).unwrap();
-      assert_eq!(share.1, b"abc");
-      multi_image += share.0;
-    }
-    assert_eq!(image, multi_image);
-  }
-}

crypto/dalek-ff-group/src/lib.rs

@@ -13,7 +13,7 @@ pub use curve25519_dalek as dalek;
 
 use dalek::{
   constants,
-  traits::Identity,
+  traits::{Identity, IsIdentity},
   scalar::Scalar as DScalar,
   edwards::{
     EdwardsPoint as DPoint,
@@ -248,7 +248,7 @@ impl Group for EdwardsPoint {
   fn random(mut _rng: impl RngCore) -> Self { unimplemented!() }
   fn identity() -> Self { Self(DPoint::identity()) }
   fn generator() -> Self { ED25519_BASEPOINT_POINT }
-  fn is_identity(&self) -> Choice { unimplemented!() }
+  fn is_identity(&self) -> Choice { (self.0.is_identity() as u8).into() }
   fn double(&self) -> Self { *self + self }
 }
 

crypto/frost/src/algorithm.rs

@@ -18,6 +18,7 @@ pub trait Algorithm<C: Curve>: Clone {
 
   /// Generate an addendum to FROST"s preprocessing stage
   fn preprocess_addendum<R: RngCore + CryptoRng>(
+    &mut self,
     rng: &mut R,
     params: &MultisigView<C>,
     nonces: &[C::F; 2],
@@ -119,6 +120,7 @@ impl<C: Curve, H: Hram<C>> Algorithm<C> for Schnorr<C, H> {
   }
 
   fn preprocess_addendum<R: RngCore + CryptoRng>(
+    &mut self,
     _: &mut R,
     _: &MultisigView<C>,
     _: &[C::F; 2],

crypto/frost/src/sign.rs

@@ -104,7 +104,7 @@ fn preprocess<R: RngCore + CryptoRng, C: Curve, A: Algorithm<C>>(
   serialized.extend(&C::G_to_bytes(&commitments[1]));
 
   serialized.extend(
-    &A::preprocess_addendum(
+    &params.algorithm.preprocess_addendum(
       rng,
       &params.view,
       &nonces