Use NonEmpty for SenderSignedData (#17700)

Removes the need for several asserts, since all failures will now be
deserialization errors

crates/sui-core/src/unit_tests/transaction_tests.rs

@@ -18,8 +18,7 @@ use sui_types::{
     multisig::{MultiSig, MultiSigPublicKey},
     signature::GenericSignature,
     transaction::{
-        AuthenticatorStateUpdate, GenesisTransaction, TransactionDataAPI, TransactionExpiration,
-        TransactionKind,
+        AuthenticatorStateUpdate, GenesisTransaction, TransactionDataAPI, TransactionKind,
     },
     utils::{load_test_vectors, to_sender_signed_transaction},
     zk_login_authenticator::ZkLoginAuthenticator,
@@ -116,79 +115,6 @@ async fn test_handle_transfer_transaction_extra_signature() {
     .await;
 }
 
-// TODO: verify that these cases are not exploitable via consensus input
-#[sim_test]
-async fn test_empty_sender_signed_data() {
-    do_transaction_test(
-        0,
-        |_| {},
-        |tx| {
-            let data = tx.data_mut_for_testing();
-            data.inner_vec_mut_for_testing().clear();
-        },
-        |err| {
-            assert_matches!(
-                err,
-                SuiError::UserInputError {
-                    error: UserInputError::Unsupported { .. }
-                }
-            );
-        },
-    )
-    .await;
-}
-
-#[sim_test]
-async fn test_multiple_sender_signed_data() {
-    do_transaction_test(
-        0,
-        |_| {},
-        |tx| {
-            let data = tx.data_mut_for_testing();
-            let tx_vec = data.inner_vec_mut_for_testing();
-            assert_eq!(tx_vec.len(), 1);
-            let mut new = tx_vec[0].clone();
-            // make sure second message has unique digest
-            *new.intent_message.value.expiration_mut_for_testing() =
-                TransactionExpiration::Epoch(123);
-            tx_vec.push(new);
-        },
-        |err| {
-            assert_matches!(
-                err,
-                SuiError::UserInputError {
-                    error: UserInputError::Unsupported { .. }
-                }
-            );
-        },
-    )
-    .await;
-}
-
-#[sim_test]
-async fn test_duplicate_sender_signed_data() {
-    do_transaction_test(
-        0,
-        |_| {},
-        |tx| {
-            let data = tx.data_mut_for_testing();
-            let tx_vec = data.inner_vec_mut_for_testing();
-            assert_eq!(tx_vec.len(), 1);
-            let new = tx_vec[0].clone();
-            tx_vec.push(new);
-        },
-        |err| {
-            assert_matches!(
-                err,
-                SuiError::UserInputError {
-                    error: UserInputError::Unsupported { .. }
-                }
-            );
-        },
-    )
-    .await;
-}
-
 #[sim_test]
 async fn test_empty_gas_data() {
     do_transaction_test_skip_cert_checks(
@@ -1563,29 +1489,7 @@ async fn test_handle_certificate_errors() {
         &*authority_state.secret,
     );
 
-    let mut empty_tx = transfer_transaction.clone();
-    let data = empty_tx.data_mut_for_testing();
-    data.inner_vec_mut_for_testing().clear();
-
     let committee = epoch_store.committee().deref().clone();
-    let ct = CertifiedTransaction::new(
-        data.clone(),
-        vec![signed_transaction.auth_sig().clone()],
-        &committee,
-    )
-    .unwrap();
-
-    let err = client
-        .handle_certificate_v2(ct.clone(), Some(socket_addr))
-        .await
-        .unwrap_err();
-
-    assert_matches!(
-        err,
-        SuiError::UserInputError {
-            error: UserInputError::Unsupported(message)
-        } if message == "SenderSignedData must contain exactly one transaction"
-    );
 
     let tx = VerifiedTransaction::new_consensus_commit_prologue(0, 0, 42);
     let ct = CertifiedTransaction::new(

crates/sui-types/src/base_types.rs

@@ -58,6 +58,8 @@ use move_core_types::language_storage::TypeTag;
 use rand::Rng;
 use schemars::JsonSchema;
 use serde::ser::Error;
+use serde::ser::SerializeSeq;
+use serde::Serializer;
 use serde::{Deserialize, Serialize};
 use serde_with::serde_as;
 use shared_crypto::intent::HashingIntentScope;
@@ -1353,3 +1355,83 @@ impl fmt::Display for ObjectType {
         }
     }
 }
+
+// SizeOneVec is a wrapper around Vec<T> that enforces the size of the vec to be 1.
+// This seems pointless, but it allows us to have fields in protocol messages that are
+// current enforced to be of size 1, but might later allow other sizes, and to have
+// that constraint enforced in the serialization/deserialization layer, instead of
+// requiring manual input validation.
+#[derive(Debug, Deserialize, Clone, Eq, PartialEq, Hash, Ord, PartialOrd)]
+#[serde(try_from = "Vec<T>")]
+pub struct SizeOneVec<T> {
+    e: T,
+}
+
+impl<T> SizeOneVec<T> {
+    pub fn new(e: T) -> Self {
+        Self { e }
+    }
+
+    pub fn element(&self) -> &T {
+        &self.e
+    }
+
+    pub fn element_mut(&mut self) -> &mut T {
+        &mut self.e
+    }
+
+    pub fn iter(&self) -> std::iter::Once<&T> {
+        std::iter::once(&self.e)
+    }
+}
+
+impl<T> Serialize for SizeOneVec<T>
+where
+    T: Serialize,
+{
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        let mut seq = serializer.serialize_seq(Some(1))?;
+        seq.serialize_element(&self.e)?;
+        seq.end()
+    }
+}
+
+impl<T> TryFrom<Vec<T>> for SizeOneVec<T> {
+    type Error = anyhow::Error;
+
+    fn try_from(mut v: Vec<T>) -> Result<Self, Self::Error> {
+        if v.len() != 1 {
+            Err(anyhow!("Expected a vec of size 1"))
+        } else {
+            Ok(SizeOneVec {
+                e: v.pop().unwrap(),
+            })
+        }
+    }
+}
+
+#[test]
+fn test_size_one_vec_is_transparent() {
+    let regular = vec![42u8];
+    let size_one = SizeOneVec::new(42u8);
+
+    // Vec -> SizeOneVec serialization is transparent
+    let regular_ser = bcs::to_bytes(&regular).unwrap();
+    let size_one_deser = bcs::from_bytes::<SizeOneVec<u8>>(&regular_ser).unwrap();
+    assert_eq!(size_one, size_one_deser);
+
+    // other direction works too
+    let size_one_ser = bcs::to_bytes(&SizeOneVec::new(43u8)).unwrap();
+    let regular_deser = bcs::from_bytes::<Vec<u8>>(&size_one_ser).unwrap();
+    assert_eq!(regular_deser, vec![43u8]);
+
+    // we get a deserialize error when deserializing a vec with size != 1
+    let empty_ser = bcs::to_bytes(&Vec::<u8>::new()).unwrap();
+    bcs::from_bytes::<SizeOneVec<u8>>(&empty_ser).unwrap_err();
+
+    let size_greater_than_one_ser = bcs::to_bytes(&vec![1u8, 2u8]).unwrap();
+    bcs::from_bytes::<SizeOneVec<u8>>(&size_greater_than_one_ser).unwrap_err();
+}

crates/sui-types/src/transaction.rs

@@ -2160,7 +2160,7 @@ impl TransactionDataAPI for TransactionDataV1 {
 impl TransactionDataV1 {}
 
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)]
-pub struct SenderSignedData(Vec<SenderSignedTransaction>);
+pub struct SenderSignedData(SizeOneVec<SenderSignedTransaction>);
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct SenderSignedTransaction {
@@ -2249,41 +2249,25 @@ impl SenderSignedTransaction {
 
 impl SenderSignedData {
     pub fn new(tx_data: TransactionData, tx_signatures: Vec<GenericSignature>) -> Self {
-        Self(vec![SenderSignedTransaction {
+        Self(SizeOneVec::new(SenderSignedTransaction {
             intent_message: IntentMessage::new(Intent::sui_transaction(), tx_data),
             tx_signatures,
-        }])
-    }
-
-    pub(crate) fn inner_transactions(&self) -> &[SenderSignedTransaction] {
-        &self.0
+        }))
     }
 
     pub fn new_from_sender_signature(tx_data: TransactionData, tx_signature: Signature) -> Self {
-        Self(vec![SenderSignedTransaction {
+        Self(SizeOneVec::new(SenderSignedTransaction {
             intent_message: IntentMessage::new(Intent::sui_transaction(), tx_data),
             tx_signatures: vec![tx_signature.into()],
-        }])
-    }
-
-    pub fn inner_vec_mut_for_testing(&mut self) -> &mut Vec<SenderSignedTransaction> {
-        &mut self.0
+        }))
     }
 
     pub fn inner(&self) -> &SenderSignedTransaction {
-        // assert is safe - SenderSignedTransaction::verify ensures length is 1.
-        assert_eq!(self.0.len(), 1);
-        self.0
-            .first()
-            .expect("SenderSignedData must contain exactly one transaction")
+        self.0.element()
     }
 
     pub fn inner_mut(&mut self) -> &mut SenderSignedTransaction {
-        // assert is safe - SenderSignedTransaction::verify ensures length is 1.
-        assert_eq!(self.0.len(), 1);
-        self.0
-            .get_mut(0)
-            .expect("SenderSignedData must contain exactly one transaction")
+        self.0.element_mut()
     }
 
     // This function does not check validity of the signature
@@ -2354,17 +2338,6 @@ impl SenderSignedData {
 
     /// Validate untrusted user transaction, including its size, input count, command count, etc.
     pub fn validity_check(&self, config: &ProtocolConfig, epoch: EpochId) -> SuiResult {
-        // SenderSignedData must contain exactly one transaction.
-        // Many operations assume this invariant, so it is safest to check this one before anything else.
-        fp_ensure!(
-            self.inner_transactions().len() == 1,
-            SuiError::UserInputError {
-                error: UserInputError::Unsupported(
-                    "SenderSignedData must contain exactly one transaction".to_string()
-                )
-            }
-        );
-
         // CRITICAL!!
         // Users cannot send system transactions.
         let tx_data = &self.transaction_data();