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

chore!: Update to acvm 0.11.0 #1322

Merged
merged 12 commits into from May 12, 2023
Merged

chore!: Update to acvm 0.11.0 #1322

merged 12 commits into from May 12, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented May 8, 2023

Related issue(s)

Resolves #1301

Description

Summary of changes

This updates the project to ACVM 0.11.0

This is the version of ACVM that introduced the Result return type to many of the functions on the Backend trait. This allows us to better surface errors from backends—and as such, aztec_backend has been updated to return results for almost all errors (the only unwraps remaining are in the merkle implementation).

This version of ACVM also introduced keccak into the constraint system, so we needed to upgrade barretenberg also. This may cause the same OOM error that the other Keccak PR is exhibiting.

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

@TomAFrench
Copy link
Member

I've renamed the type parameters to B as naming them Backend is very confusing as it shadows the Backend trait.

@TomAFrench
Copy link
Member

I've updated this to use the commit for the 0.1.0 release of acvm-backend-barretenberg pending noir-lang/acvm-backend-barretenberg#178

@TomAFrench TomAFrench marked this pull request as ready for review May 11, 2023 08:02
TomAFrench
TomAFrench previously approved these changes May 11, 2023
Copy link
Member

@TomAFrench TomAFrench 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've made a couple of tweaks at the end so will let you have last look.

I'm not a fan of how the errors require CliError to be generic as it seems like we're taking on extra complexity (type parameters and casting traits) for not much benefit. I'd like to make a separate proposal which imo simplifies things later.

@phated
Copy link
Contributor Author

phated commented May 11, 2023

I'm fine with your try_vecmap changes, even though I often don't like to introduce unnecessary lambdas into rust code.

I'm not a fan of how the errors require CliError to be generic as it seems like we're taking on extra complexity (type parameters and casting traits) for not much benefit. I'd like to make a separate proposal which imo simplifies things later.

Happy to take a look, but I experimented with different options and the only two ways I could find to make std::error::Error work on traits is to use Associated Types or Box<dyn std::error::Error + 'lifetime> and I'm not okay with that type leaking into each backend implementations to avoid a generic in nargo-cli.

This still needs an approval from someone that isn't me.

@TomAFrench TomAFrench added this pull request to the merge queue May 12, 2023
Merged via the queue into master with commit da47368 May 12, 2023
9 checks passed
@TomAFrench TomAFrench deleted the phated/acvm-0.11 branch May 12, 2023 06:09
TomAFrench added a commit that referenced this pull request May 17, 2023
* master: (66 commits)
  feat(nargo)!: retire print-acir in favour of flag (#1328)
  chore(ssa): enable cse for assert (#1350)
  chore(ssa refactor): Add basic instruction simplification (#1329)
  chore(noir): Release 0.6.0 (#1279)
  feat: enable to_radix for any field element (#1343)
  chore(ssa refactor): Simplify inlining pass and fix inlining failure (#1337)
  chore!: Update to acvm 0.11.0 (#1322)
  feat: Add ECDSA secp256k1 builtin test (#1294)
  chore: add support for encoding/decoding inputs from JSON (#1325)
  feat: Issue an error when attempting to use a `return` expression (#1330)
  chore(ssa refactor): Fix inlining bug (#1335)
  fix: to-bits and to-radix for > 128 bits (#1312)
  chore(parser): Parser error optimisation (#1292)
  chore(ssa refactor): Implement function inlining (#1293)
  chore: fix installation link in readme (#1326)
  chore: fix installation link in readme (#1326)
  feat(stdlib): Add keccak (#1249)
  fix: Parsing nested generics (#1319)
  chore(ssa refactor): Document some SSA-gen functions (#1321)
  fix: Assigning to tuple fields (#1318)
  ...
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.

Update to ACVM v0.11.0
2 participants