From aee4c2dde7576fad1c47e407ee0dca43dac2b1b4 Mon Sep 17 00:00:00 2001 From: Ilyas Ridhuan Date: Mon, 23 Sep 2024 13:21:13 +0100 Subject: [PATCH] fix: unencryptedlogs witgen (#8669) Please read [contributing guidelines](CONTRIBUTING.md) and remove this line. --- .../vm/avm/tests/execution.test.cpp | 23 +++---- .../barretenberg/vm/avm/trace/execution.cpp | 4 +- .../vm/avm/trace/kernel_trace.cpp | 7 ++- .../vm/avm/trace/kernel_trace.hpp | 10 +-- .../src/barretenberg/vm/avm/trace/trace.cpp | 62 +++++++++++++++++-- .../cpp/src/barretenberg/vm/constants.hpp | 6 +- .../contracts/avm_test_contract/src/main.nr | 2 +- .../src/logs/unencrypted_l2_log.ts | 5 +- .../simulator/src/public/side_effect_trace.ts | 6 +- 9 files changed, 91 insertions(+), 34 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp index e9d7fabaa67..37b39d528fa 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp @@ -1831,15 +1831,14 @@ TEST_F(AvmExecutionTests, ExecutorThrowsWithIncorrectNumberOfPublicInputs) TEST_F(AvmExecutionTests, kernelOutputEmitOpcodes) { // Set values into the first register to emit - std::string bytecode_hex = to_hex(OpCode::SET_8) + // opcode Set - "00" // Indirect flag - + to_hex(AvmMemoryTag::U32) + - "01" // value 1 - "01" // dst_offset 1 - // Cast set to field - + to_hex(OpCode::CAST_8) + // opcode CAST - "00" // Indirect flag - + to_hex(AvmMemoryTag::FF) + + std::string bytecode_hex = to_hex(OpCode::SET_8) + // opcode Set + "00" // Indirect flag + + to_hex(AvmMemoryTag::U32) + // tag U32 + "01" // value 1 + "01" // dst_offset 1 + + to_hex(OpCode::CAST_8) + // opcode CAST (to field) + "00" // Indirect flag + + to_hex(AvmMemoryTag::FF) + // tag FF "01" // dst 1 "01" // dst 1 + to_hex(OpCode::EMITNOTEHASH) + // opcode EMITNOTEHASH @@ -1902,13 +1901,15 @@ TEST_F(AvmExecutionTests, kernelOutputEmitOpcodes) // CHECK EMIT UNENCRYPTED LOG auto emit_log_row = std::ranges::find_if(trace.begin(), trace.end(), [](Row r) { return r.main_sel_op_emit_unencrypted_log == 1; }); - EXPECT_EQ(emit_log_row->main_ia, 1); + // Trust me bro for now, this is the truncated sha output + FF expected_hash = FF(std::string("0x006db65fd59fd356f6729140571b5bcd6bb3b83492a16e1bf0a3884442fc3c8a")); + EXPECT_EQ(emit_log_row->main_ia, expected_hash); EXPECT_EQ(emit_log_row->main_side_effect_counter, 2); uint32_t emit_log_out_offset = START_EMIT_UNENCRYPTED_LOG_WRITE_OFFSET; auto emit_log_kernel_out_row = std::ranges::find_if(trace.begin(), trace.end(), [&](Row r) { return r.main_clk == emit_log_out_offset; }); - EXPECT_EQ(emit_log_kernel_out_row->main_kernel_value_out, 1); + EXPECT_EQ(emit_log_kernel_out_row->main_kernel_value_out, expected_hash); EXPECT_EQ(emit_log_kernel_out_row->main_kernel_side_effect_out, 2); feed_output(emit_log_out_offset, 1, 2, 0); diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp index 6f56786ebee..6f3ad4ecd0b 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp @@ -367,10 +367,12 @@ VmPublicInputs Execution::convert_public_inputs(std::vector const& public_in // For EMITUNENCRYPTEDLOG for (size_t i = 0; i < MAX_UNENCRYPTED_LOGS_PER_CALL; i++) { size_t dest_offset = START_EMIT_UNENCRYPTED_LOG_WRITE_OFFSET + i; - size_t pcpi_offset = PCPI_NEW_UNENCRYPTED_LOGS_OFFSET + (i * 2); + size_t pcpi_offset = + PCPI_NEW_UNENCRYPTED_LOGS_OFFSET + (i * 3); // 3 because we have metadata, this is the window size ko_values[dest_offset] = public_inputs_vec[pcpi_offset]; ko_side_effect[dest_offset] = public_inputs_vec[pcpi_offset + 1]; + ko_metadata[dest_offset] = public_inputs_vec[pcpi_offset + 2]; } return public_inputs; diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.cpp index 4fbc902d466..348e08df33a 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.cpp @@ -265,10 +265,13 @@ void AvmKernelTraceBuilder::op_l1_to_l2_msg_exists(uint32_t clk, kernel_trace.push_back(entry); } -void AvmKernelTraceBuilder::op_emit_unencrypted_log(uint32_t clk, uint32_t side_effect_counter, const FF& log_hash) +void AvmKernelTraceBuilder::op_emit_unencrypted_log(uint32_t clk, + uint32_t side_effect_counter, + const FF& log_hash, + const FF& log_length) { uint32_t offset = START_EMIT_UNENCRYPTED_LOG_WRITE_OFFSET + emit_unencrypted_log_offset; - perform_kernel_output_lookup(offset, side_effect_counter, log_hash, FF(0)); + perform_kernel_output_lookup(offset, side_effect_counter, log_hash, log_length); emit_unencrypted_log_offset++; KernelTraceEntry entry = { diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.hpp index e4c74e282f3..bea60ff400a 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/kernel_trace.hpp @@ -57,8 +57,8 @@ class AvmKernelTraceBuilder { std::unordered_map kernel_output_selector_counter; AvmKernelTraceBuilder(uint32_t initial_side_effect_counter, VmPublicInputs public_inputs, ExecutionHints hints) - : initial_side_effect_counter(initial_side_effect_counter) - , public_inputs(std::move(public_inputs)) + : public_inputs(std::move(public_inputs)) + , initial_side_effect_counter(initial_side_effect_counter) , hints(std::move(hints)) {} @@ -92,14 +92,16 @@ class AvmKernelTraceBuilder { void op_nullifier_exists(uint32_t clk, uint32_t side_effect_counter, const FF& nullifier, uint32_t result); void op_emit_nullifier(uint32_t clk, uint32_t side_effect_counter, const FF& nullifier); void op_l1_to_l2_msg_exists(uint32_t clk, uint32_t side_effect_counter, const FF& message, uint32_t result); - void op_emit_unencrypted_log(uint32_t clk, uint32_t side_effect_counter, const FF& log_hash); + void op_emit_unencrypted_log(uint32_t clk, uint32_t side_effect_counter, const FF& log_hash, const FF& log_length); void op_emit_l2_to_l1_msg(uint32_t clk, uint32_t side_effect_counter, const FF& l2_to_l1_msg, const FF& recipient); + // This is temporarily made public so we can access PIs + VmPublicInputs public_inputs; + private: std::vector kernel_trace; uint32_t initial_side_effect_counter; - VmPublicInputs public_inputs; ExecutionHints hints; // Output index counters diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp index 8d493b33e82..c5d335904a3 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp @@ -14,6 +14,7 @@ #include #include "barretenberg/common/assert.hpp" +#include "barretenberg/common/serialize.hpp" #include "barretenberg/common/throw_or_abort.hpp" #include "barretenberg/crypto/pedersen_commitment/pedersen.hpp" #include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp" @@ -2493,18 +2494,67 @@ void AvmTraceBuilder::op_emit_unencrypted_log(uint8_t indirect, uint32_t log_offset, [[maybe_unused]] uint32_t log_size_offset) { + std::vector bytes_to_hash; + auto const clk = static_cast(main_trace.size()) + 1; // FIXME: read (and constrain) log_size_offset auto [resolved_log_offset, resolved_log_size_offset] = unpack_indirects<2>(indirect, { log_offset, log_size_offset }); - auto log_size = unconstrained_read_from_memory(resolved_log_size_offset); - // FIXME: we need to constrain the log_size_offset mem read (and tag check), not just one field! - // FIXME: we shouldn't pass resolved_log_offset.offset; we should modify create_kernel_output_opcode to take an - // addresswithmode. - Row row = create_kernel_output_opcode(indirect, clk, resolved_log_offset.offset); - kernel_trace_builder.op_emit_unencrypted_log(clk, side_effect_counter, row.main_ia); + FF contract_address = std::get<0>(kernel_trace_builder.public_inputs)[ADDRESS_SELECTOR]; + std::vector contract_address_bytes = contract_address.to_buffer(); + + // Unencrypted logs are hashed with sha256 and truncated to 31 bytes - and then padded back to 32 bytes + bytes_to_hash.insert(bytes_to_hash.end(), + std::make_move_iterator(contract_address_bytes.begin()), + std::make_move_iterator(contract_address_bytes.end())); + + uint32_t log_size = static_cast(unconstrained_read_from_memory(resolved_log_size_offset)); + // The size is in fields of 32 bytes, the length used for the hash is in terms of bytes + uint32_t num_bytes = log_size * 32; + std::vector log_size_bytes = to_buffer(num_bytes); + // Add the log size to the hash to bytes + bytes_to_hash.insert(bytes_to_hash.end(), + std::make_move_iterator(log_size_bytes.begin()), + std::make_move_iterator(log_size_bytes.end())); + + // Load the log values from memory + // This first read might be indirect which subsequent reads should not be + FF initial_direct_addr = resolved_log_offset.mode == AddressingMode::DIRECT + ? resolved_log_offset.offset + : unconstrained_read_from_memory(resolved_log_offset.offset); + + AddressWithMode direct_field_addr = AddressWithMode(static_cast(initial_direct_addr)); + // We need to read the rest of the log_size number of elements + std::vector log_value_bytes; + for (uint32_t i = 0; i < log_size; i++) { + FF log_value = unconstrained_read_from_memory(direct_field_addr + i); + std::vector log_value_byte = log_value.to_buffer(); + bytes_to_hash.insert(bytes_to_hash.end(), + std::make_move_iterator(log_value_byte.begin()), + std::make_move_iterator(log_value_byte.end())); + } + + std::array output = crypto::sha256(bytes_to_hash); + // Truncate the hash to 31 bytes so it will be a valid field element + FF trunc_hash = FF(from_buffer(output.data()) >> 8); + + // The + 32 here is for the contract_address in bytes, the +4 is for the extra 4 bytes that contain log_size and is + // prefixed to message see toBuffer in unencrypted_l2_log.ts + FF length_of_preimage = num_bytes + 32 + 4; + // The + 4 is because the kernels store the length of the + // processed log as 4 bytes; thus for this length value to match the log length stored in the kernels, we need to + // add four to the length here. [Copied from unencrypted_l2_log.ts] + FF metadata_log_length = length_of_preimage + 4; + Row row = Row{ + .main_clk = clk, + .main_ia = trunc_hash, + .main_ib = metadata_log_length, + .main_internal_return_ptr = internal_return_ptr, + .main_pc = pc++, + }; + kernel_trace_builder.op_emit_unencrypted_log(clk, side_effect_counter, trunc_hash, metadata_log_length); row.main_sel_op_emit_unencrypted_log = FF(1); // Constrain gas cost diff --git a/barretenberg/cpp/src/barretenberg/vm/constants.hpp b/barretenberg/cpp/src/barretenberg/vm/constants.hpp index c7024d89587..79fe4c34745 100644 --- a/barretenberg/cpp/src/barretenberg/vm/constants.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/constants.hpp @@ -71,7 +71,11 @@ inline const uint32_t PCPI_NEW_L2_TO_L1_MSGS_OFFSET = inline const uint32_t PCPI_START_SIDE_EFFECT_COUNTER_OFFSET = PCPI_NEW_L2_TO_L1_MSGS_OFFSET + (MAX_L2_TO_L1_MSGS_PER_CALL * L2_TO_L1_MESSAGE_LENGTH); -inline const uint32_t PCPI_NEW_UNENCRYPTED_LOGS_OFFSET = PCPI_START_SIDE_EFFECT_COUNTER_OFFSET + /*1 item gap*/ 1; +// The +2 here is because the order is +// START_SIDE_EFFECT_COUNTER +// END_SIDE_EFFECT_COUNTER -> + 1 +// NEW_UNENCRYPTED_LOGS -> + 2 +inline const uint32_t PCPI_NEW_UNENCRYPTED_LOGS_OFFSET = PCPI_START_SIDE_EFFECT_COUNTER_OFFSET + 2; // END INDEXES in the PUBLIC_CIRCUIT_PUBLIC_INPUTS diff --git a/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr b/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr index 7dfe55891f7..bb2456c71b1 100644 --- a/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr @@ -544,7 +544,7 @@ contract AvmTest { dep::aztec::oracle::debug_log::debug_log("get_da_gas_left"); let _ = get_da_gas_left(inputs); dep::aztec::oracle::debug_log::debug_log("emit_unencrypted_log"); - // let _ = emit_unencrypted_log(inputs); + let _ = emit_unencrypted_log(inputs); dep::aztec::oracle::debug_log::debug_log("note_hash_exists"); let _ = note_hash_exists(inputs, 1, 2); dep::aztec::oracle::debug_log::debug_log("new_note_hash"); diff --git a/yarn-project/circuit-types/src/logs/unencrypted_l2_log.ts b/yarn-project/circuit-types/src/logs/unencrypted_l2_log.ts index d24273a7346..26616746340 100644 --- a/yarn-project/circuit-types/src/logs/unencrypted_l2_log.ts +++ b/yarn-project/circuit-types/src/logs/unencrypted_l2_log.ts @@ -21,10 +21,7 @@ export class UnencryptedL2Log { ) {} get length(): number { - // We want the length of the buffer output from function_l2_logs -> toBuffer to equal the stored log length in the kernels. - // The kernels store the length of the processed log as 4 bytes; thus for this length value to match the log length stored in the kernels, - // we need to add four to the length here. - // https://github.com/AztecProtocol/aztec-packages/issues/6578#issuecomment-2125003435 + // This +4 is because we prefix the log length - see toBuffer below return this.data.length + AztecAddress.SIZE_IN_BYTES + 4; } diff --git a/yarn-project/simulator/src/public/side_effect_trace.ts b/yarn-project/simulator/src/public/side_effect_trace.ts index 5d7b0a33a70..7bff4286bdc 100644 --- a/yarn-project/simulator/src/public/side_effect_trace.ts +++ b/yarn-project/simulator/src/public/side_effect_trace.ts @@ -183,10 +183,8 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { const basicLogHash = Fr.fromBuffer(ulog.hash()); this.unencryptedLogs.push(ulog); this.allUnencryptedLogs.push(ulog); - // We want the length of the buffer output from function_l2_logs -> toBuffer to equal the stored log length in the kernels. - // The kernels store the length of the processed log as 4 bytes; thus for this length value to match the log length stored in the kernels, - // we need to add four to the length here. - // https://github.com/AztecProtocol/aztec-packages/issues/6578#issuecomment-2125003435 + // This length is for charging DA and is checked on-chain - has to be length of log preimage + 4 bytes. + // The .length call also has a +4 but that is unrelated this.unencryptedLogsHashes.push(new LogHash(basicLogHash, this.sideEffectCounter, new Fr(ulog.length + 4))); this.logger.debug(`NEW_UNENCRYPTED_LOG cnt: ${this.sideEffectCounter}`); this.incrementSideEffectCounter();