Skip to content
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

Update serialization logic of OOD frame #358

Merged

Conversation

Al-Kindi-0
Copy link
Contributor

Moves away from serializing the current and next raw in a zig-zag manner and serializes them instead as two vectors one after the other.

Comment on lines +70 to 71
// save the evaluations of the current and then next evaluations for each polynomial
let (main_and_aux_trace_states, lagrange_trace_states) = trace_ood_frame.to_trace_states();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we build elements_to_hash below, should we try to arrange things so that we always end up with 4 base field elements per extension field element (for 64-bit fields)? Basically, the arrangement would look something like so:

For sequence of degree 2 extension field elements: (a_0, a_1), (b_0, b_1), (c_0, c_1), ..., elements_to_hash would look like:

[a_0, a_1, 0, 0, b_0, b_1, 0, 0, c_0, c_1, 0, 0, ...]

And then, if we were in the degree 3 extension field, it'd look like:

[a_0, a_1, a_2, 0, b_0, b_1, b_2, 0, c_0, c_1, c_2, 0, ...]

One open question here is how to handle this generically given that we could also be in a 128-bit field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep it as it is for now as the benefits of the layout you proposed are still not clear to me, especially given its "unnaturalness".

Copy link
Contributor Author

@Al-Kindi-0 Al-Kindi-0 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe you are thinking of it in light of the following proposal 0xPolygonMiden/miden-vm#1610 (comment) ?
If yes, then we can include it in this PR to build on it the solution in the previously mentioned issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One open question here is how to handle this generically given that we could also be in a 128-bit field.

I would probably just go for the simplest solution of conditioning on the case of $64$ bit primes. Once we commit to this change downstream, we can come back and modify the implementation to something more generic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe you are thinking of it in light of the following proposal 0xPolygonMiden/miden-vm#1610 (comment) ?
If yes, then we can include it in this PR to build on it the solution in the previously mentioned issue.

Yes, that's what I had in mind. Basically, "unlashing" becomes more "uniform" across different extension fields and also potentially easier to process in that context (though, it depends on whether this methodology will be used in the VM)..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the question is do we do it now with partial genericity or do we leave it till we need it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep as is for now, but create an issue to look into it.

Copy link
Collaborator

@irakliyk irakliyk left a 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! Let's first merge #357, then rebase and merge this PR.

Comment on lines +70 to 71
// save the evaluations of the current and then next evaluations for each polynomial
let (main_and_aux_trace_states, lagrange_trace_states) = trace_ood_frame.to_trace_states();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep as is for now, but create an issue to look into it.

@Al-Kindi-0 Al-Kindi-0 force-pushed the al-update-serialization-of-ood-frame branch from ec199cf to 6130210 Compare January 31, 2025 08:48
@Al-Kindi-0 Al-Kindi-0 marked this pull request as ready for review January 31, 2025 08:56
@irakliyk irakliyk merged commit 73dcf60 into facebook:main Jan 31, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants