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.

Sign up for GitHub

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

Jump to bottom

Migrate to use new Winterfell release #1658

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Migrate to use new Winterfell release #1658

wants to merge 3 commits into from

Conversation

Al-Kindi-0
Copy link
Collaborator

No description provided.

@Al-Kindi-0 Al-Kindi-0 mentioned this pull request Feb 10, 2025
Closed
plafer
plafer approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM!

Left a few questions

stdlib/tests/crypto/stark/mod.rs
@@ -9,6 +9,7 @@ use verifier_recursive::{generate_advice_inputs, VerifierData};

// Note: Changes to MidenVM may cause this test to fail when some of the assumptions documented
// in `stdlib/asm/crypto/stark/verifier.masm` are violated.
#[ignore = "depends on horner_eval_*"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning on merging with this #[ignore] still there, and removing it in #1656? Or rebasing #1656 on top of this one, removing the #[ignore] there, and then merging both at once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once, this and #1656 are merged, there should be another PR updating the MASM code to remove the above #[ignore].

@plafer plafer mentioned this pull request Feb 10, 2025
Open
bobbinth
bobbinth approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth 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! I think we'll merge this after miden-crypto v0.14.0 is released, right?

Also, one question: should we wait to merge it until #1652 is merged? This way, we won't need to migrate miden-base to the latest Winterfell. Though, maybe this wouldn't be a problem anyways.

@Al-Kindi-0
Copy link
Collaborator Author

Looks good! Thank you! I think we'll merge this after miden-crypto v0.14.0 is released, right?

That's my thinking as well.

Also, one question: should we wait to merge it until #1652 is merged? This way, we won't need to migrate miden-base to the latest Winterfell. Though, maybe this wouldn't be a problem anyways.

I think it should not be a problem but it is fine both ways

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plafer plafer plafer approved these changes

@bobbinth bobbinth bobbinth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Al-Kindi-0 @plafer @bobbinth