Profile risc0 and use AccountTrie in server #115
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marijanp
approved these changes
Jun 7, 2024
Comment on lines
+187
to
+190
| // SECURITY ASUMPTION: see Account docs | ||
| // if withdrawer_account.public_key != *withdrawer { | ||
| // return Err(("hash collision detected on withdrawer account").into()); | ||
| // } |
There was a problem hiding this comment.
Do we need this at a later stage?
| @@ -35,6 +35,7 @@ mod error { | |||
| use super::*; | |||
| use core::fmt::{Debug, Display}; | |||
|
|
|||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
Is this going to be used in the future?
There was a problem hiding this comment.
Hopefully not, but maybe.
no_std Error should be in 1.80 I think.
Comment on lines
+117
to
+120
| // SECURITY ASUMPTION: see Account docs | ||
| // if sender_account.public_key != *sender { | ||
| // return Err(("hash collision detected on sender account").into()); | ||
| // } |
There was a problem hiding this comment.
I just left that anywhere I made the assumption.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge #94 before this PR.
Changes
We now use the same code in the circuit and server.
We now precheck transactions before attempting to include them in a batch.
We now rely on the security of sha(public_key) instead of also checking the public key matches the one used to create an account. This made the circuit a bit more efficient (see 3526834).
Profiling
Our circuit is already pretty optimized unfortunately.
60% of cycles are spent calculating the merkle root, 30% on deserializing the data into risc0.
Maybe we can use the older risc0 custom sterilization instead of serde.
I can align the data a bit better, maybe I can optimize
calc_subtree_hasha bit, but not much.