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

Update arkworks to 0.4.2 for all crates #1265

Conversation

chiro-hiro
Copy link
Contributor

@chiro-hiro chiro-hiro commented Oct 4, 2023

This PR just migrate all crates to ark-ec@0.4.2 and ark-ff@0.4.2. I will upgrade the code first then fix all the issue regarding to test and CI/CD.

@chiro-hiro chiro-hiro marked this pull request as ready for review November 15, 2023 13:20
@chiro-hiro
Copy link
Contributor Author

Upgrade the test suite and rewrite test cases.

ark-test-curves = "0.4.2"
ark-algebra-test-templates = "0.4.2"
ark-serialize="0.4.2"

@dannywillems dannywillems self-requested a review November 16, 2023 12:05
@dannywillems dannywillems self-assigned this Nov 16, 2023
@dannywillems
Copy link
Member

Hey, thanks for the contribution!
I try to build on my machine but I have errors regarding the trait F in the Poseidon. Can you have a look?
I opened #1358 for the CI. We will fix the run here later.

@dannywillems
Copy link
Member

It also requires to update ark-ff and ark-ec in the other libraries, like poseidon, visu, etc. Otherwise, we have conflicting versions (0.3.0 and 0.4.2, see Cargo.lock).

@chiro-hiro
Copy link
Contributor Author

Hi @dannywillems,

I think update the whole proof-system to 0.4.2 would be a big task. So I tried to break down it to multiple PRs. If you think this PR good enough I will open a new PR that update 0.4.2 for all crates.

@dannywillems
Copy link
Member

Hi @chiro-hiro. Unfortunately, I cannot test this PR without having the whole repository bumped to 0.4.2. However, by having a manual look, it looks good!
That would be very helpful if you can bump up to 0.4.2 for the others. Thanks!

@chiro-hiro chiro-hiro changed the title Update arkworks to 0.4.2 for pallas and vesta Update arkworks to 0.4.2 for all crates Dec 6, 2023
@chiro-hiro
Copy link
Contributor Author

Hi @dannywillems,

I'm working on it, going to upgrade all other crates to make sure that everything will work fine with arkworks 0.4.2

@chiro-hiro
Copy link
Contributor Author

chiro-hiro commented Dec 14, 2023

Hi @dannywillems,

I have a problem with the migration. In the latest update we have poly-commitment/src/pairing_proof.rs, there are some test on curve bn254 this curve still using arkworks 0.4.0. I'm stuck here and still need to figure out what should we do? here are some possible options:

  • Implement bn254 in proof-systems/curves with arkworks 0.4.2
  • Waiting for arkworks update their curves
  • PR to arkworks

Which one do you prefer?

@chiro-hiro
Copy link
Contributor Author

Hi @dannywillems, any update on this?

@mrmr1993
Copy link
Member

mrmr1993 commented Jan 4, 2024

I'm concerned that this upgrade seems to significantly reduce the performance of the proof system. I think we should hold off merging until we know what's causing the performance regression, and how to fix it.

@chiro-hiro
Copy link
Contributor Author

chiro-hiro commented Jan 5, 2024

Hi @mrmr1993, thanks for your reply. We definitely need to inspect the root cause of this performance issue. Could you share with me your opinion on bn254 issue and what should we do next?.

Btw, I will keep maintain this branch to minimize the conflict with the upstream.

@mrmr1993
Copy link
Member

mrmr1993 commented Jan 8, 2024

Could you share with me your opinion on bn254 issue and what should we do next?

No opinion yet, we'll want to figure out the performance issues before thinking about how/whether to continue with the upgrade.

@volhovm
Copy link
Member

volhovm commented Feb 14, 2024

An important PR, but converting it to draft before we resolve the performance issue/are ready to proceed.

@volhovm volhovm marked this pull request as draft February 14, 2024 14:51
@chiro-hiro chiro-hiro force-pushed the feature/update_curves_to_arkworks_042 branch from 4482410 to 848ed02 Compare July 12, 2024 12:23
@chiro-hiro
Copy link
Contributor Author

Catch up with upstream:master one last remaining issue is trait bound of PolyComm<G>.

    Blocking waiting for file lock on build directory
   Compiling kimchi v0.1.0 (/home/chiro/Git/proof-systems/kimchi)
error[E0369]: cannot subtract `&PolyComm<G>` from `&PolyComm<G>`
   --> kimchi/src/verifier.rs:927:25
    |
927 |         &chunked_f_comm - &chunked_t_comm.scale(zeta_to_domain_size - G::ScalarField::one())
    |         --------------- ^ ------------------------------------------------------------------ &PolyComm<G>
    |         |
    |         &PolyComm<G>
    |
help: consider further restricting this bound
    |
765 |     G: KimchiCurve + ark_ec::Group,
    |                    +++++++++++++++

For more information about this error, try `rustc --explain E0369`.
error: could not compile `kimchi` (lib) due to previous error

@volhovm
Copy link
Member

volhovm commented Sep 21, 2024

@volhovm volhovm closed this Sep 21, 2024
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