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

Rebased logUp implementation #1235

Merged
merged 18 commits into from
Sep 27, 2023
Merged

Conversation

LindaGuiga
Copy link
Contributor

This PR corresponds to @wborgeaud's implementation of logUp, rebased on the new main.
There are a few changes from the original implementation, as this PR takes into account @ulrich-haboeck's comments on the original PR. The changes are:

  • we use CTL challenges for logUp (instead of generating new challenges)
  • we remove one helper column by changing the Z polynomial verification, as per the suggestions
  • additional asserts: sanity checks in ArithmeticStark and BytePackingStark to ensure the value is below the maximal value, and: total_num_logUp_entries < char(F)
  • changes in the comments following the suggestions
  • a change in function names: eval_lookups_checks -> eval_packed_lookups_generic and eval_lookups_checks_circuit -> eval_ext_lookups_circuit, to follow suggestions.

A couple of comments are not addressed here:

  • "Also asked at a different place: Why are local and next values vectors, whereas for StarkEvaluationVars they are slices." => I believe the reason why LookupCheckVars have Vecs while StarkEvaluationVars have slices is that in the Lookup case, we do not know the size of the local and next values in advance.
  • "I might miss something here, but why do we want to have the values of other auxiliary polys (besides the running sums/products of the lookups) at the shifted value g*x (i.e. the point corresponding to the next step in time)? Is that for convenience, as we put all the auxiliary polys under a single Merkle cap?"
  • "Just a side question: Do we ever want to have zk for a STARK?"

@LindaGuiga
Copy link
Contributor Author

We removed the where clauses related to CpuStark columns: [(); CpuStark::<F, D>::COLUMNS], as it seemed to fix the compiler errors. I have no idea why this "fix" works (or why the issue appeared in the first place). Does anyone (for example @dlubarov?) have any insight in this regard?

@Nashtare
Copy link
Collaborator

Nashtare commented Sep 15, 2023

We removed the where clauses related to CpuStark columns: [(); CpuStark::<F, D>::COLUMNS], as it seemed to fix the compiler errors. I have no idea why this "fix" works (or why the issue appeared in the first place). Does anyone (for example @dlubarov?) have any insight in this regard?

@LindaGuiga @dlubarov
I couldn't figure out why the error message was mixing ArithmeticStark and CpuStark, so I spent more time on this weird issue with the where clauses.
After some local experimentations I realized that pulling the latest main changes had removed this error. I can't get why the previous revision was failing, though it seems more likely to be a bug from the unstable feature than anything else.

@pgebheim pgebheim requested a review from wborgeaud September 18, 2023 16:07
Copy link
Contributor

@wborgeaud wborgeaud left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@LindaGuiga LindaGuiga merged commit 1ff6d4a into 0xPolygonZero:main Sep 27, 2023
2 checks passed
@wborgeaud wborgeaud mentioned this pull request Oct 2, 2023
@Nashtare Nashtare deleted the new-logup branch January 9, 2024 13:50
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.

3 participants