Skip to content

Fix halo2_proofs features for wasm32 build & add ci for wasm build. #381

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.

Sign up for GitHub

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

Merged
nuttycom merged 1 commit into from
Mar 20, 2023
Merged

Fix halo2_proofs features for wasm32 build & add ci for wasm build. #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,22 @@ jobs:
command: test
args: --verbose

build:
name: Build target ${{ matrix.target }}
runs-on: ubuntu-latest
strategy:
matrix:
target:
- wasm32-wasi
Comment on lines +29 to +31
Copy link
Contributor

@sellout sellout Mar 20, 2023

Choose a reason for hiding this comment

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

Just curious – are we expecting to have additional cross-targets, or is this simply to have consistent variable declarations? (as opposed to using env)

Copy link
Contributor Author

@nuttycom nuttycom Mar 20, 2023

Choose a reason for hiding this comment

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

On a couple of the other crates we have additional targets; I only added this one because that's the only target that librustzcash has in its CI.


steps:
- uses: actions/checkout@v3
- name: Add target
run: rustup target add ${{ matrix.target }}
- run: cargo fetch
- name: Build for ${{ matrix.target }} target
run: cargo build --verbose --no-default-features --target ${{ matrix.target }}

bitrot:
name: Bitrot check
runs-on: ubuntu-latest
Expand Down
5 changes: 4 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ ff = "0.13"
fpe = "0.5"
group = { version = "0.13", features = ["wnaf-memuse"] }
halo2_gadgets = "0.2"
halo2_proofs = "0.2"
halo2_proofs = { version = "0.2", default-features = false, features = ["batch"] }
Copy link
Contributor

@str4d str4d Mar 20, 2023

Choose a reason for hiding this comment

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

This makes the halo2-batch feature flag below unusable, because it marks halo2_proofs/batch as a required feature flag. But if WASM is working with this PR, that means the halo2-batch feature flag is unnecessary.

hex = "0.4"
lazy_static = "1"
memuse = { version = "0.2.1", features = ["nonempty"] }
Expand Down Expand Up @@ -66,6 +66,9 @@ pprof = { version = "0.9", features = ["criterion", "flamegraph"] } # MSRV 1.56
bench = false

[features]
default = ["halo2-batch", "halo2-multicore"]
Copy link
Contributor

@daira daira Mar 20, 2023

Choose a reason for hiding this comment

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

Do these need to be named differently from the downstream features? They are about batching and/or using multicore for Orchard Action proofs; the fact that those are Halo 2 proofs is an implementation detail.

Copy link
Contributor Author

@nuttycom nuttycom Mar 20, 2023

Choose a reason for hiding this comment

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

My concern here is that these feature flags don't actually change anything in orchard code; they're there exclusively because Rust doesn't allow you to disable default features of a transitive dependency in any other way. So they really are about halo2, not orchard.

Copy link
Contributor

@str4d str4d Mar 20, 2023

Choose a reason for hiding this comment

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

If orchard did not require multicore for performance, then we would just leave it disabled here completely, and then leave it up to the end user to enable multicore on halo2_proofs if they so choose. But given that we know the default use case for orchard requires it for performance, we need it on by default, and thus exposed as orchard functionality.

Copy link
Contributor

@str4d str4d Mar 20, 2023

Choose a reason for hiding this comment

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

Put another way, this is effectively the same as having a std flag. If a crate doesn't itself use std features, it can be no-std compatible by annotating itself as such and disabling any std feature flags on its dependencies. But if it does require std for its default use case, then it offers a std feature flag to allow downstreams to selectively disable it. We don't prefix std based on which of our dependencies requires it by default.

Copy link
Contributor Author

@nuttycom nuttycom Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed in #383

halo2-batch = ["halo2_proofs/batch"]
Copy link
Contributor

@str4d str4d Mar 20, 2023

Choose a reason for hiding this comment

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

Per above, I think this can be removed:

Suggested change
halo2-batch = ["halo2_proofs/batch"]

However, if we do retain it (and allow halo2_proofs/batch to be disabled), then orchard::circuit::Proof::add_to_batch needs to be placed behind this feature flag. And I agree with @daira that in this case, the feature flag should be the same as the upstream feature (so batch).

halo2-multicore = ["halo2_proofs/multicore"]
Copy link
Contributor

@str4d str4d Mar 20, 2023

Choose a reason for hiding this comment

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

Suggested change
halo2-multicore = ["halo2_proofs/multicore"]
multicore = ["halo2_proofs/multicore"]

This is then the same thing we do in zcash_proofs: https://github.com/zcash/librustzcash/blob/0cff3c67ed4f16368b5f34986dbe1834390b340b/zcash_proofs/Cargo.toml#L54

dev-graph = ["halo2_proofs/dev-graph", "image", "plotters"]
test-dependencies = ["proptest"]

Expand Down