Initial impl for IdSecret#320
Merged
Merged
Conversation
Benchmark for d44204aClick to view benchmark
|
Benchmark for 8250efdClick to view benchmark
|
Benchmark for 8250efdClick to view benchmark
|
Benchmark for 8250efdClick to view benchmark
|
Benchmark for 4055caeClick to view benchmark
|
Benchmark for 4055caeClick to view benchmark
|
Benchmark for 4055caeClick to view benchmark
|
sydhds
marked this pull request as ready for review
June 18, 2025 10:05
Benchmark for 81859a3Click to view benchmark
|
Benchmark for b132459Click to view benchmark
|
Benchmark for 81859a3Click to view benchmark
|
Benchmark for b132459Click to view benchmark
|
Benchmark for 81859a3Click to view benchmark
|
Benchmark for b132459Click to view benchmark
|
Benchmark for e0fcd15Click to view benchmark
|
Benchmark for e0fcd15Click to view benchmark
|
Benchmark for e0fcd15Click to view benchmark
|
Benchmark for 70a484fClick to view benchmark
|
Benchmark for 70a484fClick to view benchmark
|
Benchmark for 70a484fClick to view benchmark
|
Benchmark for b3c12e9Click to view benchmark
|
Benchmark for b3c12e9Click to view benchmark
|
Benchmark for b3c12e9Click to view benchmark
|
seemenkina
reviewed
Jun 20, 2025
| } else { | ||
| println!("DUPLICATE message ID detected! Reveal identity secret hash: {leaked_identity_secret_hash}"); | ||
| println!( | ||
| "DUPLICATE message ID detected! Reveal identity secret hash: ********" |
Collaborator
There was a problem hiding this comment.
I think it makes sense to print the secret in the example, because we mostly use it for display, and if we can't show the secret on the screen, it would be weird.
Contributor
There was a problem hiding this comment.
@sydhds
I think it's fine to keep the code as-is and use the Display macro for IdSecret, since the secret key is just a random value in this example.
Zeroize only clears memory after it's no longer in use—it doesn't protect data while it's active. So keeping the example simple is better, and printing the secret isn't a problem here.
|
|
||
| Ok([ | ||
| ("identitySecret", vec![rln_witness.identity_secret]), | ||
| // FIXME |
Contributor
Author
There was a problem hiding this comment.
Pb here is that we are creating a copy of IdSecret (because *rln_witness.identity_secret create a copy of the underlying Fr) so it "leaks" the secret key here. Problem here is the return type of the function [(_, Vec)] as we ideally should return the IdSecret here. Any idea of how to fix this?
Collaborator
There was a problem hiding this comment.
I'm not sure we can change anything.
the function is used just before the start of the witness calculation and there we need a field element already, not another type
I guess it makes sense to leave a comment here?
Contributor
Author
There was a problem hiding this comment.
I've added an enum and added some zeroize in iden3calc code. Let me know what you think about this :)
vinhtc27
reviewed
Jun 23, 2025
| for (index, identity) in &self.local_identities { | ||
| println!("User Index: {index}"); | ||
| println!("+ Identity Secret Hash: {}", identity.identity_secret_hash); | ||
| println!("+ Identity Secret Hash: ********"); |
Contributor
There was a problem hiding this comment.
Same goes for this example
Contributor
Author
There was a problem hiding this comment.
Sorry should be good now
vinhtc27
reviewed
Jun 23, 2025
Contributor
There was a problem hiding this comment.
There are a few Clippy warnings in this file when using the stateless feature, such as with cargo make test_stateless on rln module or in the rln-wasm module.
vinhtc27
reviewed
Jun 23, 2025
Benchmark for 799be5aClick to view benchmark
|
vinhtc27
approved these changes
Jul 7, 2025
Contributor
Or you can leave it and merge this PR, I'll resolve it later this week while doing the update CI task for zerokit. |
Collaborator
I think it makes sense to follow it. it's not so much to change |
Collaborator
vinhtc27
approved these changes
Jul 7, 2025
sydhds
force-pushed
the
secret-hash-zeroize_2
branch
from
July 8, 2025 07:30
1aa7cf8 to
503b760
Compare
Benchmark for 7940167Click to view benchmark
|
Benchmark for 7940167Click to view benchmark
|
Benchmark for 7940167Click to view benchmark
|
seemenkina
self-requested a review
July 8, 2025 07:40
seemenkina
approved these changes
Jul 8, 2025
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
No description provided.