Fix coinbase height deserialization

zebra-chain/src/transparent.rs

@@ -26,7 +26,7 @@ use proptest_derive::Arbitrary;
 #[cfg(any(test, feature = "proptest-impl"))]
 mod arbitrary;
 #[cfg(test)]
-mod prop;
+mod tests;
 
 use crate::{
     amount::{Amount, NonNegative},

zebra-chain/src/transparent/serialize.rs

@@ -65,7 +65,7 @@ impl ZcashDeserialize for OutPoint {
 ///
 /// The height may consume `0..=5` bytes at the stat of the coinbase data.
 /// The genesis block does not include an encoded coinbase height.
-fn parse_coinbase_height(
+pub(crate) fn parse_coinbase_height(
     mut data: Vec<u8>,
 ) -> Result<(block::Height, CoinbaseData), SerializationError> {
     use block::Height;
@@ -78,20 +78,33 @@ fn parse_coinbase_height(
         // Blocks 17 through 128 exclusive encode block height with the `0x01` opcode.
         // The Bitcoin encoding requires that the most significant byte is below 0x80.
         (Some(0x01), len) if len >= 2 && data[1] < 0x80 => {
-            Ok((Height(data[1] as u32), CoinbaseData(data.split_off(2))))
+            let h = data[1] as u32;
+            if (17..128).contains(&h) {
+                Ok((Height(h), CoinbaseData(data.split_off(2))))
+            } else {
+                Err(SerializationError::Parse("Invalid block height"))
+            }
         }
         // Blocks 128 through 32768 exclusive encode block height with the `0x02` opcode.
         // The Bitcoin encoding requires that the most significant byte is below 0x80.
-        (Some(0x02), len) if len >= 3 && data[2] < 0x80 => Ok((
-            Height(data[1] as u32 + ((data[2] as u32) << 8)),
-            CoinbaseData(data.split_off(3)),
-        )),
-        // Blocks 65536 through 2**23 exclusive encode block height with the `0x03` opcode.
+        (Some(0x02), len) if len >= 3 && data[2] < 0x80 => {
+            let h = data[1] as u32 + ((data[2] as u32) << 8);
+            if (128..32_768).contains(&h) {
+                Ok((Height(h), CoinbaseData(data.split_off(3))))
+            } else {
+                Err(SerializationError::Parse("Invalid block height"))
+            }
+        }
+        // Blocks 32768 through 2**23 exclusive encode block height with the `0x03` opcode.
         // The Bitcoin encoding requires that the most significant byte is below 0x80.
-        (Some(0x03), len) if len >= 4 && data[3] < 0x80 => Ok((
-            Height(data[1] as u32 + ((data[2] as u32) << 8) + ((data[3] as u32) << 16)),
-            CoinbaseData(data.split_off(4)),
-        )),
+        (Some(0x03), len) if len >= 4 && data[3] < 0x80 => {
+            let h = data[1] as u32 + ((data[2] as u32) << 8) + ((data[3] as u32) << 16);
+            if (32_768..8_388_608).contains(&h) {
+                Ok((Height(h), CoinbaseData(data.split_off(4))))
+            } else {
+                Err(SerializationError::Parse("Invalid block height"))
+            }
+        }
         // The genesis block does not encode the block height by mistake; special case it.
         // The first five bytes are [4, 255, 255, 7, 31], the little-endian encoding of
         // 520_617_983.
@@ -112,7 +125,7 @@ fn parse_coinbase_height(
                 + ((data[2] as u32) << 8)
                 + ((data[3] as u32) << 16)
                 + ((data[4] as u32) << 24);
-            if h <= Height::MAX.0 {
+            if (8_388_608..=Height::MAX.0).contains(&h) {
                 Ok((Height(h), CoinbaseData(data.split_off(5))))
             } else {
                 Err(SerializationError::Parse("Invalid block height"))
@@ -137,7 +150,7 @@ fn parse_coinbase_height(
 ///
 /// This check is required, because the genesis block does not include an encoded
 /// coinbase height,
-fn write_coinbase_height<W: io::Write>(
+pub(crate) fn write_coinbase_height<W: io::Write>(
     height: block::Height,
     coinbase_data: &CoinbaseData,
     mut w: W,
@@ -164,10 +177,10 @@ fn write_coinbase_height<W: io::Write>(
     } else if let h @ 17..=127 = height.0 {
         w.write_u8(0x01)?;
         w.write_u8(h as u8)?;
-    } else if let h @ 128..=32767 = height.0 {
+    } else if let h @ 128..=32_767 = height.0 {
         w.write_u8(0x02)?;
         w.write_u16::<LittleEndian>(h as u16)?;
-    } else if let h @ 32768..=8_388_607 = height.0 {
+    } else if let h @ 32_768..=8_388_607 = height.0 {
         w.write_u8(0x03)?;
         w.write_u8(h as u8)?;
         w.write_u8((h >> 8) as u8)?;

zebra-chain/src/transparent/tests.rs

@@ -0,0 +1,2 @@
+mod prop;
+mod vectors;

zebra-chain/src/transparent/tests/prop.rs

@@ -0,0 +1,100 @@
+//! Property tests for transparent inputs and outputs.
+
+use zebra_test::prelude::*;
+
+use crate::{block, fmt::SummaryDebug, transaction::arbitrary::MAX_ARBITRARY_ITEMS, LedgerState};
+
+use super::super::{
+    serialize::{parse_coinbase_height, write_coinbase_height},
+    Input,
+};
+
+use proptest::collection::vec;
+
+#[test]
+fn coinbase_has_height() -> Result<()> {
+    zebra_test::init();
+
+    let strategy =
+        any::<block::Height>().prop_flat_map(|height| Input::arbitrary_with(Some(height)));
+
+    proptest!(|(input in strategy)| {
+        let is_coinbase = matches!(input, Input::Coinbase { .. });
+        prop_assert!(is_coinbase);
+    });
+
+    Ok(())
+}
+
+#[test]
+fn input_coinbase_vecs_only_have_coinbase_input() -> Result<()> {
+    zebra_test::init();
+
+    let strategy = LedgerState::coinbase_strategy(None, None, false)
+        .prop_flat_map(|ledger_state| Input::vec_strategy(ledger_state, MAX_ARBITRARY_ITEMS));
+
+    proptest!(|(inputs in strategy.prop_map(SummaryDebug))| {
+        let len = inputs.len();
+        for (ind, input) in inputs.into_iter().enumerate() {
+            let is_coinbase = matches!(input, Input::Coinbase { .. });
+            if ind == 0 {
+                prop_assert!(is_coinbase);
+                prop_assert_eq!(1, len);
+            } else {
+                prop_assert!(!is_coinbase);
+            }
+        }
+    });
+
+    Ok(())
+}
+
+#[test]
+fn coinbase_height_round_trip_from_random_input() -> Result<()> {
+    zebra_test::init();
+
+    let strategy =
+        any::<block::Height>().prop_flat_map(|height| Input::arbitrary_with(Some(height)));
+
+    proptest!(|(input in strategy)| {
+        let (height, data) = match input {
+            Input::Coinbase { height, data, .. } => (height, data),
+            _ => unreachable!("all inputs will have coinbase height and data"),
+        };
+        let mut encoded = Vec::new();
+        write_coinbase_height(height, &data, &mut encoded)?;
+        let decoded = parse_coinbase_height(encoded)?;
+
+        prop_assert_eq!(height, decoded.0);
+    });
+
+    Ok(())
+}
+
+proptest! {
+    #[test]
+    fn coinbase_height_round_trip_from_random_bytes(mut height_bytes in vec(any::<u8>(), 1..5)) {
+        let mut encoded1 = vec![height_bytes.len() as u8];
+        encoded1.append(&mut height_bytes);
+
+        let decoded = parse_coinbase_height(encoded1.clone()).ok();
+
+        if decoded.is_some() {
+            let mut encoded2 = Vec::new();
+            write_coinbase_height(decoded.as_ref().unwrap().0, &decoded.unwrap().1, &mut encoded2)?;
+            prop_assert_eq!(encoded2, encoded1);
+        }
+    }
+
+    #[test]
+    fn coinbase_height_round_trip_from_random_byte(height_byte in vec(any::<u8>(), 1..2)) {
+        let encoded1 = height_byte;
+        let decoded = parse_coinbase_height(encoded1.clone()).ok();
+
+        if decoded.is_some() {
+            let mut encoded2 = Vec::new();
+            write_coinbase_height(decoded.as_ref().unwrap().0, &decoded.unwrap().1, &mut encoded2)?;
+            prop_assert_eq!(encoded2, encoded1);
+        }
+    }
+}

zebra-chain/src/transparent/tests/vectors.rs

@@ -0,0 +1,39 @@
+use super::super::serialize::parse_coinbase_height;
+
+#[test]
+fn parse_coinbase_height_mins() {
+    zebra_test::init();
+
+    // examples with height 1:
+
+    let case1 = vec![0x51];
+    assert!(!parse_coinbase_height(case1.clone()).is_err());
+    assert_eq!(parse_coinbase_height(case1).unwrap().0 .0, 1);
+
+    let case2 = vec![0x01, 0x01];
+    assert!(parse_coinbase_height(case2).is_err());
+
+    let case3 = vec![0x02, 0x01, 0x00];
+    assert!(parse_coinbase_height(case3).is_err());
+
+    let case4 = vec![0x03, 0x01, 0x00, 0x00];
+    assert!(parse_coinbase_height(case4).is_err());
+
+    let case5 = vec![0x04, 0x01, 0x00, 0x00, 0x00];
+    assert!(parse_coinbase_height(case5).is_err());
+
+    // examples with height 17:
+
+    let case1 = vec![0x01, 0x11];
+    assert!(!parse_coinbase_height(case1.clone()).is_err());
+    assert_eq!(parse_coinbase_height(case1).unwrap().0 .0, 17);
+
+    let case2 = vec![0x02, 0x11, 0x00];
+    assert!(parse_coinbase_height(case2).is_err());
+
+    let case3 = vec![0x03, 0x11, 0x00, 0x00];
+    assert!(parse_coinbase_height(case3).is_err());
+
+    let case4 = vec![0x04, 0x11, 0x00, 0x00, 0x00];
+    assert!(parse_coinbase_height(case4).is_err());
+}