-
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
Migrate to next release of Winterfell #1533
base: next
Are you sure you want to change the base?
Conversation
072797b
to
bb307b7
Compare
Thank you! Not a review yet, but one interesting thing to check would the performance between this and the |
I am seeing a bit of a degradation between this branch and This branch
NEXT
|
I have run the Fibonacci example as I am getting the following error when running the Blake one:
I have also tried to benchmark main but I am getting an error at the assembler level:
|
Is it possible that you have a really old version of |
main is indeed behind but next is up to date |
Here is the numbers for main:
|
Here are the current results after fixing a bug when using partitioned hashing for commitment: Current
Next
|
403b5d8
to
151c4d7
Compare
Here are the result after the most recent patch to Winterfell: Next
This
|
151c4d7
to
578be7f
Compare
This is what i'm getting on my machine:
This is roughly in-line with what we were getting before. One thing that really stands out to me is that trace generation (main + auxiliary) takes up 0.75 seconds. Ideally, we should figure out how to bring this down to under 100ms. |
core/Cargo.toml
Outdated
memchr = { version = "2.7", default-features = false } | ||
miden-crypto = { version = "0.11", default-features = false } | ||
miden-crypto = { git = "https://github.com/0xPolygonMiden/crypto", branch = "next", default-features = false } |
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.
miden-crypto
v0.12.0 is now on crates.io - so, we can use it here now.
Yes, that's high indeed. We should start investigating ways of "parallelizing" witness generation as this will be useful to us in many different places. |
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 think one remaining thing is to make sure Metal prover is also updated to work with the lates interface. Are you able to check this on your end?
I wouldn't be able to test on my machine. |
Describe your changes
Checklist before requesting a review
next
according to naming convention.