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

[Fix] Prevent passing in too many public inputs to Varuna #2094

Merged
merged 5 commits into from
Feb 11, 2024
Merged

[Fix] Prevent passing in too many public inputs to Varuna #2094

merged 5 commits into from
Feb 11, 2024

Conversation

howardwu
Copy link
Member

Motivation

cd synthesizer/snark
cargo test test_varuna_verify_public_input_size --lib

@howardwu howardwu requested a review from vicsn October 15, 2023 20:21
@vicsn
Copy link
Collaborator

vicsn commented Oct 17, 2023 •
edited
Loading

Some notes about the fixes:

  • handling errors in nested closures is very annoying, so I replaced some with simple for-loops
    - we don't use ensure! anywhere yet in varuna.rs, so followed the existing setup when throwing a SNARKError::PublicInputSizeMismatch

@vicsn vicsn marked this pull request as ready for review October 17, 2023 12:53
Base automatically changed from staging to testnet3 October 19, 2023 18:52
@howardwu howardwu requested a review from ljedrz December 27, 2023 04:27
ljedrz
ljedrz reviewed Dec 27, 2023
View reviewed changes
algorithms/src/snark/varuna/varuna.rs Outdated
.map(|input| input.borrow().to_field_elements())
.map(|input| {
let input = input.borrow().to_field_elements()?;
if input.len() > input_domain.size() - 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is input domain size guaranteed to be non-zero?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This let me down an interesting rabbit hole. Yes it is guaranteed to be non-zero because we're always passing in a public input. And the - 1 is no longer needed in this somewhat related PR: https://github.com/AleoHQ/snarkVM/pull/2268

Copy link
Member Author

Choose a reason for hiding this comment

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

So is the -1 still needed here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, merged in the mainnet branch and resolved the conflict.

ljedrz
ljedrz approved these changes Dec 27, 2023
View reviewed changes
Copy link
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM with 2 questions addressed 👍

@howardwu
Copy link
Member Author

@vicsn is this PR still relevant?

@vicsn vicsn changed the title Prepares test for debug Prevent passing in too many public inputs to Varuna Feb 10, 2024
@vicsn
Copy link
Collaborator

vicsn commented Feb 10, 2024

@vicsn is this PR still relevant?

@howardwu yes it is, made the PR title more descriptive.

@vicsn vicsn changed the base branch from testnet3 to mainnet February 11, 2024 13:03
@howardwu howardwu changed the base branch from mainnet to testnet3 February 11, 2024 18:02
@howardwu howardwu changed the base branch from testnet3 to mainnet February 11, 2024 18:03
@howardwu
Copy link
Member Author

LGTM

@howardwu howardwu merged commit 3de2007 into mainnet Feb 11, 2024
76 of 78 checks passed
@howardwu howardwu deleted the bug/varuna branch February 11, 2024 18:03
@howardwu howardwu mentioned this pull request Feb 12, 2024
Draft
@raychu86 raychu86 changed the title Prevent passing in too many public inputs to Varuna [Fix] Prevent passing in too many public inputs to Varuna Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljedrz ljedrz ljedrz approved these changes

@vicsn vicsn Awaiting requested review from vicsn

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
@howardwu @vicsn @ljedrz