Fixing failing tests for optimized azks #206

Jasleen1 · 2022-06-08T22:37:40Z

Working through the checklist detailed here: #189 (comment)

* Add left and right child for tree node * Add hash to HistoryTreeNode * Update set/get child * Update hash at parent * Update hashes in the tree * Add epoch to set_child for updating last updated epoch * Remove unnecessary last epoch update (already done in set_child) * Enable higher level tests * Update and pass leaf insertion tests * Pass tree insertion tests * Remove commented out HistoryTreeNode tests * Remove commented out unused functions * Remove printlns * Remove printlns from AZKS * Remove storage from function when not needed * Fix warnings for cargo build and test * Remove HistoryNodeState and HistoryChildState * Remove get_epoch_lte_epoch * Remove HistoryNodeState and HistoryChildState from storage layer * Remove get_epoch_lte_epoch usage * Format * Fix clippy warnings * Remove birth_epoch * Disable tests to merge * Remove needless question mark * Disable mysql tests * Disable test_mysql_db test

slawlor

So generally, it seems like we've either deleted a lot of tests or at least disabled them. Is this considered WIP? Like eventually we'll restore them right?

Otherwise it's just a lot of tiny styling nits and whatnot, but this looks really, really good! Thanks!

slawlor · 2022-06-09T15:05:41Z

akd/src/append_only_zks.rs

@@ -297,26 +266,27 @@ impl Azks {
        &self,
        storage: &S,
        label: NodeLabel,
-        epoch: u64,
+        _epoch: u64,


nit: If we no longer need the epoch for these proofs (and it isn't going to be put back later) can we clean up the function signature?

slawlor · 2022-06-09T15:06:32Z

akd/src/append_only_zks.rs

-            // really you shouldn't even be here. Later do error checking
-            return Ok((unchanged, leaves));
-        }
+        // if node.get_birth_epoch() > end_epoch {


nit: From the comment it looks like this wasn't necessary anyways. Can we cleanup this artifact?

slawlor · 2022-06-09T15:09:51Z

akd/src/append_only_zks.rs

@@ -725,6 +680,37 @@ mod tests {
        Ok(())
    }

+    #[tokio::test]
+    #[allow(dead_code)]


nit: Do we need the dead code attribute? is it not in a #[cfg(test)] block? If not can we move them to a

#[cfg(test)] mod tests { ... }

slawlor · 2022-06-09T15:11:24Z

akd/src/history_tree_node.rs

@@ -141,50 +157,24 @@ impl HistoryTreeNode {
    pub(crate) async fn get_from_storage<S: Storage + Send + Sync>(
        storage: &S,
        key: &NodeKey,
-        current_epoch: u64,
+        _current_epoch: u64,


nit: Remove the unused (unless we're going to add it back later)?

slawlor · 2022-06-09T15:11:53Z

akd/src/history_tree_node.rs

    ) -> Result<Vec<HistoryTreeNode>, StorageError> {
        let node_records: Vec<DbRecord> = storage.batch_get::<HistoryTreeNode>(keys).await?;
        let mut nodes = Vec::<HistoryTreeNode>::new();
-        for (i, node) in node_records.into_iter().enumerate() {
+        for (_i, node) in node_records.into_iter().enumerate() {


nit: Don't enumerate if we don't need the iterator index. so just for node in node_records.into_iter()

slawlor · 2022-06-09T15:13:20Z

akd/src/history_tree_node.rs

-                }
-                debug!("END insert single leaf (dir_self = None)");
-                Ok(())
+                let result = match child_node {


nit: Remove the assignment of result, and just have the match statement end the fn.

slawlor · 2022-06-09T15:13:59Z

akd/src/history_tree_node.rs

    }

    /// Inserts a child into this node, adding the state to the state at this epoch,
    /// without updating its own hash.
-    #[async_recursion]
-    pub(crate) async fn set_child<S: Storage + Sync + Send, H: Hasher>(
+    // #[async_recursion]


nit: just delete this

slawlor · 2022-06-09T15:15:27Z

akd/src/history_tree_node.rs

-                    // birth epoch but we're less than the "last" epoch.
-                    // db query is necessary
-                    None
+                    panic!(


nit: can we return AkdError instead of panicing?

slawlor · 2022-06-09T15:20:01Z

akd/src/storage/types.rs

@@ -20,8 +19,8 @@ pub enum StorageType {
    Azks = 1,
    /// HistoryTreeNode
    HistoryTreeNode = 2,
-    /// HistoryNodeState
-    HistoryNodeState = 3,
+    /// EOZ: HistoryNodeState = 3 was removed from here.


To answer the question in the comment: We're going to do a DB break anyways, we can update this to 3. If we were adding types and trying for a db upgrade, then yeah. But until we release "version 1" we don't have much to worry about if we're going to break all backwards compat.

Jasleen1 · 2022-06-09T15:25:31Z

So generally, it seems like we've either deleted a lot of tests or at least disabled them. Is this considered WIP? Like eventually we'll restore them right?

Otherwise it's just a lot of tiny styling nits and whatnot, but this looks really, really good! Thanks!

Thanks a lot for looking it over already! Yes, it's a WIP, I'm working on fixing all the tests @eozturk1 highlighted in #189 (comment)

eozturk1 · 2022-06-09T17:39:32Z

Created #207 to track the next steps (replaces the comment in #189 ).

eozturk1 · 2022-06-09T17:45:06Z

@slawlor I'll re-iterate over the deleted/commented out tests and bring the ones we will need back. Tracked in #208.

eozturk1 · 2022-06-10T22:10:58Z

akd/src/append_only_zks.rs

@@ -790,6 +787,66 @@ mod tests {
        Ok(())
    }

+    #[tokio::test]
+    #[allow(dead_code)]
+    async fn test_nonmembership_proof_v_small() -> Result<(), AkdError> {


nit: very small?

eozturk1 · 2022-06-10T22:12:55Z

akd/src/append_only_zks.rs

+        let num_nodes = 2;
+
+        let mut insertion_set: Vec<Node<Blake3>> = vec![];
+
+        for i in 0..num_nodes {
+            let mut label_arr = [0u8; 32];
+            label_arr[31] = u8::from(i);
+            let label = NodeLabel::new(label_arr, 256u32);
+            let mut input = [0u8; 32];
+            input[31] = u8::from(i);
+            let hash = Blake3Digest::new(input);
+            let node = Node::<Blake3> { label, hash };
+            insertion_set.push(node);
+        }


Could we move this part to a separate helper function?

What about generate_random_node()?

If we want a specific property for the node, let's add this as a comment.

eozturk1 · 2022-06-10T22:13:42Z

akd/src/append_only_zks.rs

+
+        for i in 0..num_nodes {
+            let mut label_arr = [0u8; 32];
+            label_arr[31] = u8::from(i);


Why do we do this?

eozturk1 · 2022-06-10T22:16:12Z

akd/src/append_only_zks.rs

+    #[allow(dead_code)]
+    async fn test_nonmembership_proof_v_small() -> Result<(), AkdError> {


Can you remove #[allow(dead_code)] and add a description for the test?

eozturk1 · 2022-06-10T22:24:32Z

akd/src/append_only_zks.rs

+    #[allow(dead_code)]
+    async fn test_nonmembership_proof_small() -> Result<(), AkdError> {


Same as above function.

eozturk1 · 2022-06-10T22:28:57Z

akd/src/directory.rs

+            .await?;
+        let commitment_label = self
+            .vrf
+            .get_node_label_from_vrf_pf::<H>(existence_vrf)


nit: We can keep this as proof, no need to shorten :)

eozturk1 · 2022-06-10T22:31:38Z

akd/src/tests.rs

@@ -57,7 +57,7 @@ async fn test_simple_publish() -> Result<(), AkdError> {
    Ok(())
 }

-// #[tokio::test]
+#[tokio::test]
 #[allow(dead_code)]


We should remove #[allow(dead_code)]s since we have enabled the tests.

eozturk1 · 2022-06-10T22:33:59Z

akd/src/utils.rs

    value: &AkdValue,
 ) -> H::Digest {
-    let proof = get_commitment_proof::<H>(commitment_key, label, version, value);
+    let proof = get_commitment_proof::<H>(commitment_key, label, value);
    H::hash(&[i2osp_array(value), i2osp_array(&proof.as_bytes())].concat())


Should we use i2osp_array before each hash operation?

eozturk1 · 2022-06-10T22:35:47Z

akd/src/append_only_zks.rs

+        let db = AsyncInMemoryDatabase::new();
+        let mut azks = Azks::new::<_, Blake3>(&db).await?;
+        let search_label = insertion_set[0].label;
+        azks.batch_insert_leaves::<_, Blake3>(&db, insertion_set.clone()[1..2].to_vec())


Let's separate the node we are searching for from the nodes we are inserting into the tree.

let node_to_search = generate_random_node(); let insertion_set = generate_nodes(); // uses generate_random_node() azks.batch_insert_leaves(insertion_set); let proof = azks.get_non_membership_proof(node_to_search.label); // Verify proof

…assing

Jasleen1 · 2022-06-15T23:03:50Z

This PR addresses #144 and #207, except the last task in the checklist of #207.

kevinlewi

There are a couple of allow(dead_code)'s. Can they be removed?

kevinlewi · 2022-06-16T18:46:58Z

akd/src/proof_structs.rs

+    pub epochs: Vec<u64>,
+}
+
+// FIXME: need to fix the documentation


Does this still need to be addressed?

Will be addressing in a cleanup PR

kevinlewi · 2022-06-16T18:47:29Z

akd_client/src/tests.rs

@@ -392,6 +407,7 @@ async fn test_history_proof_multiple_epochs() -> Result<(), AkdError> {
 }

 #[tokio::test]
+#[allow(dead_code)]


Can we remove this?

All #[allow(dead_code)]s should be removed after enabling the tests. I mentioned this in a few other places. @Jasleen1 said she would do a clean up before merging.

kevinlewi · 2022-06-16T18:47:42Z

akd_client/src/tests.rs

@@ -436,6 +449,7 @@ async fn test_history_proof_single_epoch() -> Result<(), AkdError> {
 }

 #[tokio::test]
+#[allow(dead_code)]


Can we remove this?

kevinlewi · 2022-06-16T22:04:37Z

Ok cool. Also, we want to merge this into the eozturk1/optimized-azks branch, not main, right?

eozturk1 and others added 2 commits June 1, 2022 22:43

Fixed membership and intermediate membership tests

511927b

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 8, 2022

Jasleen1 requested review from eozturk1, slawlor, kevinlewi and afterdusk June 8, 2022 22:38

slawlor reviewed Jun 9, 2022

View reviewed changes

Fixed non membership tests

c430808

eozturk1 reviewed Jun 10, 2022

View reviewed changes

eozturk1 mentioned this pull request Jun 11, 2022

Improve Tests #208

Closed

Harjasleen Malvai added 9 commits June 14, 2022 15:02

WIP: Fixed key history for optimization

2c83374

WIP: Minor bug fixes in key history and updated client to match

901c78c

WIP: removed epochs from leaf stored hash

64f8fae

WIP: Updated audit to match ozks, all but read during publish tests p…

a72349f

…assing

Minor cleanup

cb06288

Added more testing for facebook#144

c19763c

Changed HistoryTreeNode to TreeNode

6da526c

Uncommented previously commented tests

340f013

Addressed nits

4b65c00

Jasleen1 marked this pull request as ready for review June 15, 2022 23:02

This was referenced Jun 15, 2022

Updating append only for oZKS #205

Closed

Optimized azks implementation #186

Closed

Verification failure with small trees #144

Closed

kevinlewi reviewed Jun 16, 2022

View reviewed changes

Removed allow dead code from passing tests

57380f1

Jasleen1 closed this Jun 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing failing tests for optimized azks #206

Fixing failing tests for optimized azks #206

Jasleen1 commented Jun 8, 2022

slawlor left a comment

slawlor Jun 9, 2022

slawlor Jun 9, 2022

slawlor Jun 9, 2022

slawlor Jun 9, 2022

slawlor Jun 9, 2022

slawlor Jun 9, 2022

slawlor Jun 9, 2022

slawlor Jun 9, 2022

slawlor Jun 9, 2022

Jasleen1 commented Jun 9, 2022 •

edited

eozturk1 commented Jun 9, 2022

eozturk1 commented Jun 9, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

eozturk1 Jun 10, 2022

Jasleen1 commented Jun 15, 2022

kevinlewi left a comment

kevinlewi Jun 16, 2022

Jasleen1 Jun 16, 2022

kevinlewi Jun 16, 2022

eozturk1 Jun 16, 2022

kevinlewi Jun 16, 2022

kevinlewi commented Jun 16, 2022

		#[allow(dead_code)]
		async fn test_nonmembership_proof_v_small() -> Result<(), AkdError> {

		#[allow(dead_code)]
		async fn test_nonmembership_proof_small() -> Result<(), AkdError> {

Fixing failing tests for optimized azks #206

Fixing failing tests for optimized azks #206

Conversation

Jasleen1 commented Jun 8, 2022

slawlor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasleen1 commented Jun 9, 2022 • edited

eozturk1 commented Jun 9, 2022

eozturk1 commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasleen1 commented Jun 15, 2022

kevinlewi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinlewi commented Jun 16, 2022

Jasleen1 commented Jun 9, 2022 •

edited