Add test covering deletion of stale ValueStates #142
Conversation
facebook-github-bot
added
the
CLA Signed
| let current_azks = akd.retrieve_current_azks().await?; | ||
| let root_hash = akd.get_root_hash::<Blake3>(¤t_azks).await?; | ||
|
|
||
| let history_proof = akd.key_history(&AkdLabel("hello".to_string())).await?; |
There was a problem hiding this comment.
What's the value in the key history proof if the previous values have been deleted? This doesn't return error?
There was a problem hiding this comment.
I guess in other words, the storage layer is returning GetError("NotFound") or something for some valuestate, and how is the proof still being constructed?
There was a problem hiding this comment.
From what I can tell, the proof is constructed for the ValueStates that are retrieved. get_user_data would only return the latest ValueState when the previous ones have been deleted, and an update proof is only created for that ValueState.
So wouldn't AKD avoid looking up ValueStates that are already deleted and therefore not run into the "NotFound" errors?
There was a problem hiding this comment.
Hmm OK so that's no ideal. In that case it should be basing the updates off of the state of the node, not the user data retrieved. @Jasleen1 this is a change that will need to occur. There were updates at some epochs, but there just happen to no longer be values associated to those updates.
In the current implementation it's simply stating "there was no change at the given epoch".
| let history_proof = akd.key_history(&AkdLabel("hello".to_string())).await?; | ||
| let (root_hashes, previous_root_hashes) = | ||
| get_key_history_hashes(&akd, &history_proof).await?; | ||
| key_history_verify::<Blake3>( |
There was a problem hiding this comment.
Ah the client isn't presently validating the hash from plaintext_value to leaf value: ref.
Therefore to-date this client verification just assumes the leaf hash IS the hash of the plaintext_value without ever verifying it. @Jasleen1 is that intended or this is to be filled out once VRF's (#122) are included?
At which point I expect this test to fail unless we explicitly add handling support for missing plaintext_value's
There was a problem hiding this comment.
See reply to above comment - we can also wait for VRFs to be committed before merging this PR.
| let lookup_proof = akd.lookup(AkdLabel("hello2".to_string())).await?; | ||
| lookup_verify::<Blake3>(root_hash, AkdLabel("hello2".to_string()), lookup_proof)?; | ||
|
|
||
| let audit_proof = akd.audit(1, 2).await?; |
There was a problem hiding this comment.
Audit proof shouldn't care, since it (by design) doesn't need the plaintext values to determine a mutation hasn't occurred in history.
There was a problem hiding this comment.
Yup I am aware, but I was thinking that a test to demonstrate the property couldn't hurt :)
|
I'm OK with shipping this for now, but I think we need @kevinlewi or @Jasleen1 to give some oversight on follow-up to some of the functionality we currently exhibit in this PR. |
|
Thanks for putting this up! Indeed, let's wait for VRF to be landed first |
This PR adds a test verifying that stale
ValueStaterecords can be deleted from storage without affecting AKD functionality. The scenario under which this may happen is described in #130 under 1. Stale ValueStates.