From fb8c73c9500de06c68184e6056d48de1400fa677 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 5 Dec 2019 11:06:26 +0000 Subject: [PATCH 1/7] Use iterators in construct_mmr_tree --- librustzcash/src/rustzcash.rs | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/librustzcash/src/rustzcash.rs b/librustzcash/src/rustzcash.rs index 0bfd5ad..16acd87 100644 --- a/librustzcash/src/rustzcash.rs +++ b/librustzcash/src/rustzcash.rs @@ -1199,31 +1199,17 @@ fn construct_mmr_tree( ) }; - let mut peaks = Vec::new(); - for i in 0..p_len { - peaks.push(( - indices[i], - match MMREntry::from_bytes(cbranch, &nodes[i][..]) { - Ok(entry) => entry, - _ => { - return Err("Invalid encoding"); - } // error + let mut peaks: Vec<_> = indices + .iter() + .zip(nodes.iter()) + .map( + |(index, node)| match MMREntry::from_bytes(cbranch, &node[..]) { + Ok(entry) => Ok((*index, entry)), + Err(_) => Err("Invalid encoding"), }, - )); - } - - let mut extra = Vec::new(); - for i in p_len..(p_len + e_len) { - extra.push(( - indices[i], - match MMREntry::from_bytes(cbranch, &nodes[i][..]) { - Ok(entry) => entry, - _ => { - return Err("Invalid encoding"); - } // error - }, - )); - } + ) + .collect::>()?; + let extra = peaks.split_off(p_len); Ok(MMRTree::new(t_len, peaks, extra)) } From 8ad33e50a6d6e2bacbfce43b08398b1f942b379d Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 5 Dec 2019 11:09:55 +0000 Subject: [PATCH 2/7] Use explicit sizes for pointers to arrays in FFI --- librustzcash/src/rustzcash.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/librustzcash/src/rustzcash.rs b/librustzcash/src/rustzcash.rs index 16acd87..d0ac6fb 100644 --- a/librustzcash/src/rustzcash.rs +++ b/librustzcash/src/rustzcash.rs @@ -1228,8 +1228,8 @@ pub extern "system" fn librustzcash_mmr_append( p_len: size_t, // New node pointer nn_ptr: *const [u8; zcash_mmr::MAX_NODE_DATA_SIZE], - // Return of root commitment (32 byte hash) - rt_ret: *mut u8, + // Return of root commitment + rt_ret: *mut [u8; 32], // Return buffer for appended leaves, should be pre-allocated of log2(t_len)+1 length buf_ret: *mut [c_uchar; zcash_mmr::MAX_NODE_DATA_SIZE], ) -> u32 { @@ -1269,7 +1269,7 @@ pub extern "system" fn librustzcash_mmr_append( .root_node() .expect("Just added, should resolve always; qed"); unsafe { - slice::from_raw_parts_mut(rt_ret, 32).copy_from_slice(&root_node.data().subtree_commitment); + (*rt_ret).copy_from_slice(&root_node.data().subtree_commitment); for (idx, next_buf) in slice::from_raw_parts_mut(buf_ret, return_count as usize) .iter_mut() @@ -1300,8 +1300,8 @@ pub extern "system" fn librustzcash_mmr_delete( p_len: size_t, // Extra nodes loaded (for deletion) count e_len: size_t, - // Return of root commitment (32 byte hash) - rt_ret: *mut u8, + // Return of root commitment + rt_ret: *mut [u8; 32], ) -> u32 { let mut tree = match construct_mmr_tree(cbranch, t_len, ni_ptr, n_ptr, p_len, e_len) { Ok(t) => t, @@ -1318,7 +1318,7 @@ pub extern "system" fn librustzcash_mmr_delete( }; unsafe { - slice::from_raw_parts_mut(rt_ret, 32).copy_from_slice( + (*rt_ret).copy_from_slice( &tree .root_node() .expect("Just generated without errors, root should be resolving") @@ -1334,7 +1334,7 @@ pub extern "system" fn librustzcash_mmr_delete( pub extern "system" fn librustzcash_mmr_hash_node( cbranch: u32, n_ptr: *const [u8; zcash_mmr::MAX_NODE_DATA_SIZE], - h_ret: *mut u8, + h_ret: *mut [u8; 32], ) -> u32 { let node_bytes: &[u8; zcash_mmr::MAX_NODE_DATA_SIZE] = unsafe { match n_ptr.as_ref() { @@ -1349,7 +1349,7 @@ pub extern "system" fn librustzcash_mmr_hash_node( }; unsafe { - slice::from_raw_parts_mut(h_ret, 32).copy_from_slice(&node.hash()[..]); + (*h_ret).copy_from_slice(&node.hash()[..]); } return 0; From 573510115d2c3c921e1fb6cd12b093a8fa89b012 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 5 Dec 2019 11:11:32 +0000 Subject: [PATCH 3/7] Clean up remainder of MMR code --- librustzcash/src/rustzcash.rs | 2 +- librustzcash/src/tests/mmr.rs | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/librustzcash/src/rustzcash.rs b/librustzcash/src/rustzcash.rs index d0ac6fb..e843795 100644 --- a/librustzcash/src/rustzcash.rs +++ b/librustzcash/src/rustzcash.rs @@ -1352,5 +1352,5 @@ pub extern "system" fn librustzcash_mmr_hash_node( (*h_ret).copy_from_slice(&node.hash()[..]); } - return 0; + 0 } diff --git a/librustzcash/src/tests/mmr.rs b/librustzcash/src/tests/mmr.rs index 6e40f7f..6c57632 100644 --- a/librustzcash/src/tests/mmr.rs +++ b/librustzcash/src/tests/mmr.rs @@ -10,7 +10,7 @@ struct TreeView { extra: Vec<(u32, Entry)>, } -fn draft(into: &mut Vec<(u32, Entry)>, vec: &Vec, peak_pos: usize, h: u32) { +fn draft(into: &mut Vec<(u32, Entry)>, vec: &[NodeData], peak_pos: usize, h: u32) { let node_data = vec[peak_pos - 1].clone(); let peak: Entry = match h { 0 => node_data.into(), @@ -24,8 +24,8 @@ fn draft(into: &mut Vec<(u32, Entry)>, vec: &Vec, peak_pos: usize, h: into.push(((peak_pos - 1) as u32, peak)); } -fn prepare_tree(vec: &Vec) -> TreeView { - assert!(vec.len() > 0); +fn prepare_tree(vec: &[NodeData]) -> TreeView { + assert!(!vec.is_empty()); // integer log2 of (vec.len()+1), -1 let mut h = (32 - ((vec.len() + 1) as u32).leading_zeros() - 1) - 1; @@ -39,8 +39,8 @@ fn prepare_tree(vec: &Vec) -> TreeView { loop { if peak_pos > vec.len() { // left child, -2^h - peak_pos = peak_pos - (1 << h); - h = h - 1; + peak_pos -= 1 << h; + h -= 1; } if peak_pos <= vec.len() { @@ -51,7 +51,7 @@ fn prepare_tree(vec: &Vec) -> TreeView { last_peak_h = h; // right sibling - peak_pos = peak_pos + (1 << (h + 1)) - 1; + peak_pos += (1 << (h + 1)) - 1; } if h == 0 { @@ -67,7 +67,7 @@ fn prepare_tree(vec: &Vec) -> TreeView { while h > 0 { let left_pos = peak_pos - (1 << h); let right_pos = peak_pos - 1; - h = h - 1; + h -= 1; // drafting left child draft(&mut extra, vec, left_pos, h); @@ -85,8 +85,8 @@ fn prepare_tree(vec: &Vec) -> TreeView { } } -fn preload_tree_append(vec: &Vec) -> (Vec, Vec<[u8; zcash_mmr::MAX_ENTRY_SIZE]>) { - assert!(vec.len() > 0); +fn preload_tree_append(vec: &[NodeData]) -> (Vec, Vec<[u8; zcash_mmr::MAX_ENTRY_SIZE]>) { + assert!(!vec.is_empty()); let tree_view = prepare_tree(vec); @@ -107,9 +107,9 @@ fn preload_tree_append(vec: &Vec) -> (Vec, Vec<[u8; zcash_mmr::MA // also returns number of peaks fn preload_tree_delete( - vec: &Vec, + vec: &[NodeData], ) -> (Vec, Vec<[u8; zcash_mmr::MAX_ENTRY_SIZE]>, usize) { - assert!(vec.len() > 0); + assert!(!vec.is_empty()); let tree_view = prepare_tree(vec); From 583a04b4de4fa0a042f0b55ddaee0a794ba29b34 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 5 Dec 2019 14:23:59 +0000 Subject: [PATCH 4/7] Pass array references correctly in MMR tests --- librustzcash/src/tests/mmr.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/librustzcash/src/tests/mmr.rs b/librustzcash/src/tests/mmr.rs index 6c57632..aca4766 100644 --- a/librustzcash/src/tests/mmr.rs +++ b/librustzcash/src/tests/mmr.rs @@ -181,7 +181,7 @@ fn append() { peaks.as_ptr(), peaks.len(), &new_node_data, - rt_ret.as_mut_ptr(), + &mut rt_ret, buf_ret.as_mut_ptr(), ); @@ -220,7 +220,7 @@ fn delete() { nodes.as_ptr(), peak_count, indices.len() - peak_count, - rt_ret.as_mut_ptr(), + &mut rt_ret, ); // Deleting from full tree of 9 height would result in cascade deleting of 10 nodes From edcd884fe89ffd0eefa73668095b883c61303291 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 5 Dec 2019 14:24:31 +0000 Subject: [PATCH 5/7] Simplify short array copies --- librustzcash/src/rustzcash.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/librustzcash/src/rustzcash.rs b/librustzcash/src/rustzcash.rs index e843795..3a6aa5d 100644 --- a/librustzcash/src/rustzcash.rs +++ b/librustzcash/src/rustzcash.rs @@ -1269,7 +1269,7 @@ pub extern "system" fn librustzcash_mmr_append( .root_node() .expect("Just added, should resolve always; qed"); unsafe { - (*rt_ret).copy_from_slice(&root_node.data().subtree_commitment); + *rt_ret = root_node.data().subtree_commitment; for (idx, next_buf) in slice::from_raw_parts_mut(buf_ret, return_count as usize) .iter_mut() @@ -1318,13 +1318,11 @@ pub extern "system" fn librustzcash_mmr_delete( }; unsafe { - (*rt_ret).copy_from_slice( - &tree - .root_node() - .expect("Just generated without errors, root should be resolving") - .data() - .subtree_commitment, - ); + *rt_ret = tree + .root_node() + .expect("Just generated without errors, root should be resolving") + .data() + .subtree_commitment; } truncate_len @@ -1349,7 +1347,7 @@ pub extern "system" fn librustzcash_mmr_hash_node( }; unsafe { - (*h_ret).copy_from_slice(&node.hash()[..]); + *h_ret = node.hash(); } 0 From f1619f896caa5e021c92e5c57fcea8f96326a474 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 5 Dec 2019 14:29:08 +0000 Subject: [PATCH 6/7] Clearer variable names in MMR tests --- librustzcash/src/tests/mmr.rs | 41 ++++++++++++++++------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/librustzcash/src/tests/mmr.rs b/librustzcash/src/tests/mmr.rs index aca4766..4107a75 100644 --- a/librustzcash/src/tests/mmr.rs +++ b/librustzcash/src/tests/mmr.rs @@ -10,8 +10,8 @@ struct TreeView { extra: Vec<(u32, Entry)>, } -fn draft(into: &mut Vec<(u32, Entry)>, vec: &[NodeData], peak_pos: usize, h: u32) { - let node_data = vec[peak_pos - 1].clone(); +fn draft(into: &mut Vec<(u32, Entry)>, nodes: &[NodeData], peak_pos: usize, h: u32) { + let node_data = nodes[peak_pos - 1].clone(); let peak: Entry = match h { 0 => node_data.into(), _ => Entry::new( @@ -24,27 +24,27 @@ fn draft(into: &mut Vec<(u32, Entry)>, vec: &[NodeData], peak_pos: usize, h: u32 into.push(((peak_pos - 1) as u32, peak)); } -fn prepare_tree(vec: &[NodeData]) -> TreeView { - assert!(!vec.is_empty()); +fn prepare_tree(nodes: &[NodeData]) -> TreeView { + assert!(!nodes.is_empty()); - // integer log2 of (vec.len()+1), -1 - let mut h = (32 - ((vec.len() + 1) as u32).leading_zeros() - 1) - 1; + // integer log2 of (nodes.len()+1), -1 + let mut h = (32 - ((nodes.len() + 1) as u32).leading_zeros() - 1) - 1; let mut peak_pos = (1 << (h + 1)) - 1; - let mut nodes = Vec::new(); + let mut peaks = Vec::new(); // used later let mut last_peak_pos = 0; let mut last_peak_h = 0; loop { - if peak_pos > vec.len() { + if peak_pos > nodes.len() { // left child, -2^h peak_pos -= 1 << h; h -= 1; } - if peak_pos <= vec.len() { - draft(&mut nodes, vec, peak_pos, h); + if peak_pos <= nodes.len() { + draft(&mut peaks, nodes, peak_pos, h); // save to be used in next loop last_peak_pos = peak_pos; @@ -70,25 +70,22 @@ fn prepare_tree(vec: &[NodeData]) -> TreeView { h -= 1; // drafting left child - draft(&mut extra, vec, left_pos, h); + draft(&mut extra, nodes, left_pos, h); // drafting right child - draft(&mut extra, vec, right_pos, h); + draft(&mut extra, nodes, right_pos, h); // continuing on right slope peak_pos = right_pos; } - TreeView { - peaks: nodes, - extra, - } + TreeView { peaks, extra } } -fn preload_tree_append(vec: &[NodeData]) -> (Vec, Vec<[u8; zcash_mmr::MAX_ENTRY_SIZE]>) { - assert!(!vec.is_empty()); +fn preload_tree_append(nodes: &[NodeData]) -> (Vec, Vec<[u8; zcash_mmr::MAX_ENTRY_SIZE]>) { + assert!(!nodes.is_empty()); - let tree_view = prepare_tree(vec); + let tree_view = prepare_tree(nodes); let mut indices = Vec::new(); let mut bytes = Vec::new(); @@ -107,11 +104,11 @@ fn preload_tree_append(vec: &[NodeData]) -> (Vec, Vec<[u8; zcash_mmr::MAX_E // also returns number of peaks fn preload_tree_delete( - vec: &[NodeData], + nodes: &[NodeData], ) -> (Vec, Vec<[u8; zcash_mmr::MAX_ENTRY_SIZE]>, usize) { - assert!(!vec.is_empty()); + assert!(!nodes.is_empty()); - let tree_view = prepare_tree(vec); + let tree_view = prepare_tree(nodes); let mut indices = Vec::new(); let mut bytes = Vec::new(); From cca16702487c2a42b09e3cae96ecc3cf9415e5db Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 5 Dec 2019 14:33:03 +0000 Subject: [PATCH 7/7] Clarify length of return buffer for appended leaves --- librustzcash/src/rustzcash.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/librustzcash/src/rustzcash.rs b/librustzcash/src/rustzcash.rs index 3a6aa5d..6dadf67 100644 --- a/librustzcash/src/rustzcash.rs +++ b/librustzcash/src/rustzcash.rs @@ -1230,7 +1230,7 @@ pub extern "system" fn librustzcash_mmr_append( nn_ptr: *const [u8; zcash_mmr::MAX_NODE_DATA_SIZE], // Return of root commitment rt_ret: *mut [u8; 32], - // Return buffer for appended leaves, should be pre-allocated of log2(t_len)+1 length + // Return buffer for appended leaves, should be pre-allocated of ceiling(log2(t_len)) length buf_ret: *mut [c_uchar; zcash_mmr::MAX_NODE_DATA_SIZE], ) -> u32 { let new_node_bytes: &[u8; zcash_mmr::MAX_NODE_DATA_SIZE] = unsafe {