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

feat!: Update to ACVM v0.11.0 #151

Merged
merged 19 commits into from
May 10, 2023
Merged

feat!: Update to ACVM v0.11.0 #151

merged 19 commits into from
May 10, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented May 1, 2023

This reworks the backend to leverage the changes for fallible functions. It introduces an error enum with admittedly bad variant names and uses them to map error cases throughout the codebase so panic and/or unwrap isn't used to handle most values.

There are still some TODOs to cleanup even more unwrap cases and it relies on something like noir-lang/acvm#251, but this should be a solid skeleton for how a backend can surface errors to nargo.

Closes #159

@kevaundray
Copy link
Contributor

You can change the commit to point to acvm 0.11 -- Is this ready for review? If not, then I can push a new PR with the fallible traits but has the same behaviour as without the fallible traits.

Main goal here is to get Keccak upstreamed -- Alternatively, I can add Keccak into the pwg for now by copying this code into here.

Then removing it once we use acvm > 0.11

@phated
Copy link
Contributor Author

phated commented May 4, 2023

Promoting from draft because acvm 0.11 was released but we still need to workshop the errors and stuff.

@phated phated marked this pull request as ready for review May 4, 2023 22:15
@phated
Copy link
Contributor Author

phated commented May 4, 2023

I've completed my TryInto TODOs and split the wasm errors into their own enum. src/merkle.rs still has a lot of unwraps but I don't think we need to update that for this to land.

Besides error variant naming and error messages, I think this should be good-to-go. Will wait on @TomAFrench to do a full review and give feedback on those.

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.

A couple of these error types don't need to exist in my mind (comments below).

We've also bundled in a change which changes WASMValue to cover not just return values but all arguments as well. I'm going to split off this change so I can review errors in isolation.

src/schnorr.rs Outdated Show resolved Hide resolved
src/schnorr.rs Outdated Show resolved Hide resolved
src/schnorr.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@phated phated linked an issue May 5, 2023 that may be closed by this pull request
1 task
@phated phated changed the title feat!: Rework backend for fallible traits to avoid panic & unwrap feat!: Update to ACVM v0.11.0 May 5, 2023
@phated phated mentioned this pull request May 5, 2023
1 task
TomAFrench
TomAFrench previously approved these changes May 6, 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.

I think that we can remove a number of errors which are included here. I don't think we should expose these as users never need to be concerned with them. I don't think it's worth blocking over however as we don't have semver set up yet so every release is breaking and we can just remove them progressively.

src/schnorr.rs Outdated Show resolved Hide resolved
src/schnorr.rs Outdated Show resolved Hide resolved
src/composer.rs Outdated Show resolved Hide resolved
src/merkle.rs Outdated Show resolved Hide resolved
src/merkle.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@phated phated requested a review from TomAFrench May 8, 2023 21:15
@TomAFrench
Copy link
Member

Some thoughts on error variants

// These variants shouldn't exist as we can prevent them at compile time
SchnorrConvert(Vec<u8>),
SchnorrSlice { source: TryFromSliceError },
FieldElementSlice { source: TryFromSliceError },
FieldToArray(usize, usize),

// Can be replaced with a MalformedOpcode(Opcode) error
// This error can be defined down in ACVM and be shared across backends
MissingOutput(usize),
MissingLeaf,
MissingLeafIndex,
MissingPublicKeyX,
MissingPublicKeyY,
MissingSignature(usize),
MissingPublicKey(&'static str, usize),

// I think we can leave this as a panic tbh.
// Calling a backend with an unsupported opcode is calling the `solve` function outside of contract.
Aes,

// Unsure what to do these
Pow2CeilOverflow(u32),
WasmError,

@phated
Copy link
Contributor Author

phated commented May 9, 2023

// These variants shouldn't exist as we can prevent them at compile time
SchnorrConvert(Vec<u8>),
SchnorrSlice { source: TryFromSliceError },
FieldElementSlice { source: TryFromSliceError },
FieldToArray(usize, usize),

I looked through the compiler and we currently aren't preventing them at compile time so we should leave them for this to land sooner. We need to fix things in bb-sys to ensure this at compile time.

// Can be replaced with a MalformedOpcode(Opcode) error
// This error can be defined down in ACVM and be shared across backends
MissingOutput(usize),
MissingLeaf,
MissingLeafIndex,
MissingPublicKeyX,
MissingPublicKeyY,
MissingSignature(usize),
MissingPublicKey(&'static str, usize),

I'm fine joining these into one error, but these are the TryFrom<Circuit> and shouldn't be defined in ACVM

// I think we can leave this as a panic tbh.
// Calling a backend with an unsupported opcode is calling the `solve` function outside of contract.
Aes,

I'm going to leave this because it's easier to not have to grep for all the different types of ways to panic.

// Unsure what to do these
Pow2CeilOverflow(u32),
WasmError,

We can leave them as-is then.

@phated phated requested a review from kobyhallx May 9, 2023 16:44
@phated
Copy link
Contributor Author

phated commented May 9, 2023

I think this PR is good-to-go and we should hold any other changes/PRs until it lands. We need to get this stuff upstreamed.

@phated phated requested a review from TomAFrench May 9, 2023 17:53
Copy link
Contributor

@kobyhallx kobyhallx 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 great! Thank You!

@kobyhallx kobyhallx added this pull request to the merge queue May 10, 2023
Merged via the queue into master with commit 9202415 May 10, 2023
@kobyhallx kobyhallx deleted the phated/fallible branch May 10, 2023 10:25
TomAFrench added a commit that referenced this pull request May 10, 2023
* master:
  feat!: Update to ACVM v0.11.0 (#151)
@github-actions github-actions bot mentioned this pull request May 10, 2023
TomAFrench added a commit that referenced this pull request May 12, 2023
* acvm-0.12.0:
  fix bad rebase
  chore: Release 0.1.2 (#183)
  fix: Remove star dependencies to allow publishing (#182)
  chore: Release 0.1.1 (#181)
  fix: Add description so crate can be published (#180)
  chore: update readme to new name and add contract note (#177)
  feat!: update to acvm with non-homogeneous bb calls (#169)
  update to latest changes
  chore: Release 0.1.0 (#173)
  use patch syntax
  chore: update to use new black box solver interface
  feat!: update to target acvm-84b5d18d
  chore(ci): Update tokens (#174)
  chore: Add release-please and publish workflows to project (#172)
  feat!: Update to ACVM v0.11.0 (#151)
  chore: remove usage of `std::mem::forget` (#164)
  chore: Enforce proper conversion of memory into fixed length array (#163)
  chore: Add test for Keccak256 constraint (#158)
  chore: use `WASMValue` for wasm arguments as well as return values (#157)
  feat!: Add Keccak constraints (#150)
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.

Update to ACVM v0.11.0
4 participants