feat!: integrate AVM proving #6775
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @fcarreiro and the rest of your teammates on |
| /** | ||
| * Generates proofs for the public and public kernel circuits. | ||
| */ | ||
| export interface PublicProver { |
| */ | ||
| publicKernelRequests: PublicKernelRequest[]; | ||
| publicProvingRequests: PublicProvingRequest[]; |
There was a problem hiding this comment.
I had discussed with Phil this rename.
There was a problem hiding this comment.
We need this to be separate because the AVM cannot prove any bytecode right now. However, it CAN prove add_args. So that's what we are proving here. As we start proving more, we can change the method.
In other e2e tests (e.g., e2e_prover), avm proving IS enabled, but it does not throw if it fails.
There was a problem hiding this comment.
Note for myself: add this to the CI.
There was a problem hiding this comment.
Ah, it's already there. The e2e test runs everything under e2e_prover.
| @@ -525,9 +525,19 @@ void avm_prove(const std::filesystem::path& bytecode_path, | |||
| auto const [verification_key, proof] = avm_trace::Execution::prove(avm_bytecode, call_data); | |||
| // TODO(ilyas): <#4887>: Currently we only need these two parts of the vk, look into pcs_verification key reqs | |||
There was a problem hiding this comment.
This has to be solved at some point. Also related to: #6773
| @@ -144,8 +151,7 @@ export async function generateKeyForNoirCircuit( | |||
| result = await executeBB(pathToBB, `vk_as_fields`, asFieldsArgs, log); | |||
| } | |||
| const duration = timer.ms(); | |||
| // Cleanup the bytecode file | |||
There was a problem hiding this comment.
I think it's better if the inputs (and outputs) are not cleaned up here. It's already done by the caller.
| @@ -591,8 +591,8 @@ async function fsCache<T>( | |||
| } else { | |||
| try { | |||
| run = !expectedCacheKey.equals(await fs.readFile(cacheFilePath)); | |||
| } catch (err) { | |||
| if (err && err instanceof Error && 'code' in err && err.code === 'ENOENT') { | |||
There was a problem hiding this comment.
From a discussion with Alex. See nodejs/node#31852
Benchmark resultsNo metrics with a significant change found. Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
| /** Full path of the public key. */ | ||
| pkPath?: string; | ||
| /** Base directory for the VKs (raw, fields). */ | ||
| vkPath?: string; | ||
| /** Full path of the proof. */ | ||
| proofPath?: string; | ||
| /** Full path of the contract. */ | ||
| contractPath?: string; | ||
| }; |
| inputSize: input.toBuffer().length, | ||
| circuitSize: verificationKey.circuitSize, | ||
| numPublicInputs: verificationKey.numPublicInputs, | ||
| } satisfies CircuitProvingStats, |
There was a problem hiding this comment.
TIL keyword satisfies....
| /** | ||
| * Simpler version of FullProverTest. | ||
| */ | ||
| export class FullProverTestAvm { |
There was a problem hiding this comment.
Can't pull some of this into a shared parent class with FullProverTest? Fine if not!
There was a problem hiding this comment.
Will this separate test class still exist after the AVM can prove everything?
There was a problem hiding this comment.
Ah I see, the difference isn't just that it's testing the AVM, it's that it's using the AVM test contract and just doing addArgsReturn. You can probably ignore these comments! I guess once the AVM can prove everything, we can use this class for the simple test, and the other (more full) class for the token test (possibly with modifications if necessary?)
| abstract handle( | ||
| tx: Tx, | ||
| publicKernelPublicInputs: PublicKernelCircuitPublicInputs, | ||
| ): Promise<{ | ||
| /** | ||
| * The collection of public kernel requests | ||
| */ | ||
| kernelRequests: PublicKernelRequest[]; | ||
| /** | ||
| * the output of the public kernel circuit for this phase | ||
| */ | ||
| publicKernelOutput: PublicKernelCircuitPublicInputs; | ||
| /** | ||
| * the final output of the public kernel circuit for this phase | ||
| */ | ||
| finalKernelOutput?: KernelCircuitPublicInputs; | ||
| /** | ||
| * revert reason, if any | ||
| */ | ||
| revertReason: SimulationError | undefined; | ||
| returnValues: NestedProcessReturnValues[]; | ||
| /** Gas used during the execution this particular phase. */ | ||
| gasUsed: Gas | undefined; | ||
| }>; | ||
| abstract handle(tx: Tx, publicKernelPublicInputs: PublicKernelCircuitPublicInputs): Promise<PhaseResult>; |
| let gasUsed = Gas.empty(); | ||
|
|
||
| let kernelPublicOutput: PublicKernelCircuitPublicInputs = previousPublicKernelOutput; |
There was a problem hiding this comment.
We pretty much renamed kernelOutput to kernelPublicOutput here. Should it be publicKernelOutput? Otherwise I'm not sure the word public is adding any value here.
There was a problem hiding this comment.
Yeah, I struggled to make sense of this. The original code (well, and this one) actually handle 2 types of outputs/inputs for the kernel:
- the PublicKernelPrivateInputs (which are put into the private proving info); and
- the PublicKernelPublicInputs (which are used/returned as single kernelOutputs)
the idea to use public here was to make it evident that these were the PUBLIC kernel PUBLIC inputs/outputs (and not the private ones).
| @@ -278,7 +299,7 @@ export class BBNativeRollupProver implements ServerCircuitProver { | |||
| workingDirectory: string, | |||
| ): Promise<{ circuitOutput: Output; vkData: VerificationKeyData; provingResult: BBSuccess }> { | |||
| // Have the ACVM write the partial witness here | |||
| const outputWitnessFile = `${workingDirectory}/partial-witness.gz`; | |||
| const outputWitnessFile = path.join(workingDirectory, 'partial-witness.gz'); | |||
| } else { | ||
| logger.warn(`Error thrown when proving AVM circuit: ${err}`); | ||
| logger.warn(`AVM_PROVING_STRICT is off, faking AVM proof and carrying on...`); | ||
| return { proof: makeEmptyProof(), verificationKey: VerificationKeyData.makeFake() }; |
There was a problem hiding this comment.
Does this work because the public kernels do not aggregate on the avm proof?
There was a problem hiding this comment.
Yes! The public kernels are, AFAIK, currently not taking any VM proof as input. This is because there's still no support for recursive verification for honk proofs in noir. When that changes, this should change. Hopefully we're feature-complete by then!
fcarreiro
force-pushed
the
fc/avm-proving
branch
2 times, most recently
from
2e8c302
to
f027c6c
Compare
No description provided.