From 83adbe50b80c3108825d9b91c693560c5a72c0a0 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 1 Dec 2021 19:42:02 -0300 Subject: [PATCH 1/8] fix parse_coinbase_height() --- zebra-chain/src/transparent/serialize.rs | 41 ++++++++++++++++-------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/zebra-chain/src/transparent/serialize.rs b/zebra-chain/src/transparent/serialize.rs index f1da030c982..860ac6a6e56 100644 --- a/zebra-chain/src/transparent/serialize.rs +++ b/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, ) -> 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 h < 128 { + 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")) @@ -164,10 +177,10 @@ fn write_coinbase_height( } 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::(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)?; From 976992614ad0ffae182033116bb4a66d33955076 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 1 Dec 2021 19:43:02 -0300 Subject: [PATCH 2/8] move tests and create test for parse_coinbase_height() --- zebra-chain/src/transparent.rs | 2 +- zebra-chain/src/transparent/tests.rs | 2 ++ .../src/transparent/{ => tests}/prop.rs | 4 +--- zebra-chain/src/transparent/tests/vectors.rs | 19 +++++++++++++++++++ 4 files changed, 23 insertions(+), 4 deletions(-) create mode 100644 zebra-chain/src/transparent/tests.rs rename zebra-chain/src/transparent/{ => tests}/prop.rs (94%) create mode 100644 zebra-chain/src/transparent/tests/vectors.rs diff --git a/zebra-chain/src/transparent.rs b/zebra-chain/src/transparent.rs index 2d4f4a5cf17..8392c5c7435 100644 --- a/zebra-chain/src/transparent.rs +++ b/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}, diff --git a/zebra-chain/src/transparent/tests.rs b/zebra-chain/src/transparent/tests.rs new file mode 100644 index 00000000000..cc95d9d451f --- /dev/null +++ b/zebra-chain/src/transparent/tests.rs @@ -0,0 +1,2 @@ +mod prop; +mod vectors; diff --git a/zebra-chain/src/transparent/prop.rs b/zebra-chain/src/transparent/tests/prop.rs similarity index 94% rename from zebra-chain/src/transparent/prop.rs rename to zebra-chain/src/transparent/tests/prop.rs index fd4a9462dbf..d02266a773e 100644 --- a/zebra-chain/src/transparent/prop.rs +++ b/zebra-chain/src/transparent/tests/prop.rs @@ -1,12 +1,10 @@ //! Property tests for transparent inputs and outputs. -//! -//! TODO: Move this module into a `tests` submodule. use zebra_test::prelude::*; use crate::{block, fmt::SummaryDebug, transaction::arbitrary::MAX_ARBITRARY_ITEMS, LedgerState}; -use super::Input; +use super::super::Input; #[test] fn coinbase_has_height() -> Result<()> { diff --git a/zebra-chain/src/transparent/tests/vectors.rs b/zebra-chain/src/transparent/tests/vectors.rs new file mode 100644 index 00000000000..55209aee8d6 --- /dev/null +++ b/zebra-chain/src/transparent/tests/vectors.rs @@ -0,0 +1,19 @@ +use super::super::serialize::parse_coinbase_height; + +#[test] +fn parse_coinbase_height_mins() { + zebra_test::init(); + + 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()); +} From b35984cfc73e91d6b2641ab9ace9462a9743562e Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 1 Dec 2021 20:41:49 -0300 Subject: [PATCH 3/8] add a coinbase height round trip prop test --- zebra-chain/src/transparent/serialize.rs | 2 +- zebra-chain/src/transparent/tests/prop.rs | 27 ++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/zebra-chain/src/transparent/serialize.rs b/zebra-chain/src/transparent/serialize.rs index 860ac6a6e56..bea0a237bf4 100644 --- a/zebra-chain/src/transparent/serialize.rs +++ b/zebra-chain/src/transparent/serialize.rs @@ -150,7 +150,7 @@ pub(crate) fn parse_coinbase_height( /// /// This check is required, because the genesis block does not include an encoded /// coinbase height, -fn write_coinbase_height( +pub(crate) fn write_coinbase_height( height: block::Height, coinbase_data: &CoinbaseData, mut w: W, diff --git a/zebra-chain/src/transparent/tests/prop.rs b/zebra-chain/src/transparent/tests/prop.rs index d02266a773e..cd2a77ac0a2 100644 --- a/zebra-chain/src/transparent/tests/prop.rs +++ b/zebra-chain/src/transparent/tests/prop.rs @@ -4,7 +4,10 @@ use zebra_test::prelude::*; use crate::{block, fmt::SummaryDebug, transaction::arbitrary::MAX_ARBITRARY_ITEMS, LedgerState}; -use super::super::Input; +use super::super::{ + serialize::{parse_coinbase_height, write_coinbase_height}, + Input, +}; #[test] fn coinbase_has_height() -> Result<()> { @@ -43,3 +46,25 @@ fn input_coinbase_vecs_only_have_coinbase_input() -> Result<()> { Ok(()) } + +#[test] +fn coinbase_height_round_trip() -> Result<()> { + zebra_test::init(); + + let strategy = + any::().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(()) +} From 8c2064526a8858554ec3f91ae8297012f76fa78b Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 2 Dec 2021 09:54:18 -0300 Subject: [PATCH 4/8] fix range Co-authored-by: teor --- zebra-chain/src/transparent/serialize.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/transparent/serialize.rs b/zebra-chain/src/transparent/serialize.rs index bea0a237bf4..54bdc86b86e 100644 --- a/zebra-chain/src/transparent/serialize.rs +++ b/zebra-chain/src/transparent/serialize.rs @@ -79,7 +79,7 @@ pub(crate) fn parse_coinbase_height( // The Bitcoin encoding requires that the most significant byte is below 0x80. (Some(0x01), len) if len >= 2 && data[1] < 0x80 => { let h = data[1] as u32; - if h < 128 { + if (17..128).contains(&h) { Ok((Height(h), CoinbaseData(data.split_off(2)))) } else { Err(SerializationError::Parse("Invalid block height")) From 73907af994d587b420c653b659fa58d4113cb9d0 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 2 Dec 2021 10:20:05 -0300 Subject: [PATCH 5/8] extend examples in test --- zebra-chain/src/transparent/tests/vectors.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/zebra-chain/src/transparent/tests/vectors.rs b/zebra-chain/src/transparent/tests/vectors.rs index 55209aee8d6..1218b796a00 100644 --- a/zebra-chain/src/transparent/tests/vectors.rs +++ b/zebra-chain/src/transparent/tests/vectors.rs @@ -4,6 +4,26 @@ use super::super::serialize::parse_coinbase_height; 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); From a40815d236d6bab5053737b0c73f3c55b44a40e1 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 2 Dec 2021 11:23:49 -0300 Subject: [PATCH 6/8] add more round trip testing --- zebra-chain/src/transparent/tests/prop.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/transparent/tests/prop.rs b/zebra-chain/src/transparent/tests/prop.rs index cd2a77ac0a2..c4f90e75148 100644 --- a/zebra-chain/src/transparent/tests/prop.rs +++ b/zebra-chain/src/transparent/tests/prop.rs @@ -9,6 +9,8 @@ use super::super::{ Input, }; +use proptest::collection::vec; + #[test] fn coinbase_has_height() -> Result<()> { zebra_test::init(); @@ -48,7 +50,7 @@ fn input_coinbase_vecs_only_have_coinbase_input() -> Result<()> { } #[test] -fn coinbase_height_round_trip() -> Result<()> { +fn coinbase_height_round_trip_from_random_input() -> Result<()> { zebra_test::init(); let strategy = @@ -68,3 +70,19 @@ fn coinbase_height_round_trip() -> Result<()> { Ok(()) } + +proptest! { + #[test] + fn coinbase_height_round_trip_from_random_bytes(mut height_bytes in vec(any::(), 1..4)) { + 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); + } + } +} From 3354107d7c9140e581adcfab13efe9f8ec98466d Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Fri, 3 Dec 2021 10:56:36 -0300 Subject: [PATCH 7/8] extend the range of test Co-authored-by: teor --- zebra-chain/src/transparent/tests/prop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/transparent/tests/prop.rs b/zebra-chain/src/transparent/tests/prop.rs index c4f90e75148..478ecc02bfd 100644 --- a/zebra-chain/src/transparent/tests/prop.rs +++ b/zebra-chain/src/transparent/tests/prop.rs @@ -73,7 +73,7 @@ fn coinbase_height_round_trip_from_random_input() -> Result<()> { proptest! { #[test] - fn coinbase_height_round_trip_from_random_bytes(mut height_bytes in vec(any::(), 1..4)) { + fn coinbase_height_round_trip_from_random_bytes(mut height_bytes in vec(any::(), 1..5)) { let mut encoded1 = vec![height_bytes.len() as u8]; encoded1.append(&mut height_bytes); From 46e9a48219ead0ea612288b521a3c9123df82fed Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Fri, 3 Dec 2021 19:24:04 -0300 Subject: [PATCH 8/8] add test for single byte --- zebra-chain/src/transparent/tests/prop.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/zebra-chain/src/transparent/tests/prop.rs b/zebra-chain/src/transparent/tests/prop.rs index 478ecc02bfd..3c76582878a 100644 --- a/zebra-chain/src/transparent/tests/prop.rs +++ b/zebra-chain/src/transparent/tests/prop.rs @@ -85,4 +85,16 @@ proptest! { prop_assert_eq!(encoded2, encoded1); } } + + #[test] + fn coinbase_height_round_trip_from_random_byte(height_byte in vec(any::(), 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); + } + } }