Skip to content
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

Skalman pok new interface #29

Merged
merged 37 commits into from
Feb 9, 2021
Merged

Skalman pok new interface #29

merged 37 commits into from
Feb 9, 2021

Conversation

drskalman
Copy link
Collaborator

  • PoP: using schnorr proof.
  • Moving the backend to arkworks.
  • Remove some redundants trait.

drskalman and others added 30 commits September 24, 2020 12:34
Commenting out lots of problematic types to make engine.rs compiles.

let mut k_as_hash = <H as Digest>::new().chain(r_point_as_bytes).chain(message.0).finalize();
let random_scalar : &mut [u8] = k_as_hash.as_mut_slice();
random_scalar[31] &= 31; // BROKEN HACK DO BOT DEPLOY
Copy link
Collaborator

@burdges burdges Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right this part.. what is the group order again? they just added hash to field, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes they did. It wasn't merged at the time, but I think it should have made it to the master/main now: #32 .

let mut r_point_as_bytes = Vec::<u8>::new();
r_point.into_affine().write(&mut r_point_as_bytes).unwrap();

let mut k_as_hash = <H as Digest>::new().chain(r_point_as_bytes).chain(message.0).finalize();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one should include the public key here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M was public key before but I thought we decided to replace with any arbitary message which can be public key as well. You want public key to be a mandatory part of the message?


impl<E: EngineBLS, H: Digest> ProofOfPossessionGenerator<E,H> for SecretKey<E> {

fn generate_pok(&self, message: Message) -> SchnorrProof<E> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to use a transcript abstraction instead of Message like in schnorrkel, ring-vrf, and now the apk crate. Message made sense for BLS signatures message. And maybe still does. I do not see much point in a schnorr proof.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should maybe make this PoK do both G1 and G2 if we're going that route, no? I guess that's a seperate thing that depends upon only Engine. Ugh.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made issues #30 and #31 to follow up with these.

}
// // #[test]
// // fn foo() { }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do global tests in a follow up, not sure what's not tested yet actually.


use pairing::{CurveAffine, CurveProjective}; // Engine, Field, PrimeField, SqrtField
use ark_ec::ProjectiveCurve;
use ark_serialize::{CanonicalSerialize};

use super::*;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll keep these I guess, and they could be useful if collecting by gossip, but we do not envision using them ourselves, and maybe https://eprint.iacr.org/2021/005 provides interesting ides for gossip situations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, what do you mean by "these"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the whole file with these aggregate BLS verifiers.

#[inline]
fn uncompressed_size(&self) -> usize {
self.0.into_affine().uncompressed_size()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just pass this through to the CanonicalSerialize on the underlying type?

I'm not sure what I think of this CanonicalSerialize trait overall yet, but I guess it's what arkworks does. Is it what celo does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, we can't. because projective points can't be canonically serialized (and hence ProjectiveCurve trait does not implement CanonicalSerialize. I think turn them into affine and call the serialization functions is the closest thing we do. I can look into celo's approach though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we serializing projective without going through affine here? Why? Did i do that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we go through affine as in self.0.into_affine() . What I meant was that we just can't simply derive CanonicalSerialize for the Public/Private key because we need to perform this into_affine() step manually.

You basically reimplemented serialization from scratch, there was no official serialization in the original pairing library.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could pass this through to the underlying affine curve then maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just merge this and then we can make another issue for passing these through.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could pass this through to the underlying affine curve then maybe?

I thought I was doing that. Could you show me what is the proper way of doing it?

@burdges
Copy link
Collaborator

burdges commented Feb 9, 2021

We should discuss the above things, but I'm satisfied this moves in the correct direction, so we can merge it and then focus on more detailed issues.

@burdges burdges merged commit 4dbc199 into master Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants