From fb0616b08b0f5441a80ccbf978b2d6a448b73e10 Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 10 May 2023 15:58:13 +0200 Subject: [PATCH 01/22] Add TODOs for checked_sub locations --- ssz-rs-derive/src/lib.rs | 1 + ssz-rs/src/bitlist.rs | 5 +++++ ssz-rs/src/bitvector.rs | 1 + ssz-rs/src/de.rs | 1 + ssz-rs/src/merkleization/mod.rs | 10 ++++++++++ 5 files changed, 18 insertions(+) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 0eb3f20a..966a3da1 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -228,6 +228,7 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let (_, end) = span[1]; container.__ssz_rs_set_by_index(index, &encoding[start..end])?; + // TODO: checked_sub total_bytes_read += end - start; } diff --git a/ssz-rs/src/bitlist.rs b/ssz-rs/src/bitlist.rs index 0d92ccb5..e613e972 100644 --- a/ssz-rs/src/bitlist.rs +++ b/ssz-rs/src/bitlist.rs @@ -51,6 +51,7 @@ impl fmt::Debug for Bitlist { let value = i32::from(*bit); write!(f, "{value}")?; bits_written += 1; + // TODO: checked_sub if bits_written % 4 == 0 && index != len - 1 { write!(f, "_")?; } @@ -110,6 +111,7 @@ impl Bitlist { *last |= 1u8 << marker_index; } } + // TODO: checked_sub Ok(buffer.len() - start_len) } } @@ -161,12 +163,15 @@ impl Deserialize for Bitlist { let (last_byte, prefix) = encoding.split_last().unwrap(); let mut result = BitlistInner::from_slice(prefix); let last = BitlistInner::from_element(*last_byte); + // TODO: checked_sub let high_bit_index = 8 - last.trailing_zeros(); + // TODO: checked_sub if !last[high_bit_index - 1] { return Err(DeserializeError::InvalidByte(*last_byte)) } + // TODO: checked_sub for bit in last.iter().take(high_bit_index - 1) { result.push(*bit); } diff --git a/ssz-rs/src/bitvector.rs b/ssz-rs/src/bitvector.rs index 28376ee4..2edcce9b 100644 --- a/ssz-rs/src/bitvector.rs +++ b/ssz-rs/src/bitvector.rs @@ -60,6 +60,7 @@ impl fmt::Debug for Bitvector { let value = i32::from(*bit); write!(f, "{value}")?; bits_written += 1; + // TODO: checked_sub if bits_written % 4 == 0 && index != len - 1 { write!(f, "_")?; } diff --git a/ssz-rs/src/de.rs b/ssz-rs/src/de.rs index 55cf9e18..94486e22 100644 --- a/ssz-rs/src/de.rs +++ b/ssz-rs/src/de.rs @@ -71,6 +71,7 @@ where if remainder != 0 { return Err(DeserializeError::AdditionalInput { provided: encoding.len(), + // TODO: checked_sub expected: encoding.len() - remainder, }) } diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 83691f7e..50280469 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -48,6 +48,7 @@ impl std::error::Error for MerkleizationError {} pub fn pack_bytes(buffer: &mut Vec) { let data_len = buffer.len(); if data_len % BYTES_PER_CHUNK != 0 { + // TODO: checked_sub let bytes_to_pad = BYTES_PER_CHUNK - data_len % BYTES_PER_CHUNK; let pad = vec![0u8; bytes_to_pad]; buffer.extend_from_slice(&pad); @@ -116,11 +117,13 @@ fn merkleize_chunks_with_virtual_padding( let height = leaf_count.trailing_zeros() + 1; if chunk_count == 0 { + // TODO: checked_sub let depth = height - 1; return Ok(CONTEXT[depth as usize].try_into().expect("can produce a single root chunk")) } let mut layer = chunks.to_vec(); + // TODO: checked_sub let mut last_index = chunk_count - 1; for k in (1..height).rev() { for i in (0..2usize.pow(k)).step_by(2) { @@ -129,6 +132,7 @@ fn merkleize_chunks_with_virtual_padding( Ordering::Less => { let focus = &mut layer[parent_index * BYTES_PER_CHUNK..(i + 2) * BYTES_PER_CHUNK]; + // TODO: checked_sub let children_index = focus.len() - 2 * BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); let (left, right) = children.split_at_mut(BYTES_PER_CHUNK); @@ -145,9 +149,11 @@ fn merkleize_chunks_with_virtual_padding( Ordering::Equal => { let focus = &mut layer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; + // TODO: checked_sub let children_index = focus.len() - BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); let (left, _) = children.split_at_mut(BYTES_PER_CHUNK); + // TODO: checked_sub let depth = height - k - 1; let right = &CONTEXT[depth as usize]; if parent.is_empty() { @@ -265,7 +271,9 @@ mod tests { debug_assert!(chunks.len() % BYTES_PER_CHUNK == 0); debug_assert!(leaf_count.next_power_of_two() == leaf_count); + // TODO: checked_sub let node_count = 2 * leaf_count - 1; + // TODO: checked_sub let interior_count = node_count - leaf_count; let leaf_start = interior_count * BYTES_PER_CHUNK; @@ -280,8 +288,10 @@ mod tests { } for i in (1..node_count).rev().step_by(2) { + // TODO: checked_sub let parent_index = (i - 1) / 2; let focus = &mut buffer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; + // TODO: checked_sub let children_index = focus.len() - 2 * BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); let left = &children[0..BYTES_PER_CHUNK]; From 404f368e3acb287ff4146af59fdc7aebd650ebec Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 17 May 2023 13:22:58 +0200 Subject: [PATCH 02/22] Remove extra split_at_mut --- ssz-rs/src/merkleization/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 50280469..fb317951 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -151,8 +151,7 @@ fn merkleize_chunks_with_virtual_padding( &mut layer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; // TODO: checked_sub let children_index = focus.len() - BYTES_PER_CHUNK; - let (parent, children) = focus.split_at_mut(children_index); - let (left, _) = children.split_at_mut(BYTES_PER_CHUNK); + let (parent, left) = focus.split_at_mut(children_index); // TODO: checked_sub let depth = height - k - 1; let right = &CONTEXT[depth as usize]; From 4c0b8f0be9b92876ff4f2e2bc404f7902ab8624e Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 17 May 2023 14:42:55 +0200 Subject: [PATCH 03/22] Replace slicing with split_at --- ssz-rs/src/merkleization/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index fb317951..52d1ad68 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -293,8 +293,7 @@ mod tests { // TODO: checked_sub let children_index = focus.len() - 2 * BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); - let left = &children[0..BYTES_PER_CHUNK]; - let right = &children[BYTES_PER_CHUNK..2 * BYTES_PER_CHUNK]; + let (left, right) = children.split_at(BYTES_PER_CHUNK); hash_nodes(&mut hasher, left, right, &mut parent[..BYTES_PER_CHUNK]); } Ok(buffer[0..BYTES_PER_CHUNK].try_into().expect("can produce a single root chunk")) From c7c467ed3e7684c1e79bc33a633525fb0e08cf1e Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 17 May 2023 14:44:23 +0200 Subject: [PATCH 04/22] Replace magic number with BYTES_PER_CHUNK --- ssz-rs/src/merkleization/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 52d1ad68..6c055ea3 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -279,7 +279,7 @@ mod tests { let mut hasher = Sha256::new(); let mut buffer = vec![0u8; node_count * BYTES_PER_CHUNK]; buffer[leaf_start..leaf_start + chunks.len()].copy_from_slice(chunks); - let zero_chunk = [0u8; 32]; + let zero_chunk = [0u8; BYTES_PER_CHUNK]; for i in chunks.len()..leaf_count { let start = leaf_start + (i * BYTES_PER_CHUNK); let end = leaf_start + (i + 1) * BYTES_PER_CHUNK; From 479b76bc4d8c021d008fac70ae383d8e30e3c8fb Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 17 May 2023 14:47:13 +0200 Subject: [PATCH 05/22] Measure the length of an incomplete last chunk --- ssz-rs/src/merkleization/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 6c055ea3..06716874 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -46,10 +46,10 @@ impl Display for MerkleizationError { impl std::error::Error for MerkleizationError {} pub fn pack_bytes(buffer: &mut Vec) { - let data_len = buffer.len(); - if data_len % BYTES_PER_CHUNK != 0 { + let incomplete_chunk_len = buffer.len() % BYTES_PER_CHUNK; + if incomplete_chunk_len != 0 { // TODO: checked_sub - let bytes_to_pad = BYTES_PER_CHUNK - data_len % BYTES_PER_CHUNK; + let bytes_to_pad = BYTES_PER_CHUNK - incomplete_chunk_len; let pad = vec![0u8; bytes_to_pad]; buffer.extend_from_slice(&pad); } From 6e90e37595b908588622baa3de0cf631266691aa Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Mon, 15 May 2023 11:18:46 +0200 Subject: [PATCH 06/22] Handle checked_sub cases --- ssz-rs-derive/src/lib.rs | 7 ++++--- ssz-rs/src/bitlist.rs | 15 +++++++++----- ssz-rs/src/bitvector.rs | 2 +- ssz-rs/src/de.rs | 2 +- ssz-rs/src/merkleization/mod.rs | 35 ++++++++++++++++++++++++--------- 5 files changed, 42 insertions(+), 19 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 966a3da1..bcadb62f 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -192,8 +192,9 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { Some(field_name) => quote_spanned! { f.span() => let bytes_read = if <#field_type>::is_variable_size() { let end = start + #BYTES_PER_LENGTH_OFFSET; - let next_offset = u32::deserialize(&encoding[start..end])?; - offsets.push((#i, next_offset as usize)); + let next_offset = u32::deserialize(&encoding[start..end])? as usize; + offsets.last().map(|(_, previous_offset)| assert!(next_offset >= *previous_offset)); + offsets.push((#i, next_offset)); #BYTES_PER_LENGTH_OFFSET } else { @@ -228,7 +229,7 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let (_, end) = span[1]; container.__ssz_rs_set_by_index(index, &encoding[start..end])?; - // TODO: checked_sub + // checked_sub is unnecessary, as offsets are increasing total_bytes_read += end - start; } diff --git a/ssz-rs/src/bitlist.rs b/ssz-rs/src/bitlist.rs index e613e972..92038451 100644 --- a/ssz-rs/src/bitlist.rs +++ b/ssz-rs/src/bitlist.rs @@ -51,7 +51,7 @@ impl fmt::Debug for Bitlist { let value = i32::from(*bit); write!(f, "{value}")?; bits_written += 1; - // TODO: checked_sub + // checked_sub is unnecessary, as len >= 1 when this for loop runs if bits_written % 4 == 0 && index != len - 1 { write!(f, "_")?; } @@ -111,7 +111,7 @@ impl Bitlist { *last |= 1u8 << marker_index; } } - // TODO: checked_sub + // checked_sub is unnecessary, as buffer.len() > start_len Ok(buffer.len() - start_len) } } @@ -161,17 +161,22 @@ impl Deserialize for Bitlist { } let (last_byte, prefix) = encoding.split_last().unwrap(); + if *last_byte == 0u8 { + return Err(DeserializeError::InvalidByte(*last_byte)); + } + let mut result = BitlistInner::from_slice(prefix); let last = BitlistInner::from_element(*last_byte); - // TODO: checked_sub + // checked_sub is unnecessary, as last_byte != 0, so last.trailing_zeros <= 7 + // high_bit_index >= 1 let high_bit_index = 8 - last.trailing_zeros(); - // TODO: checked_sub + // checked_sub is unnecessary, as high_bit_index >= 1 if !last[high_bit_index - 1] { return Err(DeserializeError::InvalidByte(*last_byte)) } - // TODO: checked_sub + // checked_sub is unnecessary, as high_bit_index >= 1 for bit in last.iter().take(high_bit_index - 1) { result.push(*bit); } diff --git a/ssz-rs/src/bitvector.rs b/ssz-rs/src/bitvector.rs index 2edcce9b..4ee933fb 100644 --- a/ssz-rs/src/bitvector.rs +++ b/ssz-rs/src/bitvector.rs @@ -60,7 +60,7 @@ impl fmt::Debug for Bitvector { let value = i32::from(*bit); write!(f, "{value}")?; bits_written += 1; - // TODO: checked_sub + // checked_sub is unnecessary, as len >= 1 for bitvectors if bits_written % 4 == 0 && index != len - 1 { write!(f, "_")?; } diff --git a/ssz-rs/src/de.rs b/ssz-rs/src/de.rs index 94486e22..643c9446 100644 --- a/ssz-rs/src/de.rs +++ b/ssz-rs/src/de.rs @@ -71,7 +71,7 @@ where if remainder != 0 { return Err(DeserializeError::AdditionalInput { provided: encoding.len(), - // TODO: checked_sub + // checked_sub is unnecessary, as encoding.len() > remainder expected: encoding.len() - remainder, }) } diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 06716874..618bdb90 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -48,7 +48,7 @@ impl std::error::Error for MerkleizationError {} pub fn pack_bytes(buffer: &mut Vec) { let incomplete_chunk_len = buffer.len() % BYTES_PER_CHUNK; if incomplete_chunk_len != 0 { - // TODO: checked_sub + // checked_sub is unnecessary, as BYTES_PER_CHUNK > incomplete_chunk_len let bytes_to_pad = BYTES_PER_CHUNK - incomplete_chunk_len; let pad = vec![0u8; bytes_to_pad]; buffer.extend_from_slice(&pad); @@ -117,13 +117,13 @@ fn merkleize_chunks_with_virtual_padding( let height = leaf_count.trailing_zeros() + 1; if chunk_count == 0 { - // TODO: checked_sub + // checked_sub is unnecessary, as height >= 1 let depth = height - 1; return Ok(CONTEXT[depth as usize].try_into().expect("can produce a single root chunk")) } let mut layer = chunks.to_vec(); - // TODO: checked_sub + // checked_sub is unnecessary, as we return early when chunk_count == 0 let mut last_index = chunk_count - 1; for k in (1..height).rev() { for i in (0..2usize.pow(k)).step_by(2) { @@ -132,11 +132,17 @@ fn merkleize_chunks_with_virtual_padding( Ordering::Less => { let focus = &mut layer[parent_index * BYTES_PER_CHUNK..(i + 2) * BYTES_PER_CHUNK]; - // TODO: checked_sub + // checked_sub is unnecessary: + // i >= parent_index + // and + // focus.len() = (i + 2 - parent_index) * BYTES_PER_CHUNK + // so focus.len() >= 2 * BYTES_PER_CHUNK let children_index = focus.len() - 2 * BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); let (left, right) = children.split_at_mut(BYTES_PER_CHUNK); if parent.is_empty() { + // TODO: Why not i == 0? Is there another case where the left node is used + // to store the parent? // NOTE: have to specially handle the situation where the children nodes and // parent node share memory hasher.update(&left); @@ -149,10 +155,15 @@ fn merkleize_chunks_with_virtual_padding( Ordering::Equal => { let focus = &mut layer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; - // TODO: checked_sub + // checked_sub is unnecessary: + // i >= parent_index + // and + // focus.len() = (i + 1 - parent_index) * BYTES_PER_CHUNK + // so focus.len() >= BYTES_PER_CHUNK let children_index = focus.len() - BYTES_PER_CHUNK; let (parent, left) = focus.split_at_mut(children_index); - // TODO: checked_sub + // checked_sub is unnecessary, as k <= height - 1 + // so depth >= height - (height - 1) - 1 = 0 let depth = height - k - 1; let right = &CONTEXT[depth as usize]; if parent.is_empty() { @@ -270,9 +281,9 @@ mod tests { debug_assert!(chunks.len() % BYTES_PER_CHUNK == 0); debug_assert!(leaf_count.next_power_of_two() == leaf_count); - // TODO: checked_sub + // checked_sub is unnecessary, as leaf_count != 0 (0.next_power_of_two() == 1) let node_count = 2 * leaf_count - 1; - // TODO: checked_sub + // checked_sub is unnecessary, as node_count >= leaf_count let interior_count = node_count - leaf_count; let leaf_start = interior_count * BYTES_PER_CHUNK; @@ -287,10 +298,16 @@ mod tests { } for i in (1..node_count).rev().step_by(2) { - // TODO: checked_sub + // checked_sub is unnecessary, as i >= 1 let parent_index = (i - 1) / 2; let focus = &mut buffer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; // TODO: checked_sub + // focus.len() = (i + 1 - parent_index) * BYTES_PER_CHUNK + // = ((2*i + 2 - i + 1) / 2) * BYTES_PER_CHUNK + // = ((i + 3) / 2) * BYTES_PER_CHUNK + // and + // i >= 1 + // so focus.len() >= 2 * BYTES_PER_CHUNK let children_index = focus.len() - 2 * BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); let (left, right) = children.split_at(BYTES_PER_CHUNK); From 79a7c711f09f1b54d5823388cfded55b3f8586bf Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Mon, 29 May 2023 14:14:23 +0200 Subject: [PATCH 07/22] Reformat checked subtraction comments --- ssz-rs-derive/src/lib.rs | 2 +- ssz-rs/src/bitlist.rs | 10 ++++----- ssz-rs/src/bitvector.rs | 2 +- ssz-rs/src/de.rs | 2 +- ssz-rs/src/merkleization/mod.rs | 38 +++++++++++++++++---------------- 5 files changed, 28 insertions(+), 26 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index bcadb62f..b6b9a8f0 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -229,7 +229,7 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let (_, end) = span[1]; container.__ssz_rs_set_by_index(index, &encoding[start..end])?; - // checked_sub is unnecessary, as offsets are increasing + // checked subtraction is unnecessary, as offsets are increasing; qed total_bytes_read += end - start; } diff --git a/ssz-rs/src/bitlist.rs b/ssz-rs/src/bitlist.rs index 92038451..483bbfcc 100644 --- a/ssz-rs/src/bitlist.rs +++ b/ssz-rs/src/bitlist.rs @@ -51,7 +51,7 @@ impl fmt::Debug for Bitlist { let value = i32::from(*bit); write!(f, "{value}")?; bits_written += 1; - // checked_sub is unnecessary, as len >= 1 when this for loop runs + // checked subtraction is unnecessary, as len >= 1 when this for loop runs; qed if bits_written % 4 == 0 && index != len - 1 { write!(f, "_")?; } @@ -111,7 +111,7 @@ impl Bitlist { *last |= 1u8 << marker_index; } } - // checked_sub is unnecessary, as buffer.len() > start_len + // checked subtraction is unnecessary, as buffer.len() > start_len; qed Ok(buffer.len() - start_len) } } @@ -167,16 +167,16 @@ impl Deserialize for Bitlist { let mut result = BitlistInner::from_slice(prefix); let last = BitlistInner::from_element(*last_byte); - // checked_sub is unnecessary, as last_byte != 0, so last.trailing_zeros <= 7 + // checked subtraction is unnecessary, as last_byte != 0, so last.trailing_zeros <= 7; qed // high_bit_index >= 1 let high_bit_index = 8 - last.trailing_zeros(); - // checked_sub is unnecessary, as high_bit_index >= 1 + // checked subtraction is unnecessary, as high_bit_index >= 1; qed if !last[high_bit_index - 1] { return Err(DeserializeError::InvalidByte(*last_byte)) } - // checked_sub is unnecessary, as high_bit_index >= 1 + // checked subtraction is unnecessary, as high_bit_index >= 1; qed for bit in last.iter().take(high_bit_index - 1) { result.push(*bit); } diff --git a/ssz-rs/src/bitvector.rs b/ssz-rs/src/bitvector.rs index 4ee933fb..d46d627f 100644 --- a/ssz-rs/src/bitvector.rs +++ b/ssz-rs/src/bitvector.rs @@ -60,7 +60,7 @@ impl fmt::Debug for Bitvector { let value = i32::from(*bit); write!(f, "{value}")?; bits_written += 1; - // checked_sub is unnecessary, as len >= 1 for bitvectors + // checked subtraction is unnecessary, as len >= 1 for bitvectors; qed if bits_written % 4 == 0 && index != len - 1 { write!(f, "_")?; } diff --git a/ssz-rs/src/de.rs b/ssz-rs/src/de.rs index 643c9446..c6b64f00 100644 --- a/ssz-rs/src/de.rs +++ b/ssz-rs/src/de.rs @@ -71,7 +71,7 @@ where if remainder != 0 { return Err(DeserializeError::AdditionalInput { provided: encoding.len(), - // checked_sub is unnecessary, as encoding.len() > remainder + // checked subtraction is unnecessary, as encoding.len() > remainder; qed expected: encoding.len() - remainder, }) } diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 618bdb90..92e2bc90 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -48,7 +48,7 @@ impl std::error::Error for MerkleizationError {} pub fn pack_bytes(buffer: &mut Vec) { let incomplete_chunk_len = buffer.len() % BYTES_PER_CHUNK; if incomplete_chunk_len != 0 { - // checked_sub is unnecessary, as BYTES_PER_CHUNK > incomplete_chunk_len + // checked subtraction is unnecessary, as BYTES_PER_CHUNK > incomplete_chunk_len; qed let bytes_to_pad = BYTES_PER_CHUNK - incomplete_chunk_len; let pad = vec![0u8; bytes_to_pad]; buffer.extend_from_slice(&pad); @@ -117,13 +117,13 @@ fn merkleize_chunks_with_virtual_padding( let height = leaf_count.trailing_zeros() + 1; if chunk_count == 0 { - // checked_sub is unnecessary, as height >= 1 + // checked subtraction is unnecessary, as height >= 1; qed let depth = height - 1; return Ok(CONTEXT[depth as usize].try_into().expect("can produce a single root chunk")) } let mut layer = chunks.to_vec(); - // checked_sub is unnecessary, as we return early when chunk_count == 0 + // checked subtraction is unnecessary, as we return early when chunk_count == 0; qed let mut last_index = chunk_count - 1; for k in (1..height).rev() { for i in (0..2usize.pow(k)).step_by(2) { @@ -132,11 +132,11 @@ fn merkleize_chunks_with_virtual_padding( Ordering::Less => { let focus = &mut layer[parent_index * BYTES_PER_CHUNK..(i + 2) * BYTES_PER_CHUNK]; - // checked_sub is unnecessary: - // i >= parent_index - // and + // checked subtraction is unnecessary: // focus.len() = (i + 2 - parent_index) * BYTES_PER_CHUNK - // so focus.len() >= 2 * BYTES_PER_CHUNK + // and + // i >= parent_index + // so focus.len() >= 2 * BYTES_PER_CHUNK; qed let children_index = focus.len() - 2 * BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); let (left, right) = children.split_at_mut(BYTES_PER_CHUNK); @@ -155,15 +155,17 @@ fn merkleize_chunks_with_virtual_padding( Ordering::Equal => { let focus = &mut layer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; - // checked_sub is unnecessary: - // i >= parent_index - // and + // checked subtraction is unnecessary: // focus.len() = (i + 1 - parent_index) * BYTES_PER_CHUNK - // so focus.len() >= BYTES_PER_CHUNK + // and + // i >= parent_index + // so focus.len() >= BYTES_PER_CHUNK; qed let children_index = focus.len() - BYTES_PER_CHUNK; let (parent, left) = focus.split_at_mut(children_index); - // checked_sub is unnecessary, as k <= height - 1 - // so depth >= height - (height - 1) - 1 = 0 + // checked subtraction is unnecessary: + // k <= height - 1 + // so depth >= height - (height - 1) - 1 + // = 0; qed let depth = height - k - 1; let right = &CONTEXT[depth as usize]; if parent.is_empty() { @@ -281,9 +283,9 @@ mod tests { debug_assert!(chunks.len() % BYTES_PER_CHUNK == 0); debug_assert!(leaf_count.next_power_of_two() == leaf_count); - // checked_sub is unnecessary, as leaf_count != 0 (0.next_power_of_two() == 1) + // checked subtraction is unnecessary, as leaf_count != 0 (0.next_power_of_two() == 1); qed let node_count = 2 * leaf_count - 1; - // checked_sub is unnecessary, as node_count >= leaf_count + // checked subtraction is unnecessary, as node_count >= leaf_count; qed let interior_count = node_count - leaf_count; let leaf_start = interior_count * BYTES_PER_CHUNK; @@ -298,16 +300,16 @@ mod tests { } for i in (1..node_count).rev().step_by(2) { - // checked_sub is unnecessary, as i >= 1 + // checked subtraction is unnecessary, as i >= 1; qed let parent_index = (i - 1) / 2; let focus = &mut buffer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; - // TODO: checked_sub + // checked subtraction is unnecessary: // focus.len() = (i + 1 - parent_index) * BYTES_PER_CHUNK // = ((2*i + 2 - i + 1) / 2) * BYTES_PER_CHUNK // = ((i + 3) / 2) * BYTES_PER_CHUNK // and // i >= 1 - // so focus.len() >= 2 * BYTES_PER_CHUNK + // so focus.len() >= 2 * BYTES_PER_CHUNK; qed let children_index = focus.len() - 2 * BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); let (left, right) = children.split_at(BYTES_PER_CHUNK); From 39d20faa012dbf8e8fcc5a6fefe03c614fc05952 Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Tue, 6 Jun 2023 13:48:09 +0200 Subject: [PATCH 08/22] Clarify comment Co-authored-by: Alex Stokes --- ssz-rs/src/bitlist.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssz-rs/src/bitlist.rs b/ssz-rs/src/bitlist.rs index 483bbfcc..8d7fbef4 100644 --- a/ssz-rs/src/bitlist.rs +++ b/ssz-rs/src/bitlist.rs @@ -168,7 +168,7 @@ impl Deserialize for Bitlist { let mut result = BitlistInner::from_slice(prefix); let last = BitlistInner::from_element(*last_byte); // checked subtraction is unnecessary, as last_byte != 0, so last.trailing_zeros <= 7; qed - // high_bit_index >= 1 + // therefore: high_bit_index >= 1 let high_bit_index = 8 - last.trailing_zeros(); // checked subtraction is unnecessary, as high_bit_index >= 1; qed From 4d1999af742d90b15386a383cbc02fbf40f28299 Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 7 Jun 2023 10:15:01 +0200 Subject: [PATCH 09/22] Add test case for empty last byte in Bitlist --- ssz-rs/src/bitlist.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ssz-rs/src/bitlist.rs b/ssz-rs/src/bitlist.rs index 8d7fbef4..a37373d4 100644 --- a/ssz-rs/src/bitlist.rs +++ b/ssz-rs/src/bitlist.rs @@ -283,6 +283,12 @@ mod tests { let expected = Bitlist::from_iter(vec![false, false, false, true, true, false, false, false, true]); assert_eq!(result, expected); + + let bytes = vec![24u8, 0u8]; + let result = Bitlist::::deserialize(&bytes).expect_err("test data is incorrect"); + let expected = + DeserializeError::InvalidByte(0u8); + assert_eq!(result.to_string(), expected.to_string()); } #[test] From 536f46a5bd4dc64839170851586ebbe0ca9e83da Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 7 Jun 2023 10:24:37 +0200 Subject: [PATCH 10/22] Add SAFETY prefix to comments --- ssz-rs-derive/src/lib.rs | 3 ++- ssz-rs/src/bitlist.rs | 11 ++++++----- ssz-rs/src/bitvector.rs | 2 +- ssz-rs/src/de.rs | 2 +- ssz-rs/src/merkleization/mod.rs | 22 ++++++++++++---------- 5 files changed, 22 insertions(+), 18 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index b6b9a8f0..d54c25be 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -229,7 +229,8 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let (_, end) = span[1]; container.__ssz_rs_set_by_index(index, &encoding[start..end])?; - // checked subtraction is unnecessary, as offsets are increasing; qed + // SAFETY: checked subtraction is unnecessary, + // as offsets are increasing; qed total_bytes_read += end - start; } diff --git a/ssz-rs/src/bitlist.rs b/ssz-rs/src/bitlist.rs index a37373d4..d77aa90f 100644 --- a/ssz-rs/src/bitlist.rs +++ b/ssz-rs/src/bitlist.rs @@ -51,7 +51,7 @@ impl fmt::Debug for Bitlist { let value = i32::from(*bit); write!(f, "{value}")?; bits_written += 1; - // checked subtraction is unnecessary, as len >= 1 when this for loop runs; qed + // SAFETY: checked subtraction is unnecessary, as len >= 1 when this for loop runs; qed if bits_written % 4 == 0 && index != len - 1 { write!(f, "_")?; } @@ -111,7 +111,7 @@ impl Bitlist { *last |= 1u8 << marker_index; } } - // checked subtraction is unnecessary, as buffer.len() > start_len; qed + // SAFETY: checked subtraction is unnecessary, as buffer.len() > start_len; qed Ok(buffer.len() - start_len) } } @@ -167,16 +167,17 @@ impl Deserialize for Bitlist { let mut result = BitlistInner::from_slice(prefix); let last = BitlistInner::from_element(*last_byte); - // checked subtraction is unnecessary, as last_byte != 0, so last.trailing_zeros <= 7; qed + // SAFETY: checked subtraction is unnecessary, + // as last_byte != 0, so last.trailing_zeros <= 7; qed // therefore: high_bit_index >= 1 let high_bit_index = 8 - last.trailing_zeros(); - // checked subtraction is unnecessary, as high_bit_index >= 1; qed + // SAFETY: checked subtraction is unnecessary, as high_bit_index >= 1; qed if !last[high_bit_index - 1] { return Err(DeserializeError::InvalidByte(*last_byte)) } - // checked subtraction is unnecessary, as high_bit_index >= 1; qed + // SAFETY: checked subtraction is unnecessary, as high_bit_index >= 1; qed for bit in last.iter().take(high_bit_index - 1) { result.push(*bit); } diff --git a/ssz-rs/src/bitvector.rs b/ssz-rs/src/bitvector.rs index d46d627f..ee096fc4 100644 --- a/ssz-rs/src/bitvector.rs +++ b/ssz-rs/src/bitvector.rs @@ -60,7 +60,7 @@ impl fmt::Debug for Bitvector { let value = i32::from(*bit); write!(f, "{value}")?; bits_written += 1; - // checked subtraction is unnecessary, as len >= 1 for bitvectors; qed + // SAFETY: checked subtraction is unnecessary, as len >= 1 for bitvectors; qed if bits_written % 4 == 0 && index != len - 1 { write!(f, "_")?; } diff --git a/ssz-rs/src/de.rs b/ssz-rs/src/de.rs index c6b64f00..693cc854 100644 --- a/ssz-rs/src/de.rs +++ b/ssz-rs/src/de.rs @@ -71,7 +71,7 @@ where if remainder != 0 { return Err(DeserializeError::AdditionalInput { provided: encoding.len(), - // checked subtraction is unnecessary, as encoding.len() > remainder; qed + // SAFETY: checked subtraction is unnecessary, as encoding.len() > remainder; qed expected: encoding.len() - remainder, }) } diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 92e2bc90..07bb581d 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -48,7 +48,8 @@ impl std::error::Error for MerkleizationError {} pub fn pack_bytes(buffer: &mut Vec) { let incomplete_chunk_len = buffer.len() % BYTES_PER_CHUNK; if incomplete_chunk_len != 0 { - // checked subtraction is unnecessary, as BYTES_PER_CHUNK > incomplete_chunk_len; qed + // SAFETY: checked subtraction is unnecessary, + // as BYTES_PER_CHUNK > incomplete_chunk_len; qed let bytes_to_pad = BYTES_PER_CHUNK - incomplete_chunk_len; let pad = vec![0u8; bytes_to_pad]; buffer.extend_from_slice(&pad); @@ -117,13 +118,13 @@ fn merkleize_chunks_with_virtual_padding( let height = leaf_count.trailing_zeros() + 1; if chunk_count == 0 { - // checked subtraction is unnecessary, as height >= 1; qed + // SAFETY: checked subtraction is unnecessary, as height >= 1; qed let depth = height - 1; return Ok(CONTEXT[depth as usize].try_into().expect("can produce a single root chunk")) } let mut layer = chunks.to_vec(); - // checked subtraction is unnecessary, as we return early when chunk_count == 0; qed + // SAFETY: checked subtraction is unnecessary, as we return early when chunk_count == 0; qed let mut last_index = chunk_count - 1; for k in (1..height).rev() { for i in (0..2usize.pow(k)).step_by(2) { @@ -132,7 +133,7 @@ fn merkleize_chunks_with_virtual_padding( Ordering::Less => { let focus = &mut layer[parent_index * BYTES_PER_CHUNK..(i + 2) * BYTES_PER_CHUNK]; - // checked subtraction is unnecessary: + // SAFETY: checked subtraction is unnecessary: // focus.len() = (i + 2 - parent_index) * BYTES_PER_CHUNK // and // i >= parent_index @@ -155,14 +156,14 @@ fn merkleize_chunks_with_virtual_padding( Ordering::Equal => { let focus = &mut layer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; - // checked subtraction is unnecessary: + // SAFETY: checked subtraction is unnecessary: // focus.len() = (i + 1 - parent_index) * BYTES_PER_CHUNK // and // i >= parent_index // so focus.len() >= BYTES_PER_CHUNK; qed let children_index = focus.len() - BYTES_PER_CHUNK; let (parent, left) = focus.split_at_mut(children_index); - // checked subtraction is unnecessary: + // SAFETY: checked subtraction is unnecessary: // k <= height - 1 // so depth >= height - (height - 1) - 1 // = 0; qed @@ -283,9 +284,10 @@ mod tests { debug_assert!(chunks.len() % BYTES_PER_CHUNK == 0); debug_assert!(leaf_count.next_power_of_two() == leaf_count); - // checked subtraction is unnecessary, as leaf_count != 0 (0.next_power_of_two() == 1); qed + // SAFETY: checked subtraction is unnecessary, + // as leaf_count != 0 (0.next_power_of_two() == 1); qed let node_count = 2 * leaf_count - 1; - // checked subtraction is unnecessary, as node_count >= leaf_count; qed + // SAFETY: checked subtraction is unnecessary, as node_count >= leaf_count; qed let interior_count = node_count - leaf_count; let leaf_start = interior_count * BYTES_PER_CHUNK; @@ -300,10 +302,10 @@ mod tests { } for i in (1..node_count).rev().step_by(2) { - // checked subtraction is unnecessary, as i >= 1; qed + // SAFETY: checked subtraction is unnecessary, as i >= 1; qed let parent_index = (i - 1) / 2; let focus = &mut buffer[parent_index * BYTES_PER_CHUNK..(i + 1) * BYTES_PER_CHUNK]; - // checked subtraction is unnecessary: + // SAFETY: checked subtraction is unnecessary: // focus.len() = (i + 1 - parent_index) * BYTES_PER_CHUNK // = ((2*i + 2 - i + 1) / 2) * BYTES_PER_CHUNK // = ((i + 3) / 2) * BYTES_PER_CHUNK From 392cd26684b9cdf05e389234ee7461e5d40b20d5 Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 7 Jun 2023 13:31:05 +0200 Subject: [PATCH 11/22] Replace assert! with OffsetNotIncreasing error --- ssz-rs-derive/src/lib.rs | 11 ++++++++++- ssz-rs/src/de.rs | 6 ++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index d54c25be..3f964fc6 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -193,7 +193,16 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let bytes_read = if <#field_type>::is_variable_size() { let end = start + #BYTES_PER_LENGTH_OFFSET; let next_offset = u32::deserialize(&encoding[start..end])? as usize; - offsets.last().map(|(_, previous_offset)| assert!(next_offset >= *previous_offset)); + + if let Some((_, previous_offset)) = offsets.last() { + if next_offset < *previous_offset { + return Err(DeserializeError::OffsetNotIncreasing { + start: *previous_offset, + end: next_offset, + }); + } + }; + offsets.push((#i, next_offset)); #BYTES_PER_LENGTH_OFFSET diff --git a/ssz-rs/src/de.rs b/ssz-rs/src/de.rs index 693cc854..949f68e1 100644 --- a/ssz-rs/src/de.rs +++ b/ssz-rs/src/de.rs @@ -23,6 +23,11 @@ pub enum DeserializeError { InvalidInstance(InstanceError), /// An invalid type was encountered. InvalidType(TypeError), + /// An offset was found with start > end. + OffsetNotIncreasing { + start: usize, + end: usize, + }, } impl From for DeserializeError { @@ -48,6 +53,7 @@ impl Display for DeserializeError { ), DeserializeError::InvalidInstance(err) => write!(f, "invalid instance: {err}"), DeserializeError::InvalidType(err) => write!(f, "invalid type: {err}"), + DeserializeError::OffsetNotIncreasing { start, end } => write!(f, "invalid offset points to byte {end} before byte {start}"), } } } From b2314613706a70e32b4184f845d1e9ee3872d3d3 Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Wed, 7 Jun 2023 13:37:50 +0200 Subject: [PATCH 12/22] Add notes about slice lengths after splitting --- ssz-rs/src/merkleization/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 07bb581d..cdd6aef0 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -162,6 +162,7 @@ fn merkleize_chunks_with_virtual_padding( // i >= parent_index // so focus.len() >= BYTES_PER_CHUNK; qed let children_index = focus.len() - BYTES_PER_CHUNK; + // NOTE: left.len() == BYTES_PER_CHUNK let (parent, left) = focus.split_at_mut(children_index); // SAFETY: checked subtraction is unnecessary: // k <= height - 1 @@ -313,6 +314,7 @@ mod tests { // i >= 1 // so focus.len() >= 2 * BYTES_PER_CHUNK; qed let children_index = focus.len() - 2 * BYTES_PER_CHUNK; + // NOTE: children.len() == 2 * BYTES_PER_CHUNK let (parent, children) = focus.split_at_mut(children_index); let (left, right) = children.split_at(BYTES_PER_CHUNK); hash_nodes(&mut hasher, left, right, &mut parent[..BYTES_PER_CHUNK]); From 9bf89058fe7dbee4825c2809ca4a75983d13bd6d Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Thu, 8 Jun 2023 16:04:31 +0200 Subject: [PATCH 13/22] Swap InvalidByte for InstanceError::Bounded --- ssz-rs/src/bitlist.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ssz-rs/src/bitlist.rs b/ssz-rs/src/bitlist.rs index de7edb14..e180937e 100644 --- a/ssz-rs/src/bitlist.rs +++ b/ssz-rs/src/bitlist.rs @@ -178,7 +178,10 @@ impl Deserialize for Bitlist { let additional_members = bit_length - 1; // skip marker bit let total_members = result.len() + additional_members; if total_members > N { - return Err(DeserializeError::InvalidByte(*last_byte)) + return Err(DeserializeError::InvalidInstance(InstanceError::Bounded { + bound: N, + provided: total_members, + })) } result.extend_from_bitslice(&last[..additional_members]); From 2ad9a4b43b4bcc2db7ab932a57b459f98069a738 Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Thu, 8 Jun 2023 16:07:05 +0200 Subject: [PATCH 14/22] rustfmt --- ssz-rs/src/bitlist.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ssz-rs/src/bitlist.rs b/ssz-rs/src/bitlist.rs index e180937e..48dfc3d9 100644 --- a/ssz-rs/src/bitlist.rs +++ b/ssz-rs/src/bitlist.rs @@ -164,7 +164,7 @@ impl Deserialize for Bitlist { let (last_byte, prefix) = encoding.split_last().unwrap(); if *last_byte == 0u8 { - return Err(DeserializeError::InvalidByte(*last_byte)); + return Err(DeserializeError::InvalidByte(*last_byte)) } let mut result = BitlistInner::from_slice(prefix); @@ -294,8 +294,7 @@ mod tests { let bytes = vec![24u8, 0u8]; let result = Bitlist::::deserialize(&bytes).expect_err("test data is incorrect"); - let expected = - DeserializeError::InvalidByte(0u8); + let expected = DeserializeError::InvalidByte(0u8); assert_eq!(result.to_string(), expected.to_string()); } From f541e76f08b78d3303d0e44ac3d73f4aec5fd193 Mon Sep 17 00:00:00 2001 From: David Dunn <26876072+doubledup@users.noreply.github.com> Date: Fri, 9 Jun 2023 10:36:52 +0200 Subject: [PATCH 15/22] Only push increasing offsets --- ssz-rs-derive/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 0e6830fc..e9824beb 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -200,7 +200,6 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { } )?; let next_offset = u32::deserialize(target)? as usize; - offsets.push((#i, next_offset)); if let Some((_, previous_offset)) = offsets.last() { if next_offset < *previous_offset { @@ -211,6 +210,8 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { } }; + offsets.push((#i, next_offset)); + #BYTES_PER_LENGTH_OFFSET } else { let encoded_length = <#field_type>::size_hint(); From e30398126a4c6ed8754b023e0cd872aed599a56e Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Fri, 9 Jun 2023 14:39:18 -0600 Subject: [PATCH 16/22] Update ssz-rs-derive/src/lib.rs --- ssz-rs-derive/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index fe4afff0..f413897e 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -207,9 +207,9 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { return Err(DeserializeError::OffsetNotIncreasing { start: *previous_offset, end: next_offset, - }); + }) } - }; + } offsets.push((#i, next_offset)); From 3efa277818539815d27e09af6ed247796c2c741d Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Fri, 9 Jun 2023 14:55:34 -0600 Subject: [PATCH 17/22] Update ssz-rs/src/merkleization/mod.rs --- ssz-rs/src/merkleization/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 7e513e93..cc430ec2 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -49,8 +49,7 @@ pub fn pack_bytes(buffer: &mut Vec) { // SAFETY: checked subtraction is unnecessary, // as BYTES_PER_CHUNK > incomplete_chunk_len; qed let bytes_to_pad = BYTES_PER_CHUNK - incomplete_chunk_len; - let pad = vec![0u8; bytes_to_pad]; - buffer.extend_from_slice(&pad); + buffer.resize(buffer.len() + bytes_to_pad, 0); } } From 220b6c46d13887bba6354d8b16e73ba2b7dc8afb Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Fri, 9 Jun 2023 14:56:55 -0600 Subject: [PATCH 18/22] Update ssz-rs/src/merkleization/mod.rs --- ssz-rs/src/merkleization/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index cc430ec2..e959f200 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -43,6 +43,8 @@ impl Display for MerkleizationError { #[cfg(feature = "std")] impl std::error::Error for MerkleizationError {} +// Ensures `buffer` can be exactly broken up into `BYTES_PER_CHUNK` chunks of bytes +// via padding any partial chunks at the end of `buffer` pub fn pack_bytes(buffer: &mut Vec) { let incomplete_chunk_len = buffer.len() % BYTES_PER_CHUNK; if incomplete_chunk_len != 0 { From 946b4427991d916f7a9f52788547fdd6c78f3dc0 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Fri, 9 Jun 2023 17:25:40 -0600 Subject: [PATCH 19/22] simplify naive merkle hashing code --- ssz-rs/src/merkleization/mod.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index e959f200..fa2be5d7 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -319,12 +319,6 @@ mod tests { let mut hasher = Sha256::new(); let mut buffer = vec![0u8; node_count * BYTES_PER_CHUNK]; buffer[leaf_start..leaf_start + chunks.len()].copy_from_slice(chunks); - let zero_chunk = [0u8; BYTES_PER_CHUNK]; - for i in chunks.len()..leaf_count { - let start = leaf_start + (i * BYTES_PER_CHUNK); - let end = leaf_start + (i + 1) * BYTES_PER_CHUNK; - buffer[start..end].copy_from_slice(&zero_chunk); - } for i in (1..node_count).rev().step_by(2) { // SAFETY: checked subtraction is unnecessary, as i >= 1; qed @@ -343,7 +337,7 @@ mod tests { let (left, right) = children.split_at(BYTES_PER_CHUNK); hash_nodes(&mut hasher, left, right, &mut parent[..BYTES_PER_CHUNK]); } - Ok(buffer[0..BYTES_PER_CHUNK].try_into().expect("can produce a single root chunk")) + Ok(buffer[..BYTES_PER_CHUNK].try_into().expect("can produce a single root chunk")) } #[test] From 6acccedb92e6cb1468b4df6fc530ac09eb1db36f Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Fri, 9 Jun 2023 18:01:39 -0600 Subject: [PATCH 20/22] formatting: code org --- ssz-rs/src/merkleization/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index fa2be5d7..91515786 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -110,15 +110,13 @@ fn merkleize_chunks_with_virtual_padding( chunks: &[u8], leaf_count: usize, ) -> Result { - let chunk_count = chunks.len() / BYTES_PER_CHUNK; - - let mut hasher = Sha256::new(); debug_assert!(chunks.len() % BYTES_PER_CHUNK == 0); // NOTE: This also asserts that leaf_count != 0 debug_assert!(leaf_count.next_power_of_two() == leaf_count); // SAFETY: this holds as long as leaf_count != 0 and usize is no longer than u64 debug_assert!((leaf_count.trailing_zeros() as usize) < MAX_MERKLE_TREE_DEPTH); + let chunk_count = chunks.len() / BYTES_PER_CHUNK; let height = leaf_count.trailing_zeros() + 1; if chunk_count == 0 { @@ -132,6 +130,7 @@ fn merkleize_chunks_with_virtual_padding( let mut layer = chunks.to_vec(); // SAFETY: checked subtraction is unnecessary, as we return early when chunk_count == 0; qed let mut last_index = chunk_count - 1; + let mut hasher = Sha256::new(); for k in (1..height).rev() { for i in (0..2usize.pow(k)).step_by(2) { let parent_index = i / 2; From 9400cc4f892bbc6f0e5b2b3e144856217b7de56c Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Fri, 9 Jun 2023 18:25:05 -0600 Subject: [PATCH 21/22] add some clarifying docs --- ssz-rs/src/merkleization/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index 91515786..d87a52eb 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -102,6 +102,10 @@ include!(concat!(env!("OUT_DIR"), "/context.rs")); /// of two and this can be quite large for some types. "Zero" subtrees are virtualized to avoid the /// memory and computation cost of large trees with partially empty leaves. /// +/// The implementation approach treats `chunks` as the bottom layer of a perfect binary tree +/// and for each height performs the hashing required to compute the parent layer in place. +/// This process is repated until the root is computed. +/// /// Invariant: `chunks.len() % BYTES_PER_CHUNK == 0` /// Invariant: `leaf_count.next_power_of_two() == leaf_count` /// Invariant: `leaf_count != 0` @@ -131,7 +135,9 @@ fn merkleize_chunks_with_virtual_padding( // SAFETY: checked subtraction is unnecessary, as we return early when chunk_count == 0; qed let mut last_index = chunk_count - 1; let mut hasher = Sha256::new(); + // for each layer of the tree, starting from the bottom and walking up to the root: for k in (1..height).rev() { + // for each pair of nodes in this layer: for i in (0..2usize.pow(k)).step_by(2) { let parent_index = i / 2; match i.cmp(&last_index) { From 8bde6b564c0af1bd7c8d041c5081ab8ef0aa95af Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Fri, 9 Jun 2023 18:25:23 -0600 Subject: [PATCH 22/22] factor out hashing from node juggling --- ssz-rs/src/merkleization/mod.rs | 42 +++++++++++++-------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/ssz-rs/src/merkleization/mod.rs b/ssz-rs/src/merkleization/mod.rs index d87a52eb..d3b4df96 100644 --- a/ssz-rs/src/merkleization/mod.rs +++ b/ssz-rs/src/merkleization/mod.rs @@ -140,7 +140,7 @@ fn merkleize_chunks_with_virtual_padding( // for each pair of nodes in this layer: for i in (0..2usize.pow(k)).step_by(2) { let parent_index = i / 2; - match i.cmp(&last_index) { + let (parent, left, right) = match i.cmp(&last_index) { Ordering::Less => { // SAFETY: index is safe because (i+1)*BYTES_PER_CHUNK < layer.len(): // i < last_index == chunk_count - 1 == (layer.len() / BYTES_PER_CHUNK) - 1 @@ -156,19 +156,9 @@ fn merkleize_chunks_with_virtual_padding( let children_index = focus.len() - 2 * BYTES_PER_CHUNK; let (parent, children) = focus.split_at_mut(children_index); let (left, right) = children.split_at_mut(BYTES_PER_CHUNK); - if parent.is_empty() { - // TODO: Why not i == 0? Is there another case where the left node is used - // to store the parent? - // NOTE: have to specially handle the situation where the children nodes and - // parent node share memory - hasher.update(&left); - hasher.update(right); - left.copy_from_slice(&hasher.finalize_reset()); - } else { - // SAFETY: index is safe because parent.len() % BYTES_PER_CHUNK == 0 and - // parent isn't empty; qed - hash_nodes(&mut hasher, left, right, &mut parent[..BYTES_PER_CHUNK]); - } + + // NOTE: we do not need mutability on `right` here so drop that capability + (parent, left, &*right) } Ordering::Equal => { // SAFETY: index is safe because i*BYTES_PER_CHUNK < layer.len(): @@ -193,20 +183,22 @@ fn merkleize_chunks_with_virtual_padding( // depth <= height - 1 == leaf_count.trailing_zeros() // leaf_count.trailing_zeros() < MAX_MERKLE_TREE_DEPTH == CONTEXT.len(); qed let right = &CONTEXT[depth as usize]; - if parent.is_empty() { - // NOTE: have to specially handle the situation where the children nodes and - // parent node share memory - hasher.update(&left); - hasher.update(right); - left.copy_from_slice(&hasher.finalize_reset()); - } else { - // SAFETY: index is safe because parent.len() % BYTES_PER_CHUNK == 0 and - // parent isn't empty; qed - hash_nodes(&mut hasher, left, right, &mut parent[..BYTES_PER_CHUNK]); - } + (parent, left, right) } _ => break, }; + if i == 0 { + // NOTE: nodes share memory here and so we can't use the `hash_nodes` utility + // as the disjunct nature is reflect in that functions type signature + // so instead we will just replicate here. + hasher.update(&left); + hasher.update(right); + left.copy_from_slice(&hasher.finalize_reset()); + } else { + // SAFETY: index is safe because parent.len() % BYTES_PER_CHUNK == 0 and + // parent isn't empty; qed + hash_nodes(&mut hasher, left, right, &mut parent[..BYTES_PER_CHUNK]); + } } last_index /= 2; }