Remove IOPTranscript
#52
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mmagician
reviewed
Jan 9, 2024
poly-commit/src/linear_codes/mod.rs
Outdated
| } | ||
| // TODO Check this: we changed, now we squeeze all elements and then abosrb all of them | ||
| let r = sponge.squeeze_field_elements::<F>(n_rows); | ||
| sponge.absorb(&r); |
There was a problem hiding this comment.
why are we doing this, actually? I'm no expert on building the IOP transcripts, but it seems counterintuitive to reabsorb whatever we just squeezed. Maybe @mmaker would know if it would be safe to just remove that?
There was a problem hiding this comment.
yes it's just useless, better to remove it
There was a problem hiding this comment.
I don't know either. I just imitated get_and_append_challenge here.
poly-commit/src/linear_codes/mod.rs
Outdated
| .append_serializable_element(b"point", element) | ||
| .map_err(|_| Error::TranscriptError)?; | ||
| } | ||
| // TODO Why not absorbing the vector all at once? |
There was a problem hiding this comment.
So, previously we iterated over the vector and appended each item to the transcript. I wonder why we didn't append the entire vector. That's what I did here, and I wanted to double-check with you if it is okay.
There was a problem hiding this comment.
+1 on absorbing directly the vector, but you are also absorbing the length here-- I'm hoping to remove the need for it altogether. I assume verification is changed accordingly ?
There was a problem hiding this comment.
Thanks! Yes, the verification is symmetric to this part too.
mmaker
approved these changes
Jan 9, 2024
poly-commit/src/linear_codes/mod.rs
Outdated
| } | ||
| // TODO Check this: we changed, now we squeeze all elements and then abosrb all of them | ||
| let r = sponge.squeeze_field_elements::<F>(n_rows); | ||
| sponge.absorb(&r); |
There was a problem hiding this comment.
yes it's just useless, better to remove it
poly-commit/src/linear_codes/mod.rs
Outdated
| .append_serializable_element(b"point", element) | ||
| .map_err(|_| Error::TranscriptError)?; | ||
| } | ||
| // TODO Why not absorbing the vector all at once? |
There was a problem hiding this comment.
+1 on absorbing directly the vector, but you are also absorbing the length here-- I'm hoping to remove the need for it altogether. I assume verification is changed accordingly ?
autquis
added a commit
that referenced
this pull request
Jan 17, 2024
* 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
autquis
added a commit
that referenced
this pull request
Jan 17, 2024
* 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is just for discussion regarding this change.
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.
Pendingsection inCHANGELOG.mdFiles changedin the Github PR explorer