-
Notifications
You must be signed in to change notification settings - Fork 10
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
base: master
Are you sure you want to change the base?
Updates for CGGMP'24 #170
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #170 +/- ##
==========================================
+ Coverage 93.06% 94.20% +1.13%
==========================================
Files 36 42 +6
Lines 7012 8779 +1767
==========================================
+ Hits 6526 8270 +1744
- Misses 486 509 +23 ☔ View full report in Codecov by Sentry. |
12afc7a
to
e0565ce
Compare
cdb7c84
to
0ca10d8
Compare
0fc38fb
to
235afa0
Compare
To replace the old dec when we are converting Presigning
3fdc0fa
to
3efcfd4
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.
.collect() | ||
} | ||
|
||
fn check_evidence<M>(expected_description: &str) -> Result<(), LocalError> |
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.
Naming nitpick: this does more than merely checking some data against some other; it actually drives the protocol (and checks the outcome). In my head "check" is more "compare these two things and yell if they don't match".
How about a refactor to something more classic like "let r: Result<…, …> = run(); assert_eq!(r, …, "bla bla");"?
Less invasively: a rename to assert_outcome(expected)
?
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.
How about a refactor to something more classic like "let r: Result<…, …> = run(); assert_eq!(r, …, "bla bla");"?
It checks a number of things internally:
- the malicious node does not create any malicious behavior evidence
- all the other nodes create an evidence for the malicious node
- the evidence description matches the expected one
Not sure if it's worth packing in one variable.
} | ||
|
||
#[test] | ||
fn invalid_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.
We could make the tests return Result<(), LocalErr>
and then use ?
instead of the unwraps... I'd cut down one line for each check_invalid_message_evidence
...
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.
Hm, right, I keep forgetting that's a possibility for Rust tests. I'll try it out.
} | ||
} | ||
|
||
check_evidence::<Override>("Protocol error: A hash mismatch in Round 2").unwrap(); |
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 to say that these overrides allowing fault injection are REALLY cool; a long time in the making and it's nice to see them come together.
let signers = (0..3).map(TestSigner::new).collect::<Vec<_>>(); | ||
let all_ids = signers | ||
.iter() | ||
.map(|signer| signer.verifying_key()) |
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.
.map(|signer| signer.verifying_key()) | |
.map(TestSigner::verifying_key) |
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.
Huh, strange that clippy did not suggest that
|
||
fn check_evidence<M>(expected_description: &str) -> Result<(), LocalError> | ||
where | ||
M: Misbehaving<Id, (), EntryPoint = KeyRefresh<P, Id>>, |
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's a pity that check_evidence
and make_entry_points
have to be instantiated for each EntryPoint. As a new reader of the code, I have to look rather carefully before I spot that this one is for KeyRefresh
and the other is KeyInit
.
Maybe we can have a macro at some point to make the critical parts more visible, e.g.:
check_evidence!(KeyInit, …, …);
check_evidence!(KeyRefresh, …, …);
rng: &mut impl CryptoRngCore, | ||
entry_points: Vec<(SP::Signer, M::EntryPoint)>, | ||
behavior: &B, | ||
associated_data: &<<<M::EntryPoint as EntryPoint<SP::Verifier>>::Protocol as Protocol<SP::Verifier>>::ProtocolError as ProtocolError<SP::Verifier>>::AssociatedData, |
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.
LOL Rust
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 wish it either allowed me to create temporary aliases, or did not require explicit trait qualifications when there's no ambiguity
EP: 'static + Debug + EntryPoint<SP::Verifier>, | ||
SP: SessionParameters, | ||
{ | ||
let prefix = match part { |
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, but maybe "phase" is more descriptive than "part" here? ModifyPhase
and CheckPhase
etc?
"The payload was expected to contain a message, but is `None`" | ||
}; | ||
|
||
let expected_str = format!("{prefix} error: {error}"); |
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.
let expected_str = format!("{prefix} error: {error}"); | |
let expected_description = format!("{prefix} error: {error}"); |
entry_points, | ||
&ModifyPart::new(round_num, part), | ||
associated_data, | ||
&expected_str, |
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.
&expected_str, | |
&expected_description, |
let context = &round3.context; | ||
let aux = (&context.sid_hash, &context.my_id, &round3.rho); | ||
|
||
// Make a proof for a random secret. This won't pass verification. |
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.
These comments are very useful.
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'll add more on the second pass, I was just a little tired of writing all these tests :)
"Deserialization error" | ||
} else { | ||
// TODO: a bug in `manul`, this message should be the exact opposite | ||
"The payload was expected to contain a message, but is `None`" |
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.
Fixed in entropyxyz/manul#86
} | ||
|
||
#[test] | ||
fn invalid_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.
Echo broadcast tests to be added after entropyxyz/manul#86 is in
Fixes #157
Fixes #43 (in progress)
Fixes #91 (in progress)
Fixes #5 (in progress)
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
proofIsInvertible
trait, documenting the choice between GCD andinvert()
In progress:
TODO: