Relax DKG ceremony constraints #173
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. |
piotr-roslaniec
force-pushed
the
relax-dkg-ceremony
branch
from
b01d5b5
to
3d98758
Compare
piotr-roslaniec
force-pushed
the
relax-dkg-ceremony
branch
6 times, most recently
from
550b198
to
175dda7
Compare
piotr-roslaniec
force-pushed
the
relax-dkg-ceremony
branch
from
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, |
| @@ -286,12 +286,20 @@ impl AggregatedTranscript { | |||
|
|
|||
| 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?
| #[test_case(4, 4; "number of shares (validators) is a power of 2")] | ||
| #[test_case(7, 7; "number of shares (validators) is not a power of 2")] | ||
| #[test_case(4, 6; "number of validators greater than the number of shares")] | ||
| 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
| let insufficient_aggregate = | ||
| 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.
piotr-roslaniec
force-pushed
the
relax-dkg-ceremony
branch
3 times, most recently
from
6b90715
to
7d8dfde
Compare
piotr-roslaniec
force-pushed
the
relax-dkg-ceremony
branch
3 times, most recently
from
99f0dad
to
626159f
Compare
piotr-roslaniec
force-pushed
the
relax-dkg-ceremony
branch
from
626159f
to
5e9d46e
Compare
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: