Remove unsafe creation of dalek_ff_group::EdwardsPoint in BP+

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

@@ -87,9 +87,8 @@ impl Bulletproofs {
     Ok(if !plus {
       Bulletproofs::Original(OriginalStruct::prove(rng, outputs))
     } else {
-      use dalek_ff_group::EdwardsPoint as DfgPoint;
       Bulletproofs::Plus(
-        AggregateRangeStatement::new(outputs.iter().map(|com| DfgPoint(com.calculate())).collect())
+        AggregateRangeStatement::new(outputs.iter().map(Commitment::calculate).collect())
           .unwrap()
           .prove(rng, &Zeroizing::new(AggregateRangeWitness::new(outputs.to_vec()).unwrap()))
           .unwrap(),
@@ -104,13 +103,7 @@ impl Bulletproofs {
       Bulletproofs::Original(bp) => bp.verify(rng, commitments),
       Bulletproofs::Plus(bp) => {
         let mut verifier = BatchVerifier::new(1);
-        // If this commitment is torsioned (which is allowed), this won't be a well-formed
+        let Some(statement) = AggregateRangeStatement::new(commitments.to_vec()) else {
-        // dfg::EdwardsPoint (expected to be of prime-order)
-        // The actual BP+ impl will perform a torsion clear though, making this safe
-        // TODO: Have AggregateRangeStatement take in dalek EdwardsPoint for clarity on this
-        let Some(statement) = AggregateRangeStatement::new(
-          commitments.iter().map(|c| dalek_ff_group::EdwardsPoint(*c)).collect(),
-        ) else {
           return false;
         };
         if !statement.verify(rng, &mut verifier, (), bp.clone()) {
@@ -135,9 +128,7 @@ impl Bulletproofs {
     match self {
       Bulletproofs::Original(bp) => bp.batch_verify(rng, verifier, id, commitments),
       Bulletproofs::Plus(bp) => {
-        let Some(statement) = AggregateRangeStatement::new(
+        let Some(statement) = AggregateRangeStatement::new(commitments.to_vec()) else {
-          commitments.iter().map(|c| dalek_ff_group::EdwardsPoint(*c)).collect(),
-        ) else {
           return false;
         };
         statement.verify(rng, verifier, id, bp.clone())

coins/monero/src/ringct/bulletproofs/plus/aggregate_range_proof.rs

@@ -9,6 +9,7 @@ use group::{
   ff::{Field, PrimeField},
   Group, GroupEncoding,
 };
+use curve25519_dalek::EdwardsPoint as DalekPoint;
 use dalek_ff_group::{Scalar, EdwardsPoint};
 
 use crate::{
@@ -28,7 +29,7 @@ use crate::{
 #[derive(Clone, Debug)]
 pub(crate) struct AggregateRangeStatement {
   generators: Generators,
-  V: Vec<EdwardsPoint>,
+  V: Vec<DalekPoint>,
 }
 
 impl Zeroize for AggregateRangeStatement {
@@ -57,7 +58,7 @@ pub struct AggregateRangeProof {
 }
 
 impl AggregateRangeStatement {
-  pub(crate) fn new(V: Vec<EdwardsPoint>) -> Option<Self> {
+  pub(crate) fn new(V: Vec<DalekPoint>) -> Option<Self> {
     if V.is_empty() || (V.len() > MAX_M) {
       return None;
     }
@@ -98,11 +99,14 @@ impl AggregateRangeStatement {
     }
     let mn = V.len() * N;
 
+    // 2, 4, 6, 8... powers of z, of length equivalent to the amount of commitments
     let mut z_pow = Vec::with_capacity(V.len());
+    // z**2
+    z_pow.push(z * z);
 
     let mut d = ScalarVector::new(mn);
     for j in 1 ..= V.len() {
-      z_pow.push(z.pow(Scalar::from(2 * u64::try_from(j).unwrap()))); // TODO: Optimize this
+      z_pow.push(*z_pow.last().unwrap() * z_pow[0]);
       d = d + &(Self::d_j(j, V.len()) * (z_pow[j - 1]));
     }
 
@@ -157,7 +161,7 @@ impl AggregateRangeStatement {
       return None;
     }
     for (commitment, witness) in self.V.iter().zip(witness.0.iter()) {
-      if witness.calculate() != **commitment {
+      if witness.calculate() != *commitment {
         return None;
       }
     }
@@ -170,9 +174,9 @@ impl AggregateRangeStatement {
     // Commitments aren't transmitted INV_EIGHT though, so this multiplies by INV_EIGHT to enable
     // clearing its cofactor without mutating the value
     // For some reason, these values are transcripted * INV_EIGHT, not as transmitted
-    let mut V = V.into_iter().map(|V| EdwardsPoint(V.0 * crate::INV_EIGHT())).collect::<Vec<_>>();
+    let V = V.into_iter().map(|V| V * crate::INV_EIGHT()).collect::<Vec<_>>();
     let mut transcript = initial_transcript(V.iter());
-    V.iter_mut().for_each(|V| *V = V.mul_by_cofactor());
+    let mut V = V.into_iter().map(|V| EdwardsPoint(V.mul_by_cofactor())).collect::<Vec<_>>();
 
     // Pad V
     while V.len() < padded_pow_of_2(V.len()) {
@@ -239,9 +243,11 @@ impl AggregateRangeStatement {
   ) -> bool {
     let Self { generators, V } = self;
 
-    let mut V = V.into_iter().map(|V| EdwardsPoint(V.0 * crate::INV_EIGHT())).collect::<Vec<_>>();
+    let V = V.into_iter().map(|V| V * crate::INV_EIGHT()).collect::<Vec<_>>();
     let mut transcript = initial_transcript(V.iter());
-    V.iter_mut().for_each(|V| *V = V.mul_by_cofactor());
+    // With the torsion clear, wrap it into a EdwardsPoint from dalek-ff-group
+    // (which is prime-order)
+    let V = V.into_iter().map(|V| EdwardsPoint(V.mul_by_cofactor())).collect::<Vec<_>>();
 
     let generators = generators.reduce(V.len() * N);
 

coins/monero/src/ringct/bulletproofs/plus/transcript.rs

@@ -1,6 +1,7 @@
 use std_shims::{sync::OnceLock, vec::Vec};
 
-use dalek_ff_group::{Scalar, EdwardsPoint};
+use curve25519_dalek::EdwardsPoint;
+use dalek_ff_group::Scalar;
 
 use monero_generators::{hash_to_point as raw_hash_to_point};
 use crate::{hash, hash_to_scalar as dalek_hash};

coins/monero/src/tests/bulletproofs/plus/aggregate_range_proof.rs

@@ -2,7 +2,7 @@ use rand_core::{RngCore, OsRng};
 
 use multiexp::BatchVerifier;
 use group::ff::Field;
-use dalek_ff_group::{Scalar, EdwardsPoint};
+use dalek_ff_group::Scalar;
 
 use crate::{
   Commitment,
@@ -19,7 +19,7 @@ fn test_aggregate_range_proof() {
     for _ in 0 .. m {
       commitments.push(Commitment::new(*Scalar::random(&mut OsRng), OsRng.next_u64()));
     }
-    let commitment_points = commitments.iter().map(|com| EdwardsPoint(com.calculate())).collect();
+    let commitment_points = commitments.iter().map(Commitment::calculate).collect();
     let statement = AggregateRangeStatement::new(commitment_points).unwrap();
     let witness = AggregateRangeWitness::new(commitments).unwrap();