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.

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

refactor: use BigEndian for SolidityTranscript #619

Merged
merged 9 commits into from
Jul 3, 2024
Merged

Conversation

alxiong
Copy link
Contributor

@alxiong alxiong commented Jun 25, 2024

closes: #618

This PR:

Changes behavior of SolidityTranscript, see the linked issue for description

This PR does not:

Question: should we also optionally append tau for circuits that does support lookup? currently in contract, we have to do a useless get_and_append_challenge call for this unused challenge.

@chancharles92 @philippecamacho


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 (main)
  • 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 relevant changelog entries to the CHANGELOG.md of touched crates.
  • Re-reviewed Files changed in the GitHub PR explorer

@alxiong alxiong requested a review from mrain as a code owner June 25, 2024 11:29
@alxiong alxiong force-pushed the patch-sol-transcript branch from c516b79 to f08c3d0 Compare June 25, 2024 14:12
@alxiong
Copy link
Contributor Author

alxiong commented Jun 26, 2024

Somehow there's still one test failed after ce7cff8.

after some digging, I'm unable to locate the cause. @chancharles92 you are probably more familiar with this gadget, do you have any clue where did i forget to update?


update: fixed in 44b7c42

@alxiong
Copy link
Contributor Author

alxiong commented Jun 28, 2024

reminder for myself after I address comments for this PR:

  • update changelog, and bump version as I forgot to do in a previous PR

mrain
mrain previously approved these changes Jul 1, 2024
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

Generally LGTM.

I agree with the removal of state field inside SolidityTranscript struct, because anyway it's not used to construct the hasher. Same field also exists in the RescueTranscript, we should consider removing that as well.

@philippecamacho
Copy link
Contributor

Should not we add an if statement to raise an error here if the plonk instance does not support lookups?

philippecamacho
philippecamacho previously approved these changes Jul 1, 2024
Copy link
Contributor

@philippecamacho philippecamacho left a comment

Choose a reason for hiding this comment

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

Added a minor comment. LGTM.

@alxiong
Copy link
Contributor Author

alxiong commented Jul 2, 2024

because anyway it's not used to construct the hasher.

yes, but previously, we were using it. in this PR, we removed it.

Same field also exists in the RescueTranscript, we should consider removing that as well.

that would require refactoring of the recursion circuit, since we are not using it atm, maybe we leave it as it is for now.

cc @mrain

@alxiong
Copy link
Contributor Author

alxiong commented Jul 2, 2024

Should not we add an if statement to raise an error here if the plonk instance does not support lookups?

we have already does the protection from the caller side: in snark.rs

            let plookup_evals = if circuits[i].support_lookup() {
                let evals = prover.compute_plookup_evaluations(
                    prove_keys[i],
                    &challenges,
                    &online_oracles[i],
                )?;
                transcript.append_plookup_evaluations::<E>(&evals)?;
                Some(evals)
            } else {
                None
            };

we only invoke it if .supports_lookup() evaluates to true.
cc @philippecamacho

@alxiong alxiong dismissed stale reviews from philippecamacho and mrain via 4c421e7 July 2, 2024 03:34
@alxiong
Copy link
Contributor Author

alxiong commented Jul 2, 2024

maintainer check: @mrain are you happy with jf-plonk-only bump in 4c421e7?

(if so, i'll create a tag named jf-plonk-v0.5.0 later)

Copy link
Contributor

@philippecamacho philippecamacho left a comment

Choose a reason for hiding this comment

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

LGTM

@alxiong alxiong merged commit d468cb0 into main Jul 3, 2024
5 checks passed
@alxiong alxiong deleted the patch-sol-transcript branch July 3, 2024 02:44
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.

More efficient SolidityTranscript
3 participants