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

eth_contract_from_vk assumes the backend serializes public input data in the vkey #411

Closed
jp4g opened this issue Jul 5, 2023 · 5 comments · Fixed by #420
Closed

eth_contract_from_vk assumes the backend serializes public input data in the vkey #411

jp4g opened this issue Jul 5, 2023 · 5 comments · Fixed by #420
Labels
enhancement New feature or request

Comments

@jp4g
Copy link
Contributor

jp4g commented Jul 5, 2023

Problem

Trying to integrate eth_contract_from_vk of the SmartContract trait for a halo2 backend.

Barretenberg provides the number of public inputs in the vkey and handles commitments with each input.

However, Halo2 simply grabs the entire instance column (pub input wire polynomial) and commits to it. The snark-verifier package needed to generate evm contracts must know both the number of instance columns used AND the number of inputs for each column.

In practice, there is currently no way to use the existing acvm backend API and the halo2 verification key to generate a smart contract. We have gotten the functionality fully working and tested with different numbers of public inputs, however we have to hard-code the number of public inputs in eth_contract_from_vk.

Happy Case

As the halo2 backend currently exists, the minimum necessity would be to have the API optionally accept num_public_inputs: usize. This is because the backend currently only has one instance column and will only ever serialize the inputs in a single wire polynomial. Including this functionality would unblock the current halo2_backend work.

However, this limits the possibility of using multiple instance columns in halo2. Currently, the black box functionality is highly limited. However, with ECC and other functionality added, there is a possibility that the most efficient implementation uses another instance column rather than appending every public input to the single input column. In this case, it might be desirable to add an optional circuit: Circuit parameter as is required with [prove_with_pk](https://github.com/noir-lang/acvm/blob/fc3240d456d0128f6eb42096beb8b7a586ea48da/acvm/src/lib.rs#L141) (for one example). This would allow the circuit to compute the exact number of public values per instance column.

Alternatives Considered

Opening issues to investigate serializing the number of public inputs per instance column in the verification key for Halo2, though this is largely superfluous information to the verification of the proof

Additional Context

Largely open to suggestions/ other diagnostics to accomplish this goal

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@jp4g jp4g added the enhancement New feature or request label Jul 5, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 5, 2023
@Savio-Sou
Copy link
Contributor

Pardon the stupid questions as I likely lack the essential knowledge and contexts:

have the API optionally accept num_public_inputs: usize

Isn't the number of public inputs determinable from the compiled ACIR?

add an optional circuit: Circuit parameter as is required with prove_with_pk

Do you mean exposing the option for Noir devs to access and tinker with multiple instance columns at the Noir level?

@jp4g
Copy link
Contributor Author

jp4g commented Jul 6, 2023

No they are good q's!

Isn't the number of public inputs determinable from the compiled ACIR?

Yes, if we pass in a circuit: Circuit parameter, we could call circuit.public_inputs() to obtain the total number of public inputs. Passing in num_public_inputs is just the absolute minimum but I think passing in the ACIR is preferable.

Do you mean exposing the option for Noir devs to access and tinker with multiple instance columns at the Noir level?

Probably not. This would be quite an interesting feature to control the proof composition from Noir, but this would clash with Noir's main goal of abstracting these parts of ZKP away from the dev.

Instead, I am thinking about Aztec contributors adding different backends into Noir. In the case of Halo2's backend, having access to the entire ACIR allows us to calculate how many public inputs are used in each instance column if we elect to add more instance columns as future black box functions are added.

As of the current halo2_backend, we wouldn't need to do anything more than call circuit.public_inputs()!

@kevaundray
Copy link
Contributor

I think adding the Circuit to the function signature should be fine -- if possible, can you open a PR?

@jp4g
Copy link
Contributor Author

jp4g commented Jul 10, 2023

@kevaundray if this looks right, should I open pr's for nargo and acvm-backend-barretenberg as well to make sure things work with barretenberg?

@TomAFrench
Copy link
Member

if this looks right, should I open pr's for nargo and acvm-backend-barretenberg as well to make sure things work with barretenberg?

Nah, don't worry about this. We can handle this in the next release rollout as it'll be a small additonal change (You'd end up dealing with more of our changes that we would yours).

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants