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

match supernova::Proof::verify to nova::Proof::verify #758

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

mpenciak
Copy link
Contributor

This PR addresses #695 by matching the nova::Proof::verify implementaton.

Do not merge until PR #72 in Arecibo is merged, and the cargo.toml change can be reverted.

@mpenciak mpenciak changed the title match supernova::Proof::verify with nova::Proof::verify match supernova::Proof::verify to nova::Proof::verify Oct 16, 2023
porcuquine
porcuquine previously approved these changes Oct 16, 2023
@porcuquine
Copy link
Collaborator

I approved this, pending the mechanical changes to merge based on arecibo#72 noted in the description.

@mpenciak mpenciak marked this pull request as ready for review October 17, 2023 03:59
@mpenciak mpenciak requested a review from a team as a code owner October 17, 2023 03:59
let z0_secondary = Self::z0_secondary();
let zi_secondary = z0_secondary.clone();
Copy link
Member

Choose a reason for hiding this comment

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

You should not need this clone. Here's one way to make this work:

        let zi_secondary = &z0_secondary;

        let (zi_primary_verified, zi_secondary_verified) = match self {
            Self::Recursive(p) => p.verify(&pp.pp, circuit_index, z0_primary, &z0_secondary),
            Self::Compressed(_) => unimplemented!(),
        }?;

        Ok(zi_primary == zi_primary_verified && *zi_secondary == zi_secondary_verified)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, we have a similar case in nova.rs:

let zi_secondary = z0_secondary.clone();

Copy link
Member

Choose a reason for hiding this comment

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

Oh! @mpenciak are you up for seeing if the same fix applies?

Copy link
Contributor Author

@mpenciak mpenciak Oct 17, 2023

Choose a reason for hiding this comment

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

Unfortunately the same fix doesn't work in nova.rs because the Self::Compressed branch takes ownership of z0_secondary...

(we may actually end up needing to re-add in the clone after the compressed version for supernova is added in for the same reason)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then it's a matter of time until we need to clone it anyway

@huitseeker huitseeker added this pull request to the merge queue Oct 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 17, 2023
@huitseeker huitseeker added this pull request to the merge queue Oct 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2023
@huitseeker huitseeker added this pull request to the merge queue Oct 17, 2023
Merged via the queue into lurk-lab:master with commit 7e2a2ce Oct 17, 2023
9 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.

None yet

4 participants