Expose sideloaded verification keys #1606
Conversation
rpanic
changed the title
There was a problem hiding this comment.
Awesome, thanks so much for contributing this @rpanic!
I have just a few suggestions
src/lib/proof-system/zkprogram.ts
Outdated
| if (Provable.inProver()) { | ||
| Pickles.sideLoaded.inProver(computedTag, vk.data); | ||
| } | ||
| const circuitVk = Pickles.sideLoaded.vkToCircuit(() => vk.data); |
There was a problem hiding this comment.
would love to understand what these exactly are doing.
vkToCircuit looks like introducing the vk as witnesses.
in inProver? associates the vk.data with the tag?
Just curious where exactly pickles needs this association
same question for inCircuit below
There was a problem hiding this comment.
Very good question, to be honest, I got the instructions on how to use the exposed pickles calls from matthew and just accepted them as-is. Would also be interested in what the background on those is @mrmr1993
| @@ -191,8 +198,95 @@ class Proof<Input, Output> { | |||
| } | |||
| } | |||
|
|
|||
| var sideloadedKeysCounter = 0; | |||
|
|
|||
| class DynamicProof<Input, Output> extends ProofBase<Input, Output> { | |||
There was a problem hiding this comment.
Since DynamicProof is the API entry-point to using side loaded vks, it would be good to add a doccomment here, which
- explains what this is and how to use it
- highlights the different trust model it associates with the zkapp proofs we are verifying: While verifying a
Proofproves that its public input / output originated from one of a known and hard-coded set of circuits, verifying aDynamicProofjust means the proof is valid against the associated verification key. It's on the user to constrain that verification key to be anything that is meaningful for the application at hand (and since your merkle tree example does a great job at demonstrating how that might work, you could link to it in the comment)
| static maxProofsVerified = 1 as const; | ||
| } | ||
|
|
||
| const add = ZkProgram({ |
There was a problem hiding this comment.
this example makes me a bit uncomfortable, since add isn't proving that DynamicMultiplyProof came from the multiply program, and that might not be obvious to users seeing this example.
I think adding a TODO comment where you highlight this caveat would be enough to prevent this pattern accidentally ending up in a user app and creating a vulnerability
src/index.ts
Outdated
| @@ -1,5 +1,5 @@ | |||
| export type { ProvablePure } from './lib/provable/types/provable-intf.js'; | |||
| export { Ledger, initializeBindings } from './snarky.js'; | |||
| export { Ledger, initializeBindings, Pickles } from './snarky.js'; | |||
There was a problem hiding this comment.
wait, why are we exporting the Pickles namespace? if there's no specific reason, I'd rather avoid exporting an undocumented and easy-to-misuse low-level API
There was a problem hiding this comment.
Oh, that is a leftover from testing, thanks for catching that!
src/lib/proof-system/zkprogram.ts
Outdated
| @@ -787,14 +919,43 @@ function picklesRuleFromFunction( | |||
| let input = fromFieldVars(publicInputType, publicInput); | |||
| result = await func(input, ...finalArgs); | |||
| } | |||
|
|
|||
| proofs.forEach(({ proofInstance, classReference }, index) => { | |||
| if (proofInstance instanceof DynamicProof) { | |||
There was a problem hiding this comment.
nit: early return if !(proofInstance instanceof DynamicProof) would be cleaner
src/snarky.d.ts
Outdated
| sideLoaded: { | ||
| // Create a side-loaded key tag | ||
| create:(name: string, numProofsVerified: 0 | 1 | 2, publicInputLength: number, publicOutputLength: number) => unknown /* tag */, | ||
| // Instantiate the verification key inside the circuit (required). | ||
| inCircuit:(tag: unknown, verificationKey: unknown) => undefined, | ||
| // Instantiate the verification key in prover-only logic (also required). | ||
| inProver:(tag: unknown, verificationKey: string) => undefined, | ||
| // Create an in-circuit representation of a verification key | ||
| vkToCircuit:(verificationKey: () => string) => unknown /* verificationKeyInCircuit */, | ||
| // Get the digest of a verification key in the circuit | ||
| vkDigest:(verificationKeyInCircuit: unknown) => MlArray<FieldVar>, | ||
| }; |
There was a problem hiding this comment.
nit: this doesn't look code-formatted
| let program1Proof: Proof<Field, void>; | ||
| let program2Proof: Proof<Program2Struct, Field>; | ||
|
|
||
| let program1Vk: VerificationKey; | ||
| let program2Vk: VerificationKey; | ||
|
|
||
| before(async () => { | ||
| program1Vk = (await program1.compile()).verificationKey; | ||
| program2Vk = (await program2.compile()).verificationKey; | ||
|
|
||
| // Generate sample proofs | ||
| const proof1 = await program1.foo(Field(1), Field(1)); | ||
| program1Proof = proof1; | ||
|
|
||
| const proof2 = await program2.foo( | ||
| { field1: Field(1), field2: Field(2) }, | ||
| Field(3) | ||
| ); | ||
| program2Proof = proof2; | ||
|
|
||
| await sideloadedProgram.compile(); | ||
| }); |
There was a problem hiding this comment.
I find this before() pattern with variable reassignments unreadable and error-prone. I think it's the legacy of Jest + missing top-level await. Can you just move all that preparation logic to before the describe() block?
This PR implements sideloaded verification keys as outlined in this RFC: MinaFoundation/Core-Grants#3
Bindings: o1-labs/o1js-bindings#269
Replacement to #1238
Closes #673