Fixup transaction validation for 1.2.0

consensus/api/proto/consensus_common.proto

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

consensus/api/src/conversions.rs

@@ -9,122 +9,90 @@
 //! `mc_transaction_core::validation::TransactionValidationError`.
 
 use crate::consensus_common::ProposeTxResult;
-use mc_transaction_core::{ring_signature, validation::TransactionValidationError};
+use mc_transaction_core::{ring_signature, validation::TransactionValidationError as Error};
 use std::convert::{From, TryInto};
 
 /// Convert TransactionValidationError --> ProposeTxResult.
-impl From<TransactionValidationError> for ProposeTxResult {
-    fn from(src: TransactionValidationError) -> Self {
+impl From<Error> for ProposeTxResult {
+    fn from(src: Error) -> Self {
         match src {
-            TransactionValidationError::InputsProofsLengthMismatch => {
-                Self::InputsProofsLengthMismatch
-            }
-            TransactionValidationError::NoInputs => Self::NoInputs,
-            TransactionValidationError::TooManyInputs => Self::TooManyInputs,
-            TransactionValidationError::InsufficientInputSignatures => {
-                Self::InsufficientInputSignatures
-            }
-            TransactionValidationError::InvalidInputSignature => Self::InvalidInputSignature,
-            TransactionValidationError::InvalidTransactionSignature(_e) => {
-                Self::InvalidTransactionSignature
-            }
-            TransactionValidationError::InvalidRangeProof => Self::InvalidRangeProof,
-            TransactionValidationError::InsufficientRingSize => Self::InsufficientRingSize,
-            TransactionValidationError::TombstoneBlockExceeded => Self::TombstoneBlockExceeded,
-            TransactionValidationError::TombstoneBlockTooFar => Self::TombstoneBlockTooFar,
-            TransactionValidationError::NoOutputs => Self::NoOutputs,
-            TransactionValidationError::TooManyOutputs => Self::TooManyOutputs,
-            TransactionValidationError::ExcessiveRingSize => Self::ExcessiveRingSize,
-            TransactionValidationError::DuplicateRingElements => Self::DuplicateRingElements,
-            TransactionValidationError::UnsortedRingElements => Self::UnsortedRingElements,
-            TransactionValidationError::UnequalRingSizes => Self::UnequalRingSizes,
-            TransactionValidationError::UnsortedKeyImages => Self::UnsortedKeyImages,
-            TransactionValidationError::ContainsSpentKeyImage => Self::ContainsSpentKeyImage,
-            TransactionValidationError::DuplicateKeyImages => Self::DuplicateKeyImages,
-            TransactionValidationError::DuplicateOutputPublicKey => Self::DuplicateOutputPublicKey,
-            TransactionValidationError::ContainsExistingOutputPublicKey => {
-                Self::ContainsExistingOutputPublicKey
-            }
-            TransactionValidationError::MissingTxOutMembershipProof => {
-                Self::MissingTxOutMembershipProof
-            }
-            TransactionValidationError::InvalidTxOutMembershipProof => {
-                Self::InvalidTxOutMembershipProof
-            }
-            TransactionValidationError::InvalidRistrettoPublicKey => {
-                Self::InvalidRistrettoPublicKey
-            }
-            TransactionValidationError::InvalidLedgerContext => Self::InvalidLedgerContext,
-            TransactionValidationError::Ledger(_) => Self::Ledger,
-            TransactionValidationError::MembershipProofValidationError => {
-                Self::MembershipProofValidationError
-            }
-            TransactionValidationError::TxFeeError => Self::TxFeeError,
-            TransactionValidationError::KeyError => Self::KeyError,
-            TransactionValidationError::UnsortedInputs => Self::UnsortedInputs,
-            TransactionValidationError::MissingMemo => Self::MissingMemo,
+            Error::InputsProofsLengthMismatch => Self::InputsProofsLengthMismatch,
+            Error::NoInputs => Self::NoInputs,
+            Error::TooManyInputs => Self::TooManyInputs,
+            Error::InsufficientInputSignatures => Self::InsufficientInputSignatures,
+            Error::InvalidInputSignature => Self::InvalidInputSignature,
+            Error::InvalidTransactionSignature(_e) => Self::InvalidTransactionSignature,
+            Error::InvalidRangeProof => Self::InvalidRangeProof,
+            Error::InsufficientRingSize => Self::InsufficientRingSize,
+            Error::TombstoneBlockExceeded => Self::TombstoneBlockExceeded,
+            Error::TombstoneBlockTooFar => Self::TombstoneBlockTooFar,
+            Error::NoOutputs => Self::NoOutputs,
+            Error::TooManyOutputs => Self::TooManyOutputs,
+            Error::ExcessiveRingSize => Self::ExcessiveRingSize,
+            Error::DuplicateRingElements => Self::DuplicateRingElements,
+            Error::UnsortedRingElements => Self::UnsortedRingElements,
+            Error::UnequalRingSizes => Self::UnequalRingSizes,
+            Error::UnsortedKeyImages => Self::UnsortedKeyImages,
+            Error::ContainsSpentKeyImage => Self::ContainsSpentKeyImage,
+            Error::DuplicateKeyImages => Self::DuplicateKeyImages,
+            Error::DuplicateOutputPublicKey => Self::DuplicateOutputPublicKey,
+            Error::ContainsExistingOutputPublicKey => Self::ContainsExistingOutputPublicKey,
+            Error::MissingTxOutMembershipProof => Self::MissingTxOutMembershipProof,
+            Error::InvalidTxOutMembershipProof => Self::InvalidTxOutMembershipProof,
+            Error::InvalidRistrettoPublicKey => Self::InvalidRistrettoPublicKey,
+            Error::InvalidLedgerContext => Self::InvalidLedgerContext,
+            Error::Ledger(_) => Self::Ledger,
+            Error::MembershipProofValidationError => Self::MembershipProofValidationError,
+            Error::TxFeeError => Self::TxFeeError,
+            Error::KeyError => Self::KeyError,
+            Error::UnsortedInputs => Self::UnsortedInputs,
+            Error::MissingMemo => Self::MissingMemo,
+            Error::MemosNotAllowed => Self::MemosNotAllowed,
         }
     }
 }
 
 /// Convert ProposeTxResult --> TransactionValidationError.
-impl TryInto<TransactionValidationError> for ProposeTxResult {
+impl TryInto<Error> for ProposeTxResult {
     type Error = &'static str;
 
-    fn try_into(self) -> Result<TransactionValidationError, Self::Error> {
+    fn try_into(self) -> Result<Error, Self::Error> {
         match self {
             Self::Ok => Err("Ok value cannot be convererted into TransactionValidationError"),
-            Self::InputsProofsLengthMismatch => {
-                Ok(TransactionValidationError::InputsProofsLengthMismatch)
-            }
-            Self::NoInputs => Ok(TransactionValidationError::NoInputs),
-            Self::TooManyInputs => Ok(TransactionValidationError::TooManyInputs),
-            Self::InsufficientInputSignatures => {
-                Ok(TransactionValidationError::InsufficientInputSignatures)
-            }
-            Self::InvalidInputSignature => Ok(TransactionValidationError::InvalidInputSignature),
-            Self::InvalidTransactionSignature => {
-                Ok(TransactionValidationError::InvalidTransactionSignature(
-                    ring_signature::Error::InvalidSignature,
-                ))
-            }
-            Self::InvalidRangeProof => Ok(TransactionValidationError::InvalidRangeProof),
-            Self::InsufficientRingSize => Ok(TransactionValidationError::InsufficientRingSize),
-            Self::TombstoneBlockExceeded => Ok(TransactionValidationError::TombstoneBlockExceeded),
-            Self::TombstoneBlockTooFar => Ok(TransactionValidationError::TombstoneBlockTooFar),
-            Self::NoOutputs => Ok(TransactionValidationError::NoOutputs),
-            Self::TooManyOutputs => Ok(TransactionValidationError::TooManyOutputs),
-            Self::ExcessiveRingSize => Ok(TransactionValidationError::ExcessiveRingSize),
-            Self::DuplicateRingElements => Ok(TransactionValidationError::DuplicateRingElements),
-            Self::UnsortedRingElements => Ok(TransactionValidationError::UnsortedRingElements),
-            Self::UnequalRingSizes => Ok(TransactionValidationError::UnequalRingSizes),
-            Self::UnsortedKeyImages => Ok(TransactionValidationError::UnsortedKeyImages),
-            Self::ContainsSpentKeyImage => Ok(TransactionValidationError::ContainsSpentKeyImage),
-            Self::DuplicateKeyImages => Ok(TransactionValidationError::DuplicateKeyImages),
-            Self::DuplicateOutputPublicKey => {
-                Ok(TransactionValidationError::DuplicateOutputPublicKey)
-            }
-            Self::ContainsExistingOutputPublicKey => {
-                Ok(TransactionValidationError::ContainsExistingOutputPublicKey)
-            }
-            Self::MissingTxOutMembershipProof => {
-                Ok(TransactionValidationError::MissingTxOutMembershipProof)
-            }
-            Self::InvalidTxOutMembershipProof => {
-                Ok(TransactionValidationError::InvalidTxOutMembershipProof)
-            }
-            Self::InvalidRistrettoPublicKey => {
-                Ok(TransactionValidationError::InvalidRistrettoPublicKey)
-            }
-            Self::InvalidLedgerContext => Ok(TransactionValidationError::InvalidLedgerContext),
-            Self::Ledger => Ok(TransactionValidationError::Ledger(String::default())),
-            Self::MembershipProofValidationError => {
-                Ok(TransactionValidationError::MembershipProofValidationError)
-            }
-            Self::TxFeeError => Ok(TransactionValidationError::TxFeeError),
-            Self::KeyError => Ok(TransactionValidationError::KeyError),
-            Self::UnsortedInputs => Ok(TransactionValidationError::UnsortedInputs),
-            Self::MissingMemo => Ok(TransactionValidationError::MissingMemo),
+            Self::InputsProofsLengthMismatch => Ok(Error::InputsProofsLengthMismatch),
+            Self::NoInputs => Ok(Error::NoInputs),
+            Self::TooManyInputs => Ok(Error::TooManyInputs),
+            Self::InsufficientInputSignatures => Ok(Error::InsufficientInputSignatures),
+            Self::InvalidInputSignature => Ok(Error::InvalidInputSignature),
+            Self::InvalidTransactionSignature => Ok(Error::InvalidTransactionSignature(
+                ring_signature::Error::InvalidSignature,
+            )),
+            Self::InvalidRangeProof => Ok(Error::InvalidRangeProof),
+            Self::InsufficientRingSize => Ok(Error::InsufficientRingSize),
+            Self::TombstoneBlockExceeded => Ok(Error::TombstoneBlockExceeded),
+            Self::TombstoneBlockTooFar => Ok(Error::TombstoneBlockTooFar),
+            Self::NoOutputs => Ok(Error::NoOutputs),
+            Self::TooManyOutputs => Ok(Error::TooManyOutputs),
+            Self::ExcessiveRingSize => Ok(Error::ExcessiveRingSize),
+            Self::DuplicateRingElements => Ok(Error::DuplicateRingElements),
+            Self::UnsortedRingElements => Ok(Error::UnsortedRingElements),
+            Self::UnequalRingSizes => Ok(Error::UnequalRingSizes),
+            Self::UnsortedKeyImages => Ok(Error::UnsortedKeyImages),
+            Self::ContainsSpentKeyImage => Ok(Error::ContainsSpentKeyImage),
+            Self::DuplicateKeyImages => Ok(Error::DuplicateKeyImages),
+            Self::DuplicateOutputPublicKey => Ok(Error::DuplicateOutputPublicKey),
+            Self::ContainsExistingOutputPublicKey => Ok(Error::ContainsExistingOutputPublicKey),
+            Self::MissingTxOutMembershipProof => Ok(Error::MissingTxOutMembershipProof),
+            Self::InvalidTxOutMembershipProof => Ok(Error::InvalidTxOutMembershipProof),
+            Self::InvalidRistrettoPublicKey => Ok(Error::InvalidRistrettoPublicKey),
+            Self::InvalidLedgerContext => Ok(Error::InvalidLedgerContext),
+            Self::Ledger => Ok(Error::Ledger(String::default())),
+            Self::MembershipProofValidationError => Ok(Error::MembershipProofValidationError),
+            Self::TxFeeError => Ok(Error::TxFeeError),
+            Self::KeyError => Ok(Error::KeyError),
+            Self::UnsortedInputs => Ok(Error::UnsortedInputs),
+            Self::MissingMemo => Ok(Error::MissingMemo),
+            Self::MemosNotAllowed => Ok(Error::MemosNotAllowed),
         }
     }
 }

transaction/core/src/validation/error.rs

@@ -125,6 +125,9 @@ pub enum TransactionValidationError {
 
     /// A TxOut is missing the required memo field
     MissingMemo,
+
+    /// A TxOut includes a memo, but this is not allowed yet
+    MemosNotAllowed,
 }
 
 impl From<mc_crypto_keys::KeyError> for TransactionValidationError {

transaction/core/src/validation/validate.rs

@@ -38,10 +38,6 @@ pub fn validate<R: RngCore + CryptoRng>(
 
     validate_number_of_outputs(&tx.prefix, MAX_OUTPUTS)?;
 
-    // Bring this back in Issue #905 (Make memos mandatory)
-    // https://github.com/mobilecoinfoundation/mobilecoin/issues/905
-    // validate_memos_exist(tx)?;
-
     validate_ring_sizes(&tx.prefix, RING_SIZE)?;
 
     validate_ring_elements_are_unique(&tx.prefix)?;
@@ -65,6 +61,13 @@ pub fn validate<R: RngCore + CryptoRng>(
     // Note: The transaction must not contain a Key Image that has previously been
     // spent. This must be checked outside the enclave.
 
+    // In the 1.2 release, it is planned that clients will know to read memos,
+    // but memos will not be allowed to exist in the chain until the next release.
+    // If we implement "block-version-based protocol evolution, then this function
+    // would become block-version aware and this coudl become a branch.
+    validate_no_memos_exist(tx)?;
+    // validate_memos_exist(tx)?;
+
     Ok(())
 }
 
@@ -195,7 +198,20 @@ fn validate_outputs_public_keys_are_unique(tx: &Tx) -> TransactionValidationResu
     Ok(())
 }
 
-/// All outputs have a memo (old-style TxOuts are rejected)
+/// All outputs have no memo (new-style TxOuts (Post MCIP #3) are rejected)
+fn validate_no_memos_exist(tx: &Tx) -> TransactionValidationResult<()> {
+    if tx
+        .prefix
+        .outputs
+        .iter()
+        .any(|output| output.e_memo.is_some())
+    {
+        return Err(TransactionValidationError::MemosNotAllowed);
+    }
+    Ok(())
+}
+
+/// All outputs have a memo (old-style TxOuts (Pre MCIP #3) are rejected)
 ///
 /// Note: This is only under test for now, and can become live
 /// at the time that we address mobilecoinfoundation/mobilecoin/issues/905
@@ -393,6 +409,8 @@ pub fn validate_tombstone(
 
 #[cfg(test)]
 mod tests {
+    use super::*;
+
     extern crate alloc;
 
     use alloc::vec::Vec;
@@ -401,16 +419,7 @@ mod tests {
         constants::RING_SIZE,
         tokens::Mob,
         tx::{Tx, TxOutMembershipHash, TxOutMembershipProof},
-        validation::{
-            error::TransactionValidationError,
-            validate::{
-                validate_inputs_are_sorted, validate_key_images_are_unique,
-                validate_membership_proofs, validate_memos_exist, validate_number_of_inputs,
-                validate_number_of_outputs, validate_outputs_public_keys_are_unique,
-                validate_ring_elements_are_unique, validate_ring_sizes, validate_signature,
-                validate_tombstone, validate_transaction_fee, MAX_TOMBSTONE_BLOCKS,
-            },
-        },
+        validation::error::TransactionValidationError,
         Token,
     };
 
@@ -492,19 +501,45 @@ mod tests {
     }
 
     #[test]
-    // Should return MissingMemo when memos are missing in the outputs
+    // Should return MissingMemo when memos are missing in any the outputs
     fn test_validate_memos_exist() {
         let (mut tx, _) = create_test_tx();
-        for ref mut output in tx.prefix.outputs.iter_mut() {
-            output.e_memo = None;
-        }
+
+        assert_eq!(validate_memos_exist(&tx), Ok(()));
+
+        tx.prefix.outputs.first_mut().unwrap().e_memo = None;
 
         assert_eq!(
             validate_memos_exist(&tx),
             Err(TransactionValidationError::MissingMemo)
         );
     }
 
+    #[test]
+    // Should return MemosNotAllowed when memos are present in any of the outputs
+    fn test_validate_no_memos_exist() {
+        let (mut tx, _) = create_test_tx();
+
+        assert_eq!(
+            validate_no_memos_exist(&tx),
+            Err(TransactionValidationError::MemosNotAllowed)
+        );
+
+        let len = tx.prefix.outputs.len();
+        for ref mut output in tx.prefix.outputs.iter_mut().take(len - 1) {
+            output.e_memo = None;
+        }
+
+        assert_eq!(
+            validate_no_memos_exist(&tx),
+            Err(TransactionValidationError::MemosNotAllowed)
+        );
+
+        tx.prefix.outputs.last_mut().unwrap().e_memo = None;
+
+        assert_eq!(validate_no_memos_exist(&tx), Ok(()));
+    }
+
     #[test]
     // Should return Ok(()) when the Tx's membership proofs are correct and agree
     // with ledger.

transaction/core/test-utils/src/lib.rs

@@ -15,7 +15,7 @@ pub use mc_transaction_core::{
     tx::{Tx, TxOut, TxOutMembershipElement, TxOutMembershipHash},
     Block, BlockID, BlockIndex, Token, BLOCK_VERSION,
 };
-use mc_transaction_std::{EmptyMemoBuilder, InputCredentials, TransactionBuilder};
+use mc_transaction_std::{InputCredentials, NoMemoBuilder, TransactionBuilder};
 use mc_util_from_random::FromRandom;
 use rand::{seq::SliceRandom, Rng};
 use tempdir::TempDir;
@@ -88,7 +88,7 @@ pub fn create_transaction_with_amount<L: Ledger, R: RngCore + CryptoRng>(
     rng: &mut R,
 ) -> Tx {
     let mut transaction_builder =
-        TransactionBuilder::new(MockFogResolver::default(), EmptyMemoBuilder::default());
+        TransactionBuilder::new(MockFogResolver::default(), NoMemoBuilder::default());
 
     // The first transaction in the origin block should contain enough outputs to
     // use as mixins.