Skip to content

Commit

Permalink
chore: Wire bb skip cleanup for bb prover (#9100)
Browse files Browse the repository at this point in the history
The BB_SKIP_CLEANUP flag was not being set in prover-client tests, and
it was also not being wired in the prover client config, used for the
prover agent, meaning we would not be able to preserve bb working dir in
actual prover agent runs.

Requested by @jeanmon
  • Loading branch information
spalladino authored Oct 9, 2024
1 parent c1150c9 commit bba5674
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 23 deletions.
2 changes: 1 addition & 1 deletion yarn-project/bb-prover/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ export interface BBConfig {
bbBinaryPath: string;
bbWorkingDirectory: string;
/** Whether to skip tmp dir cleanup for debugging purposes */
bbSkipCleanup?: boolean;
bbSkipCleanup: boolean;
}

export interface ACVMConfig {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,7 @@ describe('proof_verification', () => {
const bb = await getBBConfig(logger);
const acvm = await getACVMConfig(logger);

circuitVerifier = await BBCircuitVerifier.new({
bbBinaryPath: bb!.bbBinaryPath,
bbWorkingDirectory: bb!.bbWorkingDirectory,
});
circuitVerifier = await BBCircuitVerifier.new(bb!);

bbTeardown = bb!.cleanup;
acvmTeardown = acvm!.cleanup;
Expand Down
10 changes: 7 additions & 3 deletions yarn-project/end-to-end/src/fixtures/get_bb_config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { type DebugLogger, fileURLToPath } from '@aztec/aztec.js';
import { type BBConfig } from '@aztec/bb-prover';

import fs from 'node:fs/promises';
import { tmpdir } from 'node:os';
Expand All @@ -7,13 +8,14 @@ import path from 'path';
const {
BB_RELEASE_DIR = 'barretenberg/cpp/build/bin',
BB_BINARY_PATH,
BB_SKIP_CLEANUP = '',
TEMP_DIR = tmpdir(),
BB_WORKING_DIRECTORY = '',
} = process.env;

export const getBBConfig = async (
logger: DebugLogger,
): Promise<{ bbBinaryPath: string; bbWorkingDirectory: string; cleanup: () => Promise<void> } | undefined> => {
): Promise<(BBConfig & { cleanup: () => Promise<void> }) | undefined> => {
try {
const bbBinaryPath =
BB_BINARY_PATH ??
Expand All @@ -32,13 +34,15 @@ export const getBBConfig = async (

await fs.mkdir(bbWorkingDirectory, { recursive: true });

const bbSkipCleanup = ['1', 'true'].includes(BB_SKIP_CLEANUP);

const cleanup = async () => {
if (directoryToCleanup) {
if (directoryToCleanup && !bbSkipCleanup) {
await fs.rm(directoryToCleanup, { recursive: true, force: true });
}
};

return { bbBinaryPath, bbWorkingDirectory, cleanup };
return { bbSkipCleanup, bbBinaryPath, bbWorkingDirectory, cleanup };
} catch (err) {
logger.error(`Native BB not available, error: ${err}`);
return undefined;
Expand Down
29 changes: 16 additions & 13 deletions yarn-project/prover-client/src/config.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
import { type BBConfig } from '@aztec/bb-prover';
import { type ProverConfig, proverConfigMappings } from '@aztec/circuit-types';
import { type ConfigMappingsType, getConfigFromMappings } from '@aztec/foundation/config';
import { type ConfigMappingsType, booleanConfigHelper, getConfigFromMappings } from '@aztec/foundation/config';

/**
* The prover configuration.
*/
export type ProverClientConfig = ProverConfig & {
/** The URL to the Aztec prover node to take proving jobs from */
proverJobSourceUrl?: string;
/** The working directory to use for simulation/proving */
acvmWorkingDirectory: string;
/** The path to the ACVM binary */
acvmBinaryPath: string;
/** The working directory to for proving */
bbWorkingDirectory: string;
/** The path to the bb binary */
bbBinaryPath: string;
};
export type ProverClientConfig = ProverConfig &
BBConfig & {
/** The URL to the Aztec prover node to take proving jobs from */
proverJobSourceUrl?: string;
/** The working directory to use for simulation/proving */
acvmWorkingDirectory: string;
/** The path to the ACVM binary */
acvmBinaryPath: string;
};

export const proverClientConfigMappings: ConfigMappingsType<ProverClientConfig> = {
proverJobSourceUrl: {
Expand All @@ -38,6 +36,11 @@ export const proverClientConfigMappings: ConfigMappingsType<ProverClientConfig>
env: 'BB_BINARY_PATH',
description: 'The path to the bb binary',
},
bbSkipCleanup: {
env: 'BB_SKIP_CLEANUP',
description: 'Whether to skip cleanup of bb temporary files',
...booleanConfigHelper(false),
},
...proverConfigMappings,
};

Expand Down
6 changes: 6 additions & 0 deletions yarn-project/prover-client/src/mocks/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const {
TEMP_DIR = '/tmp',
BB_BINARY_PATH = '',
BB_WORKING_DIRECTORY = '',
BB_SKIP_CLEANUP = '',
NOIR_RELEASE_DIR = 'noir-repo/target/release',
ACVM_BINARY_PATH = '',
ACVM_WORKING_DIRECTORY = '',
Expand All @@ -58,12 +59,17 @@ export const getEnvironmentConfig = async (logger: DebugLogger) => {
const acvmWorkingDirectory = ACVM_WORKING_DIRECTORY ? ACVM_WORKING_DIRECTORY : `${tempWorkingDirectory}/acvm`;
await fs.mkdir(acvmWorkingDirectory, { recursive: true });
logger.verbose(`Using native ACVM binary at ${expectedAcvmPath} with working directory ${acvmWorkingDirectory}`);

const bbSkipCleanup = ['1', 'true'].includes(BB_SKIP_CLEANUP);
bbSkipCleanup && logger.verbose(`Not going to clean up BB working directory ${bbWorkingDirectory} after run`);

return {
acvmWorkingDirectory,
bbWorkingDirectory,
expectedAcvmPath,
expectedBBPath,
directoryToCleanup: ACVM_WORKING_DIRECTORY && BB_WORKING_DIRECTORY ? undefined : tempWorkingDirectory,
bbSkipCleanup,
};
} catch (err) {
logger.verbose(`Native BB not available, error: ${err}`);
Expand Down
3 changes: 2 additions & 1 deletion yarn-project/prover-client/src/mocks/test_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,12 @@ export class TestContext {
acvmWorkingDirectory: config.acvmWorkingDirectory,
bbBinaryPath: config.expectedBBPath,
bbWorkingDirectory: config.bbWorkingDirectory,
bbSkipCleanup: config.bbSkipCleanup,
};
localProver = await createProver(bbConfig);
}

if (config?.directoryToCleanup) {
if (config?.directoryToCleanup && !config.bbSkipCleanup) {
directoriesToCleanup.push(config.directoryToCleanup);
}

Expand Down
2 changes: 1 addition & 1 deletion yarn-project/pxe/src/pxe_service/create_pxe_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,5 @@ function createProver(config: PXEServiceConfig, logSuffix?: string) {
}
const bbConfig = config as Required<Pick<PXEServiceConfig, 'bbBinaryPath' | 'bbWorkingDirectory'>> & PXEServiceConfig;
const log = createDebugLogger('aztec:pxe:bb-native-prover' + (logSuffix ? `:${logSuffix}` : ''));
return BBNativePrivateKernelProver.new(bbConfig, log);
return BBNativePrivateKernelProver.new({ bbSkipCleanup: false, ...bbConfig }, log);
}

0 comments on commit bba5674

Please sign in to comment.