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

feat: Add SerdeObject support for pasta curves #39

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CPerezz
Copy link
Member

@CPerezz CPerezz commented May 8, 2023

With the current serialization primitives we have here, pasta curves did not have any way to be serialized as we did not implement SerdeObject for it.

This PR adds this support which resolves
#23. The code needs to have unsafe chunks as we need to transmute memory in order to be able to access the private fields of the pasta-curves crate structs.

This should be revisited and re-thought once
privacy-scaling-explorations/halo2#176 is addressed.

@CPerezz
Copy link
Member Author

CPerezz commented May 8, 2023

Should I add unit-tests too? I do think so TBH. But wanted to know if they're maybe already done somewhere else in halo2.

src/pasta/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

The implementation looks correct to me! I've added a few comments, mostly about:

  • Reducing the number of unsafe
  • Exploring code deduplication

I definitely think there should be unit tests here, for each type and for different values: do the serialization and deserialization and test equality.

// The transmute is the only way to get access to the internal array.
// Note that this is safe as long as the underlying array length is the same
// and the types contained are the same too.
let limbs: [u64; 4] = unsafe { std::mem::transmute_copy(&self) };
Copy link
Member

Choose a reason for hiding this comment

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

Could you use the From<Fp> for AccessibleFp to get the [u64; 4] avoiding the explicit unsafe here?
Maybe like this let value: AccessibleFp = self.clone().into()

Copy link
Member Author

Choose a reason for hiding this comment

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

This will force us to clone when not needed in reality. THat's why I don't like it.
I can do it if you feel like it has to. But I'd prefer avoiding and having faster serialization.

src/pasta/mod.rs Outdated Show resolved Hide resolved
src/pasta/mod.rs Outdated Show resolved Hide resolved
}
}

struct AccessibleFq(pub [u64; 4]);
Copy link
Member

Choose a reason for hiding this comment

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

The code for Fp and Fq seems the same, but just swapping Fp by Fq, leading to a lot of code duplication.
Both types implement the PrimeField, so I wonder if this fact could be used to do a generic implementation that works for both? Or perhaps another option is to write the code in a macro and then apply it for Fp and Fq.

src/pasta/mod.rs Outdated Show resolved Hide resolved
}
}

struct AccessibleVestaPoint {
Copy link
Member

Choose a reason for hiding this comment

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

This one has the same code as AccessiblePallasPoint, the only difference is the call to .is_on_curve(). So I also wonder if the code can be deduplicated 🤔

@CPerezz CPerezz marked this pull request as draft July 14, 2023 07:35
@CPerezz
Copy link
Member Author

CPerezz commented Aug 4, 2023

Addressed most of the review comments.

Pending actions:

  • Re-write as macros to de-duplicate code.
  • Write unit tests for serialization/deserialization.

With the current serialization primitives we have here, pasta curves did
not have any way to be serialized as we did not implement `SerdeObject`
for it.

This PR adds this support which resolves
#23.
The code needs to have `unsafe` chunks as we need to transmute memory
in order to be able to access the private fields of the pasta-curves
crate structs.

This should be revisited and re-thought once
privacy-scaling-explorations/halo2#176 is
addressed.
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.

3 participants