Merge pull request mobilecoinfoundation#561 from mfaulk/1994-transact…

…ion-outputs-must-be-sorted

[MCC-1994] Adds validate_outputs_are_sorted

consensus/api/proto/consensus_common.proto

@@ -77,6 +77,7 @@ enum ProposeTxResult {
     UnsortedInputs = 39;
     MissingMemo = 40;
     MemosNotAllowed = 41;
+    UnsortedOutputs = 42;
 }
 
 /// Response from TxPropose RPC call.

consensus/api/src/conversions.rs

@@ -46,6 +46,7 @@ impl From<Error> for ProposeTxResult {
             Error::TxFeeError => Self::TxFeeError,
             Error::KeyError => Self::KeyError,
             Error::UnsortedInputs => Self::UnsortedInputs,
+            Error::UnsortedOutputs => Self::UnsortedOutputs,
             Error::MissingMemo => Self::MissingMemo,
             Error::MemosNotAllowed => Self::MemosNotAllowed,
         }
@@ -91,6 +92,7 @@ impl TryInto<Error> for ProposeTxResult {
             Self::TxFeeError => Ok(Error::TxFeeError),
             Self::KeyError => Ok(Error::KeyError),
             Self::UnsortedInputs => Ok(Error::UnsortedInputs),
+            Self::UnsortedOutputs => Ok(Error::UnsortedOutputs),
             Self::MissingMemo => Ok(Error::MissingMemo),
             Self::MemosNotAllowed => Ok(Error::MemosNotAllowed),
         }

transaction/core/src/validation/error.rs

@@ -78,6 +78,9 @@ pub enum TransactionValidationError {
      */
     UnsortedInputs,
 
+    /// Outputs must be sorted by public_key, ascending.
+    UnsortedOutputs,
+
     /// Key Images must be sorted.
     UnsortedKeyImages,
 

transaction/core/src/validation/validate.rs

@@ -4,15 +4,14 @@
 
 extern crate alloc;
 
-use alloc::{format, vec::Vec};
-
-use super::error::{TransactionValidationError, TransactionValidationResult};
+use super::{TransactionValidationError, TransactionValidationResult};
 use crate::{
     constants::*,
     membership_proofs::{derive_proof_at_index, is_membership_proof_valid},
     tx::{Tx, TxOut, TxOutMembershipProof, TxPrefix},
     BlockVersion, CompressedCommitment,
 };
+use alloc::{format, vec::Vec};
 use mc_common::HashSet;
 use mc_crypto_keys::CompressedRistrettoPublic;
 use rand_core::{CryptoRng, RngCore};
@@ -54,6 +53,8 @@ pub fn validate<R: RngCore + CryptoRng>(
 
     validate_inputs_are_sorted(&tx.prefix)?;
 
+    validate_outputs_are_sorted(&tx.prefix)?;
+
     validate_membership_proofs(&tx.prefix, root_proofs)?;
 
     validate_signature(tx, csprng)?;
@@ -149,26 +150,20 @@ fn validate_ring_elements_are_unique(tx_prefix: &TxPrefix) -> TransactionValidat
         .flat_map(|tx_in| tx_in.ring.iter())
         .collect();
 
-    let mut uniques = HashSet::default();
-    for tx_out in &ring_elements {
-        if !uniques.insert(tx_out) {
-            return Err(TransactionValidationError::DuplicateRingElements);
-        }
-    }
-
-    Ok(())
+    check_unique(
+        &ring_elements,
+        TransactionValidationError::DuplicateRingElements,
+    )
 }
 
 /// Elements in a ring must be sorted.
 fn validate_ring_elements_are_sorted(tx_prefix: &TxPrefix) -> TransactionValidationResult<()> {
     for tx_in in &tx_prefix.inputs {
-        if !tx_in
-            .ring
-            .windows(2)
-            .all(|w| w[0].public_key < w[1].public_key)
-        {
-            return Err(TransactionValidationError::UnsortedRingElements);
-        }
+        check_sorted(
+            &tx_in.ring,
+            |a, b| a.public_key < b.public_key,
+            TransactionValidationError::UnsortedRingElements,
+        )?;
     }
 
     Ok(())
@@ -177,38 +172,38 @@ fn validate_ring_elements_are_sorted(tx_prefix: &TxPrefix) -> TransactionValidat
 /// Inputs must be sorted by the public key of the first ring element of each
 /// input.
 fn validate_inputs_are_sorted(tx_prefix: &TxPrefix) -> TransactionValidationResult<()> {
-    let inputs_are_sorted = tx_prefix.inputs.windows(2).all(|w| {
-        !w[0].ring.is_empty()
-            && !w[1].ring.is_empty()
-            && w[0].ring[0].public_key < w[1].ring[0].public_key
-    });
-    if !inputs_are_sorted {
-        return Err(TransactionValidationError::UnsortedInputs);
-    }
-
-    Ok(())
+    check_sorted(
+        &tx_prefix.inputs,
+        |a, b| {
+            !a.ring.is_empty() && !b.ring.is_empty() && a.ring[0].public_key < b.ring[0].public_key
+        },
+        TransactionValidationError::UnsortedInputs,
+    )
 }
 
-/// All key images within the transaction must be unique.
-fn validate_key_images_are_unique(tx: &Tx) -> TransactionValidationResult<()> {
-    let mut uniques = HashSet::default();
-    for key_image in tx.key_images() {
-        if !uniques.insert(key_image) {
-            return Err(TransactionValidationError::DuplicateKeyImages);
-        }
-    }
-    Ok(())
+/// Transaction outputs must be sorted by public_key, ascending.
+fn validate_outputs_are_sorted(tx_prefix: &TxPrefix) -> TransactionValidationResult<()> {
+    check_sorted(
+        &tx_prefix.outputs,
+        |a, b| a.public_key < b.public_key,
+        TransactionValidationError::UnsortedOutputs,
+    )
 }
 
 /// All output public keys within the transaction must be unique.
 fn validate_outputs_public_keys_are_unique(tx: &Tx) -> TransactionValidationResult<()> {
-    let mut uniques = HashSet::default();
-    for public_key in tx.output_public_keys() {
-        if !uniques.insert(public_key) {
-            return Err(TransactionValidationError::DuplicateOutputPublicKey);
-        }
-    }
-    Ok(())
+    check_unique(
+        &tx.output_public_keys(),
+        TransactionValidationError::DuplicateOutputPublicKey,
+    )
+}
+
+/// All key images within the transaction must be unique.
+fn validate_key_images_are_unique(tx: &Tx) -> TransactionValidationResult<()> {
+    check_unique(
+        &tx.key_images(),
+        TransactionValidationError::DuplicateKeyImages,
+    )
 }
 
 /// All outputs have no memo (new-style TxOuts (Post MCIP #3) are rejected)
@@ -414,23 +409,35 @@ pub fn validate_tombstone(
     Ok(())
 }
 
+fn check_sorted<T>(
+    values: &[T],
+    ordered: fn(&T, &T) -> bool,
+    err: TransactionValidationError,
+) -> TransactionValidationResult<()> {
+    if !values.windows(2).all(|pair| ordered(&pair[0], &pair[1])) {
+        Err(err)
+    } else {
+        Ok(())
+    }
+}
+
+fn check_unique<T: Eq + core::hash::Hash>(
+    values: &[T],
+    err: TransactionValidationError,
+) -> TransactionValidationResult<()> {
+    let mut uniques = HashSet::default();
+    for x in values {
+        if !uniques.insert(x) {
+            return Err(err);
+        }
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
-
-    extern crate alloc;
-
-    use alloc::vec::Vec;
-
-    use crate::{
-        constants::RING_SIZE,
-        tokens::Mob,
-        tx::{Tx, TxOutMembershipHash, TxOutMembershipProof},
-        validation::error::TransactionValidationError,
-        Token,
-    };
-
-    use crate::membership_proofs::Range;
+    use crate::{membership_proofs::Range, tokens::Mob, tx::TxOutMembershipHash, Token};
     use mc_crypto_keys::{CompressedRistrettoPublic, ReprBytes};
     use mc_ledger_db::{Ledger, LedgerDB};
     use mc_transaction_core_test_utils::{
@@ -867,6 +874,46 @@ mod tests {
         }
     }
 
+    #[test]
+    /// Should reject a transaction with unsorted outputs.
+    fn test_validate_outputs_are_sorted() {
+        for block_version in BlockVersion::iterator() {
+            let (tx, _ledger) = create_test_tx(block_version);
+
+            let mut output_a = tx.prefix.outputs.get(0).unwrap().clone();
+            output_a.public_key = CompressedRistrettoPublic::from(&[1u8; 32]);
+
+            let mut output_b = output_a.clone();
+            output_b.public_key = CompressedRistrettoPublic::from(&[2u8; 32]);
+
+            assert!(output_a.public_key < output_b.public_key);
+
+            {
+                let mut tx_prefix = tx.prefix.clone();
+                // A single output is trivially sorted.
+                tx_prefix.outputs = vec![output_a.clone()];
+                assert_eq!(validate_outputs_are_sorted(&tx_prefix), Ok(()));
+            }
+
+            {
+                let mut tx_prefix = tx.prefix.clone();
+                // Outputs sorted by public_key, ascending.
+                tx_prefix.outputs = vec![output_a.clone(), output_b.clone()];
+                assert_eq!(validate_outputs_are_sorted(&tx_prefix), Ok(()));
+            }
+
+            {
+                let mut tx_prefix = tx.prefix.clone();
+                // Outputs are not correctly sorted.
+                tx_prefix.outputs = vec![output_b.clone(), output_a.clone()];
+                assert_eq!(
+                    validate_outputs_are_sorted(&tx_prefix),
+                    Err(TransactionValidationError::UnsortedOutputs)
+                );
+            }
+        }
+    }
+
     #[test]
     /// validate_key_images_are_unique rejects duplicate key image.
     fn test_validate_key_images_are_unique_rejects_duplicate() {