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

fix(avm-simulator): L1TOL2MESSAGEEXISTS opcode #5807

Merged
merged 1 commit into from
Apr 17, 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
5 changes: 5 additions & 0 deletions yarn-project/pxe/src/simulator_oracle/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,11 @@ export class SimulatorOracle implements DBOracle {
return new MessageLoadOracleInputs(messageIndex, siblingPath);
}

// Only used in public.
public getL1ToL2LeafValue(_leafIndex: bigint): Promise<Fr | undefined> {
throw new Error('Unimplemented in private!');
}

/**
* Gets the index of a commitment in the note hash tree.
* @param commitment - The commitment.
Expand Down
5 changes: 1 addition & 4 deletions yarn-project/simulator/src/avm/avm_simulator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {
initContext,
initExecutionEnvironment,
initGlobalVariables,
initL1ToL2MessageOracleInput,
initMachineState,
randomMemoryBytes,
randomMemoryFields,
Expand Down Expand Up @@ -484,9 +483,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
const calldata = [msgHash, leafIndex];

const context = initContext({ env: initExecutionEnvironment({ calldata }) });
jest
.spyOn(context.persistableState.hostStorage.commitmentsDb, 'getL1ToL2MembershipWitness')
.mockResolvedValue(initL1ToL2MessageOracleInput(leafIndex.toBigInt()));
jest.spyOn(context.persistableState.hostStorage.commitmentsDb, 'getL1ToL2LeafValue').mockResolvedValue(msgHash);
const bytecode = getAvmTestContractBytecode('l1_to_l2_msg_exists');
const results = await new AvmSimulator(context).executeBytecode(bytecode);

Expand Down
22 changes: 2 additions & 20 deletions yarn-project/simulator/src/avm/fixtures/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { SiblingPath } from '@aztec/circuit-types';
import { GasFees, GasSettings, GlobalVariables, Header, L1_TO_L2_MSG_TREE_HEIGHT } from '@aztec/circuits.js';
import { GasFees, GasSettings, GlobalVariables, Header } from '@aztec/circuits.js';
import { FunctionSelector } from '@aztec/foundation/abi';
import { AztecAddress } from '@aztec/foundation/aztec-address';
import { EthAddress } from '@aztec/foundation/eth-address';
Expand All @@ -8,12 +7,7 @@ import { Fr } from '@aztec/foundation/fields';
import { mock } from 'jest-mock-extended';
import merge from 'lodash.merge';

import {
type CommitmentsDB,
MessageLoadOracleInputs,
type PublicContractsDB,
type PublicStateDB,
} from '../../index.js';
import { type CommitmentsDB, type PublicContractsDB, type PublicStateDB } from '../../index.js';
import { AvmContext } from '../avm_context.js';
import { AvmContextInputs, AvmExecutionEnvironment } from '../avm_execution_environment.js';
import { AvmMachineState } from '../avm_machine_state.js';
Expand Down Expand Up @@ -111,18 +105,6 @@ export function allSameExcept(original: any, overrides: any): any {
return merge({}, original, overrides);
}

/**
* Create an empty L1ToL2Message oracle input
*/
export function initL1ToL2MessageOracleInput(
leafIndex?: bigint,
): MessageLoadOracleInputs<typeof L1_TO_L2_MSG_TREE_HEIGHT> {
return new MessageLoadOracleInputs<typeof L1_TO_L2_MSG_TREE_HEIGHT>(
leafIndex ?? 0n,
new SiblingPath(L1_TO_L2_MSG_TREE_HEIGHT, Array(L1_TO_L2_MSG_TREE_HEIGHT)),
);
}

/**
* Adjust the user index to account for the AvmContextInputs size.
* This is a hack for testing, and should go away once AvmContextInputs themselves go away.
Expand Down
23 changes: 11 additions & 12 deletions yarn-project/simulator/src/avm/journal/journal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { Fr } from '@aztec/foundation/fields';
import { type MockProxy, mock } from 'jest-mock-extended';

import { type CommitmentsDB, type PublicContractsDB, type PublicStateDB } from '../../index.js';
import { initL1ToL2MessageOracleInput } from '../fixtures/index.js';
import { HostStorage } from './host_storage.js';
import { AvmPersistableStateManager, type JournalData } from './journal.js';

Expand Down Expand Up @@ -114,28 +113,28 @@ describe('journal', () => {
]);
});
it('checkL1ToL2MessageExists works for missing message', async () => {
const utxo = new Fr(2);
const msgHash = new Fr(2);
const leafIndex = new Fr(42);

const exists = await journal.checkL1ToL2MessageExists(utxo, leafIndex);
const exists = await journal.checkL1ToL2MessageExists(msgHash, leafIndex);
expect(exists).toEqual(false);

const journalUpdates = journal.flush();
expect(journalUpdates.l1ToL2MessageChecks).toEqual([
expect.objectContaining({ leafIndex: leafIndex, msgHash: utxo, exists: false }),
expect.objectContaining({ leafIndex: leafIndex, msgHash, exists: false }),
]);
});
it('checkL1ToL2MessageExists works for existing nullifiers', async () => {
const utxo = new Fr(2);
it('checkL1ToL2MessageExists works for existing msgHash', async () => {
const msgHash = new Fr(2);
const leafIndex = new Fr(42);

commitmentsDb.getL1ToL2MembershipWitness.mockResolvedValue(initL1ToL2MessageOracleInput(leafIndex.toBigInt()));
const exists = await journal.checkL1ToL2MessageExists(utxo, leafIndex);
commitmentsDb.getL1ToL2LeafValue.mockResolvedValue(msgHash);
const exists = await journal.checkL1ToL2MessageExists(msgHash, leafIndex);
expect(exists).toEqual(true);

const journalUpdates = journal.flush();
expect(journalUpdates.l1ToL2MessageChecks).toEqual([
expect.objectContaining({ leafIndex: leafIndex, msgHash: utxo, exists: true }),
expect.objectContaining({ leafIndex: leafIndex, msgHash, exists: true }),
]);
});
it('Should maintain nullifiers', async () => {
Expand All @@ -150,11 +149,11 @@ describe('journal', () => {
});
it('Should maintain l1 messages', () => {
const recipient = EthAddress.fromField(new Fr(1));
const utxo = new Fr(2);
journal.writeL1Message(recipient, utxo);
const msgHash = new Fr(2);
journal.writeL1Message(recipient, msgHash);

const journalUpdates = journal.flush();
expect(journalUpdates.newL1Messages).toEqual([{ recipient, content: utxo }]);
expect(journalUpdates.newL1Messages).toEqual([{ recipient, content: msgHash }]);
});
});

Expand Down
23 changes: 5 additions & 18 deletions yarn-project/simulator/src/avm/journal/journal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,24 +172,11 @@ export class AvmPersistableStateManager {
* @returns exists - whether the message exists in the L1 to L2 Messages tree
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is this opcode supposed to handle nullified messages? See comment in #5805

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned, I don't think exists should necessarily take into account if it is "active", just that it is in the tree. Separately, you might not be able to even know if it is nullified because you might not know the secret 🤷

*/
public async checkL1ToL2MessageExists(msgHash: Fr, msgLeafIndex: Fr): Promise<boolean> {
let exists = false;
try {
// The following 2 values are used to compute a message nullifier. Given that here we do not care about getting
// non-nullified messages we can just pass in random values and the nullifier check will effectively be ignored
// (no nullifier will be found).
const ignoredContractAddress = AztecAddress.random();
const ignoredSecret = Fr.random();
const gotMessage = await this.hostStorage.commitmentsDb.getL1ToL2MembershipWitness(
ignoredContractAddress,
msgHash,
ignoredSecret,
);
exists = gotMessage !== undefined && gotMessage.index == msgLeafIndex.toBigInt();
} catch {
// error getting message - doesn't exist!
exists = false;
}
this.log.debug(`l1ToL2Messages(${msgHash})@${msgLeafIndex} ?? exists: ${exists}.`);
const valueAtIndex = await this.hostStorage.commitmentsDb.getL1ToL2LeafValue(msgLeafIndex.toBigInt());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the value what we want from the tree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Who is this comment for? Should be the same value as for the membership proof so seems fine yes?

This is essentially the same as should be checked in AVM?

const exists = valueAtIndex?.equals(msgHash) ?? false;
this.log.debug(
`l1ToL2Messages(@${msgLeafIndex}) ?? exists: ${exists}, expected: ${msgHash}, found: ${valueAtIndex}.`,
);
this.trace.traceL1ToL2MessageCheck(msgHash, msgLeafIndex, exists);
return Promise.resolve(exists);
}
Expand Down
36 changes: 28 additions & 8 deletions yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,7 @@ import { type CommitmentsDB } from '../../index.js';
import { type AvmContext } from '../avm_context.js';
import { Field, Uint8 } from '../avm_memory_types.js';
import { InstructionExecutionError } from '../errors.js';
import {
initContext,
initExecutionEnvironment,
initHostStorage,
initL1ToL2MessageOracleInput,
} from '../fixtures/index.js';
import { initContext, initExecutionEnvironment, initHostStorage } from '../fixtures/index.js';
import { AvmPersistableStateManager } from '../journal/journal.js';
import {
EmitNoteHash,
Expand Down Expand Up @@ -348,15 +343,14 @@ describe('Accrued Substate', () => {

// mock commitments db to show message exists
const commitmentsDb = mock<CommitmentsDB>();
commitmentsDb.getL1ToL2MembershipWitness.mockResolvedValue(initL1ToL2MessageOracleInput(leafIndex.toBigInt()));
commitmentsDb.getL1ToL2LeafValue.mockResolvedValue(msgHash.toFr());
const hostStorage = initHostStorage({ commitmentsDb });
context = initContext({ persistableState: new AvmPersistableStateManager(hostStorage) });

context.machineState.memory.set(msgHashOffset, msgHash);
context.machineState.memory.set(msgLeafIndexOffset, leafIndex);
await new L1ToL2MessageExists(/*indirect=*/ 0, msgHashOffset, msgLeafIndexOffset, existsOffset).execute(context);

// never created, doesn't exist!
const exists = context.machineState.memory.getAs<Uint8>(existsOffset);
expect(exists).toEqual(new Uint8(1));

Expand All @@ -365,6 +359,32 @@ describe('Accrued Substate', () => {
expect.objectContaining({ leafIndex: leafIndex.toFr(), msgHash: msgHash.toFr(), exists: true }),
]);
});

it('Should correctly show false when another L1ToL2 message exists at that index', async () => {
const msgHash = new Field(69n);
const leafIndex = new Field(42n);
const msgHashOffset = 0;
const msgLeafIndexOffset = 1;
const existsOffset = 2;

const commitmentsDb = mock<CommitmentsDB>();
commitmentsDb.getL1ToL2LeafValue.mockResolvedValue(Fr.ZERO);
const hostStorage = initHostStorage({ commitmentsDb });
context = initContext({ persistableState: new AvmPersistableStateManager(hostStorage) });

context.machineState.memory.set(msgHashOffset, msgHash);
context.machineState.memory.set(msgLeafIndexOffset, leafIndex);
await new L1ToL2MessageExists(/*indirect=*/ 0, msgHashOffset, msgLeafIndexOffset, existsOffset).execute(context);

// never created, doesn't exist!
const exists = context.machineState.memory.getAs<Uint8>(existsOffset);
expect(exists).toEqual(new Uint8(0));

const journalState = context.persistableState.flush();
expect(journalState.l1ToL2MessageChecks).toEqual([
expect.objectContaining({ leafIndex: leafIndex.toFr(), msgHash: msgHash.toFr(), exists: false }),
]);
});
});

describe('EmitUnencryptedLog', () => {
Expand Down
6 changes: 6 additions & 0 deletions yarn-project/simulator/src/public/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ export interface CommitmentsDB {
secret: Fr,
): Promise<MessageLoadOracleInputs<typeof L1_TO_L2_MSG_TREE_HEIGHT>>;

/**
* @param leafIndex the leaf to look up
* @returns The l1 to l2 leaf value or undefined if not found.
*/
getL1ToL2LeafValue(leafIndex: bigint): Promise<Fr | undefined>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of introducing getL1ToL2LeafValue, IIUC I could change getL1ToL2MembershipWitness to take a "startIndex". Then pass the expected index, and check that the index matches after return. This would be slower when it's not found.


/**
* Gets the index of a commitment in the note hash tree.
* @param commitment - The commitment.
Expand Down
4 changes: 4 additions & 0 deletions yarn-project/simulator/src/public/public_executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,10 @@ export class WorldStateDB implements CommitmentsDB {
return new MessageLoadOracleInputs<typeof L1_TO_L2_MSG_TREE_HEIGHT>(messageIndex, siblingPath);
}

public async getL1ToL2LeafValue(leafIndex: bigint): Promise<Fr | undefined> {
return await this.db.getLeafValue(MerkleTreeId.L1_TO_L2_MESSAGE_TREE, leafIndex);
}

public async getCommitmentIndex(commitment: Fr): Promise<bigint | undefined> {
return await this.db.findLeafIndex(MerkleTreeId.NOTE_HASH_TREE, commitment);
}
Expand Down
Loading