-
Notifications
You must be signed in to change notification settings - Fork 26
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
Impl lazy commitment for rust zk trie #35
Conversation
Add @noel2004 and @omerfirmak as reviewer. Not high priority pr. I know you are busy these days. |
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.
I think there is no issue in correction. Just some notes for possible optimization about cost.
@@ -93,6 +101,9 @@ impl<H: Hashable, DB: ZktrieDatabase, const MAX_LEVELS: usize> ZkTrieImpl<H, DB, | |||
writable: true, | |||
root_hash: root, | |||
debug: false, | |||
lock: RwLock::new(()), |
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.
The zkvm is expected to be executed in sequence so does it really need to induce RwLock
? I guess it would bring in unnecessary cycles into the circuit of zkvm.
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.
I observed a cycle count increase if only execute one block.
} | ||
|
||
pub fn is_dirty_node(&self, node_key: &H) -> bool { | ||
self.dirty_storage.contains_key(&node_key.to_bytes()) |
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.
Also, to save the execution cost (cycle in zkvm circuit), can we use some more simply way to distinguish the "dirty" key? for example, suppose all the dirty keys are small integer so a long prefix of zero can be found in dirty key, which is impossible for the hashed key.
|
||
// recalculatePathUntilRoot recalculates the nodes until the Root | ||
fn recalculate_path_until_root( | ||
pub fn recalculate_path_until_root( |
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 function seems is not used anymore?
}; | ||
|
||
let (new_child_hash, is_new_child_terminal) = | ||
self.try_delete_recursive(&child_hash, node_key, &path[1..])?; |
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.
So it is in fact a tail recursion but maybe hard to be optimized. Comparing with the original recalculate_path_until_root
implement, should we care about the cost of function calling?
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.
(don't need to do extra micro optimizations now, especially at the cost of readablity)
Closing #22