Remove powers-of-2 restriction on cohort size #134
Conversation
Codecov Report
@@ Coverage Diff @@
## main #134 +/- ##
==========================================
+ Coverage 78.66% 78.80% +0.13%
==========================================
Files 24 24
Lines 4880 4865 -15
==========================================
- Hits 3839 3834 -5
+ Misses 1041 1031 -10
|
piotr-roslaniec
force-pushed
the
remove-ftt-opt
branch
from
0381d67
to
c60ab3e
Compare
There was a problem hiding this comment.
I thought this would be more painful! Great work @piotr-roslaniec, as usual 💪
ferveo/src/dkg.rs
Outdated
| ) | ||
| .expect("unable to construct domain"); | ||
| let domain = | ||
| ark_poly::MixedRadixEvaluationDomain::<E::ScalarField>::new( |
| let rng = &mut StdRng::seed_from_u64(0); | ||
| let s = ark_bls12_381::Fr::from_random_bytes(&[0u8; 32]).unwrap(); | ||
|
|
||
| for shares_num in NUM_SHARES_CASES { |
There was a problem hiding this comment.
So this is benchmarking the speed of num shares that are powers of 2 for Radix2 vs MixedRadix? Cool.
As an aside. Do we have a test somewhere for a non-power of 2 number of shares? Basically, a test that confirms that the non-powers of 2 work appropriately.
There was a problem hiding this comment.
I didn't consider this kind of test to be interesting before. I did a manual test (in REPL) before submitting the PR and that lulled me into a false sense of correctness. Now that I followed up on your suggestion, it turns out only the simple variant works for a non-power-of-two number of shares.
I will post the changes here and investigate the issue with the precomputed variant.
Thanks for catching this.
There was a problem hiding this comment.
The bug has been found and fixed. Please see my comment below.
piotr-roslaniec
force-pushed
the
remove-ftt-opt
branch
from
c60ab3e
to
df8ca37
Compare
piotr-roslaniec
force-pushed
the
remove-ftt-opt
branch
from
df8ca37
to
8f45430
Compare
| ); | ||
| assert!(result.is_err()); | ||
| // Works for both power of 2 and non-power of 2 | ||
| for shares_num in [4, 7] { |
| let domain_points: Vec<_> = dkg.0.domain.elements().collect(); | ||
| let domain_points: Vec<_> = dkg | ||
| .0 | ||
| .domain | ||
| .elements() | ||
| .take(dkg.0.dkg_params.shares_num as usize) | ||
| .collect(); |
There was a problem hiding this comment.
This is the root cause of the issue found by unit tests. The domain may have an arbitrary size, but we must use at most shares_num domain points.
Type of PR:
Required reviews:
What this does:
MixedRadixEvaluationDomain(without constraints on the number of coefficients) andRadix2EvaluationDomain(number of coefficients must be a power of two)Radix2EvaluationDomainwithGeneralEvaluationDomain(automatically select domain size) inferveoIssues fixed/closed:
Why it's needed:
Notes for reviewers:
FerveoPublicKeyserialization #136Dkginferveo::apiis using an incorrect number of domain points to create a precomputed shareBenchmarking results