-
Notifications
You must be signed in to change notification settings - Fork 13
Update to static rounds #226
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
Conversation
0e36343 to
d455ffd
Compare
74a3c0a to
ee9eb2b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #226 +/- ##
==========================================
- Coverage 90.60% 89.18% -1.43%
==========================================
Files 50 47 -3
Lines 10759 8403 -2356
==========================================
- Hits 9748 7494 -2254
+ Misses 1011 909 -102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7db7079 to
ea905c5
Compare
ea905c5 to
d788761
Compare
|
All contributors have signed the CLA ✍️ ✅ |
dvdplm
left a comment
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.
LGTM
| } | ||
| } | ||
|
|
||
| /// Reconstruct `rid` from echoed messages |
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.
| /// Reconstruct `rid` from echoed messages | |
| // Reconstruct `rid` from echoed messages |
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 intended it to be a doc comment, I think that's fine?
| let r2_ebs = messages.combined_echos::<Round2<P, Id>>(2)?; | ||
| let r2_eb = messages.previous_echo_broadcast::<Round2<P, Id>>(2)?; |
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.
We discussed this a fair bit during the manul review, but I nonetheless was surprised to see that we have to pass in the 2 here. It's a wart. :/
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 consequence of one round type being able to be used for several actual rounds.
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.
Yeah I understand it, but still had to pause. It's just a bit of an oddity. Maybe if rounds had "functional" names it'd be less jarring? Like, dunno, "KeyInitRound", "ReshareRound", etc
| @@ -47,6 +39,35 @@ use crate::{ | |||
| }, | |||
| }; | |||
|
|
|||
| /// Analogous to `Iterator::sum()`, but requires a non-empty iterator | |||
| /// (so that it can be used for types with no `default()`, like `Ciphertext`) | |||
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.
Not sure I understand this comment about Default. Wouldn't an impl Sum for CipherText work?
impl<P: PaillierParams> Sum for Ciphertext<P> {
fn sum<I: Iterator<Item = Self>>(mut iter: I) -> Self {
let mut result = iter.next().unwrap();
for item in iter {
result = result + item;
}
result
}
}
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.
It'll panic for an empty iterator, and the purpose of these functions is to have a Result instead. We can't return Result from sum().
|
I have read the CLA Document and I hereby sign the CLA |
d788761 to
2d470c3
Compare
Use static rounds from entropyxyz/manul#117 and helpers from entropyxyz/manul#113
Also fixes #216 (static round methods use
impl CryptoRngCore)