Move FROST to HashMaps

Honestly, the borrowed keys are frustrating, and this probably reduces performance while no longer offering an order when iterating. That said, they enable full u16 indexing and should mildly improve the API. Cleans the Proof of Knowledge handling present in key gen.
2025-12-09 12:49:23 +00:00 · 2022-05-24 21:41:14 -04:00
parent 5ff65bd268
commit d10c6e16dc
12 changed files with 585 additions and 747 deletions
--- a/coins/monero/src/frost.rs
+++ b/coins/monero/src/frost.rs
@@ -28,9 +28,9 @@ pub enum MultisigError {
  #[error("internal error ({0})")]
  InternalError(String),
  #[error("invalid discrete log equality proof")]
-  InvalidDLEqProof(usize),
+  InvalidDLEqProof(u16),
  #[error("invalid key image {0}")]
-  InvalidKeyImage(usize)
+  InvalidKeyImage(u16)
 }

 #[derive(Clone, Copy, PartialEq, Eq, Debug)]
@@ -168,7 +168,7 @@ impl DLEqProof {
  pub fn verify(
    &self,
    H: &DPoint,
-    l: usize,
+    l: u16,
    xG: &DPoint,
    xH: &DPoint
  ) -> Result<(), MultisigError> {
@@ -214,7 +214,7 @@ pub fn read_dleq(
  serialized: &[u8],
  start: usize,
  H: &DPoint,
-  l: usize,
+  l: u16,
  xG: &DPoint
 ) -> Result<dfg::EdwardsPoint, MultisigError> {
  // Not using G_from_slice here would enable non-canonical points and break blame
--- a/coins/monero/src/ringct/clsag/multisig.rs
+++ b/coins/monero/src/ringct/clsag/multisig.rs
@@ -148,13 +148,13 @@ impl Algorithm<Ed25519> for ClsagMultisig {
  fn process_addendum(
    &mut self,
    view: &MultisigView<Ed25519>,
-    l: usize,
+    l: u16,
    commitments: &[dfg::EdwardsPoint; 2],
    serialized: &[u8]
  ) -> Result<(), FrostError> {
    if serialized.len() != ClsagMultisig::serialized_len() {
      // Not an optimal error but...
-      Err(FrostError::InvalidCommitmentQuantity(l, 9, serialized.len() / 32))?;
+      Err(FrostError::InvalidCommitment(l))?;
    }

    if self.AH.0.is_identity().into() {
@@ -163,29 +163,28 @@ impl Algorithm<Ed25519> for ClsagMultisig {
      self.transcript.append_message(b"mask", &self.mask().to_bytes());
    }

-    let share = read_dleq(
+    // Uses the same format FROST does for the expected commitments (nonce * G where this is nonce * H)
+    // The following technically shouldn't need to be committed to, as we've committed to equivalents,
+    // yet it doesn't hurt and may resolve some unknown issues
+    self.transcript.append_message(b"participant", &l.to_be_bytes());
+
+    let mut cursor = 0;
+    self.transcript.append_message(b"image_share", &serialized[cursor .. (cursor + 32)]);
+    self.image += read_dleq(
      serialized,
-      0,
+      cursor,
      &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;
+    cursor += 96;

-    // 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"commitment_D_H", &serialized[cursor .. (cursor + 32)]);
+    self.AH.0 += read_dleq(serialized, cursor, &self.H, l, &commitments[0]).map_err(|_| FrostError::InvalidCommitment(l))?;
+    cursor += 96;

-    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))?;
+    self.transcript.append_message(b"commitment_E_H", &serialized[cursor .. (cursor + 32)]);
+    self.AH.1 += read_dleq(serialized, cursor, &self.H, l, &commitments[1]).map_err(|_| FrostError::InvalidCommitment(l))?;

    Ok(())
  }
--- a/coins/monero/src/tests/clsag.rs
+++ b/coins/monero/src/tests/clsag.rs
@@ -1,5 +1,5 @@
 #[cfg(feature = "multisig")]
-use std::{cell::RefCell, rc::Rc};
+use std::{cell::RefCell, rc::Rc, collections::HashMap};

 use rand::{RngCore, rngs::OsRng};

@@ -71,7 +71,6 @@ fn clsag() {
 #[test]
 fn clsag_multisig() -> Result<(), MultisigError> {
  let (keys, group_private) = generate_keys();
-  let t = keys[0].params().t();

  let randomness = random_scalar(&mut OsRng);
  let mut ring = vec![];
@@ -92,9 +91,10 @@ fn clsag_multisig() -> Result<(), MultisigError> {
  }

  let mask_sum = random_scalar(&mut OsRng);
-  let mut machines = Vec::with_capacity(t);
-  for i in 1 ..= t {
-    machines.push(
+  let mut machines = HashMap::new();
+  for i in 1 ..= THRESHOLD {
+    machines.insert(
+      i,
      sign::AlgorithmMachine::new(
        ClsagMultisig::new(
          Transcript::new(b"Monero Serai CLSAG Test".to_vec()),
@@ -112,15 +112,15 @@ fn clsag_multisig() -> Result<(), MultisigError> {
            )
          )))
        ).unwrap(),
-        Rc::new(keys[i - 1].clone()),
-        &(1 ..= THRESHOLD).collect::<Vec<usize>>()
+        Rc::new(keys[&i].clone()),
+        &(1 ..= THRESHOLD).collect::<Vec<_>>()
      ).unwrap()
    );
  }

  let mut signatures = sign(&mut machines, &[1; 32]);
  let signature = signatures.swap_remove(0);
-  for s in 0 .. (t - 1) {
+  for s in 0 .. usize::from(THRESHOLD - 1) {
    // Verify the commitments and the non-decoy s scalar are identical to every other signature
    // FROST will already have called verify on the produced signature, before checking individual
    // key shares. For FROST Schnorr, it's cheaper. For CLSAG, it may be more expensive? Yet it
--- a/coins/monero/src/tests/frost.rs
+++ b/coins/monero/src/tests/frost.rs
@@ -1,5 +1,7 @@
 #![cfg(feature = "multisig")]

+use std::collections::HashMap;
+
 use rand::rngs::OsRng;

 use ff::Field;
@@ -7,102 +9,87 @@ use dalek_ff_group::{ED25519_BASEPOINT_TABLE, Scalar};

 pub use frost::{
  FrostError, MultisigParams, MultisigKeys,
-  key_gen, algorithm::Algorithm, sign::{self, lagrange}
+  lagrange, key_gen, algorithm::Algorithm, sign
 };

 use crate::frost::Ed25519;

-pub const THRESHOLD: usize = 3;
-pub const PARTICIPANTS: usize = 5;
+pub const THRESHOLD: u16 = 3;
+pub const PARTICIPANTS: u16 = 5;

-pub fn generate_keys() -> (Vec<MultisigKeys<Ed25519>>, Scalar) {
-  let mut params = vec![];
-  let mut machines = vec![];
-  let mut commitments = vec![vec![]];
+fn clone_without<K: Clone + std::cmp::Eq + std::hash::Hash, V: Clone>(
+  map: &HashMap<K, V>,
+  without: &K
+) -> HashMap<K, V> {
+  let mut res = map.clone();
+  res.remove(without).unwrap();
+  res
+}
+
+pub fn generate_keys() -> (HashMap<u16, MultisigKeys<Ed25519>>, Scalar) {
+  let mut params = HashMap::new();
+  let mut machines = HashMap::new();
+  let mut commitments = HashMap::new();
  for i in 1 ..= PARTICIPANTS {
-    params.push(
+    params.insert(
+      i,
      MultisigParams::new(THRESHOLD, PARTICIPANTS, i).unwrap()
    );
-    machines.push(
+    machines.insert(
+      i,
      key_gen::StateMachine::<Ed25519>::new(
-        params[i - 1],
+        params[&i],
        "monero-sign-rs test suite".to_string()
      )
    );
-    commitments.push(machines[i - 1].generate_coefficients(&mut OsRng).unwrap());
+    commitments.insert(i, machines.get_mut(&i).unwrap().generate_coefficients(&mut OsRng).unwrap());
  }

-  let mut secret_shares = vec![];
-  for i in 1 ..= PARTICIPANTS {
-    secret_shares.push(
-      machines[i - 1].generate_secret_shares(
-        &mut OsRng,
-        commitments
-          .iter()
-          .enumerate()
-          .map(|(idx, commitments)| if idx == i { vec![] } else { commitments.to_vec() })
-          .collect()
-      ).unwrap()
+  let mut secret_shares = HashMap::new();
+  for (i, machine) in machines.iter_mut() {
+    secret_shares.insert(
+      *i,
+      machine.generate_secret_shares(&mut OsRng, clone_without(&commitments, i)).unwrap()
    );
  }

-  let mut keys = vec![];
-  for i in 1 ..= PARTICIPANTS {
-    let mut our_secret_shares = vec![vec![]];
-    our_secret_shares.extend(
-      secret_shares.iter().map(|shares| shares[i].clone()).collect::<Vec<Vec<u8>>>()
-    );
-    keys.push(machines[i - 1].complete(our_secret_shares).unwrap().clone());
+  let mut keys = HashMap::new();
+  for (i, machine) in machines.iter_mut() {
+    let mut our_secret_shares = HashMap::new();
+    for (l, shares) in &secret_shares {
+      if i == l {
+        continue;
+      }
+      our_secret_shares.insert(*l, shares[&i].clone());
+    }
+    keys.insert(*i, machine.complete(our_secret_shares).unwrap().clone());
  }

  let mut group_private = Scalar::zero();
  for i in 1 ..= THRESHOLD {
-    group_private += keys[i - 1].secret_share() * lagrange::<Scalar>(
-      i,
-      &(1 ..= THRESHOLD).collect::<Vec<usize>>()
-    );
+    group_private += keys[&i].secret_share() * lagrange::<Scalar>(i, &(1 ..= THRESHOLD).collect::<Vec<_>>());
  }
-  assert_eq!(&ED25519_BASEPOINT_TABLE * group_private, keys[0].group_key());
+  assert_eq!(&ED25519_BASEPOINT_TABLE * group_private, keys[&1].group_key());

  (keys, group_private)
 }

-#[allow(dead_code)] // Currently has some false positive
-pub fn sign<S, M: sign::StateMachine<Signature = S>>(machines: &mut Vec<M>, msg: &[u8]) -> Vec<S> {
-  assert!(machines.len() >= THRESHOLD);
+pub fn sign<S, M: sign::StateMachine<Signature = S>>(machines: &mut HashMap<u16, M>, msg: &[u8]) -> Vec<S> {
+  assert!(machines.len() >= THRESHOLD.into());

-  let mut commitments = Vec::with_capacity(PARTICIPANTS + 1);
-  commitments.resize(PARTICIPANTS + 1, None);
-  for i in 1 ..= THRESHOLD {
-    commitments[i] = Some(machines[i - 1].preprocess(&mut OsRng).unwrap());
+  let mut commitments = HashMap::new();
+  for (i, machine) in machines.iter_mut() {
+    commitments.insert(*i, machine.preprocess(&mut OsRng).unwrap());
  }

-  let mut shares = Vec::with_capacity(PARTICIPANTS + 1);
-  shares.resize(PARTICIPANTS + 1, None);
-  for i in 1 ..= THRESHOLD {
-    shares[i] = Some(
-      machines[i - 1].sign(
-        &commitments
-          .iter()
-          .enumerate()
-          .map(|(idx, value)| if idx == i { None } else { value.to_owned() })
-          .collect::<Vec<Option<Vec<u8>>>>(),
-        msg
-      ).unwrap()
-    );
+  let mut shares = HashMap::new();
+  for (i, machine) in machines.iter_mut() {
+    shares.insert(*i, machine.sign(clone_without(&commitments, i), msg).unwrap());
  }

-  let mut res = Vec::with_capacity(THRESHOLD);
-  for i in 1 ..= THRESHOLD {
-    res.push(
-      machines[i - 1].complete(
-        &shares
-          .iter()
-          .enumerate()
-          .map(|(idx, value)| if idx == i { None } else { value.to_owned() })
-          .collect::<Vec<Option<Vec<u8>>>>()
-      ).unwrap()
-    );
+  let mut res = vec![];
+  for (i, machine) in machines.iter_mut() {
+    res.push(machine.complete(clone_without(&shares, i)).unwrap())
  }
  res
 }
--- a/coins/monero/src/wallet/send/multisig.rs
+++ b/coins/monero/src/wallet/send/multisig.rs
@@ -1,4 +1,4 @@
-use std::{rc::Rc, cell::RefCell};
+use std::{cell::RefCell, rc::Rc, collections::HashMap};

 use rand_core::{RngCore, CryptoRng, SeedableRng};
 use rand_chacha::ChaCha12Rng;
@@ -18,7 +18,8 @@ use crate::{

 pub struct TransactionMachine {
  signable: SignableTransaction,
-  i: usize,
+  i: u16,
+  included: Vec<u16>,
  transcript: Transcript,

  decoys: Vec<Decoys>,
@@ -41,7 +42,7 @@ impl SignableTransaction {
    rpc: &Rpc,
    height: usize,
    keys: MultisigKeys<Ed25519>,
-    included: &[usize]
+    mut included: Vec<u16>
  ) -> Result<TransactionMachine, TransactionError> {
    let mut images = vec![];
    images.resize(self.inputs.len(), EdwardsPoint::identity());
@@ -95,6 +96,9 @@ impl SignableTransaction {
      &self.inputs
    ).await.map_err(|e| TransactionError::RpcError(e))?;

+    // Sort included before cloning it around
+    included.sort_unstable();
+
    for (i, input) in self.inputs.iter().enumerate() {
      clsags.push(
        AlgorithmMachine::new(
@@ -103,7 +107,7 @@ impl SignableTransaction {
            inputs[i].clone()
          ).map_err(|e| TransactionError::MultisigError(e))?,
          Rc::new(keys.offset(dalek_ff_group::Scalar(input.key_offset))),
-          included
+          &included
        ).map_err(|e| TransactionError::FrostError(e))?
      );
    }
@@ -114,6 +118,7 @@ impl SignableTransaction {
    Ok(TransactionMachine {
      signable: self,
      i: keys.params().i(),
+      included,
      transcript,

      decoys,
@@ -142,12 +147,9 @@ impl StateMachine for TransactionMachine {
    }

    // Iterate over each CLSAG calling preprocess
-    let mut serialized = vec![];
-    for (i, clsag) in self.clsags.iter_mut().enumerate() {
-      let preprocess = clsag.preprocess(rng)?;
-      // First 64 bytes are FROST's commitments
-      self.images[i] = CompressedEdwardsY(preprocess[64 .. 96].try_into().unwrap()).decompress().unwrap();
-      serialized.extend(&preprocess);
+    let mut serialized = Vec::with_capacity(self.clsags.len() * (64 + ClsagMultisig::serialized_len()));
+    for clsag in self.clsags.iter_mut() {
+      serialized.extend(&clsag.preprocess(rng)?);
    }
    self.our_preprocess = serialized.clone();

@@ -165,7 +167,7 @@ impl StateMachine for TransactionMachine {

  fn sign(
    &mut self,
-    commitments: &[Option<Vec<u8>>],
+    mut commitments: HashMap<u16, Vec<u8>>,
    // Drop FROST's 'msg' since we calculate the actual message in this function
    _: &[u8]
  ) -> Result<Vec<u8>, FrostError> {
@@ -177,29 +179,32 @@ impl StateMachine for TransactionMachine {
    // While each CLSAG will do this as they need to for security, they have their own transcripts
    // cloned from this TX's initial premise's transcript. For our TX transcript to have the CLSAG
    // data for entropy, it'll have to be added ourselves
-    for c in 0 .. commitments.len() {
-      self.transcript.append_message(b"participant", &u16::try_from(c).unwrap().to_le_bytes());
-      if c == self.i {
-        self.transcript.append_message(b"preprocess", &self.our_preprocess);
-      } else if let Some(commitments) = commitments[c].as_ref() {
-        self.transcript.append_message(b"preprocess", commitments);
+    commitments.insert(self.i, self.our_preprocess.clone());
+    for l in &self.included {
+      self.transcript.append_message(b"participant", &(*l).to_be_bytes());
+      // FROST itself will error if this is None, so let it
+      if let Some(preprocess) = commitments.get(l) {
+        self.transcript.append_message(b"preprocess", preprocess);
      }
    }

    // FROST commitments, image, H commitments, and their proofs
    let clsag_len = 64 + ClsagMultisig::serialized_len();

+    let mut commitments = (0 .. self.clsags.len()).map(|c| commitments.iter().map(
+      |(l, commitments)| (*l, commitments[(c * clsag_len) .. ((c + 1) * clsag_len)].to_vec())
+    ).collect::<HashMap<_, _>>()).collect::<Vec<_>>();
+
    for c in 0 .. self.clsags.len() {
      // Calculate the key images
      // Multisig will parse/calculate/validate this as needed, yet doing so here as well provides
      // the easiest API overall, as this is where the TX is (which needs the key images in its
      // message), along with where the outputs are determined (where our change output needs these
      // to be unique)
-      for (l, serialized) in commitments.iter().enumerate().filter(|(_, s)| s.is_some()) {
+      for (l, preprocess) in &commitments[c] {
        self.images[c] += CompressedEdwardsY(
-          serialized.as_ref().unwrap()[((c * clsag_len) + 64) .. ((c * clsag_len) + 96)]
-            .try_into().map_err(|_| FrostError::InvalidCommitment(l))?
-        ).decompress().ok_or(FrostError::InvalidCommitment(l))?;
+          preprocess[64 .. 96].try_into().map_err(|_| FrostError::InvalidCommitment(*l))?
+        ).decompress().ok_or(FrostError::InvalidCommitment(*l))?;
      }
    }

@@ -231,12 +236,6 @@ impl StateMachine for TransactionMachine {
      )
    };

-    let mut commitments = (0 .. self.inputs.len()).map(|c| commitments.iter().map(
-      |commitments| commitments.clone().map(
-        |commitments| commitments[(c * clsag_len) .. ((c * clsag_len) + clsag_len)].to_vec()
-      )
-    ).collect::<Vec<_>>()).collect::<Vec<_>>();
-
    let mut sorted = Vec::with_capacity(self.decoys.len());
    while self.decoys.len() != 0 {
      sorted.push((
@@ -291,14 +290,14 @@ impl StateMachine for TransactionMachine {

    // Iterate over each CLSAG calling sign
    let mut serialized = Vec::with_capacity(self.clsags.len() * 32);
-    for (c, clsag) in self.clsags.iter_mut().enumerate() {
-      serialized.extend(&clsag.sign(&commitments[c], &msg)?);
+    for clsag in self.clsags.iter_mut() {
+      serialized.extend(&clsag.sign(commitments.remove(0), &msg)?);
    }

    Ok(serialized)
  }

-  fn complete(&mut self, shares: &[Option<Vec<u8>>]) -> Result<Transaction, FrostError> {
+  fn complete(&mut self, shares: HashMap<u16, Vec<u8>>) -> Result<Transaction, FrostError> {
    if self.state() != State::Signed {
      Err(FrostError::InvalidSignTransition(State::Signed, self.state()))?;
    }
@@ -308,9 +307,9 @@ impl StateMachine for TransactionMachine {
      RctPrunable::Null => panic!("Signing for RctPrunable::Null"),
      RctPrunable::Clsag { ref mut clsags, ref mut pseudo_outs, .. } => {
        for (c, clsag) in self.clsags.iter_mut().enumerate() {
-          let (clsag, pseudo_out) = clsag.complete(&shares.iter().map(
-            |share| share.clone().map(|share| share[(c * 32) .. ((c * 32) + 32)].to_vec())
-          ).collect::<Vec<_>>())?;
+          let (clsag, pseudo_out) = clsag.complete(shares.iter().map(
+            |(l, shares)| (*l, shares[(c * 32) .. ((c + 1) * 32)].to_vec())
+          ).collect::<HashMap<_, _>>())?;
          clsags.push(clsag);
          pseudo_outs.push(pseudo_out);
        }