-
Notifications
You must be signed in to change notification settings - Fork 160
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: DYN
and DYNCALL
takes a memory address instead of digest on stack
#1535
base: next
Are you sure you want to change the base?
Conversation
Ah yes - good point! I didn't think of this. Indeed, we may need to do #1478 first. |
b83bb5c
to
26f33fa
Compare
26f33fa
to
328ac68
Compare
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! Thank you! I left some comments inline - but nothing major.
The complexity in the stack resulting from having to do a left shift and a context switch at the same time is a bit unfortunate - but I'm not seeing a better way to do it. We should probably create an issue to come back to this later and try to make the constraints a bit more uniform.
pub fn decoder_hasher_state_element(&self, element: usize, i: RowIndex) -> Felt { | ||
self.columns.get_column(DECODER_TRACE_OFFSET + HASHER_STATE_OFFSET + element)[i + 1] | ||
self.columns.get_column(DECODER_TRACE_OFFSET + HASHER_STATE_OFFSET + element)[i] | ||
} |
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.
Was this a bug previously?
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.
Yes, but for some reason it never manifested itself. I think only RESPAN
used it, so maybe looking in the next row there was always what we expected? I'm really not sure why it went unnoticed for this long.
328ac68
to
8ed6d47
Compare
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 good! Thank you! I've reviewed pretty much everything except for test code (I'll review it tomorrow).
Once docs are updated we can merge. Or we can merge sooner and update docs in another PR.
8ed6d47
to
b937816
Compare
@bobbinth docs are ready for review |
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 good! Thank you! For tests, I did a relatively superficial review - so, would appreciate another set of eyes on them (cc @Al-Kindi-0).
One other thing: I'm not sure we should commit the excalidraw files here. It is not possible to easily verify what's in these files and they blow up the line count. Maybe an alternative could be to create a shared excalidraw workspace so that we have access to all these diagrams? cc @Dominik1999.
DYN
takes a memory address instead of digest on stackDYN
and DYNCALL
takes a memory address instead of digest on stack
b937816
to
38341dc
Compare
38341dc
to
898d4ef
Compare
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, thank you!
A few questions and remarks inline mainly.
898d4ef
to
61b3a1b
Compare
61b3a1b
to
facb5ce
Compare
&+ \alpha_5 \cdot fmp + \alpha_6 \cdot b_0 + \alpha_7 \cdot b_1 + <[\alpha_8, \alpha_{11}], fn\_hash[0..3]>) \text{ | degree} = 6 | ||
\end{align*} |
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.
fn_hash is not rendering well, I think this is what I tried to refer to earlier
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 changed them all to \mathrm{fnhash}
, which should render not run into any rendering issues
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 is now rendering well on GitHub
facb5ce
to
28dcf86
Compare
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 good! Thank you!
I'll merge this PR tomorrow (after miden-base
has been migrated to the current next
).
Let's hold off on merging this until we have a corresponding PR in |
3ed930e
to
7f469c4
Compare
Closes #1091
Closes #1478