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: unencryptedlogs witgen #8669

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
23 changes: 12 additions & 11 deletions barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just some formatting

"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
Expand Down Expand Up @@ -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"));
Copy link
Contributor Author

@IlyasRidhuan IlyasRidhuan Sep 20, 2024

Choose a reason for hiding this comment

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

Now that witgen calculates the truncated sha256 hash of the logs, we need to do the same - I promise this is the real hash

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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -367,10 +367,12 @@ VmPublicInputs Execution::convert_public_inputs(std::vector<FF> 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ class AvmKernelTraceBuilder {
std::unordered_map<uint32_t, uint32_t> 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))
Copy link
Contributor Author

@IlyasRidhuan IlyasRidhuan Sep 20, 2024

Choose a reason for hiding this comment

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

Had to re-order this because public_inputs is now public (because i need it to get the contract address until we have bytecode hashing) and clang complains if it's not initialized first

, initial_side_effect_counter(initial_side_effect_counter)
, hints(std::move(hints))
{}

Expand Down Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

emit_unencrypted_log actually has metadata, but we weren't properly handling

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<KernelTraceEntry> kernel_trace;

uint32_t initial_side_effect_counter;
VmPublicInputs public_inputs;
ExecutionHints hints;

// Output index counters
Expand Down
58 changes: 52 additions & 6 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <vector>

#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"
Expand Down Expand Up @@ -2493,18 +2494,63 @@ void AvmTraceBuilder::op_emit_unencrypted_log(uint8_t indirect,
uint32_t log_offset,
[[maybe_unused]] uint32_t log_size_offset)
{
std::vector<uint8_t> bytes_to_hash;

auto const clk = static_cast<uint32_t>(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<uint8_t> 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<uint32_t>(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<uint8_t> log_size_bytes = to_buffer(num_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<uint32_t>(initial_direct_addr));
// We need to read the rest of the log_size number of elements
std::vector<uint8_t> 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<uint8_t> 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<uint8_t, 32> 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<uint256_t>(output.data()) >> 8);

// The + 32 here is for the contract_address in bytes. The + 4 is becuase the kernels store the length of the
IlyasRidhuan marked this conversation as resolved.
Show resolved Hide resolved
// 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 = num_bytes + 32 + 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);
IlyasRidhuan marked this conversation as resolved.
Show resolved Hide resolved
row.main_sel_op_emit_unencrypted_log = FF(1);

// Constrain gas cost
Expand Down
6 changes: 5 additions & 1 deletion barretenberg/cpp/src/barretenberg/vm/constants.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
6 changes: 1 addition & 5 deletions yarn-project/simulator/src/public/side_effect_trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,7 @@ 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.unencryptedLogsHashes.push(new LogHash(basicLogHash, this.sideEffectCounter, new Fr(ulog.length + 4)));
this.unencryptedLogsHashes.push(new LogHash(basicLogHash, this.sideEffectCounter, new Fr(ulog.length)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the +4 is actually already handled in the .length call of the class

this.logger.debug(`NEW_UNENCRYPTED_LOG cnt: ${this.sideEffectCounter}`);
this.incrementSideEffectCounter();
}
Expand Down
Loading