`eth_contract_from_vk` assumes the backend serializes public input data in the vkey
jp4g opened this issue · 5 comments
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
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: Circuitparameter as is required withprove_with_pk
Do you mean exposing the option for Noir devs to access and tinker with multiple instance columns at the Noir level?
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()!
I think adding the Circuit to the function signature should be fine -- if possible, can you open a PR?
@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?
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).