Fixup transaction validation for 1.2.0 (#1460)

* Fixup transaction validation for 1.2.0

Per James, I actually got the release plan wrong for the RTH stuff.
The idea was supposed to be, update all readers first, in 1.2.0,
and only in the next release may writing memos be allowed.

This means that, transaction validation should enforce for now
that memos do not exist in the chain.

In MCIP #26 we may introduce a mechanism to allow turning on the
memos. That is not yet implemented. We will continue to work on that
but by merging this to master now, we make it possible to branch
for release now, and create clients capable of reading memos, while
we could later make a release that accepts writing memos in the
future.

We can discuss separately if the scope for 1.2.0 should be to
include a `block_version`-aware `TransactionBuilder`, it's possible
that it should be and then we aren't ready to branch. But then this
commit is still a fine basis for future iteration.

* Update transaction/core/src/validation/validate.rs

Co-authored-by: Remoun Metyas <[email protected]>

* cargo fmt

* fix tests that broke when we fixed other tests

* fixup tests (there is only one output in this tx)

Co-authored-by: Remoun Metyas <[email protected]>

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" (MCIP 26), then this
+    // function would become block-version aware and this could 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,17 +501,46 @@ 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!(tx.prefix.outputs.first_mut().unwrap().e_memo.is_none());
         assert_eq!(
             validate_memos_exist(&tx),
             Err(TransactionValidationError::MissingMemo)
         );
+
+        for ref mut output in tx.prefix.outputs.iter_mut() {
+            output.e_memo = Some(Default::default());
+        }
+
+        assert_eq!(validate_memos_exist(&tx), Ok(()));
+    }
+
+    #[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!(tx.prefix.outputs.first_mut().unwrap().e_memo.is_none());
+        assert_eq!(validate_no_memos_exist(&tx), Ok(()));
+
+        tx.prefix.outputs.first_mut().unwrap().e_memo = Some(Default::default());
+
+        assert_eq!(
+            validate_no_memos_exist(&tx),
+            Err(TransactionValidationError::MemosNotAllowed)
+        );
+
+        for ref mut output in tx.prefix.outputs.iter_mut() {
+            output.e_memo = Some(Default::default());
+        }
+
+        assert_eq!(
+            validate_no_memos_exist(&tx),
+            Err(TransactionValidationError::MemosNotAllowed)
+        );
     }
 
     #[test]

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.