Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
==========================================
- Coverage 78.28% 78.17% -0.11%
==========================================
Files 24 24
Lines 5066 5224 +158
==========================================
+ Hits 3966 4084 +118
- Misses 1100 1140 +40 ☔ View full report in Codecov by Sentry. |
b01d5b5 to
3d98758
Compare
550b198 to
175dda7
Compare
175dda7 to
6fd65bd
Compare
| shares_num=shares_num, | ||
| validators_num=validators_num, | ||
| threshold=threshold, | ||
| shares_to_use=validators_num - 1, |
There was a problem hiding this comment.
| shares_to_use=validators_num - 1, | |
| shares_to_use=threshold - 1, |
| pub fn verify( | ||
| &self, | ||
| shares_num: u32, | ||
| validators_num: u32, |
There was a problem hiding this comment.
It should be possible to verify any subset of size shares_num, but I guess you still need the original number of validators to get the proper evaluation domain, so it seems both parameters are needed?
| fn test_server_api_tdec_precomputed(shares_num: u32, validators_num: u32) { | ||
| let rng = &mut StdRng::seed_from_u64(0); | ||
|
|
||
| // In precomputed variant, the security threshold is equal to the number of shares |
There was a problem hiding this comment.
Precomputed variant comment, definitely not for this PR. Opened #184
ferveo/src/api.rs
Outdated
| dkg.aggregate_transcripts(not_enough_messages).unwrap(); | ||
| let result = insufficient_aggregate.verify(SHARES_NUM, &messages); | ||
| let result = insufficient_aggregate.verify(validators_num, &messages); | ||
| assert!(result.is_err()); |
There was a problem hiding this comment.
I'm not sure this should fail only because the number of transcripts is insufficient. This test is probably expecting a failure due to an unrelated issue.
There was a problem hiding this comment.
I rewrote assertions in these tests to provide more clarity on what is being checked.
6b90715 to
7d8dfde
Compare
99f0dad to
626159f
Compare
626159f to
5e9d46e
Compare
cygnusv
left a comment
There was a problem hiding this comment.
@piotr-roslaniec and I decided to move forward for the moment, but my comments in the review are still outstanding. Some of them only require light touches to tests (mainly those related to #179), while others require a bit more of work. Comments related to the precomputed variant are also outstanding (see #184) but are not a priority.
Type of PR:
Required reviews:
What this does:
validators.len()] to be considered a successful ceremonyshares_numis effectively replaced byvalidators.len(), and from now on shares_num denotes the minimum number of shares (or validators) that the DKG ceremony accommodatesIssues fixed/closed:
Notes for reviewers: