-
Notifications
You must be signed in to change notification settings - Fork 599
chore: simplify handling of pub inputs block #11747
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
Changes from all commits
692226a
cbca5e5
f3b8d5b
27a2f8a
90a8a5d
24cb346
2c0a4f7
eb026a0
bd9b6c1
2d15211
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,7 @@ void UltraCircuitBuilder_<ExecutionTrace>::finalize_circuit(const bool ensure_no | |
| process_RAM_arrays(); | ||
| process_range_lists(); | ||
| #endif | ||
| populate_public_inputs_block(); | ||
| circuit_finalized = true; | ||
| } else { | ||
| // Gates added after first call to finalize will not be processed since finalization is only performed once | ||
|
|
@@ -1794,6 +1795,24 @@ std::array<uint32_t, 2> UltraCircuitBuilder_<ExecutionTrace>::evaluate_non_nativ | |
| return std::array<uint32_t, 2>{ lo_1_idx, hi_3_idx }; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Copy the public input idx data into the public inputs trace block | ||
| * @note | ||
| */ | ||
| template <typename ExecutionTrace> void UltraCircuitBuilder_<ExecutionTrace>::populate_public_inputs_block() | ||
| { | ||
| PROFILE_THIS_NAME("populate_public_inputs_block"); | ||
|
|
||
| // Update the public inputs block | ||
| for (const auto& idx : this->public_inputs) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eventually the |
||
| // first two wires get a copy of the public inputs | ||
| blocks.pub_inputs.populate_wires(idx, idx, this->zero_idx, this->zero_idx); | ||
| for (auto& selector : this->blocks.pub_inputs.selectors) { | ||
| selector.emplace_back(0); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @brief Called in `compute_proving_key` when finalizing circuit. | ||
| * Iterates over the cached_non_native_field_multiplication objects, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,7 @@ template <IsUltraFlavor Flavor> size_t DeciderProvingKey_<Flavor>::compute_dyadi | |
| const size_t min_size_due_to_lookups = circuit.get_tables_size(); | ||
|
|
||
| // minimum size of execution trace due to everything else | ||
| size_t min_size_of_execution_trace = circuit.public_inputs.size() + circuit.num_gates; | ||
| if constexpr (IsMegaFlavor<Flavor>) { | ||
| min_size_of_execution_trace += circuit.blocks.ecc_op.size(); | ||
| } | ||
| size_t min_size_of_execution_trace = circuit.blocks.get_total_content_size(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this right? Shouldn't it be like the last block's offset + the size of the last block? This doesn't account for the fixed sizes of the structured trace
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the method that's used in the absence of the structured trace, so its basically equivalent to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought compute_dyadic_size is used in all contexts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope we have |
||
|
|
||
| // The number of gates is the maximum required by the lookup argument or everything else, plus an optional zero row | ||
| // to allow for shifts. | ||
|
|
@@ -238,6 +235,7 @@ void DeciderProvingKey_<Flavor>::construct_databus_polynomials(Circuit& circuit) | |
| */ | ||
| template <IsUltraFlavor Flavor> | ||
| void DeciderProvingKey_<Flavor>::move_structured_trace_overflow_to_overflow_block(Circuit& circuit) | ||
| requires IsMegaFlavor<Flavor> | ||
| { | ||
| auto& blocks = circuit.blocks; | ||
| auto& overflow_block = circuit.blocks.overflow; | ||
|
|
@@ -253,9 +251,19 @@ void DeciderProvingKey_<Flavor>::move_structured_trace_overflow_to_overflow_bloc | |
| uint32_t fixed_block_size = block.get_fixed_size(); | ||
| if (block_size > fixed_block_size && block != overflow_block) { | ||
| // Disallow overflow in blocks that are not expected to be used by App circuits | ||
| ASSERT(!block.is_pub_inputs); | ||
| if constexpr (IsMegaFlavor<Flavor>) { | ||
| ASSERT(block != blocks.ecc_op); | ||
| if (block.is_pub_inputs) { | ||
| info("WARNING: Number of public inputs (", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice warnings |
||
| block_size, | ||
| ") cannot exceed capacity specified in structured trace: ", | ||
| fixed_block_size); | ||
| ASSERT(false); | ||
| } | ||
| if (block == blocks.ecc_op) { | ||
| info("WARNING: Number of ecc op gates (", | ||
| block_size, | ||
| ") cannot exceed capacity specified in structured trace: ", | ||
| fixed_block_size); | ||
| ASSERT(false); | ||
| } | ||
|
|
||
| // Set has_overflow to true if at least one block exceeds its capacity | ||
|
|
@@ -265,7 +273,6 @@ void DeciderProvingKey_<Flavor>::move_structured_trace_overflow_to_overflow_bloc | |
| // the block containing RAM/ROM gates overflows, the indices of the corresponding gates in the memory | ||
| // records need to be updated to reflect their new position in the overflow block | ||
| if (block.has_ram_rom) { | ||
|
|
||
| uint32_t overflow_cur_idx = overflow_block.trace_offset + static_cast<uint32_t>(overflow_block.size()); | ||
| overflow_cur_idx -= block.trace_offset; // we'll add block.trace_offset to everything later | ||
| uint32_t offset = overflow_cur_idx + 1; // +1 accounts for duplication of final gate | ||
|
|
@@ -316,12 +323,7 @@ void DeciderProvingKey_<Flavor>::move_structured_trace_overflow_to_overflow_bloc | |
|
|
||
| // Set the fixed size of the overflow block to its current size | ||
| if (overflow_block.size() > overflow_block.get_fixed_size()) { | ||
| info("WARNING: Structured trace overflowed beyond the prescribed fixed overflow size. This is valid in the " | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this comment was only relevant for the initial compile time overflow mechanism. It doesn't apply since the introduction of dynamic overflow support. |
||
| "context of one-off VK/proof generation but not in the IVC setting. \nPrescribed overflow size: ", | ||
| overflow_block.get_fixed_size(), | ||
| ". \nActual overflow size: ", | ||
| overflow_block.size(), | ||
| "\n"); | ||
| info("WARNING: Structured trace overflow mechanism in use. Performance may be degraded!"); | ||
| overflow_block.fixed_size = static_cast<uint32_t>(overflow_block.size()); | ||
| blocks.summarize(); | ||
| } | ||
|
|
||
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 changes simply because the hash operates on the
blocksand prior to my changes the public inputs block was not yet populated at the time of this computation