-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eozturk1/optimized azks #211
Conversation
* 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
@@ -300,47 +269,45 @@ impl Azks { | |||
&self, | |||
storage: &S, | |||
label: NodeLabel, | |||
epoch: u64, | |||
_epoch: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do we not need the epoch here? Can we remove it?
akd/src/auditor.rs
Outdated
storage::memory::AsyncInMemoryDatabase, | ||
Azks, | ||
}; | ||
|
||
// FIXME: Need to add error handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Real fixme?
akd/src/auditor.rs
Outdated
) -> Result<(), AkdError> { | ||
let unchanged_nodes = proof.unchanged_nodes; | ||
let inserted = proof.inserted; | ||
// FIXME: Need to get rid of the clone here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why? What's wrong with the clone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is left for my cleanup PR but I suspect there may be a way to do this using references that doesn't cause the borrow-checker to complain. I at least want to try to reduce clones since that operation is less efficient.
akd/src/client.rs
Outdated
@@ -211,12 +299,11 @@ fn verify_single_update_proof<H: Hasher>( | |||
let existence_vrf_proof = proof.existence_vrf_proof; | |||
let existence_at_ep_ref = &proof.existence_at_ep; | |||
let existence_at_ep = existence_at_ep_ref; | |||
// FIXME: Why does this need a reference?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response as above comment.
akd/src/client.rs
Outdated
value: &crate::AkdValue, | ||
epoch: u64, | ||
proof: &[u8], | ||
// FIXME get rid of this argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we address all the FIXME's?
akd_client/src/tests.rs
Outdated
@@ -67,6 +70,8 @@ where | |||
digest_vec | |||
} | |||
|
|||
// FIXME: may want to get rid of this altogether | |||
#[allow(unused)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if it's unused, can we?
akd_client/src/verify.rs
Outdated
@@ -100,11 +114,24 @@ fn verify_nonmembership( | |||
Ok(verified) | |||
} | |||
|
|||
// FIXME might want to remove. | |||
#[allow(unused)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we remove this if it's actually unused?
akd_client/src/verify.rs
Outdated
epoch: u64, | ||
proof: &[u8], | ||
// FIXME get rid of this argument | ||
_label: NodeLabel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think the comment addresses it :p
akd_mysql/src/mysql.rs
Outdated
@@ -1322,6 +1282,14 @@ impl Storage for AsyncMySqlDatabase { | |||
let result = async { | |||
let tic = Instant::now(); | |||
|
|||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, but we should delete this
integration_tests/src/test_util.rs
Outdated
@@ -240,6 +371,7 @@ pub(crate) async fn test_lookups<S: akd::storage::Storage + Sync + Send, V: VRFK | |||
// Reset MySQL database by logging metrics which resets the metrics, and flushing cache. | |||
// These allow us to accurately assess the additional efficiency of | |||
// bulk lookup proofs. | |||
#[allow(unused)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not used?
Merged ozks update with main and updated the corresponding failing tests in
akd_test_tools
.