Monero: add more legacy verify functions (#383)

* Add v1 ring sig verifying

* allow calculating signature hash for v1 txs

* add unreduced scalar type with recovery

I have added this type for borromen sigs, the ee field can be a normal
scalar as in the verify function the ee
field is checked against a reduced scalar mean for it to verify as
correct ee must be reduced

* change block major/ minor versions to u8

this matches Monero

I have also changed a couple varint functions to accept the `VarInt`
trait

* expose `serialize_hashable` on `Block`

* add back MLSAG verifying functions

I still need to revert the commit removing support for >1 input MLSAG FULL

This adds a new rct type to separate Full and simple rct

* add back support for multiple inputs for RCT FULL

* comment `non_adjacent_form` function

also added `#[allow(clippy::needless_range_loop)]` around a loop as without a re-write satisfying clippy without it will make the function worse.

* Improve Mlsag verifying API

* fix rebase errors

* revert the changes on `reserialize_chain`
plus other misc changes

* fix no-std

* Reduce the amount of rpc calls needed for `get_block_by_number`.
This function was causing me problems, every now and then a node would return a block with a different number than requested.

* change `serialize_hashable` to give the POW hashing blob.

Monero calculates the POW hash and the block hash using *slightly* different blobs :/

* make ring_signatures public and add length check when verifying.

* Misc improvements and bug fixes

---------

Co-authored-by: Luke Parker <lukeparker5132@gmail.com>

coins/monero/src/bin/reserialize_chain.rs

@@ -129,7 +129,9 @@ mod binaries {
         // Accordingly, making sure our signature_hash algorithm is correct is great, and further
         // making sure the verification functions are valid is appreciated
         match tx.rct_signatures.prunable {
-          RctPrunable::Null | RctPrunable::MlsagBorromean { .. } => {}
+          RctPrunable::Null |
+          RctPrunable::AggregateMlsagBorromean { .. } |
+          RctPrunable::MlsagBorromean { .. } => {}
           RctPrunable::MlsagBulletproofs { bulletproofs, .. } => {
             assert!(bulletproofs.batch_verify(
               &mut rand_core::OsRng,

coins/monero/src/block.rs

@@ -17,8 +17,8 @@ const EXISTING_BLOCK_HASH_202612: [u8; 32] =
 
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct BlockHeader {
-  pub major_version: u64,
+  pub major_version: u8,
-  pub minor_version: u64,
+  pub minor_version: u8,
   pub timestamp: u64,
   pub previous: [u8; 32],
   pub nonce: u32,
@@ -68,7 +68,7 @@ impl Block {
   pub fn write<W: Write>(&self, w: &mut W) -> io::Result<()> {
     self.header.write(w)?;
     self.miner_tx.write(w)?;
-    write_varint(&self.txs.len().try_into().unwrap(), w)?;
+    write_varint(&self.txs.len(), w)?;
     for tx in &self.txs {
       w.write_all(tx)?;
     }
@@ -79,20 +79,27 @@ impl Block {
     merkle_root(self.miner_tx.hash(), &self.txs)
   }
 
-  fn serialize_hashable(&self) -> Vec<u8> {
+  /// Serialize the block as required for the proof of work hash.
+  ///
+  /// This is distinct from the serialization required for the block hash. To get the block hash,
+  /// use the [`Block::hash`] function.
+  pub fn serialize_hashable(&self) -> Vec<u8> {
     let mut blob = self.header.serialize();
     blob.extend_from_slice(&self.tx_merkle_root());
     write_varint(&(1 + u64::try_from(self.txs.len()).unwrap()), &mut blob).unwrap();
 
-    let mut out = Vec::with_capacity(8 + blob.len());
+    blob
-    write_varint(&u64::try_from(blob.len()).unwrap(), &mut out).unwrap();
-    out.append(&mut blob);
-
-    out
   }
 
   pub fn hash(&self) -> [u8; 32] {
-    let hash = hash(&self.serialize_hashable());
+    let mut hashable = self.serialize_hashable();
+    // Monero pre-appends a VarInt of the block hashing blobs length before getting the block hash
+    // but doesn't do this when getting the proof of work hash :)
+    let mut hashing_blob = Vec::with_capacity(8 + hashable.len());
+    write_varint(&u64::try_from(hashable.len()).unwrap(), &mut hashing_blob).unwrap();
+    hashing_blob.append(&mut hashable);
+
+    let hash = hash(&hashing_blob);
     if hash == CORRECT_BLOCK_HASH_202612 {
       return EXISTING_BLOCK_HASH_202612;
     };
@@ -110,7 +117,7 @@ impl Block {
     Ok(Block {
       header: BlockHeader::read(r)?,
       miner_tx: Transaction::read(r)?,
-      txs: (0 .. read_varint(r)?).map(|_| read_bytes(r)).collect::<Result<_, _>>()?,
+      txs: (0_usize .. read_varint(r)?).map(|_| read_bytes(r)).collect::<Result<_, _>>()?,
     })
   }
 }

coins/monero/src/lib.rs

@@ -23,6 +23,12 @@ mod merkle;
 mod serialize;
 use serialize::{read_byte, read_u16};
 
+/// UnreducedScalar struct with functionality for recovering incorrectly reduced scalars.
+mod unreduced_scalar;
+
+/// Ring Signature structs and functionality.
+pub mod ring_signatures;
+
 /// RingCT structs and functionality.
 pub mod ringct;
 use ringct::RctType;

coins/monero/src/ring_signatures.rs

@@ -0,0 +1,72 @@
+use std_shims::{
+  io::{self, *},
+  vec::Vec,
+};
+
+use zeroize::Zeroize;
+
+use curve25519_dalek::{EdwardsPoint, Scalar};
+
+use monero_generators::hash_to_point;
+
+use crate::{serialize::*, hash_to_scalar};
+
+#[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
+pub struct Signature {
+  c: Scalar,
+  r: Scalar,
+}
+
+impl Signature {
+  pub fn write<W: Write>(&self, w: &mut W) -> io::Result<()> {
+    write_scalar(&self.c, w)?;
+    write_scalar(&self.r, w)?;
+    Ok(())
+  }
+
+  pub fn read<R: Read>(r: &mut R) -> io::Result<Signature> {
+    Ok(Signature { c: read_scalar(r)?, r: read_scalar(r)? })
+  }
+}
+
+#[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
+pub struct RingSignature {
+  sigs: Vec<Signature>,
+}
+
+impl RingSignature {
+  pub fn write<W: Write>(&self, w: &mut W) -> io::Result<()> {
+    for sig in &self.sigs {
+      sig.write(w)?;
+    }
+    Ok(())
+  }
+
+  pub fn read<R: Read>(members: usize, r: &mut R) -> io::Result<RingSignature> {
+    Ok(RingSignature { sigs: read_raw_vec(Signature::read, members, r)? })
+  }
+
+  pub fn verify(&self, msg: &[u8; 32], ring: &[EdwardsPoint], key_image: &EdwardsPoint) -> bool {
+    if ring.len() != self.sigs.len() {
+      return false;
+    }
+
+    let mut buf = Vec::with_capacity(32 + (32 * 2 * ring.len()));
+    buf.extend_from_slice(msg);
+
+    let mut sum = Scalar::ZERO;
+
+    for (ring_member, sig) in ring.iter().zip(&self.sigs) {
+      #[allow(non_snake_case)]
+      let Li = EdwardsPoint::vartime_double_scalar_mul_basepoint(&sig.c, ring_member, &sig.r);
+      buf.extend_from_slice(Li.compress().as_bytes());
+      #[allow(non_snake_case)]
+      let Ri = (sig.r * hash_to_point(ring_member.compress().to_bytes())) + (sig.c * key_image);
+      buf.extend_from_slice(Ri.compress().as_bytes());
+
+      sum += sig.c;
+    }
+
+    sum == hash_to_scalar(&buf)
+  }
+}

coins/monero/src/ringct/borromean.rs

@@ -1,73 +1,63 @@
 use core::fmt::Debug;
 use std_shims::io::{self, Read, Write};
 
-use curve25519_dalek::edwards::EdwardsPoint;
+use curve25519_dalek::{traits::Identity, Scalar, EdwardsPoint};
-#[cfg(feature = "experimental")]
-use curve25519_dalek::{traits::Identity, scalar::Scalar};
 
-#[cfg(feature = "experimental")]
 use monero_generators::H_pow_2;
-#[cfg(feature = "experimental")]
+
-use crate::hash_to_scalar;
+use crate::{hash_to_scalar, unreduced_scalar::UnreducedScalar, serialize::*};
-use crate::serialize::*;
 
 /// 64 Borromean ring signatures.
 ///
-/// This type keeps the data as raw bytes as Monero has some transactions with unreduced scalars in
+/// s0 and s1 are stored as `UnreducedScalar`s due to Monero not requiring they were reduced.
-/// this field. While we could use `from_bytes_mod_order`, we'd then not be able to encode this
+/// `UnreducedScalar` preserves their original byte encoding and implements a custom reduction
-/// back into it's original form.
+/// algorithm which was in use.
-///
-/// Those scalars also have a custom reduction algorithm...
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct BorromeanSignatures {
-  pub s0: [[u8; 32]; 64],
+  pub s0: [UnreducedScalar; 64],
-  pub s1: [[u8; 32]; 64],
+  pub s1: [UnreducedScalar; 64],
-  pub ee: [u8; 32],
+  pub ee: Scalar,
 }
 
 impl BorromeanSignatures {
   pub fn read<R: Read>(r: &mut R) -> io::Result<BorromeanSignatures> {
     Ok(BorromeanSignatures {
-      s0: read_array(read_bytes, r)?,
+      s0: read_array(UnreducedScalar::read, r)?,
-      s1: read_array(read_bytes, r)?,
+      s1: read_array(UnreducedScalar::read, r)?,
-      ee: read_bytes(r)?,
+      ee: read_scalar(r)?,
     })
   }
 
   pub fn write<W: Write>(&self, w: &mut W) -> io::Result<()> {
     for s0 in &self.s0 {
-      w.write_all(s0)?;
+      s0.write(w)?;
     }
     for s1 in &self.s1 {
-      w.write_all(s1)?;
+      s1.write(w)?;
     }
-    w.write_all(&self.ee)
+    write_scalar(&self.ee, w)
   }
 
-  #[cfg(feature = "experimental")]
   fn verify(&self, keys_a: &[EdwardsPoint], keys_b: &[EdwardsPoint]) -> bool {
     let mut transcript = [0; 2048];
+
     for i in 0 .. 64 {
-      // TODO: These aren't the correct reduction
-      // TODO: Can either of these be tightened?
       #[allow(non_snake_case)]
       let LL = EdwardsPoint::vartime_double_scalar_mul_basepoint(
-        &Scalar::from_bytes_mod_order(self.ee),
+        &self.ee,
         &keys_a[i],
-        &Scalar::from_bytes_mod_order(self.s0[i]),
+        &self.s0[i].recover_monero_slide_scalar(),
       );
       #[allow(non_snake_case)]
       let LV = EdwardsPoint::vartime_double_scalar_mul_basepoint(
         &hash_to_scalar(LL.compress().as_bytes()),
         &keys_b[i],
-        &Scalar::from_bytes_mod_order(self.s1[i]),
+        &self.s1[i].recover_monero_slide_scalar(),
       );
-      transcript[i .. ((i + 1) * 32)].copy_from_slice(LV.compress().as_bytes());
+      transcript[(i * 32) .. ((i + 1) * 32)].copy_from_slice(LV.compress().as_bytes());
     }
 
-    // TODO: This isn't the correct reduction
+    hash_to_scalar(&transcript) == self.ee
-    // TODO: Can this be tightened to from_canonical_bytes?
-    hash_to_scalar(&transcript) == Scalar::from_bytes_mod_order(self.ee)
   }
 }
 
@@ -90,7 +80,6 @@ impl BorromeanRange {
     write_raw_vec(write_point, &self.bit_commitments, w)
   }
 
-  #[cfg(feature = "experimental")]
   pub fn verify(&self, commitment: &EdwardsPoint) -> bool {
     if &self.bit_commitments.iter().sum::<EdwardsPoint>() != commitment {
       return false;

coins/monero/src/ringct/clsag/mod.rs

@@ -180,7 +180,7 @@ fn core(
     let c_c = mu_C * c;
 
     let L = (&s[i] * ED25519_BASEPOINT_TABLE) + (c_p * P[i]) + (c_c * C[i]);
-    let PH = hash_to_point(P[i]);
+    let PH = hash_to_point(&P[i]);
     // Shouldn't be an issue as all of the variables in this vartime statement are public
     let R = (s[i] * PH) + images_precomp.vartime_multiscalar_mul([c_p, c_c]);
 
@@ -219,7 +219,7 @@ impl Clsag {
     let pseudo_out = Commitment::new(mask, input.commitment.amount).calculate();
     let z = input.commitment.mask - mask;
 
-    let H = hash_to_point(input.decoys.ring[r][0]);
+    let H = hash_to_point(&input.decoys.ring[r][0]);
     let D = H * z;
     let mut s = Vec::with_capacity(input.decoys.ring.len());
     for _ in 0 .. input.decoys.ring.len() {
@@ -259,7 +259,7 @@ impl Clsag {
         &msg,
         nonce.deref() * ED25519_BASEPOINT_TABLE,
         nonce.deref() *
-          hash_to_point(inputs[i].2.decoys.ring[usize::from(inputs[i].2.decoys.i)][0]),
+          hash_to_point(&inputs[i].2.decoys.ring[usize::from(inputs[i].2.decoys.i)][0]),
       );
       clsag.s[usize::from(inputs[i].2.decoys.i)] =
         (-((p * inputs[i].0.deref()) + c)) + nonce.deref();

coins/monero/src/ringct/clsag/multisig.rs

@@ -116,7 +116,7 @@ impl ClsagMultisig {
     ClsagMultisig {
       transcript,
 
-      H: hash_to_point(output_key),
+      H: hash_to_point(&output_key),
       image: EdwardsPoint::identity(),
 
       details,

coins/monero/src/ringct/hash_to_point.rs

@@ -3,6 +3,6 @@ use curve25519_dalek::edwards::EdwardsPoint;
 pub use monero_generators::{hash_to_point as raw_hash_to_point};
 
 /// Monero's hash to point function, as named `ge_fromfe_frombytes_vartime`.
-pub fn hash_to_point(key: EdwardsPoint) -> EdwardsPoint {
+pub fn hash_to_point(key: &EdwardsPoint) -> EdwardsPoint {
   raw_hash_to_point(key.compress().to_bytes())
 }

coins/monero/src/ringct/mlsag.rs

@@ -3,17 +3,82 @@ use std_shims::{
   io::{self, Read, Write},
 };
 
-use curve25519_dalek::scalar::Scalar;
+use zeroize::Zeroize;
-#[cfg(feature = "experimental")]
-use curve25519_dalek::edwards::EdwardsPoint;
 
-use crate::serialize::*;
+use curve25519_dalek::{traits::IsIdentity, Scalar, EdwardsPoint};
-#[cfg(feature = "experimental")]
-use crate::{hash_to_scalar, ringct::hash_to_point};
 
-#[derive(Clone, PartialEq, Eq, Debug)]
+use monero_generators::H;
+
+use crate::{hash_to_scalar, ringct::hash_to_point, serialize::*};
+
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+#[cfg_attr(feature = "std", derive(thiserror::Error))]
+pub enum MlsagError {
+  #[cfg_attr(feature = "std", error("invalid ring"))]
+  InvalidRing,
+  #[cfg_attr(feature = "std", error("invalid amount of key images"))]
+  InvalidAmountOfKeyImages,
+  #[cfg_attr(feature = "std", error("invalid ss"))]
+  InvalidSs,
+  #[cfg_attr(feature = "std", error("key image was identity"))]
+  IdentityKeyImage,
+  #[cfg_attr(feature = "std", error("invalid ci"))]
+  InvalidCi,
+}
+
+#[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
+pub struct RingMatrix {
+  matrix: Vec<Vec<EdwardsPoint>>,
+}
+
+impl RingMatrix {
+  pub fn new(matrix: Vec<Vec<EdwardsPoint>>) -> Result<Self, MlsagError> {
+    if matrix.is_empty() {
+      Err(MlsagError::InvalidRing)?;
+    }
+    for member in &matrix {
+      if member.is_empty() || (member.len() != matrix[0].len()) {
+        Err(MlsagError::InvalidRing)?;
+      }
+    }
+
+    Ok(RingMatrix { matrix })
+  }
+
+  /// Construct a ring matrix for an individual output.
+  pub fn individual(
+    ring: &[[EdwardsPoint; 2]],
+    pseudo_out: EdwardsPoint,
+  ) -> Result<Self, MlsagError> {
+    let mut matrix = Vec::with_capacity(ring.len());
+    for ring_member in ring {
+      matrix.push(vec![ring_member[0], ring_member[1] - pseudo_out]);
+    }
+    RingMatrix::new(matrix)
+  }
+
+  pub fn iter(&self) -> impl Iterator<Item = &[EdwardsPoint]> {
+    self.matrix.iter().map(AsRef::as_ref)
+  }
+
+  /// Return the amount of members in the ring.
+  pub fn members(&self) -> usize {
+    self.matrix.len()
+  }
+
+  /// Returns the length of a ring member.
+  ///
+  /// A ring member is a vector of points for which the signer knows all of the discrete logarithms
+  /// of.
+  pub fn member_len(&self) -> usize {
+    // this is safe to do as the constructors don't allow empty rings
+    self.matrix[0].len()
+  }
+}
+
+#[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
 pub struct Mlsag {
-  pub ss: Vec<[Scalar; 2]>,
+  pub ss: Vec<Vec<Scalar>>,
   pub cc: Scalar,
 }
 
@@ -25,47 +90,124 @@ impl Mlsag {
     write_scalar(&self.cc, w)
   }
 
-  pub fn read<R: Read>(mixins: usize, r: &mut R) -> io::Result<Mlsag> {
+  pub fn read<R: Read>(mixins: usize, ss_2_elements: usize, r: &mut R) -> io::Result<Mlsag> {
     Ok(Mlsag {
-      ss: (0 .. mixins).map(|_| read_array(read_scalar, r)).collect::<Result<_, _>>()?,
+      ss: (0 .. mixins)
+        .map(|_| read_raw_vec(read_scalar, ss_2_elements, r))
+        .collect::<Result<_, _>>()?,
       cc: read_scalar(r)?,
     })
   }
 
-  #[cfg(feature = "experimental")]
   pub fn verify(
     &self,
     msg: &[u8; 32],
-    ring: &[[EdwardsPoint; 2]],
+    ring: &RingMatrix,
-    key_image: &EdwardsPoint,
+    key_images: &[EdwardsPoint],
-  ) -> bool {
+  ) -> Result<(), MlsagError> {
-    if ring.is_empty() {
+    // Mlsag allows for layers to not need linkability, hence they don't need key images
-      return false;
+    // Monero requires that there is always only 1 non-linkable layer - the amount commitments.
+    if ring.member_len() != (key_images.len() + 1) {
+      Err(MlsagError::InvalidAmountOfKeyImages)?;
     }
 
     let mut buf = Vec::with_capacity(6 * 32);
-    let mut ci = self.cc;
-    for (i, ring_member) in ring.iter().enumerate() {
     buf.extend_from_slice(msg);
 
-      #[allow(non_snake_case)]
+    let mut ci = self.cc;
-      let L =
-        |r| EdwardsPoint::vartime_double_scalar_mul_basepoint(&ci, &ring_member[r], &self.ss[i][r]);
 
-      buf.extend_from_slice(ring_member[0].compress().as_bytes());
+    // This is an iterator over the key images as options with an added entry of `None` at the
-      buf.extend_from_slice(L(0).compress().as_bytes());
+    // end for the non-linkable layer
+    let key_images_iter = key_images.iter().map(|ki| Some(*ki)).chain(core::iter::once(None));
+
+    if ring.matrix.len() != self.ss.len() {
+      Err(MlsagError::InvalidSs)?;
+    }
+
+    for (ring_member, ss) in ring.iter().zip(&self.ss) {
+      if ring_member.len() != ss.len() {
+        Err(MlsagError::InvalidSs)?;
+      }
+
+      for ((ring_member_entry, s), ki) in ring_member.iter().zip(ss).zip(key_images_iter.clone()) {
+        #[allow(non_snake_case)]
+        let L = EdwardsPoint::vartime_double_scalar_mul_basepoint(&ci, ring_member_entry, s);
+
+        buf.extend_from_slice(ring_member_entry.compress().as_bytes());
+        buf.extend_from_slice(L.compress().as_bytes());
+
+        // Not all dimensions need to be linkable, e.g. commitments, and only linkable layers need
+        // to have key images.
+        if let Some(ki) = ki {
+          if ki.is_identity() {
+            Err(MlsagError::IdentityKeyImage)?;
+          }
 
           #[allow(non_snake_case)]
-      let R = (self.ss[i][0] * hash_to_point(ring_member[0])) + (ci * key_image);
+          let R = (s * hash_to_point(ring_member_entry)) + (ci * ki);
           buf.extend_from_slice(R.compress().as_bytes());
-
+        }
-      buf.extend_from_slice(ring_member[1].compress().as_bytes());
+      }
-      buf.extend_from_slice(L(1).compress().as_bytes());
 
       ci = hash_to_scalar(&buf);
-      buf.clear();
+      // keep the msg in the buffer.
+      buf.drain(msg.len() ..);
     }
 
-    ci == self.cc
+    if ci != self.cc {
+      Err(MlsagError::InvalidCi)?
+    }
+    Ok(())
+  }
+}
+
+/// An aggregate ring matrix builder, usable to set up the ring matrix to prove/verify an aggregate
+/// MLSAG signature.
+#[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
+pub struct AggregateRingMatrixBuilder {
+  key_ring: Vec<Vec<EdwardsPoint>>,
+  amounts_ring: Vec<EdwardsPoint>,
+  sum_out: EdwardsPoint,
+}
+
+impl AggregateRingMatrixBuilder {
+  /// Create a new AggregateRingMatrixBuilder.
+  ///
+  /// Takes in the transaction's outputs; commitments and fee.
+  pub fn new(commitments: &[EdwardsPoint], fee: u64) -> Self {
+    AggregateRingMatrixBuilder {
+      key_ring: vec![],
+      amounts_ring: vec![],
+      sum_out: commitments.iter().sum::<EdwardsPoint>() + (H() * Scalar::from(fee)),
+    }
+  }
+
+  /// Push a ring of [output key, commitment] to the matrix.
+  pub fn push_ring(&mut self, ring: &[[EdwardsPoint; 2]]) -> Result<(), MlsagError> {
+    if self.key_ring.is_empty() {
+      self.key_ring = vec![vec![]; ring.len()];
+      // Now that we know the length of the ring, fill the `amounts_ring`.
+      self.amounts_ring = vec![-self.sum_out; ring.len()];
+    }
+
+    if (self.amounts_ring.len() != ring.len()) || ring.is_empty() {
+      // All the rings in an aggregate matrix must be the same length.
+      return Err(MlsagError::InvalidRing);
+    }
+
+    for (i, ring_member) in ring.iter().enumerate() {
+      self.key_ring[i].push(ring_member[0]);
+      self.amounts_ring[i] += ring_member[1]
+    }
+
+    Ok(())
+  }
+
+  /// Build and return the [`RingMatrix`]
+  pub fn build(mut self) -> Result<RingMatrix, MlsagError> {
+    for (i, amount_commitment) in self.amounts_ring.drain(..).enumerate() {
+      self.key_ring[i].push(amount_commitment);
+    }
+    RingMatrix::new(self.key_ring)
   }
 }

coins/monero/src/ringct/mod.rs

@@ -28,7 +28,7 @@ use crate::{
 
 /// Generate a key image for a given key. Defined as `x * hash_to_point(xG)`.
 pub fn generate_key_image(secret: &Zeroizing<Scalar>) -> EdwardsPoint {
-  hash_to_point(ED25519_BASEPOINT_TABLE * secret.deref()) * secret.deref()
+  hash_to_point(&(ED25519_BASEPOINT_TABLE * secret.deref())) * secret.deref()
 }
 
 #[derive(Clone, PartialEq, Eq, Debug)]
@@ -61,7 +61,7 @@ impl EncryptedAmount {
 pub enum RctType {
   /// No RCT proofs.
   Null,
-  /// One MLSAG for a single input and a Borromean range proof (RCTTypeFull).
+  /// One MLSAG for multiple inputs and Borromean range proofs (RCTTypeFull).
   MlsagAggregate,
   // One MLSAG for each input and a Borromean range proof (RCTTypeSimple).
   MlsagIndividual,
@@ -194,6 +194,10 @@ impl RctBase {
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub enum RctPrunable {
   Null,
+  AggregateMlsagBorromean {
+    borromean: Vec<BorromeanRange>,
+    mlsag: Mlsag,
+  },
   MlsagBorromean {
     borromean: Vec<BorromeanRange>,
     mlsags: Vec<Mlsag>,
@@ -220,6 +224,10 @@ impl RctPrunable {
   pub fn write<W: Write>(&self, w: &mut W, rct_type: RctType) -> io::Result<()> {
     match self {
       RctPrunable::Null => Ok(()),
+      RctPrunable::AggregateMlsagBorromean { borromean, mlsag } => {
+        write_raw_vec(BorromeanRange::write, borromean, w)?;
+        mlsag.write(w)
+      }
       RctPrunable::MlsagBorromean { borromean, mlsags } => {
         write_raw_vec(BorromeanRange::write, borromean, w)?;
         write_raw_vec(Mlsag::write, mlsags, w)
@@ -270,9 +278,13 @@ impl RctPrunable {
 
     Ok(match rct_type {
       RctType::Null => RctPrunable::Null,
-      RctType::MlsagAggregate | RctType::MlsagIndividual => RctPrunable::MlsagBorromean {
+      RctType::MlsagAggregate => RctPrunable::AggregateMlsagBorromean {
         borromean: read_raw_vec(BorromeanRange::read, outputs, r)?,
-        mlsags: decoys.iter().map(|d| Mlsag::read(*d, r)).collect::<Result<_, _>>()?,
+        mlsag: Mlsag::read(decoys[0], decoys.len() + 1, r)?,
+      },
+      RctType::MlsagIndividual => RctPrunable::MlsagBorromean {
+        borromean: read_raw_vec(BorromeanRange::read, outputs, r)?,
+        mlsags: decoys.iter().map(|d| Mlsag::read(*d, 2, r)).collect::<Result<_, _>>()?,
       },
       RctType::Bulletproofs | RctType::BulletproofsCompactAmount => {
         RctPrunable::MlsagBulletproofs {
@@ -287,13 +299,13 @@ impl RctPrunable {
             }
             Bulletproofs::read(r)?
           },
-          mlsags: decoys.iter().map(|d| Mlsag::read(*d, r)).collect::<Result<_, _>>()?,
+          mlsags: decoys.iter().map(|d| Mlsag::read(*d, 2, r)).collect::<Result<_, _>>()?,
           pseudo_outs: read_raw_vec(read_point, decoys.len(), r)?,
         }
       }
       RctType::Clsag | RctType::BulletproofsPlus => RctPrunable::Clsag {
         bulletproofs: {
-          if read_varint(r)? != 1 {
+          if read_varint::<_, u64>(r)? != 1 {
             Err(io::Error::new(io::ErrorKind::Other, "n bulletproofs instead of one"))?;
           }
           (if rct_type == RctType::Clsag { Bulletproofs::read } else { Bulletproofs::read_plus })(
@@ -309,6 +321,7 @@ impl RctPrunable {
   pub(crate) fn signature_write<W: Write>(&self, w: &mut W) -> io::Result<()> {
     match self {
       RctPrunable::Null => panic!("Serializing RctPrunable::Null for a signature"),
+      RctPrunable::AggregateMlsagBorromean { borromean, .. } |
       RctPrunable::MlsagBorromean { borromean, .. } => {
         borromean.iter().try_for_each(|rs| rs.write(w))
       }
@@ -329,30 +342,8 @@ impl RctSignatures {
   pub fn rct_type(&self) -> RctType {
     match &self.prunable {
       RctPrunable::Null => RctType::Null,
-      RctPrunable::MlsagBorromean { .. } => {
+      RctPrunable::AggregateMlsagBorromean { .. } => RctType::MlsagAggregate,
-        /*
+      RctPrunable::MlsagBorromean { .. } => RctType::MlsagIndividual,
-          This type of RctPrunable may have no outputs, yet pseudo_outs are per input
-          This will only be a valid RctSignatures if it's for a TX with inputs
-          That makes this valid for any valid RctSignatures
-
-          While it will be invalid for any invalid RctSignatures, potentially letting an invalid
-          MlsagAggregate be interpreted as a valid MlsagIndividual (or vice versa), they have
-          incompatible deserializations
-
-          This means it's impossible to receive a MlsagAggregate over the wire and interpret it
-          as a MlsagIndividual (or vice versa)
-
-          That only makes manual manipulation unsafe, which will always be true since these fields
-          are all pub
-
-          TODO: Consider making them private with read-only accessors?
-        */
-        if self.base.pseudo_outs.is_empty() {
-          RctType::MlsagAggregate
-        } else {
-          RctType::MlsagIndividual
-        }
-      }
       // RctBase ensures there's at least one output, making the following
       // inferences guaranteed/expects impossible on any valid RctSignatures
       RctPrunable::MlsagBulletproofs { .. } => {

coins/monero/src/rpc/mod.rs

@@ -305,8 +305,17 @@ impl<R: RpcConnection> Rpc<R> {
   }
 
   pub async fn get_block_by_number(&self, number: usize) -> Result<Block, RpcError> {
-    match self.get_block(self.get_block_hash(number).await?).await {
+    #[derive(Deserialize, Debug)]
-      Ok(block) => {
+    struct BlockResponse {
+      blob: String,
+    }
+
+    let res: BlockResponse =
+      self.json_rpc_call("get_block", Some(json!({ "height": number }))).await?;
+
+    let block = Block::read::<&[u8]>(&mut rpc_hex(&res.blob)?.as_ref())
+      .map_err(|_| RpcError::InvalidNode("invalid block"))?;
+
     // Make sure this is actually the block for this number
     match block.miner_tx.prefix.inputs.first() {
       Some(Input::Gen(actual)) => {
@@ -316,12 +325,7 @@ impl<R: RpcConnection> Rpc<R> {
           Err(RpcError::InvalidNode("different block than requested (number)"))
         }
       }
-          _ => {
+      _ => Err(RpcError::InvalidNode("block's miner_tx didn't have an input of kind Input::Gen")),
-            Err(RpcError::InvalidNode("block's miner_tx didn't have an input of kind Input::Gen"))
-          }
-        }
-      }
-      e => e,
     }
   }
 

coins/monero/src/serialize.rs

@@ -12,7 +12,7 @@ use curve25519_dalek::{
 const VARINT_CONTINUATION_MASK: u8 = 0b1000_0000;
 
 mod sealed {
-  pub trait VarInt: TryInto<u64> {}
+  pub trait VarInt: TryInto<u64> + TryFrom<u64> + Copy {}
   impl VarInt for u8 {}
   impl VarInt for u32 {}
   impl VarInt for u64 {}
@@ -29,8 +29,9 @@ pub(crate) fn write_byte<W: Write>(byte: &u8, w: &mut W) -> io::Result<()> {
   w.write_all(&[*byte])
 }
 
-pub(crate) fn write_varint<W: Write>(varint: &u64, w: &mut W) -> io::Result<()> {
+// This will panic if the VarInt exceeds u64::MAX
-  let mut varint = *varint;
+pub(crate) fn write_varint<W: Write, U: sealed::VarInt>(varint: &U, w: &mut W) -> io::Result<()> {
+  let mut varint: u64 = (*varint).try_into().map_err(|_| "varint exceeded u64").unwrap();
   while {
     let mut b = u8::try_from(varint & u64::from(!VARINT_CONTINUATION_MASK)).unwrap();
     varint >>= 7;
@@ -67,7 +68,7 @@ pub(crate) fn write_vec<T, W: Write, F: Fn(&T, &mut W) -> io::Result<()>>(
   values: &[T],
   w: &mut W,
 ) -> io::Result<()> {
-  write_varint(&values.len().try_into().unwrap(), w)?;
+  write_varint(&values.len(), w)?;
   write_raw_vec(f, values, w)
 }
 
@@ -93,7 +94,7 @@ pub(crate) fn read_u64<R: Read>(r: &mut R) -> io::Result<u64> {
   read_bytes(r).map(u64::from_le_bytes)
 }
 
-pub(crate) fn read_varint<R: Read>(r: &mut R) -> io::Result<u64> {
+pub(crate) fn read_varint<R: Read, U: sealed::VarInt>(r: &mut R) -> io::Result<U> {
   let mut bits = 0;
   let mut res = 0;
   while {
@@ -109,7 +110,9 @@ pub(crate) fn read_varint<R: Read>(r: &mut R) -> io::Result<u64> {
     bits += 7;
     b & VARINT_CONTINUATION_MASK == VARINT_CONTINUATION_MASK
   } {}
-  Ok(res)
+  res
+    .try_into()
+    .map_err(|_| io::Error::new(io::ErrorKind::Other, "VarInt does not fit into integer type"))
 }
 
 // All scalar fields supported by monero-serai are checked to be canonical for valid transactions
@@ -162,5 +165,5 @@ pub(crate) fn read_vec<R: Read, T, F: Fn(&mut R) -> io::Result<T>>(
   f: F,
   r: &mut R,
 ) -> io::Result<Vec<T>> {
-  read_raw_vec(f, read_varint(r)?.try_into().unwrap(), r)
+  read_raw_vec(f, read_varint(r)?, r)
 }

coins/monero/src/tests/mod.rs

@@ -1,3 +1,4 @@
+mod unreduced_scalar;
 mod clsag;
 mod bulletproofs;
 mod address;

coins/monero/src/tests/unreduced_scalar.rs

@@ -0,0 +1,32 @@
+use curve25519_dalek::scalar::Scalar;
+
+use crate::unreduced_scalar::*;
+
+#[test]
+fn recover_scalars() {
+  let test_recover = |stored: &str, recovered: &str| {
+    let stored = UnreducedScalar(hex::decode(stored).unwrap().try_into().unwrap());
+    let recovered =
+      Scalar::from_canonical_bytes(hex::decode(recovered).unwrap().try_into().unwrap()).unwrap();
+    assert_eq!(stored.recover_monero_slide_scalar(), recovered);
+  };
+
+  // https://www.moneroinflation.com/static/data_py/report_scalars_df.pdf
+  // Table 4.
+  test_recover(
+    "cb2be144948166d0a9edb831ea586da0c376efa217871505ad77f6ff80f203f8",
+    "b8ffd6a1aee47828808ab0d4c8524cb5c376efa217871505ad77f6ff80f20308",
+  );
+  test_recover(
+    "343d3df8a1051c15a400649c423dc4ed58bef49c50caef6ca4a618b80dee22f4",
+    "21113355bc682e6d7a9d5b3f2137a30259bef49c50caef6ca4a618b80dee2204",
+  );
+  test_recover(
+    "c14f75d612800ca2c1dcfa387a42c9cc086c005bc94b18d204dd61342418eba7",
+    "4f473804b1d27ab2c789c80ab21d034a096c005bc94b18d204dd61342418eb07",
+  );
+  test_recover(
+    "000102030405060708090a0b0c0d0e0f826c4f6e2329a31bc5bc320af0b2bcbb",
+    "a124cfd387f461bf3719e03965ee6877826c4f6e2329a31bc5bc320af0b2bc0b",
+  );
+}

coins/monero/src/transaction.rs

@@ -6,14 +6,12 @@ use std_shims::{
 
 use zeroize::Zeroize;
 
-use curve25519_dalek::{
+use curve25519_dalek::edwards::{EdwardsPoint, CompressedEdwardsY};
-  scalar::Scalar,
-  edwards::{EdwardsPoint, CompressedEdwardsY},
-};
 
 use crate::{
   Protocol, hash,
   serialize::*,
+  ring_signatures::RingSignature,
   ringct::{bulletproofs::Bulletproofs, RctType, RctBase, RctPrunable, RctSignatures},
 };
 
@@ -208,7 +206,7 @@ impl TransactionPrefix {
     self.timelock.write(w)?;
     write_vec(Input::write, &self.inputs, w)?;
     write_vec(Output::write, &self.outputs, w)?;
-    write_varint(&self.extra.len().try_into().unwrap(), w)?;
+    write_varint(&self.extra.len(), w)?;
     w.write_all(&self.extra)
   }
 
@@ -253,7 +251,7 @@ impl TransactionPrefix {
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct Transaction {
   pub prefix: TransactionPrefix,
-  pub signatures: Vec<Vec<(Scalar, Scalar)>>,
+  pub signatures: Vec<RingSignature>,
   pub rct_signatures: RctSignatures,
 }
 
@@ -272,11 +270,8 @@ impl Transaction {
   pub fn write<W: Write>(&self, w: &mut W) -> io::Result<()> {
     self.prefix.write(w)?;
     if self.prefix.version == 1 {
-      for sigs in &self.signatures {
+      for ring_sig in &self.signatures {
-        for sig in sigs {
+        ring_sig.write(w)?;
-          write_scalar(&sig.0, w)?;
-          write_scalar(&sig.1, w)?;
-        }
       }
       Ok(())
     } else if self.prefix.version == 2 {
@@ -305,12 +300,7 @@ impl Transaction {
         .inputs
         .iter()
         .filter_map(|input| match input {
-          Input::ToKey { key_offsets, .. } => Some(
+          Input::ToKey { key_offsets, .. } => Some(RingSignature::read(key_offsets.len(), r)),
-            key_offsets
-              .iter()
-              .map(|_| Ok((read_scalar(r)?, read_scalar(r)?)))
-              .collect::<Result<_, io::Error>>(),
-          ),
           _ => None,
         })
         .collect::<Result<_, _>>()?;
@@ -397,6 +387,10 @@ impl Transaction {
 
   /// Calculate the hash of this transaction as needed for signing it.
   pub fn signature_hash(&self) -> [u8; 32] {
+    if self.prefix.version == 1 {
+      return self.prefix.hash();
+    }
+
     let mut buf = Vec::with_capacity(2048);
     let mut sig_hash = Vec::with_capacity(96);
 

coins/monero/src/unreduced_scalar.rs

@@ -0,0 +1,137 @@
+use core::cmp::Ordering;
+
+use std_shims::{
+  sync::OnceLock,
+  io::{self, *},
+};
+
+use curve25519_dalek::scalar::Scalar;
+
+use crate::serialize::*;
+
+static PRECOMPUTED_SCALARS_CELL: OnceLock<[Scalar; 8]> = OnceLock::new();
+/// Precomputed scalars used to recover an incorrectly reduced scalar.
+#[allow(non_snake_case)]
+pub(crate) fn PRECOMPUTED_SCALARS() -> [Scalar; 8] {
+  *PRECOMPUTED_SCALARS_CELL.get_or_init(|| {
+    let mut precomputed_scalars = [Scalar::ONE; 8];
+    for (i, scalar) in precomputed_scalars.iter_mut().enumerate().skip(1) {
+      *scalar = Scalar::from(((i * 2) + 1) as u8);
+    }
+    precomputed_scalars
+  })
+}
+
+#[derive(Clone, PartialEq, Eq, Debug)]
+pub struct UnreducedScalar(pub [u8; 32]);
+
+impl UnreducedScalar {
+  pub fn write<W: Write>(&self, w: &mut W) -> io::Result<()> {
+    w.write_all(&self.0)
+  }
+
+  pub fn read<R: Read>(r: &mut R) -> io::Result<UnreducedScalar> {
+    Ok(UnreducedScalar(read_bytes(r)?))
+  }
+
+  pub fn as_bytes(&self) -> &[u8; 32] {
+    &self.0
+  }
+
+  fn as_bits(&self) -> [u8; 256] {
+    let mut bits = [0; 256];
+    for (i, bit) in bits.iter_mut().enumerate() {
+      *bit = core::hint::black_box(1 & (self.0[i / 8] >> (i % 8)))
+    }
+
+    bits
+  }
+
+  /// Computes the non-adjacent form of this scalar with width 5.
+  ///
+  /// This matches Monero's `slide` function and intentionally gives incorrect outputs under
+  /// certain conditions in order to match Monero.
+  ///
+  /// This function does not execute in constant time.
+  fn non_adjacent_form(&self) -> [i8; 256] {
+    let bits = self.as_bits();
+    let mut naf = [0i8; 256];
+    for (b, bit) in bits.into_iter().enumerate() {
+      naf[b] = bit as i8;
+    }
+
+    for i in 0 .. 256 {
+      if naf[i] != 0 {
+        // if the bit is a one, work our way up through the window
+        // combining the bits with this bit.
+        for b in 1 .. 6 {
+          if (i + b) >= 256 {
+            // if we are at the length of the array then break out
+            // the loop.
+            break;
+          }
+          // potential_carry - the value of the bit at i+b compared to the bit at i
+          let potential_carry = naf[i + b] << b;
+
+          if potential_carry != 0 {
+            if (naf[i] + potential_carry) <= 15 {
+              // if our current "bit" plus the potential carry is less than 16
+              // add it to our current "bit" and set the potential carry bit to 0.
+              naf[i] += potential_carry;
+              naf[i + b] = 0;
+            } else if (naf[i] - potential_carry) >= -15 {
+              // else if our current "bit" minus the potential carry is more than -16
+              // take it away from our current "bit".
+              // we then work our way up through the bits setting ones to zero, when
+              // we hit the first zero we change it to one then stop, this is to factor
+              // in the minus.
+              naf[i] -= potential_carry;
+              #[allow(clippy::needless_range_loop)]
+              for k in (i + b) .. 256 {
+                if naf[k] == 0 {
+                  naf[k] = 1;
+                  break;
+                }
+                naf[k] = 0;
+              }
+            } else {
+              break;
+            }
+          }
+        }
+      }
+    }
+
+    naf
+  }
+
+  /// Recover the scalar that an array of bytes was incorrectly interpreted as by Monero's `slide`
+  /// function.
+  ///
+  /// In Borromean range proofs Monero was not checking that the scalars used were
+  /// reduced. This lead to the scalar stored being interpreted as a different scalar,
+  /// this function recovers that scalar.
+  ///
+  /// See: https://github.com/monero-project/monero/issues/8438
+  pub fn recover_monero_slide_scalar(&self) -> Scalar {
+    if self.0[31] & 128 == 0 {
+      // Computing the w-NAF of a number can only give an output with 1 more bit than
+      // the number, so even if the number isn't reduced, the `slide` function will be
+      // correct when the last bit isn't set.
+      return Scalar::from_bytes_mod_order(self.0);
+    }
+
+    let precomputed_scalars = PRECOMPUTED_SCALARS();
+
+    let mut recovered = Scalar::ZERO;
+    for &numb in self.non_adjacent_form().iter().rev() {
+      recovered += recovered;
+      match numb.cmp(&0) {
+        Ordering::Greater => recovered += precomputed_scalars[(numb as usize) / 2],
+        Ordering::Less => recovered -= precomputed_scalars[((-numb) as usize) / 2],
+        Ordering::Equal => (),
+      }
+    }
+    recovered
+  }
+}

coins/monero/src/wallet/extra.rs

@@ -110,11 +110,7 @@ impl ExtraField {
         }
         nonce
       }),
-      3 => ExtraField::MergeMining(
+      3 => ExtraField::MergeMining(read_varint(r)?, read_bytes(r)?),
-        usize::try_from(read_varint(r)?)
-          .map_err(|_| io::Error::new(io::ErrorKind::Other, "varint for height exceeds usize"))?,
-        read_bytes(r)?,
-      ),
       4 => ExtraField::PublicKeys(read_vec(read_point, r)?),
       _ => Err(io::Error::new(io::ErrorKind::Other, "unknown extra field"))?,
     })

coins/monero/src/wallet/mod.rs

@@ -74,7 +74,7 @@ pub(crate) fn shared_key(
     .copy_from_slice(&hash(&[output_derivation.as_ref(), [0x8d].as_ref()].concat())[.. 8]);
 
   // || o
-  write_varint(&o.try_into().unwrap(), &mut output_derivation).unwrap();
+  write_varint(&o, &mut output_derivation).unwrap();
 
   let view_tag = hash(&[b"view_tag".as_ref(), &output_derivation].concat())[0];
 

coins/monero/src/wallet/send/multisig.rs

@@ -406,7 +406,9 @@ impl SignatureMachine<Transaction> for TransactionSignatureMachine {
           pseudo_outs.push(pseudo_out);
         }
       }
-      RctPrunable::MlsagBorromean { .. } | RctPrunable::MlsagBulletproofs { .. } => {
+      RctPrunable::AggregateMlsagBorromean { .. } |
+      RctPrunable::MlsagBorromean { .. } |
+      RctPrunable::MlsagBulletproofs { .. } => {
         unreachable!("attempted to sign a multisig TX which wasn't CLSAG")
       }
     }