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

upgrade arc_ec to v0.4 #110

Closed
wants to merge 5 commits into from
Closed

Conversation

nikkolasg
Copy link
Contributor

Description

Bring the crate to use ark_ec v0.4

closes: #XXXX


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • [] Wrote unit tests
  • [] Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@nikkolasg nikkolasg changed the title bring v0.4 upgrade arc_ec to v0.4 Feb 3, 2023
@nikkolasg nikkolasg marked this pull request as ready for review February 4, 2023 13:41
@nikkolasg
Copy link
Contributor Author

BTW we also have a preliminary branch to do PST on the other base - commit in G2 opening in G1 - if you are interested

@maramihali
Copy link

had a look at the multilinear PST, lgtm

Copy link
Member

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

mostly looks good! I left a few cleanup comments

Comment on lines +83 to +84
fn serialize_uncompressed<W: Write>(&self, writer: W) -> Result<(), SerializationError> {
Self::serialize_with_mode(&self, writer, Compress::No)
Copy link
Member

Choose a reason for hiding this comment

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

We can skip this, as the default in ark-serialize is already using:

self.serialize_with_mode(writer, Compress::No)

https://github.com/arkworks-rs/algebra/blob/7b26e9d59ada2b85c53c4aefc5e0ccbff7d52f04/serialize/src/lib.rs#L101

self.h.serialize_uncompressed(&mut writer)?;
self.beta_h.serialize_uncompressed(&mut writer)?;
self.neg_powers_of_h.serialize_uncompressed(&mut writer)
fn serialize_compressed<W: Write>(&self, writer: W) -> Result<(), SerializationError> {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines +129 to +143
fn deserialize_uncompressed<R: Read>(mut reader: R) -> Result<Self, SerializationError> {
Self::deserialize_with_mode(&mut reader, Compress::No, ark_serialize::Validate::Yes)
}
fn deserialize_compressed<R: Read>(mut reader: R) -> Result<Self, SerializationError> {
Self::deserialize_with_mode(&mut reader, Compress::Yes, ark_serialize::Validate::Yes)
}
fn deserialize_compressed_unchecked<R: Read>(
mut reader: R,
) -> Result<Self, SerializationError> {
Self::deserialize_with_mode(&mut reader, Compress::Yes, ark_serialize::Validate::No)
}
fn deserialize_uncompressed_unchecked<R: Read>(
mut reader: R,
) -> Result<Self, SerializationError> {
Self::deserialize_with_mode(&mut reader, Compress::No, ark_serialize::Validate::No)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

fn serialize_uncompressed<W: Write>(&self, mut writer: W) -> Result<(), SerializationError> {
self.powers_of_g.serialize_uncompressed(&mut writer)?;
self.powers_of_gamma_g.serialize_uncompressed(&mut writer)
fn serialize_compressed<W: Write>(&self, writer: W) -> Result<(), SerializationError> {
Copy link
Member

Choose a reason for hiding this comment

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

here too

powers_of_g: Cow::Owned(powers_of_g),
powers_of_gamma_g: Cow::Owned(powers_of_gamma_g),
})
fn deserialize_compressed<R: Read>(mut reader: R) -> Result<Self, SerializationError> {
Copy link
Member

Choose a reason for hiding this comment

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

and here

prepared_h,
prepared_beta_h,
})
fn deserialize_compressed<R: Read>(reader: R) -> Result<Self, SerializationError> {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

self.beta_h.serialize_unchecked(&mut writer)?;
self.num_vars.serialize_unchecked(&mut writer)?;
self.max_degree.serialize_unchecked(&mut writer)
fn serialize_uncompressed<W: Write>(&self, mut writer: W) -> Result<(), SerializationError> {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

self.num_vars.serialize_unchecked(&mut writer)?;
self.supported_degree.serialize_unchecked(&mut writer)?;
self.max_degree.serialize_unchecked(&mut writer)
fn serialize_uncompressed<W: Write>(&self, writer: W) -> Result<(), SerializationError> {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

impl<E: Pairing> Valid for VerifierKey<E> {
fn check(&self) -> Result<(), SerializationError> {
if self.num_vars == 0 {
return Err(SerializationError::InvalidData);
Copy link
Member

Choose a reason for hiding this comment

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

it's a shame we can't add an error message here. Perhaps it would be worthwhile to pass a message to SerializationError::InvalidData enum variant, but that requires an upstream change in ark-serialise. I'll open an issue on it, maybe we can improve it for the next release.

Copy link
Member

Choose a reason for hiding this comment

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

}

fn serialize_compressed<W: Write>(&self, writer: W) -> Result<(), SerializationError> {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@Pratyush
Copy link
Member

I moved the commits to #112, which is merged now; thanks everyone for the work!

@Pratyush Pratyush closed this Feb 15, 2023
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.

4 participants