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

chore: Remove redundant absorption to the transcript #287

Merged
merged 1 commit into from
Jan 3, 2024
Merged

chore: Remove redundant absorption to the transcript #287

merged 1 commit into from
Jan 3, 2024

Conversation

storojs72
Copy link
Contributor

Fixes #284

@storojs72
Copy link
Contributor Author

@microsoft-github-policy-service agree company="LurkLab"

@srinathsetty
Copy link
Collaborator

Hi! Thanks for the PR! I'm a little confused about the diff. The simplest fix for the issue is to just remove the following two lines. Isn't that cleaner than what is proposed here? I'm not entirely sure why there should be any other edits (e.g., regarding the absorption of comm_L_row and comm_L_col).

Prover:

Nova/src/spartan/ppsnark.rs

Line 1066 in fe5b932

transcript.absorb(b"e", &eval_vec.as_slice()); // c_vec is already in the transcript

Verifier:

Nova/src/spartan/ppsnark.rs

Line 1367 in fe5b932

transcript.absorb(b"e", &eval_vec.as_slice()); // c_vec is already in the transcript

@storojs72
Copy link
Contributor Author

Right. But, let's consider verification flow. Why do we use this manually crafted slice, if later we create eval_vec with exactly same elements? So I thought that moving absorptions without violation of their order (&eval_vec.as_slice() first, and &vec![comm_L_row, comm_L_col].as_slice second) after introducing eval_vec would be more readable.

@srinathsetty srinathsetty merged commit 7050052 into microsoft:main Jan 3, 2024
6 checks passed
huitseeker added a commit to huitseeker/Nova that referenced this pull request Feb 2, 2024
@huitseeker
Remove superfluous instances of derive(Clone) (easy) (microsoft#287)
28a4395 
* removing uneeded derive(Clone) in src/lib.rs

* removing uneeded derive(Clone) in src/provider/keccak.rs

* removing uneeded derive(Clone) in src/provider/non_hiding_kzg.rs

* removing uneeded derive(Clone) in src/supernova/error.rs

* removing uneeded derive(Clone) in src/supernova/mod.rs

* removing uneeded derive(Clone) in src/supernova/snark.rs

* removing uneeded derive(Clone) in src/spartan/batched_ppsnark.rs

* removing uneeded derive(Clone) in src/spartan/batched.rs

* removing uneeded derive(Clone) in src/errors.rs

* removing uneeded derive(Clone) in src/nifs.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove redundant absorbing to the transcript
2 participants
@storojs72 @srinathsetty