-
Notifications
You must be signed in to change notification settings - Fork 174
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
Clean up aux column generator for stack overflow table #1213
Conversation
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.
LGTM! Left a suggestion for a future PR
|
||
if self.trace_enabled { | ||
// insert a copy of the current table state into the trace | ||
self.save_current_state(clk); | ||
self.save_current_state(clk.as_int()); |
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.
Ideally there would be only one type for clock cycles. save_current_state()
(and self.trace
) still use u64
for clock cycles, while the rest uses Felt
(presumably because Felt
doesn't implement Ord
which is needed for BTreeMap
. This would probably justify the need for a ClockCycle
type (in another PR)
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.
It's a bit tricky here. Usually, clock cycles should be u32
values as we can't execute more than ClockCycle
type, we should make it based on u32
.
However, for stack initialization purposes we need to use "negative cycles". That is, we initialize the overflow tables as if some values were pushed into it before the program started executing. So, if there is one value in the stack overflow table before the program starts running, the clock cycle for that value would be set to -1
or in our filed this would be MODULUS - 1
- which is a u64
value. So, here we need to use u64
s and Felt
s.
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.
Looks great and thank you for fixing the typo!
This PR cleans up a little auxiliary column code generator for the stack overflow table.
Specifically:
OverflowTableRow::to_value()
method to compute aux request/responses.OverflowTable.update_trace
field since it is no longer needed.We can further simplify the
OverflowTable
struct by not tracking all overflow table rows and instead tracking only current ones. But I didn't do this in this PR.