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

Implement Lasso inside Hyperplonk #4

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Implement Lasso inside Hyperplonk #4

wants to merge 27 commits into from

Conversation

DoHoonKim8
Copy link
Owner

@DoHoonKim8 DoHoonKim8 commented Oct 31, 2023

Implement Lasso inside Hyperplonk.
Can run the test

cargo test --release --package plonkish_backend --lib -- backend::lookup::lasso::test::and::test --exact --nocapture
cargo test --release --package plonkish_backend --lib -- backend::lookup::lasso::test::range::test --exact --nocapture

The remaining things to do:

@DoHoonKim8 DoHoonKim8 marked this pull request as ready for review November 2, 2023 09:24
@ed255 ed255 self-requested a review November 2, 2023 09:55
@DoHoonKim8 DoHoonKim8 marked this pull request as draft November 5, 2023 07:58
@DoHoonKim8 DoHoonKim8 marked this pull request as ready for review November 6, 2023 08:33
Copy link
Collaborator

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

Initial review.
I've done an overview of the PR, here are some general comments:

  • I didn't review the memory checking, sumcheck and GKR implementations
  • I'm not sure if all the contents of this PR are related to Lasso, or there's extra code (there's a file that seems to implement logup for example)
  • I'm not sure if the code in plonkish_backend/src/frontend/halo2/lookup.rs is used. It defines two traits, but they are unused for now?
  • My understanding is that the codebase starts from Han's plonkish repository and then implements Lasso on top of it. But the git history from the plonkish repository is not here? maybe you copied the code into this repository? I highly recommend implementing Lasso on top of Han's plonkish repository keeping all the commit history from the plonkish repository; because that will make it much easier later to merge this implementation to Han's repo, or backport changes from Han's repo into here.

plonkish_backend/src/backend.rs Show resolved Hide resolved
plonkish_backend/src/backend.rs Outdated Show resolved Hide resolved
plonkish_backend/src/backend/hyperplonk.rs Show resolved Hide resolved
plonkish_backend/src/backend/hyperplonk.rs Show resolved Hide resolved
// let lookup_constraints = lookup_constraints.map(Cow::Borrowed).unwrap_or_else(|| {
// let dummy_challenge = Expression::zero();
// Cow::Owned(self::lookup_constraints(circuit_info, &dummy_challenge, &dummy_challenge).0)
// });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we include here the degree of the lasso lookup expression used to calculate the lookup indices?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah so this part is building expression to be used for zero sumcheck in HyperPlonK, and previously LogUp sumcheck is batched with HyperPlonK zero sumcheck. In case of Lasso, at the time of implementing it, I thought that Lasso sumcheck cannot be batched into HyperPlonK zero sumcheck since Lasso paper describes invoking sumcheck in the middle of IOP. So I just removed this part and built expression for Lasso lookup independently.

For now, I think we can swap the order of Lasso sumcheck and offline memory checking and then batch sumcheck with HyperPlonK zero sumcheck. Can you share your thought on this? @han0110 @ed255

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that makes sense, ideally we only need to invoke one sum check, but currently the sum check only support batching of same number of variates, so we will need 2 for Lasso + HyperPlonk

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I don't understand this fully.

Imagine we have a circuit with 4 columns a, b, c, d.
Then we have one constraint a + b * c - d (degree = 2)
And one lasso lookup with input expression a * b * c * d (degree = 4)

What should the max_degree of the circuit be?
I have search where max_degree is used and only found it's used to batch permutation checks into chunks. What else does the max_degree affect?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah that makes sense, ideally we only need to invoke one sum check, but currently the sum check only support batching of same number of variates, so we will need 2 for Lasso + HyperPlonk

In this case Lasso and HyperPlonk sumchecks have the same number of variates, so I think they can be batched. Am I misunderstanding something?

plonkish_backend/src/backend/lookup/logup.rs Outdated Show resolved Hide resolved
plonkish_backend/src/frontend/halo2/lookup.rs Outdated Show resolved Hide resolved
@DoHoonKim8
Copy link
Owner Author

  • My understanding is that the codebase starts from Han's plonkish repository and then implements Lasso on top of it. But the git history from the plonkish repository is not here? maybe you copied the code into this repository? I highly recommend implementing Lasso on top of Han's plonkish repository keeping all the commit history from the plonkish repository; because that will make it much easier later to merge this implementation to Han's repo, or backport changes from Han's repo into here.

Thanks for pointing out this. It's my mistake to remove all the git history. I will fix this.

For the reason I transferred the code from plonkish to halo2-lasso because Lasso implementation on top of plonkish makes huge changes to the original codebase which makes impossible to merge into Han's repo later(also merge into here from upstream). So I thought that it would be better to have separate repo only for this work.(Ofc I wrote at readme that this repo is fork of plonkish )

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.

4 participants