-
Notifications
You must be signed in to change notification settings - Fork 11
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
Updates for CGGMP'24 #170
Updates for CGGMP'24 #170
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #170 +/- ##
==========================================
+ Coverage 92.46% 95.39% +2.93%
==========================================
Files 35 38 +3
Lines 7030 10170 +3140
==========================================
+ Hits 6500 9702 +3202
+ Misses 530 468 -62 ☔ View full report in Codecov by Sentry. |
12afc7a
to
e0565ce
Compare
cdb7c84
to
0ca10d8
Compare
0fc38fb
to
235afa0
Compare
3efcfd4
to
0f86f18
Compare
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 have looked at Key_init and Key_refresh tests only.
As mentioned I think it's very cool to see the fault injection machinery come together, it's going to be a major help. The code is verbose and repetitive at times, but this is a complex protocol and its tests are always going to reflect that complexity.
Having the right testing tools is about finding that delicate balance between readability&maintainability and expressiveness. I'm sure we'll fiddle with this plenty mroe in the future but this strikes me as much better than we had before.
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.
Reviewed aux_gen.rs and entities.rs (I'm half-way through interactive_signing.rs but pheew, that one is intense...)
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.
Reviewed tools/
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.
Reviewing this update with any actual accuracy sort of boils down to auditing large parts CGGMP which is not something I can actually do. That said I have done my best to check that the overall structure looks sane and I have done a few deep-dives comparing the code with the paper(s).
I think it's ready to be merged, or rather: reviwing it further adds little value. FWIW I am delighted to see the much expanded test suite.
|
||
/// Returns ``true`` if the parameters satisfy a set of inequalities | ||
/// required for them to be used for the CGGMP scheme. | ||
fn are_self_consistent() -> bool { |
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 think is_
is more correct, as in "is this (one) set of params self-consistent"? Alternatively just drop the verb and call it self_consistent() -> bool
.
To replace the old dec when we are converting Presigning
Fixes #157
Fixes #43
Fixes #91
PAPER.md
, removed obsolete items.±2^l
now means[-2^(l-1)+1, 2^(l-1)]
instead of the previous[-2^l, 2^l]
). This caused a bit of a chain reaction:SecretSigned::assert_exponent_range()
logic changed.SecretSigned::random_in_exp_range*()
logic changed, and alsoexp
was changed toexponent
to match the assertion name.PublicSigned::from_xof_reader_bounded()
changed its behavior to produce the number according to the new range definition, and was renamed tofrom_xof_reader_in_range()
.PublicSigned::in_range_bits()
changed its behavior according to the new range definition, and was renamed tois_in_exponent_range()
.Ciphertext
constructor they are passed asSecretSigned
, to comply with the range requirements in the proofs.conversions::secret_unsigned_from_scalar()
removed.SecretSigned::new_modulo()
constructor to make a signed number in range [-N/2, N/2] from an Uint in range [0, N).Ciphertext::new()
anddecrypt()
(which took unsigned plaintexts), renamednew_signed()
anddecrypt_signed()
tonew()
anddecrypt()
.Ciphertext::new_with_randomizer()
was renamed tonew_with_randomizer_unsigned()
, since it's now a special one, only used in P_mul. Renamednew_with_randomizer_signed()
tonew_with_randomizer()
, andnew_public_with_randomizer_signed()
tonew_public_with_randomizer()
.e <-- ±q
is used, we are sampling the challenge as aScalar
using the newScalar::from_xof_reader()
method (and using that inП^sch
as well).aff-g
proof.prm
proof, and started usingBitVec
for its commitment.dec
proof (there were significant changes). Temporarily, it is located indec_new.rs
, will be moved todec.rs
whenPresigning
is updated.elog
proof.enc-elg
proof.aff-g*
proof.fac
proof, and implemented necessary changes to the algorithm (some variables are calculated differently, and the challenge is now a signedUint
and not aScalar
)mod
proof, and enforced invertibility conditions that were added in '24sch
proofenc
,log*
,mul
,mul*
proofs - they are not used anymore.IsInvertible
trait, documenting the choice between GCD andinvert()
ProductionParameters
toProductionParameters112
(since they correspond to 112 bits of security)Unsolved problems:
aff-g*
checks - see Find a way to testaff-g*
proof failures in the error rounds of Presigning #188