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

refactor: move the halo2_common::arithmetic to halo2_backend(post fe-be split) #281

Merged
merged 16 commits into from
Feb 22, 2024

Conversation

duguorong009
Copy link

@duguorong009 duguorong009 commented Feb 21, 2024

Description

  • move the halo2_common::arithmetic::* to halo2_backend crate

Related issues

Changes

  • move the arithmetic.rs file to halo2_backend crate
  • update the imports in related crates

Notes

The PR includes the following changes for CI check.

  • bump up the rust-toolchain from 1.67.0 to 1.73.0
  • add the resolver = "2" option in workspace cargo.toml file
  • set the getrandom crate as dependency for wasm32-* build
  • update module visibility, etc (fix clippy warnings)

@duguorong009 duguorong009 marked this pull request as draft February 21, 2024 08:59
@duguorong009
Copy link
Author

duguorong009 commented Feb 21, 2024

@ed255
The CI complains the MSRV is incompatible with dependency crate(bumpalo).
The MSRV is v1.73.0 for bumpalo(v3.15.1), while v1.67.0 for halo2.
Do you think it is good to update the MSRV of halo2?

For additional info, the bumpalo crate is dependency of wasm-bindgen-backend crate.
As you can see in the following cargo.toml, the bumpalo version is v3.0.0.
https://github.com/rustwasm/wasm-bindgen/blob/main/crates/backend/Cargo.toml#L20
Hence, the cargo resolver automatically fetches the latest version of bumpalo. (currently, v3.15.1)
The MSRV of bumpalo has been updated to v1.73.0 in v3.15.0.

cc @CPerezz @davidnevadoc

@CPerezz
Copy link
Member

CPerezz commented Feb 21, 2024

@duguorong009 We're at 1.76 (see https://releases.rs/) version of stable.

It think 3 versions less for the MSRV update is fine.
This crate doesn't have an explicit MSRV policy, but if it did it would probably be similar to this proposed policy from libc: rust-lang/libs-team#72.

In general, I like the N-3 option.

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.

@duguorong009
Copy link
Author

@duguorong009 We're at 1.76 (see https://releases.rs/) version of stable.

It think 3 versions less for the MSRV update is fine. This crate doesn't have an explicit MSRV policy, but if it did it would probably be similar to this proposed policy from libc: rust-lang/libs-team#72.

In general, I like the N-3 option.

Thanks for quick reply!
Update the rustc version to v1.73.0 in rust-toolchain file. b535a18

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.

LGTM! I noticed that this PR has many commits that are unrelated to the PR itself. For this reason I suggest using Squash and merge and cleaning up the commit message (basically removing the commit messages that are not applied here)

@ed255
Copy link
Member

ed255 commented Feb 21, 2024

@duguorong009 note that the Stable lints CI check is not passing, you'll need to fix that before merging.

@CPerezz
Copy link
Member

CPerezz commented Feb 21, 2024

Once this is merged, will bump up the MSRV which will allow #282 to pass WASM compilation CI.

@CPerezz CPerezz marked this pull request as ready for review February 21, 2024 15:53
@duguorong009
Copy link
Author

duguorong009 commented Feb 22, 2024

Once this is merged, will bump up the MSRV which will allow #282 to pass WASM compilation CI.

The CI still fails in wasm32-* build check with new errors.
Will try to debug why.

@CPerezz CPerezz merged commit 42f8e93 into main Feb 22, 2024
15 checks passed
@CPerezz
Copy link
Member

CPerezz commented Feb 22, 2024

Ohh dammit I merged as I saw green light and the approvals! But the changes in 09aaf97 aren't OK.

I did it since it only should trigger for dev-dependencies.
Filled #285 for this.

This unblocks the merge of all other PRs.

@duguorong009 duguorong009 deleted the gr@post-split-move-arithmetic branch February 22, 2024 09:28
mratsim added a commit to taikoxyz/halo2 that referenced this pull request Mar 29, 2024
Deprecate pre-ZAL API

Insert patch in `Cargo.toml` for `../halo2curves`

move ZAL to middleware

ZAL: introduce modular MSM/FFT per prover accelerators

ZAL: address clippy

halo2_common: best_fft and best_multiexp have been moved to halo2curves

zal: store engine at the prover level

zal: fix clippy

halo2_common::arithmetic -> halo2_backend::arithmetic sync privacy-scaling-explorations#281

Remove Copy trait requirement from descriptors

run fmt

zal: remove Option from PlonkEngineConfig

zal: add default engine test
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