Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat(acvm)!: Encapsulate internal state of ACVM within a struct #384

Merged
merged 23 commits into from
Jun 21, 2023

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Jun 16, 2023

Description

Problem*

Resolves #383

Summary*

This PR creates an struct to track the internal state of the ACVM execution.

Testing with full noir test suite here

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@TomAFrench
Copy link
Member Author

This can be simplified significantly with #368 and #385. We will then also be able to remove all cloning of opcodes.

@TomAFrench TomAFrench changed the base branch from master to remove-oracles June 16, 2023 16:24
@TomAFrench TomAFrench added this to the v0.16.0 milestone Jun 20, 2023
acvm/src/pwg/mod.rs Outdated Show resolved Hide resolved
acvm/src/pwg/mod.rs Outdated Show resolved Hide resolved
@kevaundray
Copy link
Contributor

kevaundray commented Jun 20, 2023

Is this aiming to encapsulate ACVM or just the PWG? Seems its just the solver from the linked issue -- Not sure if that struct should be called ACVM then, since ACVM involves both a solver and a backend. Where the backend does proving and verifying

@TomAFrench
Copy link
Member Author

TomAFrench commented Jun 20, 2023

ACVM involves both a solver and a backend. Where the backend does proving and verifying

Is that the correct way to think about it? I mean, yes if we're talking about "ACVM the repository" then this naming is suboptimal but if we're thinking about "ACVM the virtual machine" then the struct naming is accurate.

If we were talking about a zkEVM, I don't think we'd include the prover/verifier in the definition of the zkEVM but accompanying components. The zkEVM is just the circuits which take initial state and generate the output state which is similar to what the ACVM struct is doing here.

Is this aiming to encapsulate ACVM or just the PWG? Seems its just the solver from the linked issue.

Yes, I'm planning on just having this cover the solver for the foreseeable future. Proving/verifying has zero internal state so we don't need to place it on this struct.

@kevaundray
Copy link
Contributor

kevaundray commented Jun 20, 2023

Is that the correct way to think about it? I mean, yes if we're talking about "ACVM the repository" then this naming is suboptimal but if we're thinking about "ACVM the virtual machine" then the struct naming is accurate.

I've always thought of it as ACVM being instantiated with a backend but I can see where you are coming from. If thats the case, then wouldn't proving and verifying being in this repository be an abstraction leak?

(This is not blocking for me)

@TomAFrench
Copy link
Member Author

If thats the case, then wouldn't proving and verifying being in this repository be an abstraction leak?

Potentially, yes. The SmartContract and ProofSystemCompiler traits depend on ACIR rather than the ACVM itself. These interfaces could be defined elsewhere as some for of "surrounding tooling" for the ACVM.

@TomAFrench TomAFrench changed the base branch from remove-oracles to master June 21, 2023 04:16
@TomAFrench TomAFrench marked this pull request as ready for review June 21, 2023 05:49
@TomAFrench TomAFrench marked this pull request as draft June 21, 2023 06:07
@TomAFrench TomAFrench marked this pull request as ready for review June 21, 2023 06:36
@TomAFrench
Copy link
Member Author

I don't think this will be the final state of the ACVM struct but its in a mergeable condition.

@TomAFrench TomAFrench added this pull request to the merge queue Jun 21, 2023
Merged via the queue into master with commit 84d4867 Jun 21, 2023
@kevaundray kevaundray mentioned this pull request Jun 21, 2023
@TomAFrench TomAFrench deleted the acvm-struct branch June 26, 2023 04:52
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.

Refactor ACVM solver to allow tracking internal state
3 participants