Skip to content
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

refactor: stop calling public kernels #9971

Merged
merged 2 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions yarn-project/aztec-node/src/aztec-node/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ import {
} from '@aztec/p2p';
import { ProtocolContractAddress } from '@aztec/protocol-contracts';
import { GlobalVariableBuilder, SequencerClient } from '@aztec/sequencer-client';
import { PublicProcessorFactory, WASMSimulator, createSimulationProvider } from '@aztec/simulator';
import { PublicProcessorFactory, createSimulationProvider } from '@aztec/simulator';
import { type TelemetryClient } from '@aztec/telemetry-client';
import { NoopTelemetryClient } from '@aztec/telemetry-client/noop';
import { createValidatorClient } from '@aztec/validator-client';
Expand Down Expand Up @@ -733,11 +733,7 @@ export class AztecNodeService implements AztecNode {
feeRecipient,
);
const prevHeader = (await this.blockSource.getBlock(-1))?.header;
const publicProcessorFactory = new PublicProcessorFactory(
this.contractDataSource,
new WASMSimulator(),
this.telemetry,
);
const publicProcessorFactory = new PublicProcessorFactory(this.contractDataSource, this.telemetry);

const fork = await this.worldStateSynchronizer.fork();

Expand Down
30 changes: 14 additions & 16 deletions yarn-project/circuit-types/src/tx/tx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import {
ClientIvcProof,
ContractClassRegisteredEvent,
PrivateKernelTailCircuitPublicInputs,
type PublicKernelCircuitPublicInputs,
type PrivateToPublicAccumulatedData,
type ScopedLogHash,
} from '@aztec/circuits.js';
import { type Buffer32 } from '@aztec/foundation/buffer';
import { arraySerializedSizeOfNonEmpty } from '@aztec/foundation/collection';
Expand Down Expand Up @@ -344,29 +345,26 @@ export class Tx extends Gossipable {
* @param logHashes the individual log hashes we want to keep
* @param out the output to put passing logs in, to keep this function abstract
*/
public filterRevertedLogs(kernelOutput: PublicKernelCircuitPublicInputs) {
public filterRevertedLogs(
privateNonRevertible: PrivateToPublicAccumulatedData,
unencryptedLogsHashes: ScopedLogHash[],
) {
this.encryptedLogs = this.encryptedLogs.filterScoped(
kernelOutput.endNonRevertibleData.encryptedLogsHashes,
privateNonRevertible.encryptedLogsHashes,
EncryptedTxL2Logs.empty(),
);

this.unencryptedLogs = this.unencryptedLogs.filterScoped(
kernelOutput.endNonRevertibleData.unencryptedLogsHashes,
UnencryptedTxL2Logs.empty(),
);

this.noteEncryptedLogs = this.noteEncryptedLogs.filter(
kernelOutput.endNonRevertibleData.noteEncryptedLogsHashes,
privateNonRevertible.noteEncryptedLogsHashes,
EncryptedNoteTxL2Logs.empty(),
);

// See comment in enqueued_calls_processor.ts -> tx.filterRevertedLogs()
if (this.data.forPublic) {
this.contractClassLogs = this.contractClassLogs.filterScoped(
this.data.forPublic?.nonRevertibleAccumulatedData.contractClassLogsHashes,
ContractClassTxL2Logs.empty(),
);
}
this.contractClassLogs = this.contractClassLogs.filterScoped(
privateNonRevertible.contractClassLogsHashes,
ContractClassTxL2Logs.empty(),
);

this.unencryptedLogs = this.unencryptedLogs.filterScoped(unencryptedLogsHashes, UnencryptedTxL2Logs.empty());
}
}

Expand Down
3 changes: 0 additions & 3 deletions yarn-project/prover-client/src/mocks/test_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
PublicExecutionResultBuilder,
type PublicExecutor,
PublicProcessor,
RealPublicKernelCircuitSimulator,
type SimulationProvider,
WASMSimulator,
type WorldStateDB,
Expand Down Expand Up @@ -69,7 +68,6 @@ export class TestContext {

const publicExecutor = mock<PublicExecutor>();
const worldStateDB = mock<WorldStateDB>();
const publicKernel = new RealPublicKernelCircuitSimulator(new WASMSimulator());
const telemetry = new NoopTelemetryClient();

// Separated dbs for public processor and prover - see public_processor for context
Expand All @@ -89,7 +87,6 @@ export class TestContext {
const processor = PublicProcessor.create(
publicDb,
publicExecutor,
publicKernel,
globalVariables,
Header.empty(),
worldStateDB,
Expand Down
8 changes: 2 additions & 6 deletions yarn-project/prover-node/src/prover-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export class ProverNode implements ClaimsMonitorHandler, EpochMonitorHandler, Pr
private readonly contractDataSource: ContractDataSource,
private readonly worldState: WorldStateSynchronizer,
private readonly coordination: ProverCoordination & Maybe<Service>,
private readonly simulator: SimulationProvider,
private readonly _simulator: SimulationProvider,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be marked as "to be deleted"?

private readonly quoteProvider: QuoteProvider,
private readonly quoteSigner: QuoteSigner,
private readonly claimsMonitor: ClaimsMonitor,
Expand Down Expand Up @@ -243,11 +243,7 @@ export class ProverNode implements ClaimsMonitorHandler, EpochMonitorHandler, Pr
const proverDb = await this.worldState.fork(fromBlock - 1);

// Create a processor using the forked world state
const publicProcessorFactory = new PublicProcessorFactory(
this.contractDataSource,
this.simulator,
this.telemetryClient,
);
const publicProcessorFactory = new PublicProcessorFactory(this.contractDataSource, this.telemetryClient);

const cleanUp = async () => {
await publicDb.close();
Expand Down
4 changes: 2 additions & 2 deletions yarn-project/sequencer-client/src/client/sequencer-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@ export class SequencerClient {
contractDataSource: ContractDataSource,
l2BlockSource: L2BlockSource,
l1ToL2MessageSource: L1ToL2MessageSource,
simulationProvider: SimulationProvider,
_simulationProvider: SimulationProvider,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Do we have plans to remove this? Or will it be used in another PR?

telemetryClient: TelemetryClient,
) {
const publisher = new L1Publisher(config, telemetryClient);
const globalsBuilder = new GlobalVariableBuilder(config);

const publicProcessorFactory = new PublicProcessorFactory(contractDataSource, simulationProvider, telemetryClient);
const publicProcessorFactory = new PublicProcessorFactory(contractDataSource, telemetryClient);

const rollup = publisher.getRollupContract();
const [l1GenesisTime, slotDuration] = await Promise.all([
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/simulator/src/avm/avm_simulator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
const contractClass = makeContractClassPublic(0, publicFn);
const contractInstance = makeContractInstanceFromClassId(contractClass.id);

// The values here should match those in `avm_simulator.test.ts`
// The values here should match those in getContractInstance test case
const instanceGet = new SerializableContractInstance({
version: 1,
salt: new Fr(0x123),
Expand Down
52 changes: 35 additions & 17 deletions yarn-project/simulator/src/avm/journal/journal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ export class AvmPersistableStateManager {
/** Interface to perform merkle tree operations */
public merkleTrees: MerkleTreeWriteOperations;

/** Make sure a forked state is never merged twice. */
private alreadyMergedIntoParent = false;

constructor(
/** Reference to node storage */
private readonly worldStateDB: WorldStateDB,
Expand Down Expand Up @@ -79,16 +82,46 @@ export class AvmPersistableStateManager {
/**
* Create a new state manager forked from this one
*/
public fork(incrementSideEffectCounter: boolean = false) {
public fork() {
return new AvmPersistableStateManager(
this.worldStateDB,
this.trace.fork(incrementSideEffectCounter),
this.trace.fork(),
this.publicStorage.fork(),
this.nullifiers.fork(),
this.doMerkleOperations,
);
}

/**
* Accept forked world state modifications & traced side effects / hints
*/
public merge(forkedState: AvmPersistableStateManager) {
this._merge(forkedState, /*reverted=*/ false);
}

/**
* Reject forked world state modifications & traced side effects, keep traced hints
*/
public reject(forkedState: AvmPersistableStateManager) {
this._merge(forkedState, /*reverted=*/ true);
}

/**
* Commit cached storage writes to the DB.
* Keeps public storage up to date from tx to tx within a block.
*/
public async commitStorageWritesToDB() {
await this.publicStorage.commitToDB();
}

private _merge(forkedState: AvmPersistableStateManager, reverted: boolean) {
// sanity check to avoid merging the same forked trace twice
assert(!this.alreadyMergedIntoParent, 'Cannot merge forked state that has already been merged into its parent!');
Comment on lines +118 to +119
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm so, this.merge merges forkedState INTO this, IIUC? Why does it matter if this. alreadyMergedIntoParent is true? Shouldn't we be checking for forkedState. alreadyMergedIntoParent ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where alreadyMergedIntoParent is set to true (for the journal itself, I see it for the trace).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parent doesn't keep track of its forks. And it could have children formed and merged multiple times.

We just don't want to let a specific fork get merged multiple times.

And good catch, I must've missed that in journal

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right. I was confused. It should check forkedState.alreadyMerged! Good catch.

this.publicStorage.acceptAndMerge(forkedState.publicStorage);
this.nullifiers.acceptAndMerge(forkedState.nullifiers);
this.trace.merge(forkedState.trace, reverted);
}

/**
* Write to public storage, journal/trace the write.
*
Expand Down Expand Up @@ -427,21 +460,6 @@ export class AvmPersistableStateManager {
}
}

/**
* Accept nested world state modifications
*/
public mergeForkedState(forkedState: AvmPersistableStateManager) {
this.publicStorage.acceptAndMerge(forkedState.publicStorage);
this.nullifiers.acceptAndMerge(forkedState.nullifiers);
this.trace.mergeSuccessfulForkedTrace(forkedState.trace);
}

public rejectForkedState(forkedState: AvmPersistableStateManager) {
this.publicStorage.acceptAndMerge(forkedState.publicStorage);
this.nullifiers.acceptAndMerge(forkedState.nullifiers);
this.trace.mergeRevertedForkedTrace(forkedState.trace);
}

/**
* Get a contract's bytecode from the contracts DB, also trace the contract class and instance
*/
Expand Down
6 changes: 4 additions & 2 deletions yarn-project/simulator/src/avm/opcodes/external_calls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,11 @@ abstract class ExternalCall extends Instruction {
// Refund unused gas
context.machineState.refundGas(gasLeftToGas(nestedContext.machineState));

// Accept the nested call's state and trace the nested call
// Merge nested call's state and trace based on whether it succeeded.
if (success) {
context.persistableState.mergeForkedState(nestedContext.persistableState);
context.persistableState.merge(nestedContext.persistableState);
} else {
context.persistableState.reject(nestedContext.persistableState);
}
await context.persistableState.traceNestedCall(
/*nestedState=*/ nestedContext.persistableState,
Expand Down
19 changes: 6 additions & 13 deletions yarn-project/simulator/src/public/dual_side_effect_trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ export class DualSideEffectTrace implements PublicSideEffectTraceInterface {
public readonly enqueuedCallTrace: PublicEnqueuedCallSideEffectTrace,
) {}

public fork(incrementSideEffectCounter: boolean = false) {
return new DualSideEffectTrace(
this.innerCallTrace.fork(incrementSideEffectCounter),
this.enqueuedCallTrace.fork(incrementSideEffectCounter),
);
public fork() {
return new DualSideEffectTrace(this.innerCallTrace.fork(), this.enqueuedCallTrace.fork());
}

public merge(nestedTrace: this, reverted: boolean = false) {
this.enqueuedCallTrace.merge(nestedTrace.enqueuedCallTrace, reverted);
}

public getCounter() {
Expand Down Expand Up @@ -232,14 +233,6 @@ export class DualSideEffectTrace implements PublicSideEffectTraceInterface {
this.enqueuedCallTrace.traceEnqueuedCall(publicCallRequest, calldata, reverted);
}

public mergeSuccessfulForkedTrace(nestedTrace: this) {
this.enqueuedCallTrace.mergeSuccessfulForkedTrace(nestedTrace.enqueuedCallTrace);
}

public mergeRevertedForkedTrace(nestedTrace: this) {
this.enqueuedCallTrace.mergeRevertedForkedTrace(nestedTrace.enqueuedCallTrace);
}

/**
* Convert this trace to a PublicExecutionResult for use externally to the simulator.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -508,11 +508,7 @@ describe('Enqueued-call Side Effect Trace', () => {
nestedTrace.traceGetContractInstance(address, /*exists=*/ false, contractInstance);
testCounter++;

if (reverted) {
trace.mergeRevertedForkedTrace(nestedTrace);
} else {
trace.mergeSuccessfulForkedTrace(nestedTrace);
}
trace.merge(nestedTrace, reverted);

// parent trace adopts nested call's counter
expect(trace.getCounter()).toBe(testCounter);
Expand Down
Loading