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

fix!: correct the compile_circuit #320

Closed
wants to merge 10 commits into from
Closed

Conversation

guorong009
Copy link

@guorong009 guorong009 commented Apr 29, 2024

Description

Correct the logic of converting selectors to fixed columns in compile_circuit

Related issues

  • None

Changes

  • update compile_circuit
    add the logic of considering the selectors_poly when computing fixed columns
  • add a new ci check for running the examples
    • create a new sh script to test the successful run of examples
    • add ci check in github actions

Context

If you try to run the shuffle example in current main branch, you can find that it is failing here.

cargo run --package halo2_proofs --example shuffle 

The reason is that there is tiny error in the implementation converting selectors to fixed column, in compile_circuit. This PR resolves this issue.

#[cfg(feature = "circuit-params")] params: ConcreteCircuit::Params,
) -> io::Result<VerifyingKey<C>>
where
C::Scalar: SerdePrimeField + FromUniformBytes<64>,
{
let (_, cs, _) = compile_circuit_cs::<_, ConcreteCircuit>(
compress_selectors,
let (_, cs) = compile_circuit_cs::<_, ConcreteCircuit>(
Copy link
Member

Choose a reason for hiding this comment

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

There's a detail here that I don't understand. With this change the cs we get here has selector columns (they have not yet been converted to fixed columns).

Then afterwards we convert the ConstraintSystem into ConstraintSystemMid. In that conversion we're converting Expression into ExpressionMid, and if the Expression has a selector it will panic:

Expression::Selector(_) => unreachable!(),

So I think that calling vk_read or pk_read for a circuit that uses Selectors will panic.

Copy link
Author

@guorong009 guorong009 Apr 30, 2024

Choose a reason for hiding this comment

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

Hmm, I don't think so.
The pk_read & vk_read are utilities to read the keys that were already compiled & stored somewhere.
In other word, the keys include the info of fixed columns converted from Selectors, not Selectors.

@ed255
Copy link
Member

ed255 commented Apr 29, 2024

If you try to run the shuffle example in current main branch, you can find that it is failing here.

cargo run --package halo2_proofs --example shuffle 

I think this is showing a big problem we have, we don't have enough unit tests, and the real tests that we have are examples but they are not run via github actions. I think we should run the examples as check via github actions for every PR.

@davidnevadoc davidnevadoc requested review from ed255 and removed request for adria0 and CPerezz April 29, 2024 16:37
@guorong009
Copy link
Author

If you try to run the shuffle example in current main branch, you can find that it is failing here.

cargo run --package halo2_proofs --example shuffle 

I think this is showing a big problem we have, we don't have enough unit tests, and the real tests that we have are examples but they are not run via github actions. I think we should run the examples as check via github actions for every PR.

Yes, I agree. @ed255
Here, I add the CI to check examples running. 5627fce 41e4da2

@guorong009 guorong009 changed the title fix!: correct the compile_circuit_cs fix!: correct the compile_circuit Apr 30, 2024
@guorong009 guorong009 marked this pull request as draft April 30, 2024 16:37
@ed255 ed255 mentioned this pull request May 2, 2024
@guorong009
Copy link
Author

Close this PR in favor of #322 .

@guorong009 guorong009 closed this May 7, 2024
@guorong009 guorong009 deleted the gr@fix-compile-circuit-cs branch May 7, 2024 10:24
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.

2 participants