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

Dependency mess should be fixed #146

Open
4 tasks
CPerezz opened this issue Aug 16, 2024 · 2 comments
Open
4 tasks

Dependency mess should be fixed #146

CPerezz opened this issue Aug 16, 2024 · 2 comments

Comments

@CPerezz
Copy link
Member

CPerezz commented Aug 16, 2024

Currently as I worked on #142 I realized how complex the dependency graph of this workspace is.
And it's not that the workspace is crazily complex. But rather that we have:

Lots of forks across different users and repos:

# These are some patches at top-level workspace
ark-r1cs-std = { git = "https://github.com/winderica/r1cs-std", branch="cherry-pick" }
ark-bn254 = { git = "https://github.com/arnaucube/ark-curves-cherry-picked", branch="cherry-pick"}
ark-grumpkin = { git = "https://github.com/arnaucube/ark-curves-cherry-picked", branch="cherry-pick"}

# These are some deps from `folding-schemes`
arkworks_backend = { git = "https://github.com/dmpierre/arkworks_backend", branch="feat/sonobe-integration" }
ark-circom = { git = "https://github.com/arnaucube/circom-compat", default-features = false }

Some dependencies that also fall under other users:

arkworks_backend = { git = "https://github.com/dmpierre/arkworks_backend", branch="feat/sonobe-integration" }

While I don't have any issues with repos being under the users, it makes it a nightmare to actually update dependencies across the entire workspace. As always some deps have again all the arkworks stack in a older version and it's the never-ending story.

We rely on non-existing versions of crates to make all this juggling to work

If you notice, in all the Cargo.toml files we depend on ark-grumpkin, we depend on it with version 0.4.0. But if you actually look at https://crates.io/crates/ark-grumpkin/versions, you'll realize that only ark-grumpkin v0.5.0-alpha.0 exists published. Therefore, we only use an "imaginary version" such that we can then patch it at top-level workspace Cargo.toml with a custom version that @arnaucube owns.

A lot of the arkworks cherry-picks that we currently patch have already been released.

For most of the arkworks backend, 0.5.0-alpha.0 is already out. This means most, to not say all of the things we have patched should disappear or minimize as much as possible.

Action items:

  • Update all the private-owned repos on which this crate relies on to v0.5.0-alpha.0.
  • Update current crate arkworks-dependencies to v0.5.0-alpha.0.
  • Try to eliminate as many patches as possible and justify well the ones that stay.
  • Instead of patching some crates, and owning the forks privately, why not adding them to the workspace with a subtree or something similar? On this way we can much more easily control all the updates.
@CPerezz CPerezz self-assigned this Aug 16, 2024
CPerezz added a commit that referenced this issue Aug 16, 2024
As mentioned in
#146
there's a big issue that involves some dependencies of the crate.

As a temporary fix, this forces the workspace to rely on a
"non-existing" version of `ark-grumpkin` which is immediately patched at
workspace-level for a custom version that @arnaucube owns with some
cherry-picked commits.

While this allows the CI to pass and crate to build, a better solution
is needed.
github-merge-queue bot pushed a commit that referenced this issue Aug 17, 2024
* fix: Use `target_pointer_size` conditional compilation

There are some parts of the code where is needed to de/serialze
`usize`s. These, have sizes that vary depending on the target
achitecture the code is compiled for.

Hence, this adapts the de/serialization to the specific pointer size for
which the crate is being compiled to.

* change: Support WASM-compatibility and polish Cargo.toml

In order to support Wasm-compat and to simplify and improve `Cargo.toml`
readability, the follwing changes have been made:

- All the deps that can use `parallel` feature, do so. As `rayon`
  supports non-threaded targets with a fallback option. See: https://docs.rs/rayon-core/1.12.1/rayon_core/index.html#global-fallback-when-threading-is-unsupported
- `ark-grumpking` has been brought to `0.5.0-alpha.0` as `0.4.0` appears
  to not be in `crates.io` anymore. See: https://crates.io/crates/ark-grumpkin/versions
- By default, the crate uses `"ark-circom/default"` which selects the
  `wasmer/sys` feature such that it knows where wasmer is
  suposed to be run`.
- Added a `wasm` feature which forces `ark-circom/wasm` to be used
  instead. Which internally selects the `wasmer/js` backend to be used
  such that in-browser execution is possible.
- Added `getrandom` with `js` feature as dependency when `wasm32-unknown-unknown` target is selected such
  that compilation of the crate for testing or simply building is possible. Notice that with `wasi` and other wasm targets,
  this is not the case as they're automatically supported.
  For more info, please check: https://docs.rs/getrandom/latest/getrandom/#webassembly-support

* feat: Support WASM-compatibility tests in CI

Add support for both testing the build of `sonobe/folding-schemes` for
WASM-targets and also, it's build as a dependency for a WASM-crate.

This includes a build job for the three main supported rust-WASM targets
and the same but for a thrid crate creted on-the-fly which uses
`sonobe/folding-schemes` as a dependency.

* chore: Add README docs about WASM-compat & feats

* ci: don't run WASM-compat job if PR is draft

* chore: depend on `arnaucube/circom-compat` fork.

Since arnaucube/circom-compat#2 was merged, we
can already switch to it as we were depending before.

* chore: minimal build/test instructions

* fix: CI typos

* fix: Update CI to use correct feature sets

* fix: `ark-grumpkin` versioning issues

As mentioned in
#146
there's a big issue that involves some dependencies of the crate.

As a temporary fix, this forces the workspace to rely on a
"non-existing" version of `ark-grumpkin` which is immediately patched at
workspace-level for a custom version that @arnaucube owns with some
cherry-picked commits.

While this allows the CI to pass and crate to build, a better solution
is needed.

* fix: Clippy CI avoiding --all-targets

* fix: use `wasm` feat only with folding-schemes
@pmikolajczyk41
Copy link

I noticed that the Git dependencies are currently pinned to a branch name. For better stability and reproducibility, would it be possible to switch to using a specific commit rev instead of a branch? This way, the dependency won't unintentionally change if new commits are pushed to the branch.

@arnaucube
Copy link
Collaborator

arnaucube commented Aug 22, 2024

Agree. As commented in chat, the curves patches exist because when we started using the Grumpkin curve implementation there was no v0.5.0 of arkworks and Grumpkin was not included in v0.4.0, so we needed a way to use the last arkworks version while on a stable interface (ie. v0.4.0) but including also the Grumpkin curve, hence the patched fork.

We can do a 1-week coordinated effort this September (once current various implementations that are being done are completed to avoid more gitconflicts) where we coordinately upgrade all the dependencies and also Sonobe to arkworks v0.5.0 (to avoid different repos using different versions), cleaning part of the dependencies patches on the way, and we can use the same effort to try to minimize the other dependencies as you mention. Once we do that we can also change to specify the specific commit rev instead of branch in the cases that we still import a custom fork (ideally we would reduce the number of custom fork imports). Apart from the coordinated effort across multiple repos, it would be also across current branches being implemented to minimize git conflicts.

@arnaucube arnaucube added this to the Stabilize lib milestone milestone Sep 4, 2024
@arnaucube arnaucube assigned dmpierre and arnaucube and unassigned arnaucube and dmpierre Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants