Substitute `ChallengeGenerator` by the generic sponge #139

autquis · 2024-01-05T10:13:06Z

Description

In this PR, we are removing ChallengeGenerator and using only the sponge on which the PCS is generic. As a consequence, we are dropping the two strategies of generating challenges. However, as far as I noticed, the Univariate strategy was only used in a few tests and bench templates. So, I am not sure how big of an issue it is.

Closes #138
Notice that this PR is on top of #137

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (master)
Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
Wrote unit tests
Updated relevant documentation in the code
Added a relevant changelog entry to the Pending section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

poly-commit/src/lib.rs

autquis · 2024-01-08T17:19:49Z

poly-commit/src/lib.rs


-        for challenge_gen in challenge_generators {
+        for __ in 0..1 {


By deleting this for loop, the diff gets unnecessarily complicated. As soon as the PR gets a review, I will delete this for loop, and then please merge it.

PR is approved!

Pratyush · 2024-01-08T19:18:01Z

Thanks for this PR! It looks pretty good overall. Since you're looking into this code, could you double check that the way we're deriving these challenges is not vulnerable to https://blog.openzeppelin.com/the-last-challenge-attack ?

Essentially, when we're doing batch verification, we need to ensure that we absorb all the proofs in the batch before we squeeze the opening_challenge.

autquis · 2024-01-09T11:15:01Z

Of course!

Currently, the sponge does not absorb anything. Even before this PR, the ChallengeGenerator was not absorbing anything. Is this a problem in Marlin and other schemes in the crate? (I am unfamiliar with their security analysis)
For Ligero, Brakedown, and Hyrax, I am not sure how we can absorb all the transcripts in batch_check. As the current implementation of batch_check calls check in a for loop, and the check method should absorb the transcript anyhow, I do not see an immediate solution to avoid double-absorbing. 🤔

Thanks!
cc @mmagician

mmagician · 2024-01-09T11:40:40Z

True, there was only ever squeezes from a sponge...

Regarding the second point for IOP-based schemes, we anyway planned to improve the batch_check further down the line.

autquis · 2024-01-09T12:00:08Z

Thanks for the answer! Regarding batch_verify: Maybe the following rough idea helps. Instead of having check, we use two methods, check_transcript and check_proof. check_transcript has the proof and the challenges as arguments (i.e., no absorbing or squeezing). check_proof has the proof and the sponge as arguments, and needs to absorb or squeeze, then call check_transcript. batch_check does the same job as check_proof with some potential optimization for the case of many proofs.
I think it requires another discussion/issue/PR, though.

Pratyush · 2024-01-09T21:38:12Z

Ok seems like the blog post and what this PR aim to achieve are two different things; this PR talks about how to generate challenges for batch proving, and the blog post talks about batching pairings within a batch proof verification.

That is, the blog post talks about replacing this line with a squeeze from a sponge instead of a rng-derived challenge.

We should change that in a separate PR (it's not a soundness bug atm.)

Pratyush · 2024-01-09T21:42:31Z

Having resolved that confusion (sorry about that!), let's return our focus to the topic of this PR.

I think the reason why the current code is "sound" is that it assumes the sponge inside the challenge generator already absorbed the requisite commitments. For example, see the marlin code here: https://github.com/arkworks-rs/marlin/blob/026b73c20638f4f86cbae0946045934c865d5a30/src/lib.rs#L289. The absorption happens before we generate opening_challenge.

We can think about whether we want to change that or not; my inclination is that the answer is yes.

mmagician · 2024-01-09T22:46:31Z

That makes sense @Pratyush. In any case we can tackle this separately as this PR doesn't alter the old behavior (regarding what is assumed to be absorbed) right?

Pratyush

Yup, looks good. Can we file a follow-up issue?

* Add the trait bounds * Add `CommitmentState` * Update benches for the new type * Fix the name of local variable * Merge `PCCommitmentState` with `PCRandomness` * Update `README.md` * Fix a bug * Complete the merge * Simplify `hash_column` * Delete comments * Add `CommitmentState` * Make `fmt` happy * Refactor, remove `hash_columns` * Rename all params * remove cfg(benches) attributes as that feature is no longer used * Brakedown+++ (#46) * conversion to `into_iter` is a no-op * remove explicit casts to vecs * rename to use singular of `labeled_commitment` * simplify the iterators even further by zipping two iters * Apply suggestions from code review * Maybe `empty` not return `Self` * Make `empty` return `Self` * Rename `rand` to `state` * Add the type `Randomness` * Rename nonnative to emulated, as in `r1cs-std` (arkworks-rs#137) * Rename nonnative to emulated, as in `r1cs-std` * Run `fmt` * Temporarily change `Cargo.toml` * Revert `Cargo.toml` * Refactor `FoldedPolynomialStream` partially * Substitute `ChallengeGenerator` by the generic sponge (arkworks-rs#139) * Rename nonnative to emulated, as in `r1cs-std` * Run `fmt` * Temporarily change `Cargo.toml` * Substitute `ChallengeGenerator` with the generic sponge * Run `fmt` * Remove the extra file * Update modules * Delete the unnecessary loop * Revert `Cargo.toml` * Refactor `FoldedPolynomialStream` partially * Update README * Make the diff more readable * Bring the whitespace back * Make diff more readable, 2 * Fix according to breaking changes in `ark-ec` (arkworks-rs#141) * Fix for KZG10 * Fix the breaking changes in `ark-ec` * Remove the extra loop * Fix the loop range * re-use the preprocessing table * also re-use the preprocessing table for multilinear_pc --------- Co-authored-by: mmagician <marcin.gorny.94@protonmail.com> * Auxiliary opening data (arkworks-rs#134) * Add the trait bounds * Add `CommitmentState` * Update benches for the new type * Fix the name of local variable * Merge `PCCommitmentState` with `PCRandomness` * Update `README.md` * Fix a bug * Put `Randomness` in `CommitmentState` * Add a comment * Remove the extra loop * Update the comment for `CommitmentState` Co-authored-by: Marcin <marcin.gorny.94@protonmail.com> * cargo fmt --------- Co-authored-by: Marcin <marcin.gorny.94@protonmail.com> * `batch_mul_with_preprocessing` no longer takes `self` as argument (arkworks-rs#142) * batch_mul_with_preprocessing no longer takes `self` as argument * Apply suggestions from code review Co-authored-by: Pratyush Mishra <pratyush795@gmail.com> * fix variable name --------- Co-authored-by: Pratyush Mishra <pratyush795@gmail.com> * Remove `ChallengeGenerator` for Brakedown (#53) * Squash and merge `delete-chalgen` onto here * Fix Brakedown for `ChallengeGenerator` and `AsRef` for Merkle tree * Remove `IOPTranscript` (#52) * Replace the `IOPTranscript` with `CryptographicSponge` * Delete extra comments * Delete TODOs and do not absorb what you just squeezed * Remove the extra loop * Revert the incorrect changes in `bench-tamplates` --------- Co-authored-by: mmagician <marcin.gorny.94@protonmail.com> Co-authored-by: Pratyush Mishra <pratyush795@gmail.com>

* Add the trait bounds * Add `CommitmentState` * Update benches for the new type * Fix the name of local variable * Merge `PCCommitmentState` with `PCRandomness` * Update `README.md` * Fix a bug * Change `Randomness` to `CommitmentState` * Maybe `empty` not return `Self` * Make `empty` return `Self` * Rename `rand` to `state` * Partially integrate the new design into Hyrax * Update Hyrax with the shared state * Rename nonnative to emulated, as in `r1cs-std` (arkworks-rs#137) * Rename nonnative to emulated, as in `r1cs-std` * Run `fmt` * Temporarily change `Cargo.toml` * Revert `Cargo.toml` * Refactor `FoldedPolynomialStream` partially * Substitute `ChallengeGenerator` by the generic sponge (arkworks-rs#139) * Rename nonnative to emulated, as in `r1cs-std` * Run `fmt` * Temporarily change `Cargo.toml` * Substitute `ChallengeGenerator` with the generic sponge * Run `fmt` * Remove the extra file * Update modules * Delete the unnecessary loop * Revert `Cargo.toml` * Refactor `FoldedPolynomialStream` partially * Update README * Make the diff more readable * Bring the whitespace back * Make diff more readable, 2 * Fix according to breaking changes in `ark-ec` (arkworks-rs#141) * Fix for KZG10 * Fix the breaking changes in `ark-ec` * Remove the extra loop * Fix the loop range * re-use the preprocessing table * also re-use the preprocessing table for multilinear_pc --------- Co-authored-by: mmagician <marcin.gorny.94@protonmail.com> * Auxiliary opening data (arkworks-rs#134) * Add the trait bounds * Add `CommitmentState` * Update benches for the new type * Fix the name of local variable * Merge `PCCommitmentState` with `PCRandomness` * Update `README.md` * Fix a bug * Put `Randomness` in `CommitmentState` * Add a comment * Remove the extra loop * Update the comment for `CommitmentState` Co-authored-by: Marcin <marcin.gorny.94@protonmail.com> * cargo fmt --------- Co-authored-by: Marcin <marcin.gorny.94@protonmail.com> * `batch_mul_with_preprocessing` no longer takes `self` as argument (arkworks-rs#142) * batch_mul_with_preprocessing no longer takes `self` as argument * Apply suggestions from code review Co-authored-by: Pratyush Mishra <pratyush795@gmail.com> * fix variable name --------- Co-authored-by: Pratyush Mishra <pratyush795@gmail.com> * Remove ChallengeGenerator for Ligero (#56) * Squash and merge `delete-chalgen` onto here * Fix for `ChallengeGenerator` * Delete `IOPTranscript` for Hyrax (#55) * Use the sponge generic and rearrange `use`s * Use sponge instead of `IOPTransript` * Fix benches * Remove the extra loop --------- Co-authored-by: mmagician <marcin.gorny.94@protonmail.com> Co-authored-by: Pratyush Mishra <pratyush795@gmail.com>

* Add the trait bounds * Add `CommitmentState` * Update benches for the new type * Fix the name of local variable * Merge `PCCommitmentState` with `PCRandomness` * Update `README.md` * Fix a bug * Simplify `hash_column` * Delete comments * Add `CommitmentState` * Make `fmt` happy * Refactor, remove `hash_columns` * Rename all params * Maybe `empty` not return `Self` * Make `empty` return `Self` * Rename `rand` to `state` * Add type `Randomness` * Ligero+++ (#46) * conversion to `into_iter` is a no-op * remove explicit casts to vecs * rename to use singular of `labeled_commitment` * simplify the iterators even further by zipping two iters * Apply suggestions from code review * Fix tests: sponge config for univariate ligero * Rename nonnative to emulated, as in `r1cs-std` (arkworks-rs#137) * Rename nonnative to emulated, as in `r1cs-std` * Run `fmt` * Temporarily change `Cargo.toml` * Revert `Cargo.toml` * Refactor `FoldedPolynomialStream` partially * Substitute `ChallengeGenerator` by the generic sponge (arkworks-rs#139) * Rename nonnative to emulated, as in `r1cs-std` * Run `fmt` * Temporarily change `Cargo.toml` * Substitute `ChallengeGenerator` with the generic sponge * Run `fmt` * Remove the extra file * Update modules * Delete the unnecessary loop * Revert `Cargo.toml` * Refactor `FoldedPolynomialStream` partially * Update README * Make the diff more readable * Bring the whitespace back * Make diff more readable, 2 * Fix according to breaking changes in `ark-ec` (arkworks-rs#141) * Fix for KZG10 * Fix the breaking changes in `ark-ec` * Remove the extra loop * Fix the loop range * re-use the preprocessing table * also re-use the preprocessing table for multilinear_pc --------- Co-authored-by: mmagician <marcin.gorny.94@protonmail.com> * Auxiliary opening data (arkworks-rs#134) * Add the trait bounds * Add `CommitmentState` * Update benches for the new type * Fix the name of local variable * Merge `PCCommitmentState` with `PCRandomness` * Update `README.md` * Fix a bug * Put `Randomness` in `CommitmentState` * Add a comment * Remove the extra loop * Update the comment for `CommitmentState` Co-authored-by: Marcin <marcin.gorny.94@protonmail.com> * cargo fmt --------- Co-authored-by: Marcin <marcin.gorny.94@protonmail.com> * `batch_mul_with_preprocessing` no longer takes `self` as argument (arkworks-rs#142) * batch_mul_with_preprocessing no longer takes `self` as argument * Apply suggestions from code review Co-authored-by: Pratyush Mishra <pratyush795@gmail.com> * fix variable name --------- Co-authored-by: Pratyush Mishra <pratyush795@gmail.com> * Remove `ChallengeGenerator` and `IOPTranscript` for Ligero (#57) * Squash and merge `delete-chalgen` onto here * Fix Ligero for `ChallengeGenerator` and `AsRef` for Merkle tree * Fix tests: sponge config for univariate ligero * Delete `IOPTranscript` for Ligero (#54) * Replace the `IOPTranscript` with `CryptographicSponge` * Delete extra comments * Run fmt * Fix tests: sponge config for univariate ligero * Delete TODOs and do not absorb what you just squeezed * Fix unused import * Revert "Fix unused import" This reverts commit e85af90. * Try to fix * Remove the extra loop --------- Co-authored-by: mmagician <marcin.gorny.94@protonmail.com> Co-authored-by: Pratyush Mishra <pratyush795@gmail.com>

autquis added 4 commits January 3, 2024 12:31

Rename nonnative to emulated, as in r1cs-std

c7cf27a

Run fmt

3de0185

Temporarily change Cargo.toml

9d89eb1

Substitute ChallengeGenerator with the generic sponge

be44981

autquis requested a review from a team as a code owner January 5, 2024 10:13

autquis requested review from Pratyush, mmagician and weikengchen and removed request for a team January 5, 2024 10:13

autquis changed the title ~~Substitute ChallengeGenerator with the generic sponge~~ Substitute ChallengeGenerator by the generic sponge Jan 5, 2024

autquis added 2 commits January 5, 2024 11:14

Run fmt

5cac917

Remove the extra file

a12636f

autquis commented Jan 5, 2024

View reviewed changes

poly-commit/src/lib.rs Outdated Show resolved Hide resolved

autquis added 8 commits January 5, 2024 11:20

Update modules

0b469d1

Delete the unnecessary loop

b719b00

Revert Cargo.toml

0c3497a

Refactor FoldedPolynomialStream partially

11f9d72

Update README

dd4bcb9

Make the diff more readable

957bbc8

Bring the whitespace back

9053651

Make diff more readable, 2

3f10550

autquis commented Jan 8, 2024

View reviewed changes

Pratyush approved these changes Jan 14, 2024

View reviewed changes

mmagician mentioned this pull request Jan 14, 2024

Challenge generator should absorb commitments before squeezing during batching operations #140

Open

Pratyush added this pull request to the merge queue Jan 14, 2024

Merged via the queue into arkworks-rs:master with commit 3291693 Jan 14, 2024
4 checks passed

This was referenced Jan 15, 2024

Fix according to breaking changes in ark-ec #141

Merged

PolynomialCommitment trait simplification #116

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Substitute `ChallengeGenerator` by the generic sponge #139

Substitute `ChallengeGenerator` by the generic sponge #139

autquis commented Jan 5, 2024 •

edited

Loading

autquis Jan 8, 2024

Pratyush Jan 14, 2024

Pratyush commented Jan 8, 2024

autquis commented Jan 9, 2024 •

edited

Loading

mmagician commented Jan 9, 2024

autquis commented Jan 9, 2024

Pratyush commented Jan 9, 2024

Pratyush commented Jan 9, 2024

mmagician commented Jan 9, 2024 •

edited

Loading

Pratyush left a comment

Substitute ChallengeGenerator by the generic sponge #139

Substitute ChallengeGenerator by the generic sponge #139

Conversation

autquis commented Jan 5, 2024 • edited Loading

Description

autquis Jan 8, 2024

Choose a reason for hiding this comment

Pratyush Jan 14, 2024

Choose a reason for hiding this comment

Pratyush commented Jan 8, 2024

autquis commented Jan 9, 2024 • edited Loading

mmagician commented Jan 9, 2024

autquis commented Jan 9, 2024

Pratyush commented Jan 9, 2024

Pratyush commented Jan 9, 2024

mmagician commented Jan 9, 2024 • edited Loading

Pratyush left a comment

Choose a reason for hiding this comment

Substitute `ChallengeGenerator` by the generic sponge #139

Substitute `ChallengeGenerator` by the generic sponge #139

autquis commented Jan 5, 2024 •

edited

Loading

autquis commented Jan 9, 2024 •

edited

Loading

mmagician commented Jan 9, 2024 •

edited

Loading