-
Notifications
You must be signed in to change notification settings - Fork 599
feat: recording circuit inputs + oracles #12148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
417a056
63ec776
9835adb
02289ff
1a0e724
b9123fa
19dd714
f3b54a5
afc34b2
a250e0d
f2b0bd5
123de78
74b0784
2d9f666
6a410de
4ce4601
18ae44d
e214126
51cfb4e
b4b2a92
8250024
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import fs from 'fs/promises'; | ||
| import path from 'path'; | ||
|
|
||
| import { setup } from './fixtures/utils.js'; | ||
|
|
||
| /** | ||
| * Tests the circuit recorder is working as expected. To read more about it, check JSDoc of CircuitRecorder class. | ||
| */ | ||
| describe('Circuit Recorder', () => { | ||
| const RECORD_DIR = './circuit_recordings'; | ||
|
|
||
| it('records circuit execution', async () => { | ||
| // Set recording directory env var - this will activate the circuit recorder | ||
| process.env.CIRCUIT_RECORD_DIR = RECORD_DIR; | ||
|
|
||
| // Run setup which deploys an account contract and runs kernels | ||
| const { teardown } = await setup(1); | ||
|
|
||
| // Check recording directory exists | ||
| const dirExists = await fs.stat(RECORD_DIR).then( | ||
| stats => stats.isDirectory(), | ||
| () => false, | ||
| ); | ||
| expect(dirExists).toBe(true); | ||
|
|
||
| // Check recording file of a user circuit (contract circuit) exists and has expected content | ||
| { | ||
| const files = await fs.readdir(RECORD_DIR); | ||
| expect(files.length).toBeGreaterThan(0); | ||
|
|
||
| const recordingFile = files.find(f => f.startsWith('SchnorrAccount_constructor')); | ||
| expect(recordingFile).toBeDefined(); | ||
|
|
||
| const recordingContent = await fs.readFile(path.join(RECORD_DIR, recordingFile!), 'utf8'); | ||
| const recording = JSON.parse(recordingContent); | ||
|
|
||
| expect(recording).toMatchObject({ | ||
| circuitName: 'SchnorrAccount', | ||
| functionName: 'constructor', | ||
| inputs: expect.any(Object), | ||
| oracleCalls: expect.any(Array), | ||
| }); | ||
| } | ||
|
|
||
| // Then we'll do the same for a protocol circuit | ||
| { | ||
| const files = await fs.readdir(RECORD_DIR); | ||
| expect(files.length).toBeGreaterThan(0); | ||
|
|
||
| const recordingFile = files.find(f => f.startsWith('PrivateKernelInit_main')); | ||
| expect(recordingFile).toBeDefined(); | ||
|
|
||
| const recordingContent = await fs.readFile(path.join(RECORD_DIR, recordingFile!), 'utf8'); | ||
| const recording = JSON.parse(recordingContent); | ||
|
|
||
| expect(recording).toMatchObject({ | ||
| circuitName: 'PrivateKernelInit', | ||
| functionName: 'main', | ||
| inputs: expect.any(Object), | ||
| oracleCalls: expect.any(Array), | ||
| }); | ||
| } | ||
|
|
||
| // Cleanup | ||
| await fs.rm(RECORD_DIR, { recursive: true, force: true }); | ||
| delete process.env.CIRCUIT_RECORD_DIR; | ||
| await teardown(); | ||
| }, 20_000); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,6 @@ import { | |
| } from '@aztec/aztec.js'; | ||
| import { deployInstance, registerContractClass } from '@aztec/aztec.js/deployment'; | ||
| import { AnvilTestWatcher, CheatCodes } from '@aztec/aztec.js/testing'; | ||
| import type { BBNativePrivateKernelProver } from '@aztec/bb-prover'; | ||
| import { createBlobSinkClient } from '@aztec/blob-sink/client'; | ||
| import { type BlobSinkServer, createBlobSinkServer } from '@aztec/blob-sink/server'; | ||
| import { FEE_JUICE_INITIAL_MINT, GENESIS_ARCHIVE_ROOT, GENESIS_BLOCK_HASH } from '@aztec/constants'; | ||
|
|
@@ -52,9 +51,16 @@ import { FeeJuiceContract } from '@aztec/noir-contracts.js/FeeJuice'; | |
| import { getVKTreeRoot } from '@aztec/noir-protocol-circuits-types/vk-tree'; | ||
| import { ProtocolContractAddress, protocolContractTreeRoot } from '@aztec/protocol-contracts'; | ||
| import { type ProverNode, type ProverNodeConfig, createProverNode } from '@aztec/prover-node'; | ||
| import { type PXEService, type PXEServiceConfig, createPXEService, getPXEServiceConfig } from '@aztec/pxe/server'; | ||
| import { | ||
| type PXEService, | ||
| type PXEServiceConfig, | ||
| createPXEServiceWithSimulationProvider, | ||
| getPXEServiceConfig, | ||
| } from '@aztec/pxe/server'; | ||
| import type { SequencerClient } from '@aztec/sequencer-client'; | ||
| import type { TestSequencerClient } from '@aztec/sequencer-client/test'; | ||
| import { WASMSimulator } from '@aztec/simulator/client'; | ||
| import { SimulationProviderRecorderWrapper } from '@aztec/simulator/testing'; | ||
| import { getContractClassFromArtifact } from '@aztec/stdlib/contract'; | ||
| import { Gas } from '@aztec/stdlib/gas'; | ||
| import type { AztecNodeAdmin } from '@aztec/stdlib/interfaces/client'; | ||
|
|
@@ -135,18 +141,15 @@ export const setupL1Contracts = async ( | |
| * Sets up Private eXecution Environment (PXE). | ||
| * @param aztecNode - An instance of Aztec Node. | ||
| * @param opts - Partial configuration for the PXE service. | ||
| * @param firstPrivKey - The private key of the first account to be created. | ||
| * @param logger - The logger to be used. | ||
| * @param useLogSuffix - Whether to add a randomly generated suffix to the PXE debug logs. | ||
| * @param proofCreator - An optional proof creator to use | ||
| * @returns Private eXecution Environment (PXE), accounts, wallets and logger. | ||
| * @returns Private eXecution Environment (PXE), logger and teardown function. | ||
| */ | ||
| export async function setupPXEService( | ||
| aztecNode: AztecNode, | ||
| opts: Partial<PXEServiceConfig> = {}, | ||
| logger = getLogger(), | ||
| useLogSuffix = false, | ||
| proofCreator?: BBNativePrivateKernelProver, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This param was never used so I nuked it. |
||
| ): Promise<{ | ||
| /** | ||
| * The PXE instance. | ||
|
|
@@ -169,7 +172,14 @@ export async function setupPXEService( | |
| pxeServiceConfig.dataDirectory = path.join(tmpdir(), randomBytes(8).toString('hex')); | ||
| } | ||
|
|
||
| const pxe = await createPXEService(aztecNode, pxeServiceConfig, useLogSuffix, proofCreator); | ||
| const simulationProvider = new WASMSimulator(); | ||
| const simulationProviderWithRecorder = new SimulationProviderRecorderWrapper(simulationProvider); | ||
| const pxe = await createPXEServiceWithSimulationProvider( | ||
| aztecNode, | ||
| simulationProviderWithRecorder, | ||
| pxeServiceConfig, | ||
| useLogSuffix, | ||
| ); | ||
|
|
||
| const teardown = async () => { | ||
| if (!configuredDataDirectory) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import type { NoirCompiledCircuit } from '@aztec/stdlib/noir'; | ||
| import type { NoirCompiledCircuit, NoirCompiledCircuitWithName } from '@aztec/stdlib/noir'; | ||
| import type { VerificationKeyData } from '@aztec/stdlib/vks'; | ||
|
|
||
| import PrivateKernelInitJson from '../../../artifacts/private_kernel_init.json' assert { type: 'json' }; | ||
|
|
@@ -30,12 +30,12 @@ export const SimulatedClientCircuitArtifacts: Record<ClientProtocolArtifact, Noi | |
| }; | ||
|
|
||
| export class BundleArtifactProvider implements ArtifactProvider { | ||
| getClientCircuitArtifactByName(artifact: ClientProtocolArtifact): Promise<NoirCompiledCircuit> { | ||
| return Promise.resolve(ClientCircuitArtifacts[artifact]); | ||
| getClientCircuitArtifactByName(artifact: ClientProtocolArtifact): Promise<NoirCompiledCircuitWithName> { | ||
| return Promise.resolve({ ...ClientCircuitArtifacts[artifact], name: artifact.replace('Artifact', '') }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I introduced NoirCompiledCircuitWithName which as the name implies is just |
||
| } | ||
|
|
||
| getSimulatedClientCircuitArtifactByName(artifact: ClientProtocolArtifact): Promise<NoirCompiledCircuit> { | ||
| return Promise.resolve(SimulatedClientCircuitArtifacts[artifact]); | ||
| getSimulatedClientCircuitArtifactByName(artifact: ClientProtocolArtifact): Promise<NoirCompiledCircuitWithName> { | ||
| return Promise.resolve({ ...SimulatedClientCircuitArtifacts[artifact], name: artifact.replace('Artifact', '') }); | ||
| } | ||
|
|
||
| getCircuitVkByName(artifact: ClientProtocolArtifact): Promise<VerificationKeyData> { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very bad, can we try not to propagate the foreignCallHandler to all the call sites? It's actually the reason why we need a package split (the
/serverone pulls from the blobs package) and besides not being very clean (why have different providers if you're feeding the only difference they have externally?), I fear we are going to end up with a/serverautoimport somewhere we shouldn'tThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server foreign_call_handler is injected only in the tets_circuit_prover.ts so I don't see how is this a problem.

The SimulationProvider already accepted the callback for user circuits before my changes:

It's not the only difference. WASMSimulator.executeProtocolCircuit runs the ACVM in WASM, NativeACVMSimulator runs it in NativeACVM.
But I see that WASMSimulatorWithBlobs became pointless. I would argue that this means we can just nuke it and not that my change is bad.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that previously all the functionality was contained inside the simulation provider, as externally no one should have to be concerned with the foreign calls...every provider used the appropriate one and that was the argument that you gave PXE/the provers. Now that abstraction is leaking and you could end up in a situation in which you have a wasm browser provider being fed a server foreign call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. If we nuke the
WASMSimulatorWithBlobsthen essentially the dev should not make a mistake in the choice of the foreign call handler but he could no longer make a mistake in the choice of the simulator provider as there would only be 1 (WASMSimulator).Given that I don't really see how better to approach this (because I need to hijack both the circuit inputs and the foreign call handler and for that the SimulationProvider recording wrapper seems to be the best tradeoff to me) I would just nuke the
WASMSimulatorWithBlobsand rename the 2 functions calledforeignCallHandlerto be more descriptive.WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super stoked, but go for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will wait for Nico's review as well before doing that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I don't understand enough about the simulators to comment on this. We can talk about it.