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

bump halo2curves to 0.7 #364

Closed
wants to merge 1 commit into from

Conversation

kilic
Copy link

@kilic kilic commented Aug 31, 2024

No description provided.

Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!

@guorong009
Copy link

@kilic @CPerezz
I have one question about PR.

The only change is the use of msm_best, instead of old best_multiexp.
I believe this msm_best gives output different from best_multiexp.
What do you think?

More context:
Current CI fails when vector-test feature is enabled.
It means that the tests are failing, where they have the extra assertion check of proof hash.
For example, this CI failure shows that hash of proof is not equal to prefixed value, in test_success test.
https://github.com/privacy-scaling-explorations/halo2/actions/runs/10644585323/job/29509485590?pr=364#step:4:775
https://github.com/privacy-scaling-explorations/halo2/blob/main/halo2_proofs/tests/compress_selectors.rs#L487-L492

  left: "8326140d1873a91630d439a8812d1f104667144e03e0cd5c59eb358ae5d1a4eb"
 right: "acae50508de5ead584170dd83b139daf40e1026b6debbb78eb05d515173fc2dd"

I think this means that the generated proof is not equal to past one, even though it passes the verification check.
Furthermore, I think this proof diff is attributed to msm_best.

cc @davidnevadoc @ed255

@kilic
Copy link
Author

kilic commented Sep 2, 2024

@guorong009 it also doesn't pass with same bad output when changing it to msm_parallel which is the older msm. Must be smtg else.

@guorong009
Copy link

Thx for info! @kilic
I think we should clarify this issue before merge. right?
Anyway, the CI fails.
And we learn that current PR generates the proof, which is different from one generated with current main branch.
I will try to look into the issue more.

@guorong009
Copy link

guorong009 commented Sep 3, 2024

I think the main reason of different proof hash is the change in EC point serialization, not the msm_best function.
privacy-scaling-explorations/halo2curves#141
In other words, the msm_best computes the EC points and scalars the same as before.
Just new standard of EC point serialization brings the diff in proof hash.
For the context, the proof hash is computed from serialized byte vector(Vec<u8>) of proof.

@kilic
Copy link
Author

kilic commented Sep 3, 2024

Hmm I see I was thinking new serialization was compatible with the older one. So should we revert BN serialisation in halo2curves?

@guorong009
Copy link

@kilic
In the privacy-scaling-explorations/halo2curves#141 (comment), it says the serialization of compressed Bn254 point would be changed.
I also see that you didn't give explicit approval to PR.
Hence, I believe we need the discussion for your question.

cc @CPerezz @ed255 @davidnevadoc

@CPerezz
Copy link
Member

CPerezz commented Sep 3, 2024

@guorong009 @kilic I think we then need to bump the version of this crate also if we break serialization compatibility.

Although, it's not exactly a breaking change of this lib. It will break serialization of Proofs or SRS or any other struct that relies on EC points. Therefore, I think we should also bump halo2_proofs.

@davidnevadoc
Copy link

I agree with @CPerezz, we should bump the version of this crate too and keep the serialization as it is.

@davidnevadoc
Copy link

Superseded by #366

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