From 9143f969adbb19364421f960d211a7eafcee337a Mon Sep 17 00:00:00 2001 From: Chris Beck Date: Tue, 8 Feb 2022 15:57:16 -0700 Subject: [PATCH] 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 * 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 --- consensus/api/proto/consensus_common.proto | 1 + consensus/api/src/conversions.rs | 174 ++++++++------------ transaction/core/src/validation/error.rs | 3 + transaction/core/src/validation/validate.rs | 76 ++++++--- transaction/core/test-utils/src/lib.rs | 4 +- 5 files changed, 134 insertions(+), 124 deletions(-) diff --git a/consensus/api/proto/consensus_common.proto b/consensus/api/proto/consensus_common.proto index 8ffc3a9c45..75e6e0330f 100644 --- a/consensus/api/proto/consensus_common.proto +++ b/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. diff --git a/consensus/api/src/conversions.rs b/consensus/api/src/conversions.rs index 2b0bae2ffe..a38ca5e9b5 100644 --- a/consensus/api/src/conversions.rs +++ b/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 for ProposeTxResult { - fn from(src: TransactionValidationError) -> Self { +impl From 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 for ProposeTxResult { +impl TryInto for ProposeTxResult { type Error = &'static str; - fn try_into(self) -> Result { + fn try_into(self) -> Result { 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), } } } diff --git a/transaction/core/src/validation/error.rs b/transaction/core/src/validation/error.rs index 32330af913..89aa1f117a 100644 --- a/transaction/core/src/validation/error.rs +++ b/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 for TransactionValidationError { diff --git a/transaction/core/src/validation/validate.rs b/transaction/core/src/validation/validate.rs index 626044b9ae..343f2f4d47 100644 --- a/transaction/core/src/validation/validate.rs +++ b/transaction/core/src/validation/validate.rs @@ -38,10 +38,6 @@ pub fn validate( 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( // 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] diff --git a/transaction/core/test-utils/src/lib.rs b/transaction/core/test-utils/src/lib.rs index 08f370e45a..97ebd8348b 100644 --- a/transaction/core/test-utils/src/lib.rs +++ b/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( 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.