Skip to content

Fix call to supernova::RecursiveSNARK::verify#951

Merged
huitseeker merged 2 commits intolurk-lab:mainfrom
mpenciak:arecibo#175_companion
Dec 13, 2023
Merged

Fix call to supernova::RecursiveSNARK::verify#951
huitseeker merged 2 commits intolurk-lab:mainfrom
mpenciak:arecibo#175_companion

Conversation

@mpenciak
Copy link
Copy Markdown
Contributor

This is a companion PR to arecibo#175 which changes the supernova::RecursiveSNARK::verify interface by removing its dependence on circuit_index.

TODO before merge:

@mpenciak mpenciak marked this pull request as ready for review December 13, 2023 01:39
@mpenciak mpenciak requested a review from a team as a code owner December 13, 2023 01:39
z0: &[F],
zi: &[F],
last_circuit_idx: usize,
_last_circuit_idx: usize,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is no longer necessary, we should open an issue about it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd vote for removing the unused argument from the API, and fixing downstream (lurk-rs) as part of integration of this work. That could possibly be by adjusting #936.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Whoops… I thought I was looking at an arecibo change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should definitely remove rather than ignore vestigial arguments — unless I'm missing something.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Parity with Nova, I guess…

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

verify is an unified API now. We need to pass at least something, maybe PhantomData. We define the extra input for verify when implementing RecursiveSNARKTrait, which is currently usize for both Nova and SuperNova proofs.

@huitseeker huitseeker enabled auto-merge December 13, 2023 13:39
@huitseeker
Copy link
Copy Markdown
Contributor

AFAIC, this is a breakage fix PR, we can address the unused argument later.

@huitseeker huitseeker added this pull request to the merge queue Dec 13, 2023
github-actions bot pushed a commit that referenced this pull request Dec 13, 2023
@arthurpaulino
Copy link
Copy Markdown
Contributor

Yes, that's why I mentioned that we should at least open an issue for it

Merged via the queue into lurk-lab:main with commit 09bcd8a Dec 13, 2023
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.

4 participants