added support RFC compliant and non-compliant g1/g2 swapped beacons #1
Conversation
CluEleSsUK
commented
Aug 28, 2023
CluEleSsUK
commented
- replaced BLS12-381 lib with ZKcrypto's lib as it's lower level
- changed the interface almost completely, so users don't need to know whether they're using a chained or unchained network
- replaced BLS12-381 lib with ZKcrypto's lib as it's lower level - changed the interface almost completely, so users don't need to know whether they're using a chained or unchained network
|
still need to add a lot more tests to cover everything |
src/verify.rs
Outdated
| Ok(_) => panic!("expected error but got success"), | ||
| Err(e) => { | ||
| if e != expected { | ||
| panic!("expected {:?} but got {:?}", expected, e); |
There was a problem hiding this comment.
| panic!("expected {:?} but got {:?}", expected, e); | |
| panic!("expected {expected:?} but got {e:?}"); |
There was a problem hiding this comment.
Any reason not to use assert! instead of this if construct ?
There was a problem hiding this comment.
yeah, assert! gives a pretty shitty error message, especially when combo'd with matches!; perhaps there's a 3rd party testing lib I could pull in that provides this stuff, but it felt like a single function was simpler
| let p = G1Affine::from_compressed(pub_key_bytes).unwrap_or(G1Affine::identity()); | ||
|
|
||
| let q = G2Affine::from_compressed(sig_bytes).unwrap_or(G2Affine::identity()); |
There was a problem hiding this comment.
Why the unwrap_or? Shouldn't that be an error case?
Feels like a case where ? is useful.
There was a problem hiding this comment.
The ZkCrypto lib returns their own CtOption type that wraps a u8 instead of a Result >.> makes sense perhaps if you want constant time ops, but we don't care.
I could perhaps write something to turn CtOption into a Result and give error messages; as-is, the infinity point will fail at later steps, but perhaps that's error-prone
| if Sha256::digest(&beacon.signature).to_vec() != beacon.randomness { | ||
| return Err(VerificationError::InvalidRandomness); | ||
| } |
There was a problem hiding this comment.
This would fail for the PedersenBlsChained mode, no? Add some test vectors :P
There was a problem hiding this comment.
why would it fail for chained? ;o this is just checking the randomness == hash(signature which is the same for chained and unchained beacons afaiu. Also there is a test vector for default beacons
There was a problem hiding this comment.
Ah right, I was thinking of the message we're signing
src/verify.rs
Outdated
| if !beacon.previous_signature.is_empty() { | ||
| return Err(VerificationError::UnchainedHasPreviousSignature); | ||
| } |
There was a problem hiding this comment.
Does this really warrants a verification error? If a user needs the previous one and decided to bundle it in the appropriate field of a Beacon even tho they are using the unchained mode, this might not be wished
There was a problem hiding this comment.
Hmm I guess it's not an error, though it's suspicious
| }; | ||
|
|
||
| assert!(matches!( | ||
| verify_beacon(&SchemeID::PedersenBlsChained, &public_key, &beacon), |
There was a problem hiding this comment.
I can't understand how this worked given the above check I commented on
There was a problem hiding this comment.
I think you're mixing up the construction of the message being signed with the randomness derivation from the sig
|
Good shout - let me do a full revamp of the README, I'll do it in another PR though |
