refactor: Trace structure is an object #10003
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
codygunton
changed the title
codygunton
changed the title
ledwards2225
approved these changes
Nov 15, 2024
| fixed_sizes.set_fixed_block_sizes(trace_settings); | ||
|
|
||
| if (trace_settings.structure) { | ||
| info("yes trace structure"); |
| uint32_t overflow_capacity = 0; | ||
| }; | ||
|
|
||
| class MegaTraceBlock : public ExecutionTraceBlock<fr, 4, 14> { |
There was a problem hiding this comment.
Maybe at least inline comments indicating what these numbers are?
| .overflow = 0 }; | ||
|
|
||
| /** | ||
| * @brief A minimal structuring specifically tailored to the medium complexity transaction for the Cli: MentIVC |
There was a problem hiding this comment.
weird comment thing happening here
| @@ -374,7 +374,7 @@ Accumulator ECCVMSetRelationImpl<FF>::compute_grand_product_denominator(const Al | |||
| } | |||
|
|
|||
| /** | |||
| * @brief Expression for the StandardArithmetic gate. | |||
| * @brief Expression for the StandardExecutionTracemetic gate. | |||
| @@ -123,8 +123,8 @@ template <typename RecursiveFlavor> class RecursiveVerifierTest : public testing | |||
| static void test_independent_vk_hash() | |||
| { | |||
| // Retrieves the trace blocks (each consisting of a specific gate) from the recursive verifier circuit | |||
| auto get_blocks = [](size_t inner_size) | |||
| -> std::tuple<typename OuterBuilder::GateBlocks, std::shared_ptr<typename OuterFlavor::VerificationKey>> { | |||
| auto get_blocks = [](size_t inner_size) -> std::tuple<typename OuterBuilder::ExeeuctionTrace, | |||
There was a problem hiding this comment.
hmm how is this building? ExeeuctionTrace
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A trace structure is a set of block sizes that are known at compile time, but previously there was no simple way to extract any of this compile time information even at runtime without executing some heavier code or perhaps by duplicating a switch statement and manually computing a size a runtime. Motivated by this, I refactor the trace structure (formerly an enum coupled loosely to some related structures) so that it becomes a struct. At the same time I refactor related structures to use more familiar patterns. I also do some renaming and related general tidying.
sizefunctions will be implemented in a follow-on, where they will help to allocate and use a single commitment key throughoutClientIVC.Changes in structuring pattern
Old pattern:
-
TraceBlockssubclassesMegaTraceBlocks<ExecutionTraceBlock>- Each trace structure is a subclass with a different default constructor
-
set_fixed_block_sizesinstantiates afixed_block_sizesobjectMegaTraceBlocks<uint32_t>-
set_fixed_block_sizeschecks an enum to figure out which trace structure class to default construct-
set_fixed_block_sizesoverwrites thefixed_block_sizesobjectNew Pattern:
-
TraceStructureisMegaTraceBlockData<uint32_t>- Each trace structure is a
static constexprinstance ofTraceStructure.-
set_fixed_block_sizeson aMegaExecutionTraceBlocksobject takes a trace structure instance and uses it to set thefixed_sizeof the blocks.Notions of "execution trace" and "arithmetization"
ExecutionTrace_used to be the name for a purely static class that assists in proving key construction. We would have unclear lines of code that looked likeExecutionTrace_::populate(builder, proving_key)where it's not clear that is happening--which thing is being populated? The builder is notconst, but could it be? Trying to constify took me down a rabbit hole that felt out of scope, so I still don't know. I renamed this class to TraceToPolynomials for clarity and because I wanted to use the name ExecutionTrace for the container of the blocks (In the end I went withExecutionTraceBlocks) and metadata that used to be theArithemetization.Arithmetization(something I think I initially defined) is renamed toExecutionTrace, broadly speaking. I removed some of the nested class definitions fromMegaArith&UltraArithbecause I think it helps readability, and I flattened the nestedTraceBlocksclass into its container class which is now calledFooTraceBlocks--I didn't see what the nesting gave us there.Etc
To handle the
NONEstructure case, I use astd::optional.ClientIVCgets a constructor from aTraceSettingsand a booleanauto_verify_mode. Just a bit of cleanup--there were dozens of instances where this constructor was called for.I remove some unused templating.
I don't see the value of having a fully public setter for a private value so I nixed
set_fixed_size. This clarified the rather puzzling (to me) lineoverflow_block.set_fixed_size(static_cast<uint32_t>(overflow_block.size()));