Skip to content

feat: remove event selector in logs from public context #7192

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.

Sign up for GitHub

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

Merged
sklppy88 merged 1 commit into from
Jun 26, 2024
Merged

feat: remove event selector in logs from public context #7192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 6 additions & 13 deletions avm-transpiler/src/transpile.rs
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@fcarreiro fcarreiro Jun 26, 2024

Choose a reason for hiding this comment

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

To know if the changes here (and in the public context) are correct, I recommend recompiling and retranspiling avm_test_contract, then running the avm_simulator.test.ts in yarn-packages. That test runs the simulator on compiled Noir contracts, so you'd be exercising this.

Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ fn handle_foreign_call(
"avmOpcodeStaticCall" => {
handle_external_call(avm_instrs, destinations, inputs, AvmOpcode::STATICCALL);
}
"amvOpcodeEmitUnencryptedLog" => {
"avmOpcodeEmitUnencryptedLog" => {
Copy link
Copy Markdown
Contributor

@fcarreiro fcarreiro Jun 26, 2024

Choose a reason for hiding this comment

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

Thanks for the fix!

handle_emit_unencrypted_log(avm_instrs, destinations, inputs);
}
"avmOpcodeNoteHashExists" => handle_note_hash_exists(avm_instrs, destinations, inputs),
Expand Down Expand Up @@ -435,32 +435,25 @@ fn handle_emit_unencrypted_log(
destinations: &Vec<ValueOrArray>,
inputs: &Vec<ValueOrArray>,
) {
if !destinations.is_empty() || inputs.len() != 3 {
if !destinations.is_empty() || inputs.len() != 2 {
panic!(
"Transpiler expects ForeignCall::EMITUNENCRYPTEDLOG to have 0 destinations and 3 inputs, got {} and {}",
"Transpiler expects ForeignCall::EMITUNENCRYPTEDLOG to have 0 destinations and 2 inputs, got {} and {}",
destinations.len(),
inputs.len()
);
}
let event_offset = match &inputs[0] {
ValueOrArray::MemoryAddress(offset) => offset.to_usize() as u32,
_ => panic!(
"Unexpected inputs[0] (event) for ForeignCall::EMITUNENCRYPTEDLOG: {:?}",
inputs[0]
),
};

// The fields are a slice, and this is represented as a (length: Field, slice: HeapVector).
// The length field is redundant and we skipt it.
let (message_offset, message_size_offset) = match &inputs[2] {
let (message_offset, message_size_offset) = match &inputs[1] {
ValueOrArray::HeapVector(vec) => (vec.pointer.to_usize() as u32, vec.size.0 as u32),
_ => panic!("Unexpected inputs for ForeignCall::EMITUNENCRYPTEDLOG: {:?}", inputs),
};
avm_instrs.push(AvmInstruction {
opcode: AvmOpcode::EMITUNENCRYPTEDLOG,
// The message array from Brillig is indirect.
indirect: Some(FIRST_OPERAND_INDIRECT),
indirect: Some(ZEROTH_OPERAND_INDIRECT),
operands: vec![
AvmOperand::U32 { value: event_offset },
AvmOperand::U32 { value: message_offset },
AvmOperand::U32 { value: message_size_offset },
],
Expand Down
20 changes: 6 additions & 14 deletions noir-projects/aztec-nr/aztec/src/context/public_context.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,13 @@ impl PublicContext {
}
/**
* Emit a log with the given event selector and message.
*
* @param event_selector The event selector for the log.
* @param message The message to emit in the log.
*/
pub fn emit_unencrypted_log_with_selector<T, N>(
&mut self,
event_selector: Field,
log: T
) where T: Serialize<N> {
emit_unencrypted_log(event_selector, Serialize::serialize(log).as_slice());
}
// For compatibility with the selector-less API. We'll probably rename the above one.
pub fn emit_unencrypted_log<T, N>(&mut self, log: T) where T: Serialize<N> {
self.emit_unencrypted_log_with_selector(/*event_selector=*/ 5, log);
emit_unencrypted_log(Serialize::serialize(log).as_slice());
}

pub fn note_hash_exists(self, note_hash: Field, leaf_index: Field) -> bool {
note_hash_exists(note_hash, leaf_index) == 1
}
Expand Down Expand Up @@ -241,8 +233,8 @@ unconstrained fn nullifier_exists(nullifier: Field, address: Field) -> u8 {
unconstrained fn emit_nullifier(nullifier: Field) {
emit_nullifier_opcode(nullifier)
}
unconstrained fn emit_unencrypted_log(event_selector: Field, message: [Field]) {
emit_unencrypted_log_opcode(event_selector, message)
unconstrained fn emit_unencrypted_log(message: [Field]) {
emit_unencrypted_log_opcode(message)
}
unconstrained fn l1_to_l2_msg_exists(msg_hash: Field, msg_leaf_index: Field) -> u8 {
l1_to_l2_msg_exists_opcode(msg_hash, msg_leaf_index)
Expand Down Expand Up @@ -325,8 +317,8 @@ unconstrained fn nullifier_exists_opcode(nullifier: Field, address: Field) -> u8
#[oracle(avmOpcodeEmitNullifier)]
unconstrained fn emit_nullifier_opcode(nullifier: Field) {}

#[oracle(amvOpcodeEmitUnencryptedLog)]
unconstrained fn emit_unencrypted_log_opcode(event_selector: Field, message: [Field]) {}
#[oracle(avmOpcodeEmitUnencryptedLog)]
unconstrained fn emit_unencrypted_log_opcode(message: [Field]) {}

#[oracle(avmOpcodeL1ToL2MsgExists)]
unconstrained fn l1_to_l2_msg_exists_opcode(msg_hash: Field, msg_leaf_index: Field) -> u8 {}
Expand Down
7 changes: 3 additions & 4 deletions yarn-project/simulator/src/avm/avm_simulator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,6 @@ describe('AVM simulator: transpiled Noir contracts', () => {
const results = await new AvmSimulator(context).executeBytecode(bytecode);
expect(results.reverted).toBe(false);

const eventSelector = new Fr(5);
const expectedFields = [new Fr(10), new Fr(20), new Fr(30)];
const expectedString = 'Hello, world!'.split('').map(c => new Fr(c.charCodeAt(0)));
const expectedCompressedString = [
Expand All @@ -567,9 +566,9 @@ describe('AVM simulator: transpiled Noir contracts', () => {
].map(s => new Fr(Buffer.from(s)));

expect(trace.traceUnencryptedLog).toHaveBeenCalledTimes(3);
expect(trace.traceUnencryptedLog).toHaveBeenCalledWith(address, eventSelector, expectedFields);
expect(trace.traceUnencryptedLog).toHaveBeenCalledWith(address, eventSelector, expectedString);
expect(trace.traceUnencryptedLog).toHaveBeenCalledWith(address, eventSelector, expectedCompressedString);
expect(trace.traceUnencryptedLog).toHaveBeenCalledWith(address, expectedFields);
expect(trace.traceUnencryptedLog).toHaveBeenCalledWith(address, expectedString);
expect(trace.traceUnencryptedLog).toHaveBeenCalledWith(address, expectedCompressedString);
});
});

Expand Down
6 changes: 3 additions & 3 deletions yarn-project/simulator/src/avm/journal/journal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,9 @@ export class AvmPersistableStateManager {
* @param event - log event selector
* @param log - log contents
*/
public writeUnencryptedLog(contractAddress: Fr, event: Fr, log: Fr[]) {
this.log.debug(`UnencryptedL2Log(${contractAddress}) += event ${event} with ${log.length} fields.`);
this.trace.traceUnencryptedLog(contractAddress, event, log);
public writeUnencryptedLog(contractAddress: Fr, log: Fr[]) {
this.log.debug(`UnencryptedL2Log(${contractAddress}) += event with ${log.length} fields.`);
this.trace.traceUnencryptedLog(contractAddress, log);
}

/**
Expand Down
22 changes: 4 additions & 18 deletions yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,44 +305,30 @@ describe('Accrued Substate', () => {
const buf = Buffer.from([
EmitUnencryptedLog.opcode, // opcode
0x01, // indirect
...Buffer.from('02345678', 'hex'), // event selector offset
...Buffer.from('12345678', 'hex'), // log offset
...Buffer.from('a2345678', 'hex'), // length offset
]);
const inst = new EmitUnencryptedLog(
/*indirect=*/ 0x01,
/*eventSelectorOffset=*/ 0x02345678,
/*offset=*/ 0x12345678,
/*lengthOffset=*/ 0xa2345678,
);
const inst = new EmitUnencryptedLog(/*indirect=*/ 0x01, /*offset=*/ 0x12345678, /*lengthOffset=*/ 0xa2345678);

expect(EmitUnencryptedLog.deserialize(buf)).toEqual(inst);
expect(inst.serialize()).toEqual(buf);
});

it('Should append unencrypted logs correctly', async () => {
const startOffset = 0;
const eventSelector = new Fr(5);
const eventSelectorOffset = 10;
const logSizeOffset = 20;

const values = [new Fr(69n), new Fr(420n), new Fr(Fr.MODULUS - 1n)];
context.machineState.memory.setSlice(
startOffset,
values.map(f => new Field(f)),
);
context.machineState.memory.set(eventSelectorOffset, new Field(eventSelector));
context.machineState.memory.set(logSizeOffset, new Uint32(values.length));

await new EmitUnencryptedLog(
/*indirect=*/ 0,
eventSelectorOffset,
/*offset=*/ startOffset,
logSizeOffset,
).execute(context);
await new EmitUnencryptedLog(/*indirect=*/ 0, /*offset=*/ startOffset, logSizeOffset).execute(context);

expect(trace.traceUnencryptedLog).toHaveBeenCalledTimes(1);
expect(trace.traceUnencryptedLog).toHaveBeenCalledWith(address, eventSelector, values);
expect(trace.traceUnencryptedLog).toHaveBeenCalledWith(address, values);
});
});

Expand Down Expand Up @@ -386,7 +372,7 @@ describe('Accrued Substate', () => {
const instructions = [
new EmitNoteHash(/*indirect=*/ 0, /*offset=*/ 0),
new EmitNullifier(/*indirect=*/ 0, /*offset=*/ 0),
new EmitUnencryptedLog(/*indirect=*/ 0, /*eventSelector=*/ 0, /*offset=*/ 0, /*logSizeOffset=*/ 0),
new EmitUnencryptedLog(/*indirect=*/ 0, /*offset=*/ 0, /*logSizeOffset=*/ 0),
new SendL2ToL1Message(/*indirect=*/ 0, /*recipientOffset=*/ 0, /*contentOffset=*/ 1),
];

Expand Down
25 changes: 6 additions & 19 deletions yarn-project/simulator/src/avm/opcodes/accrued_substate.ts
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@fcarreiro fcarreiro Jun 26, 2024

Choose a reason for hiding this comment

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

To know if your changes here are correct, I recommend running the unit tests for the opcodes. LMK if you run into anything weird.

Original file line number Diff line number Diff line change
Expand Up @@ -217,20 +217,9 @@ export class EmitUnencryptedLog extends Instruction {
static type: string = 'EMITUNENCRYPTEDLOG';
static readonly opcode: Opcode = Opcode.EMITUNENCRYPTEDLOG;
// Informs (de)serialization. See Instruction.deserialize.
static readonly wireFormat = [
OperandType.UINT8,
OperandType.UINT8,
OperandType.UINT32,
OperandType.UINT32,
OperandType.UINT32,
];
static readonly wireFormat = [OperandType.UINT8, OperandType.UINT8, OperandType.UINT32, OperandType.UINT32];

constructor(
private indirect: number,
private eventSelectorOffset: number,
private logOffset: number,
private logSizeOffset: number,
) {
constructor(private indirect: number, private logOffset: number, private logSizeOffset: number) {
super();
}

Expand All @@ -241,22 +230,20 @@ export class EmitUnencryptedLog extends Instruction {

const memory = context.machineState.memory.track(this.type);

const [eventSelectorOffset, logOffset, logSizeOffset] = Addressing.fromWire(this.indirect).resolve(
[this.eventSelectorOffset, this.logOffset, this.logSizeOffset],
const [logOffset, logSizeOffset] = Addressing.fromWire(this.indirect).resolve(
[this.logOffset, this.logSizeOffset],
memory,
);
memory.checkTag(TypeTag.FIELD, eventSelectorOffset);
memory.checkTag(TypeTag.UINT32, logSizeOffset);
const logSize = memory.get(logSizeOffset).toNumber();
memory.checkTagsRange(TypeTag.FIELD, logOffset, logSize);

const contractAddress = context.environment.address;
const event = memory.get(eventSelectorOffset).toFr();

const memoryOperations = { reads: 2 + logSize, indirect: this.indirect };
const memoryOperations = { reads: 1 + logSize, indirect: this.indirect };
context.machineState.consumeGas(this.gasCost(memoryOperations));
const log = memory.getSlice(logOffset, logSize).map(f => f.toFr());
context.persistableState.writeUnencryptedLog(contractAddress, event, log);
context.persistableState.writeUnencryptedLog(contractAddress, log);

memory.assert(memoryOperations);
context.machineState.incrementPc();
Expand Down
8 changes: 3 additions & 5 deletions yarn-project/simulator/src/public/side_effect_trace.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { SerializableContractInstance } from '@aztec/types/contracts';

import { randomBytes, randomInt } from 'crypto';

import { Selector } from '../../../foundation/src/abi/selector.js';
import { AvmContractCallResults } from '../avm/avm_message_call_result.js';
import { initExecutionEnvironment } from '../avm/fixtures/index.js';
import { PublicSideEffectTrace, type TracedContractInstance } from './side_effect_trace.js';
Expand All @@ -25,7 +24,6 @@ describe('Side Effect Trace', () => {
const value = Fr.random();
const recipient = Fr.random();
const content = Fr.random();
const event = new Fr(randomBytes(Selector.SIZE).readUint32BE());
const log = [Fr.random(), Fr.random(), Fr.random()];

const startGasLeft = Gas.fromFields([new Fr(randomInt(10000)), new Fr(randomInt(10000))]);
Expand Down Expand Up @@ -205,13 +203,13 @@ describe('Side Effect Trace', () => {
});

it('Should trace new unencrypted logs', () => {
trace.traceUnencryptedLog(address, event, log);
trace.traceUnencryptedLog(address, log);
expect(trace.getCounter()).toBe(startCounterPlus1);

const pxResult = toPxResult(trace);
const expectLog = new UnencryptedL2Log(
AztecAddress.fromField(address),
EventSelector.fromField(event),
EventSelector.fromField(new Fr(0)),
Buffer.concat(log.map(f => f.toBuffer())),
);
expect(pxResult.unencryptedLogs.logs).toEqual([expectLog]);
Expand Down Expand Up @@ -266,7 +264,7 @@ describe('Side Effect Trace', () => {
testCounter++;
nestedTrace.traceNewL2ToL1Message(recipient, content);
testCounter++;
nestedTrace.traceUnencryptedLog(address, event, log);
nestedTrace.traceUnencryptedLog(address, log);
testCounter++;

trace.traceNestedCall(nestedTrace, avmEnvironment, startGasLeft, endGasLeft, bytecode, avmCallResults);
Expand Down
5 changes: 3 additions & 2 deletions yarn-project/simulator/src/public/side_effect_trace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,12 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface {
this.incrementSideEffectCounter();
}

public traceUnencryptedLog(contractAddress: Fr, event: Fr, log: Fr[]) {
public traceUnencryptedLog(contractAddress: Fr, log: Fr[]) {
// TODO(4805): check if some threshold is reached for max logs
const ulog = new UnencryptedL2Log(
AztecAddress.fromField(contractAddress),
EventSelector.fromField(event),
// TODO(#7198): Remove event selector from UnencryptedL2Log
EventSelector.fromField(new Fr(0)),
Copy link
Copy Markdown
Contributor

@fcarreiro fcarreiro Jun 26, 2024

Choose a reason for hiding this comment

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

You can do it in a separate PR, but I guess if event selector is being dropped (ie not enshrined) then it would make sense to remove it from UnencryptedL2Log as well (which is I think what the kernel uses).

Copy link
Copy Markdown
Contributor Author

@sklppy88 sklppy88 Jun 26, 2024

Choose a reason for hiding this comment

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

Have made #7198 to deal with this !

Buffer.concat(log.map(f => f.toBuffer())),
);
const basicLogHash = Fr.fromBuffer(ulog.hash());
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/simulator/src/public/side_effect_trace_interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export interface PublicSideEffectTraceInterface {
traceL1ToL2MessageCheck(contractAddress: Fr, msgHash: Fr, msgLeafIndex: Fr, exists: boolean): void;
// TODO(dbanks12): should new message accept contract address as arg?
traceNewL2ToL1Message(recipient: Fr, content: Fr): void;
traceUnencryptedLog(contractAddress: Fr, event: Fr, log: Fr[]): void;
traceUnencryptedLog(contractAddress: Fr, log: Fr[]): void;
// TODO(dbanks12): odd that getContractInstance is a one-off in that it accepts an entire object instead of components
traceGetContractInstance(instance: TracedContractInstance): void;
traceNestedCall(
Expand Down