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

[nuttycom]

No description provided.

[codecov-commenter]

Codecov Report

Patch and project coverage have no change.

Comparison is base (61a67f0) 83.43% compared to head (783db89) 83.43%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #381   +/-   ##
=======================================
  Coverage   83.43%   83.43%           
=======================================
  Files          32       32           
  Lines        2692     2692           
=======================================
  Hits         2246     2246           
  Misses        446      446           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[daira]

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.

[nuttycom]

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.

[str4d]

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.

[str4d]

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.

[nuttycom]

Fixed in #383

[daira]

utACK with question.

[sellout]

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

[nuttycom]

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.

[str4d]

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.

[str4d]

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).

[str4d]

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

[nuttycom]

@str4d oops, I didn't see your request for changes; I will submit another PR to fix.