Update code to be compatible with Winterfell 0.8 #1234
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
bobbinth
reviewed
Feb 8, 2024
Comment on lines
33
to
+34
| const FMP_MIN: Felt = Felt::new(crate::FMP_MIN); | ||
| const SYSCALL_FMP_MIN: Felt = Felt::new(crate::SYSCALL_FMP_MIN); | ||
| const SYSCALL_FMP_MIN: Felt = Felt::new(crate::SYSCALL_FMP_MIN as u64); |
There was a problem hiding this comment.
I think FMP_MIN and SYSCALL_FMP_MIN are actually just 32 bit values. Maybe we should declare them as such?
There was a problem hiding this comment.
I tried to do so, but it turns out that there are quite a lot of places where we use these constants (mostly FMP_MIN) as a stack elements in tests, so I decided that it will be better just to use Felt::new() in some places and don't cast them to the u64 in a large number of places in the tests
Comment on lines
744
to
751
| + alphas[2].mul_base( | ||
| Felt::try_from((i + 1) as u64) | ||
| .expect("value is greater than or equal to the field modulus"), |
There was a problem hiding this comment.
i is guaranteed to be a u32 here because it the row index, I believe. Ideally, we should change the function signature to reflect this. If this is too much for this PR, then we should leave a TODO comment and use as u32 for i.
bobbinth
force-pushed
the
al-simplify-aux
branch
from
3d06daa to
4c963d2
Compare
Fumuran
force-pushed
the
andrew-integrate-winterfell
branch
2 times, most recently
from
a6613c0 to
5e94731
Compare
This was referenced Feb 8, 2024
Fumuran
force-pushed
the
andrew-integrate-winterfell
branch
2 times, most recently
from
4cd5712 to
42861af
Compare
bobbinth
force-pushed
the
al-simplify-aux
branch
from
d8ae2e5 to
156a257
Compare
Fumuran
force-pushed
the
andrew-integrate-winterfell
branch
from
42861af to
e7479c2
Compare
bobbinth
approved these changes
Feb 9, 2024
Al-Kindi-0
approved these changes
Feb 9, 2024
Fumuran
force-pushed
the
andrew-integrate-winterfell
branch
from
e7479c2 to
55d3359
Compare
bobbinth
force-pushed
the
andrew-integrate-winterfell
branch
from
55d3359 to
66c7a10
Compare
Fumuran
force-pushed
the
andrew-integrate-winterfell
branch
from
66c7a10 to
83d5e41
Compare
Fumuran
force-pushed
the
andrew-integrate-winterfell
branch
from
83d5e41 to
0f8f103
Compare
bobbinth
pushed a commit
that referenced
this pull request
Feb 26, 2024
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.
This PR updates the code to be compatible with new 0.8 version of the Winterfell.
This update mostly contains changes in
Feltcreation, since casting ofu64toFieldElementwas removed. It also updates the serialization and deserialization of the structs, which contain vectors ofFelts orRpoDigests.