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

dense_mlpoly.rs: Fix manipulation of evaluation vector Z by bound functions #283

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

asn-d6
Copy link
Contributor

@asn-d6 asn-d6 commented Sep 20, 2024

The bound functions were folding the Z vector on itself but were not actually truncating it (even though they were changing self.len).

Hence, if you called a bound function and then evaluate() it would assert because evaluate() checks the size of the Z vector.

They left the Z vector enlarged and that was not working well with evaluate()
Copy link

vercel bot commented Sep 20, 2024

@asn-d6 is attempting to deploy a commit to the Nexus Labs Team on Vercel.

A member of the Team first needs to authorize it.

@sjudson
Copy link
Contributor

sjudson commented Sep 23, 2024

@asn-d6 thanks for the PR. We'll take a look internally, but I should note as well that this Spartan code is drawn from MSR's reference implementation so you might want to also send this PR upstream there.

EDIT: There also appears to be a failing check in the CI.

Copy link

vercel bot commented Sep 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nexus-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 24, 2024 1:21pm

@asn-d6
Copy link
Contributor Author

asn-d6 commented Sep 24, 2024

@asn-d6 thanks for the PR. We'll take a look internally, but I should note as well that this Spartan code is drawn from MSR's reference implementation so you might want to also send this PR upstream there.

EDIT: There also appears to be a failing check in the CI.

Good point. Opened a PR upstream here: microsoft/Spartan#73

Also pushed a commit that fixes the CI, I think.

@sjudson
Copy link
Contributor

sjudson commented Sep 24, 2024

Tentatively, as this appears to be a completeness issue rather than a soundness or zero-knowledge issue, I'm going to hold on it for the upstream PR (which is to an actively maintained repo) to be resolved, as we'd like to retain general parity with the reference implementation.

@asn-d6
Copy link
Contributor Author

asn-d6 commented Sep 30, 2024

Upstream PR got merged for what it's worth!

Copy link
Contributor

@sjudson sjudson left a comment

Choose a reason for hiding this comment

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

Approved due to upstream merge.

@sjudson sjudson merged commit 4657c63 into nexus-xyz:main Sep 30, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants