Makes unified container parsing enforce typecode order.

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

components/zcash_address/src/kind/unified.rs

@@ -140,7 +140,6 @@ pub(crate) mod private {
     use crate::Network;
     use std::{
         cmp,
-        collections::HashSet,
         convert::{TryFrom, TryInto},
         io::Write,
     };
@@ -283,23 +282,32 @@ pub(crate) mod private {
         }
 
         /// A private function that constructs a unified container with the
-        /// items in their given order.
+        /// specified items, which must be in ascending typecode order.
         fn try_from_items_internal(items: Vec<Self::Item>) -> Result<Self, ParseError> {
-            let mut typecodes = HashSet::with_capacity(items.len());
+            assert!(u32::from(Typecode::P2sh) == u32::from(Typecode::P2pkh) + 1);
+
+            let mut only_transparent = true;
+            let mut prev_code = None; // less than any Some
             for item in &items {
                 let t = item.typecode();
-                if typecodes.contains(&t) {
+                let t_code = Some(u32::from(t));
+                if t_code < prev_code {
+                    return Err(ParseError::InvalidEncoding(
+                        "Receivers out of order.".to_owned(),
+                    ));
+                } else if t_code == prev_code {
                     return Err(ParseError::DuplicateTypecode(t));
-                } else if (t == Typecode::P2pkh && typecodes.contains(&Typecode::P2sh))
-                    || (t == Typecode::P2sh && typecodes.contains(&Typecode::P2pkh))
-                {
+                } else if t == Typecode::P2sh && prev_code == Some(u32::from(Typecode::P2pkh)) {
+                    // P2pkh and P2sh can only be in that order and next to each other,
+                    // otherwise we would detect an out-of-order or duplicate typecode.
                     return Err(ParseError::BothP2phkAndP2sh);
                 } else {
-                    typecodes.insert(t);
+                    prev_code = t_code;
+                    only_transparent = only_transparent && t.is_transparent();
                 }
             }
 
-            if typecodes.iter().all(|t| t.is_transparent()) {
+            if only_transparent {
                 Err(ParseError::OnlyTransparent)
             } else {
                 // All checks pass!
@@ -327,21 +335,7 @@ pub trait Encoding: private::SealedContainer {
     /// * the item list may not contain two items having the same typecode
     /// * the item list may not contain only a single transparent item
     fn try_from_items(mut items: Vec<Self::Item>) -> Result<Self, ParseError> {
-        items.sort_unstable_by_key(|i| i.typecode());
-        Self::try_from_items_internal(items)
-    }
-
-    /// Constructs a value of a unified container type from a vector
-    /// of container items, preserving the order of the provided vector
-    /// in the serialized form, potentially contravening the ordering
-    /// recommended by ZIP 316.
-    ///
-    /// This function will return an error in the case that the following ZIP 316
-    /// invariants concerning the composition of a unified container are
-    /// violated:
-    /// * the item list may not contain two items having the same typecode
-    /// * the item list may not contain only a single transparent item
-    fn try_from_items_preserving_order(items: Vec<Self::Item>) -> Result<Self, ParseError> {
+        items.sort_unstable_by_key(|i| <u32>::from(i.typecode()));
         Self::try_from_items_internal(items)
     }
 

components/zcash_address/src/kind/unified/address.rs

@@ -164,8 +164,9 @@ mod tests {
     }
 
     fn arb_unified_address_for_typecodes(
-        typecodes: Vec<Typecode>,
+        mut typecodes: Vec<Typecode>,
     ) -> impl Strategy<Value = Vec<Receiver>> {
+        typecodes.sort_unstable_by_key(|tc| <u32>::from(*tc));
         typecodes
             .into_iter()
             .map(|tc| match tc {
@@ -296,6 +297,21 @@ mod tests {
         );
     }
 
+    #[test]
+    fn addresses_out_of_order() {
+        // Construct and serialize an invalid UA. This must be done using private
+        // methods, as the public API does not permit construction of such invalid values.
+        let ua = Address(vec![Receiver::Sapling([0; 43]), Receiver::P2pkh([0; 20])]);
+        let encoded = ua.to_jumbled_bytes(Address::MAINNET);
+        // ensure that decoding catches the error
+        assert_eq!(
+            Address::parse_internal(Address::MAINNET, &encoded[..]),
+            Err(ParseError::InvalidEncoding(
+                "Receivers out of order.".to_owned()
+            ))
+        );
+    }
+
     #[test]
     fn only_transparent() {
         // Encoding of `Address(vec![Receiver::P2pkh([0; 20])])`.

components/zcash_address/src/kind/unified/fvk.rs

@@ -142,7 +142,10 @@ mod tests {
 
     use super::{Fvk, ParseError, Typecode, Ufvk};
     use crate::{
-        kind::unified::{private::SealedContainer, Container, Encoding},
+        kind::unified::{
+            private::{SealedContainer, SealedItem},
+            Container, Encoding,
+        },
         Network,
     };
 
@@ -184,11 +187,16 @@ mod tests {
     }
 
     fn arb_shielded_fvk() -> impl Strategy<Value = Vec<Fvk>> {
-        prop_oneof![
+        let p = prop_oneof![
             vec![arb_sapling_fvk().boxed()],
             vec![arb_orchard_fvk().boxed()],
             vec![arb_orchard_fvk().boxed(), arb_sapling_fvk().boxed()],
-        ]
+        ];
+
+        p.prop_map(|mut items| {
+            items.sort_unstable_by_key(|item| <u32>::from(item.typecode()));
+            items
+        })
     }
 
     fn arb_transparent_fvk() -> BoxedStrategy<Fvk> {
@@ -200,7 +208,9 @@ mod tests {
             shielded in arb_shielded_fvk(),
             transparent in prop::option::of(arb_transparent_fvk()),
         ) -> Ufvk {
-            Ufvk(shielded.into_iter().chain(transparent).collect())
+            let mut items = shielded.into_iter().chain(transparent).collect::<Vec<_>>();
+            items.sort_unstable_by_key(|item| <u32>::from(item.typecode()));
+            Ufvk(items)
         }
     }
 

components/zcash_address/src/kind/unified/ivk.rs

@@ -151,7 +151,10 @@ mod tests {
 
     use super::{Ivk, ParseError, Typecode, Uivk};
     use crate::{
-        kind::unified::{private::SealedContainer, Container, Encoding},
+        kind::unified::{
+            private::{SealedContainer, SealedItem},
+            Container, Encoding,
+        },
         Network,
     };
 
@@ -174,14 +177,19 @@ mod tests {
     }
 
     fn arb_shielded_ivk() -> impl Strategy<Value = Vec<Ivk>> {
-        prop_oneof![
+        let p = prop_oneof![
             vec![uniform64().prop_map(Ivk::Sapling)],
             vec![uniform64().prop_map(Ivk::Orchard)],
             vec![
                 uniform64().prop_map(Ivk::Orchard as fn([u8; 64]) -> Ivk),
                 uniform64().prop_map(Ivk::Sapling)
             ],
-        ]
+        ];
+
+        p.prop_map(|mut items| {
+            items.sort_unstable_by_key(|item| <u32>::from(item.typecode()));
+            items
+        })
     }
 
     fn arb_transparent_ivk() -> impl Strategy<Value = Ivk> {
@@ -193,7 +201,9 @@ mod tests {
             shielded in arb_shielded_ivk(),
             transparent in prop::option::of(arb_transparent_ivk()),
         ) -> Uivk {
-            Uivk(shielded.into_iter().chain(transparent).collect())
+            let mut items = shielded.into_iter().chain(transparent).collect::<Vec<_>>();
+            items.sort_unstable_by_key(|item| <u32>::from(item.typecode()));
+            Uivk(items)
         }
     }