Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

feat!: merge barretenberg_static_lib and barretenberg_wasm #117

Merged
merged 41 commits into from
Apr 25, 2023

Conversation

TomAFrench
Copy link
Member

This PR starts the work of merging this workspace into a single crate. To simplify things, I've restricted the public interface of the crate to just be the Plonk struct (as this is necessary for nargo) to avoid any differences in the interface between targets. We should decide what else we want to expose before merging this.

The interfaces of most methods are identical between native and wasm backends. In these cases I've copied across the implementation from barretenberg_wasm and placed it in inside the function body from barretenberg_static_lib, these are then switched between with feature flags.

In a couple of cases the struct interfaces are different (Barretenberg/StandardComposer/Pippenger) so I've had to swap them out entirely based on the feature flags.

@phated phated changed the title feat: merge barretenberg_static_lib and barretenberg_wasm feat!: merge barretenberg_static_lib and barretenberg_wasm Apr 17, 2023
Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

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

Do these need to specify dep:?

@TomAFrench
Copy link
Member Author

Do these need to specify dep:?

I initially had dep: on here but that means we run into this error:

error: Package `acvm-backend-barretenberg v0.1.0 (/home/tom/Programming/aztec_backend/acvm_backend_barretenberg)` does not have feature `wasmer`.
It has an optional dependency with that name, but that dependency uses the "dep:" syntax in the features table, so it does not have an implicit feature with that name.

So it seems like having a dep: flag conflicts with switching on feature flags on that dependency. (adding dep: to the other wasmer flags also results in an error.)

https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies

@TomAFrench
Copy link
Member Author

Tomorrow I'll probably break out the different flavours of src/lib.rs into separate files for native/wasm backends. It's pretty messy with the chonky if-else.

* master:
  feat: replace `downloader` dependency with `reqwest` (#114)
* master:
  fix: replace `serialize_circuit` function with `from<&Circuit>` (#118)
* master:
  fix!: avoid exposing internals of Assignments type (#119)
@TomAFrench TomAFrench force-pushed the acvm-backend-barretenberg branch from 3f0d4a5 to 17b9165 Compare April 18, 2023 10:51
* master:
  fix: use `Barretenberg.call` to query circuit size from wasm (#121)
  chore: remove `Assignments::from_vec` (#122)
* master:
  feat!: return boolean rather than `FieldElement` from `verify_signature` (#123)
* master:
  feat: return boolean rather than `FieldElement` from `check_membership` (#124)
* master:
  feat!: Implement pseudo-builder pattern for ConstraintSystem & hide struct fields (#120)
@TomAFrench
Copy link
Member Author

We're currently only testing the native backend due to us merging these two crates. I'd like to pass in the different feature flags so we can build/test the flake with both backends as otherwise we'll not be testing the wasm backend.

Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

Left some general comments; was reading to understand more about this issue: noir-lang/acvm#211

Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

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

Some things I noticed when I pulled this down locally.

@TomAFrench
Copy link
Member Author

Ah, this still needs the multiple outputs (native vs wasm) added to the flake as well.

Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

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

I noticed a few more things. I wonder if we can drop the cfg-if block for all the setup but I won't block on it.

@TomAFrench
Copy link
Member Author

I wonder if we can drop the cfg-if block for all the setup but I won't block on it.

I don't see a good way to do this. Rust doesn't seem to like putting a scope around the two branches (similarly to how we do in pow2ceil) so we'd have to add the cfg to each and every element inside which would be uglier.

@phated
Copy link
Contributor

phated commented Apr 25, 2023

I don't see a good way to do this. Rust doesn't seem to like putting a scope around the two branches (similarly to how we do in pow2ceil) so we'd have to add the cfg to each and every element inside which would be uglier.

I'm wondering if it would be better/worse to use your cfg on a mod like you did for crs.rs?

@TomAFrench
Copy link
Member Author

TomAFrench commented Apr 25, 2023

You asked me to revert that change though 🫠 Ah gotcha, we can do it in a single file.

Yeah, I'm happy with that as it encapsulates more of the wasm internals.

Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

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

This is awesome! Let's get it merged so we can keep iterating on this crate 🎉

@phated phated added this pull request to the merge queue Apr 25, 2023
Merged via the queue into master with commit ba1d0d6 Apr 25, 2023
@phated phated deleted the acvm-backend-barretenberg branch April 25, 2023 22:32
@github-actions github-actions bot mentioned this pull request May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants