-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add Encryption and Decryption for OPRF Report Structs #904
Add Encryption and Decryption for OPRF Report Structs #904
Conversation
I will use slices instead of generic arrays to fix the trait bounds. So don't review it yet. |
I fixed it, its ready for review 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.
Looks good overall, I left a few suggestions.
Oh, and I'd suggest either cleaning up the commit history locally (I use |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
=======================================
Coverage ? 90.29%
=======================================
Files ? 182
Lines ? 26091
Branches ? 0
=======================================
Hits ? 23560
Misses ? 2531
Partials ? 0 ☔ View full report in Codecov by Sentry. |
This PR hasn't seen any updated in 2 weeks, are we still planning to work on it? |
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.
some minor feedback
ipa-core/src/report.rs
Outdated
) | ||
.unwrap(); // validated on construction | ||
|
||
let mut ct_mk = self.mk_ciphertext().to_vec(); |
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.
Can you point me to the heap allocation in the old code? I am struggling to find it - it does seem that it works w/o a roundtrip to jemalloc.
The reason I think it matters is because this will be called 50 million (or whatever shard size is) times - we got a decent chance for memory allocator to do its job properly, but this is a game I think we don't have to play as it should be possible to avoid in this code
|
||
let mut ct_mk: GenericArray<u8, CTMKLength> = | ||
*GenericArray::from_slice(self.mk_ciphertext()); | ||
// let mut ct_mk = self.mk_ciphertext().to_vec(); |
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 want to hold this change for too long, but lets make sure we clean up commented code. Fine with a follow up PR to address those
let mut ct_btt: GenericArray<u8, CTBTTLength<BK, TV, TS>> = | ||
GenericArray::from_slice(self.btt_ciphertext()).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.
you can use the same dereferencing trick *GenericArray
instead of GenericArray(...).clone()
No description provided.