-
Notifications
You must be signed in to change notification settings - Fork 234
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
feat(avm executor): kernel outputs & execution hints in executor #6769
Conversation
42c3d82
to
d2355ee
Compare
e28c940
to
5941499
Compare
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
5941499
to
27392a6
Compare
@@ -35,4 +36,31 @@ static const size_t NUM_MEM_SPACES = 256; | |||
static const uint8_t INTERNAL_CALL_SPACE_ID = 255; | |||
static const uint32_t MAX_SIZE_INTERNAL_STACK = 1 << 16; | |||
|
|||
// TODO: find a more suitable place for these | |||
/** | |||
* Auxiliary hints are required when the executor is unable to work out a witness value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flag
pol START_L1_TO_L2_MSG_EXISTS_WRITE_OFFSET = 16; | ||
pol START_EMIT_UNENCRYPTED_LOG_WRITE_OFFSET = 20; | ||
pol START_EMIT_L2_TO_l1_MSG = 24; | ||
pol START_NULLIFIER_EXISTS_OFFSET = 32; // START_NOTE_HASH_EXISTS_WRITE_OFFSET + MAX_NOTE_HASH_READ_REQUESTS_PER_CALL TODO: exists and non exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted these values to line up with the true offsets when using the real PER_CALL limits, previously they were just small ranges as an MVP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you now autogen these from TS?
ExecutionHints() | ||
{ | ||
storage_values = std::unordered_map<uint32_t, FF>(); | ||
note_hash_exists = std::unordered_map<uint32_t, bool>(); | ||
nullifier_exists = std::unordered_map<uint32_t, bool>(); | ||
l1_to_l2_msg_exists = std::unordered_map<uint32_t, bool>(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed. They are objects, not pointers, so they get default-constructed. This is extra work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in a position to "approve", but this mostly makes sense to me. Nice work! Left some comments to primarily confirm my understanding
info("output writing side effect counter: ", side_effect_counter); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to your changes, but can we handle side-effect-counters across nested calls? So a nested call starts with parent's counter+1, and then after it completes, parent absorbs child's counter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, it just starts from 0, but there is an issue to constrain it to be the same as the value provided within the public inputs
// Exists checks | ||
pol START_NOTE_HASH_EXISTS_WRITE_OFFSET = 0; | ||
pol START_EMIT_NOTE_HASH_WRITE_OFFSET = 4; | ||
pol START_NULLIFIER_EXISTS_OFFSET = 8; | ||
pol START_EMIT_NULLIFIER_WRITE_OFFSET = 12; | ||
pol START_L1_TO_L2_MSG_EXISTS_WRITE_OFFSET = 16; | ||
pol START_EMIT_UNENCRYPTED_LOG_WRITE_OFFSET = 20; | ||
pol START_EMIT_L2_TO_l1_MSG = 24; | ||
pol START_NULLIFIER_EXISTS_OFFSET = 32; // START_NOTE_HASH_EXISTS_WRITE_OFFSET + MAX_NOTE_HASH_READ_REQUESTS_PER_CALL TODO: exists and non exists | ||
pol START_L1_TO_L2_MSG_EXISTS_WRITE_OFFSET = 96; // START_NULLIFIER_EXISTS_OFFET + (MAX_NULLIFIER_READ_REQUESTS_PER_CALL + MAX_NULLIFIER_NON_EXISTENT_READ_REQUESTS_PER_CALL) | ||
|
||
// Public storage requests | ||
pol START_SLOAD_WRITE_OFFSET = 112; // START_L1_TO_L2_MSG_EXISTS_WRITE_OFFSET + MAX_L1_TO_L2_MSG_READ_REQUESTS_PER_CALL | ||
pol START_SSTORE_WRITE_OFFSET = 144; // START_SSTORE_WRITE_OFFSET + MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_CALL | ||
|
||
// Emit data | ||
pol START_EMIT_NOTE_HASH_WRITE_OFFSET = 160; // START_SLOAD_WRITE_OFFSET + MAX_PUBLIC_DATA_READS_PER_CALL | ||
pol START_EMIT_NULLIFIER_WRITE_OFFSET = 176; // START_EMIT_NOTE_HASH_WRITE_OFFSET + MAX_NEW_NOTE_HASHES_PER_CALL | ||
pol START_EMIT_L2_TO_l1_MSG = 192; // START_EMIT_NULLIFIER_WRITE_OFFSET + MAX_NEW_NULLIFIERS_PER_CALL | ||
pol START_EMIT_UNENCRYPTED_LOG_WRITE_OFFSET = 194; // START_EMIT_L2_TO_L1_MSG + MAX_NEW_L2_TO_L1_MSGS_PER_CALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these hand-set or code-generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now hand set, but they mirror values in cpp, so you just hover over the value in the LSP and copy it over, our pil does not compile time evaluate constants yet
// TODO: fix the naming here - we need it to be different as we are writing a hint | ||
Row AvmTraceBuilder::create_sload( | ||
uint32_t clk, uint32_t data_offset, FF const& data_value, FF const& slot_value, uint32_t slot_offset) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function just populating memory rows for slot and value? Is the row that is returned here, the sload
's row in the main trace or is it a row dedicated just to loading the hint into memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is temporary as sload had slightly differing behavior to the existing ones, it is called within op_sload which orchestrates it all
void AvmTraceBuilder::op_sload(uint32_t slot_offset, uint32_t value_offset) | ||
void AvmTraceBuilder::op_sload(uint32_t slot_offset, uint32_t write_offset) | ||
{ | ||
auto const clk = static_cast<uint32_t>(main_trace.size()); | ||
|
||
Row row = | ||
create_kernel_output_opcode_with_metadata(clk, value_offset, AvmMemoryTag::FF, slot_offset, AvmMemoryTag::FF); | ||
kernel_trace_builder.op_sload(clk, row.avm_main_ib, row.avm_main_ia); | ||
// Read the slot | ||
AvmMemTraceBuilder::MemRead slot_read = mem_trace_builder.read_and_load_from_memory( | ||
call_ptr, clk, IntermRegister::IB, slot_offset, AvmMemoryTag::FF, AvmMemoryTag::FF); | ||
|
||
// Get the data value from the execution_hints | ||
// TODO: for now the hints are being offset by the offset - this will NOT fly, but i struggled to get the hash | ||
// working for FF | ||
FF value = execution_hints.storage_values.at(write_offset); | ||
// TODO: throw error if the hint does not exist | ||
|
||
Row row = create_sload(clk, write_offset, value, slot_read.val, slot_offset); | ||
kernel_trace_builder.op_sload(clk, row.avm_main_ib, value); | ||
|
||
row.avm_main_sel_op_sload = FF(1); | ||
|
||
main_trace.push_back(row); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My attempt at understanding this:
- Use the instruction's
slot_offset
operand and readslot
from memory (adds row to memory trace) - Use that
slot
to retrieve a storagevalue
fromexecution_hints.storage_values
- Write the hinted
value
into memory (adds row to memory trace) - Add row to kernel trace for this storage slot->value "kernel output lookup"
- Create a new row in the main trace for the sload, including mem read for slot, mem write for value, and flag to indicate presence of a "kernel output lookup"
- Activate the selector for sload in this row
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this is pretty spot on
// Increment the write offset counter for the following row | ||
next.avm_kernel_note_hash_exist_write_offset = | ||
curr.avm_kernel_note_hash_exist_write_offset + static_cast<const int>(src.op_note_hash_exists); | ||
next.avm_kernel_emit_note_hash_write_offset = | ||
curr.avm_kernel_emit_note_hash_write_offset + static_cast<const int>(src.op_emit_note_hash); | ||
next.avm_kernel_emit_nullifier_write_offset = | ||
curr.avm_kernel_emit_nullifier_write_offset + static_cast<const int>(src.op_emit_nullifier); | ||
next.avm_kernel_nullifier_exists_write_offset = | ||
curr.avm_kernel_nullifier_exists_write_offset + static_cast<const int>(src.op_nullifier_exists); | ||
next.avm_kernel_l1_to_l2_msg_exists_write_offset = | ||
curr.avm_kernel_l1_to_l2_msg_exists_write_offset + static_cast<const int>(src.op_l1_to_l2_msg_exists); | ||
next.avm_kernel_emit_l2_to_l1_msg_write_offset = | ||
curr.avm_kernel_emit_l2_to_l1_msg_write_offset + static_cast<const int>(src.op_emit_l2_to_l1_msg); | ||
next.avm_kernel_emit_unencrypted_log_write_offset = | ||
curr.avm_kernel_emit_unencrypted_log_write_offset + static_cast<const int>(src.op_emit_unencrypted_log); | ||
next.avm_kernel_sload_write_offset = | ||
curr.avm_kernel_sload_write_offset + static_cast<const int>(src.op_sload); | ||
next.avm_kernel_sstore_write_offset = | ||
curr.avm_kernel_sstore_write_offset + static_cast<const int>(src.op_sstore); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We increment the write offset counter so that the next kernel lookup in the main trace is constrained to the proper entry in the kernel trace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean these
a89221e
to
aca948c
Compare
|
||
pol START_SLOAD_WRITE_OFFSET = 28; | ||
pol START_SSTORE_WRITE_OFFSET = 32; | ||
|
||
// TODO(https://github.com/AztecProtocol/aztec-packages/issues/6465): Must constrain write_offset counters to be less than side effect MAX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just reordered
@@ -33,16 +33,17 @@ FF AvmKernelTraceBuilder::perform_kernel_input_lookup(uint32_t selector) | |||
return result; | |||
} | |||
|
|||
void AvmKernelTraceBuilder::perform_kernel_output_lookup(uint32_t write_offset, const FF& value, const FF& metadata) | |||
void AvmKernelTraceBuilder::perform_kernel_output_lookup(uint32_t write_offset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side effect counter has been moved into the main trace
Please read contributing guidelines and remove this line.