Optimized azks #189

eozturk1 · 2022-04-14T22:47:22Z

See Issue #186.

With the removal of previous states of HistoryTreeNode, we don't have a need for keeping states for multiple epochs. In result, HistoryTreeNode, HistoryNodeState and HistoryChildState can simply be replaced with TreeNode.

This diff starts this conversion by adding the labels of its (1) left and right children, and (2) its hash to the HistoryTreeNode (not yet re-named). Actual hash updates will be added in the next diffs. Rather than keeping the node hashes at the parent, I will keep them at the node itself.

NOTE: The PR builds with cargo build but not ready for testing. The purpose of the PR is to "get it out there" to get feedback rather than merging it.

slawlor

Some nits to consider. @eozturk1 are we waiting on @Jasleen1 or @kevinlewi to give it a process pass to see if you're on the right track? There's obviously some clippy and rustfmt issues, but functionally it makes sense to me I think.

slawlor · 2022-05-09T15:34:17Z

akd/src/history_tree_node.rs

 use async_recursion::async_recursion;
 use log::debug;
 use std::convert::TryInto;
 use std::marker::{Send, Sync};
-use winter_crypto::Hasher;
+use winter_crypto::{Hasher};


nit ::Hasher

slawlor · 2022-05-09T15:39:21Z

akd/src/history_tree_node.rs

+                        Err(AkdError::HistoryTreeNode(HistoryTreeNodeError::NoChildAtEpoch(epoch, dir_leaf.unwrap())))
+                    }
+                };
+                result


nit: if you're just returning result, can you just remove the variable assignment? I.e.

match child_node { ... }

without the let result = match child_node {...}; result

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

akd/src/history_tree_node.rs

-                    self.get_value::<_, H>(storage).await?,
+                let mut leaf_hash = H::hash(&self.hash);
+                leaf_hash = H::merge(&[
+                    leaf_hash,


nit: instead of a mut, with multiple assignments can we just do

let leaf_hash = H::merge(&[ H::hash(&self.hash), hash_label::<H>(self.label), ]);

slawlor · 2022-05-09T15:43:44Z

akd/src/history_tree_node.rs

            }
-            Err(e) => Err(AkdError::Storage(e)),
+        } else {
+            panic!("Unknown child index or None.");


hmmm. I think we're trying to avoid Panic's. Is this something that can never happen? We should then not pass in an optional, or if it could happen return Err

slawlor · 2022-05-09T16:37:49Z

akd/src/history_tree_node.rs

-        for child in children.iter().take(ARITY) {
-            let hash_val = optional_history_child_state_to_hash::<H>(child);
-            new_hash = H::merge(&[new_hash, to_digest::<H>(&hash_val)?]);
+    // pub(crate) async fn get_value_without_label_at_epoch<S: Storage + Sync + Send, H: Hasher>(


nit: is this to be resurrected at some point or just no longer needed? delete it if the latter?

slawlor · 2022-05-09T16:38:24Z

akd/src/history_tree_node.rs

+        } else if dir == Some(1) {
+            child_label_to_ret = self.right_child;
+        } else {
+            panic!("No child with that index!");


nit: as before in another spot, are there better options than panic's?

eozturk1 · 2022-05-09T17:25:30Z

@slawlor, @Jasleen1 and I have done an initial pass on the code. This week I'll try and update the tests to matc the new tree construction. I'll also fix the other issues (rustfmt, clippy) and the parts you mentioned.

eozturk1 · 2022-05-24T03:59:33Z

Summary

This PR so far has removed the need for storing states-per-epoch and revised the HistoryTreeNode (referred to as node below) to contain the only (current) state (aka hash), significantly reducing the storage requirements.

Important Changes

Each node keeps a label of their left and right children, and its hash.
For leaf nodes the hash is over the initial digest and the label. Initial hash is the hash of public key (and epoch, see next steps).
For non-leaf nodes, the hash is over the left and right child, and the label.
For the root, we use the empty node hash: over the empty value and empty label.
The last_epoch has been kept to be used in append-only proofs whereas birth_epoch is not needed anymore.
For each call, set_child and update_node_hash writes the updated nodes to the storage for consistency. See below for optimizations over this.
Epochs in the functions have been kept for adding them to the hash later.
Storage has been removed from function parameters if the function operated on state or children.

Next Steps:

Improvements

Tests: We should add more tests for small trees and nodes with non-uniform distribution to increase test coverage for tree creation.
Minimizing storage writes: If a node is not written to the storage as soon as it is updated, a dirty-read may occur for consecutive operations. We might be able to minimize these writes by allowing short-term inconsistencies by restructuring the code. Another question to look into is how expensive are these instant-writes? (@afterdusk mentioned we use transactions to minimize the impact)
Tree Visualization: Noticed it is hard to track the current state of the tree through logs. Maybe a visual representation could assist us in this?

kevinlewi · 2022-06-01T22:18:22Z

Discussed in-person with @eozturk1 and @Jasleen1 , let's disable the currently-failing tests and fix clippy, so that we can land and @Jasleen1 can work on top of the changes here (filling in the rest of the checkboxes).

eozturk1 · 2022-06-01T23:16:40Z

I have disabled the failing tests and updated the next steps to list those tests.

kevinlewi · 2022-06-02T02:13:36Z

Not sure what the integration test issue is exactly, but perhaps we can merge into a separate branch so that @Jasleen1 can start working on top of it. cc: @slawlor

* Optimized azks (#189) * 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 * Fixed membership and intermediate membership tests * Fixed non membership tests * WIP: Fixed key history for optimization * WIP: Minor bug fixes in key history and updated client to match * WIP: removed epochs from leaf stored hash * WIP: Updated audit to match ozks, all but read during publish tests passing * Minor cleanup * Added more testing for #144 * Changed HistoryTreeNode to TreeNode * Uncommented previously commented tests * Addressed nits * Removed allow dead code from passing tests * Removed HistoryTreeNode references, replaced with TreeNode * Re-ran linting * Re-ran linting * Re-ran linting * Removed birth ep from being read for mysql TreeNode * Removed birth ep from being read for mysql TreeNode everywhere * mysql error due to possibly null children for a TreeNode * Allowing nul for children in mysql -- not working * Fixing the MySQL db layer * All tests passing + ran linting * Fixed descendent spelling * Reran linting * Addressing review comments Co-authored-by: Ercan Ozturk <eoz@fb.com> Co-authored-by: Harjasleen Malvai <hmalvai@fb.com> Co-authored-by: Sean Lawlor <seanlawlor@fb.com>

eozturk1 added 3 commits April 5, 2022 21:55

Add left and right child for tree node

16ba6c6

Add hash to HistoryTreeNode

6a437e7

Update set/get child

893d427

eozturk1 requested review from kevinlewi and Jasleen1 April 14, 2022 22:47

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 Apr 14, 2022

eozturk1 added 5 commits April 14, 2022 22:55

Update hash at parent

4c9f994

Update hashes in the tree

fdb7e32

Add epoch to set_child for updating last updated epoch

9a2cc4c

Remove unnecessary last epoch update (already done in set_child)

fcf4650

Enable higher level tests

ed3cdeb

slawlor reviewed May 9, 2022

View reviewed changes

afterdusk mentioned this pull request May 12, 2022

Fixture generator tool for tests #195

Merged

eozturk1 added 2 commits May 22, 2022 18:03

Update and pass leaf insertion tests

146c4d6

Pass tree insertion tests

6107c4e

eozturk1 added 13 commits May 24, 2022 10:25

Remove commented out HistoryTreeNode tests

0b13689

Remove commented out unused functions

8e95da6

Remove printlns

bc0747c

Remove printlns from AZKS

ba33738

Remove storage from function when not needed

facc740

Fix warnings for cargo build and test

79e9966

Remove HistoryNodeState and HistoryChildState

05f70c7

Remove get_epoch_lte_epoch

d9ab7fb

Remove HistoryNodeState and HistoryChildState from storage layer

7653f2b

Remove get_epoch_lte_epoch usage

01c2121

Format

bf2f48a

Fix clippy warnings

4dd8b71

Remove birth_epoch

6fb06b5

eozturk1 mentioned this pull request May 30, 2022

Verification failure with small trees #144

Closed

eozturk1 added 2 commits June 1, 2022 16:02

Disable tests to merge

98ead76

Remove needless question mark

4f16dd3

eozturk1 added 2 commits June 1, 2022 16:21

Disable mysql tests

017fabf

Disable test_mysql_db test

d1c60c0

eozturk1 merged commit 3d2f30f into facebook:eozturk1/optimized-azks Jun 2, 2022

Jasleen1 mentioned this pull request Jun 8, 2022

Fixing failing tests for optimized azks #206

Closed

eozturk1 mentioned this pull request Jun 9, 2022

oZKS Tasks #207

Closed

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimized azks #189

Optimized azks #189

eozturk1 commented Apr 14, 2022

slawlor left a comment

slawlor May 9, 2022

slawlor May 9, 2022

slawlor May 9, 2022

slawlor May 9, 2022

slawlor May 9, 2022

slawlor May 9, 2022

eozturk1 commented May 9, 2022

eozturk1 commented May 24, 2022 •

edited

Loading

kevinlewi commented Jun 1, 2022

eozturk1 commented Jun 1, 2022

kevinlewi commented Jun 2, 2022

Optimized azks #189

Optimized azks #189

Conversation

eozturk1 commented Apr 14, 2022

slawlor left a comment

Choose a reason for hiding this comment

slawlor May 9, 2022

Choose a reason for hiding this comment

slawlor May 9, 2022

Choose a reason for hiding this comment

slawlor May 9, 2022

Choose a reason for hiding this comment

slawlor May 9, 2022

Choose a reason for hiding this comment

slawlor May 9, 2022

Choose a reason for hiding this comment

slawlor May 9, 2022

Choose a reason for hiding this comment

eozturk1 commented May 9, 2022

eozturk1 commented May 24, 2022 • edited Loading

Summary

Important Changes

Next Steps:

Improvements

kevinlewi commented Jun 1, 2022

eozturk1 commented Jun 1, 2022

kevinlewi commented Jun 2, 2022

eozturk1 commented May 24, 2022 •

edited

Loading