Skip to content

Remove IPA / Part1 #377

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.

Sign up for GitHub

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

Merged
adria0 merged 5 commits into from
Nov 11, 2024
Merged

Remove IPA / Part1 #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e69386e
Remove IPA base
Oct 17, 2024
ba23a98
No longer commitments to instance in transcript.
Oct 18, 2024
ec3ab13
Remove binding from powerquery, fix docs
Oct 18, 2024
012e5c7
Merge branch 'main' of github.com:privacy-scaling-explorations/halo2 …
Oct 24, 2024
b6f4082
Revert book changes.
Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion halo2_backend/src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ pub enum SerdeFormat {
RawBytesUnchecked,
}

// Keep this trait for compatibility with IPA serialization
Comment thread
guorong009 marked this conversation as resolved.
pub trait CurveRead: CurveAffine {
/// Reads a compressed element from the buffer and attempts to parse it
/// using `from_bytes`.
Expand Down
21 changes: 3 additions & 18 deletions halo2_backend/src/plonk/lookup/prover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,16 @@ pub(in crate::plonk) struct Permuted<C: CurveAffine> {
compressed_input_expression: Polynomial<C::Scalar, LagrangeCoeff>,
permuted_input_expression: Polynomial<C::Scalar, LagrangeCoeff>,
permuted_input_poly: Polynomial<C::Scalar, Coeff>,
permuted_input_blind: Blind<C::Scalar>,

@davidnevadoc davidnevadoc Oct 23, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why are the blinders removed? 🤔

@adria0 adria0 Oct 24, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ditto (Please, take note that I removed everything that the compiler said as "unused", that are just few things.)

@guorong009 guorong009 Oct 25, 2024 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I did some research.
Found that blinders are really used in IPA commitment scheme.
But, when @kilic and @han0110 introduce the KZG commitment scheme, they didn't use this blinder/r in commitment scheme.
51d523d#diff-928bb816879ab4beb4d0ad6d02b06d158f760aa2e95f410b2694e0f803e57486R167-R171
I believe the reason is that it(ParamsKZG::commit_lagrange) should be compatible with existing IPA scheme, at that time.
In other words, the blinder/r is not needed for KZG scheme.

If so, we can safely remove all of these blinder/r features from codebase.
Plus, update ParamsKZG::commit_lagrange function definition.

Overall, I think it is better to get to know exact reason, before removing this feature from codebase.

@adria0 adria0 Oct 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Overall, I think it is better to get to know exact reason, before removing this feature from codebase.

Sure, more knowledge is always good.

@guorong009, are you talking about changing the function definition from

    /// This commits to a polynomial using its evaluations over the $2^k$ size
    /// evaluation domain. The commitment will be blinded by the blinding factor
    /// `r`.
    fn commit_lagrange(
        &self,
        engine: &impl MsmAccel<C>,
        poly: &Polynomial<C::ScalarExt, LagrangeCoeff>,
        r: Blind<C::ScalarExt>,
    ) -> C::CurveExt;

to ?

    /// This commits to a polynomial using its evaluations over the $2^k$ size
    /// evaluation domain.
    fn commit_lagrange(
        &self,
        engine: &impl MsmAccel<C>,
        poly: &Polynomial<C::ScalarExt, LagrangeCoeff>
    ) -> C::CurveExt;

if yes, this needs refactoring in other parts of the code that I think that is better to do it in another PR.

@guorong009 guorong009 Oct 25, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also, Params::commit function. @adria0

@davidnevadoc davidnevadoc Oct 25, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm looking a bit more into this. I hadn't realized that KZG was completely ignoring blinders.
I don't think there is any bugs in the proving system though, because it was always being called with Blind::default().

It seems wrong that there is no possibility to blind the witnesses... where is the zk then?
This has been a point of discussion in the past (#73 #76) but I ignore what came out of these.

@han0110 Could you comment on this? Are we blinding witnesses at some other level, or did we just remove zk?

@guorong009 guorong009 Oct 28, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we still have zk, by blinding witnesses at other level.
The ProverMulti::commit_phase has the logic of adding random scalars into blinding_factors rows of witness/advice columns.
The columns to be blinded or not, are determined by unblinded_advice_columns.
I think nobody uses this feature in circuit development.

In above 2 PRs, they discuss whether/how to remove the blinding_factors row logic from the codebase.
As long as we "blind" some rows of witness columns, there would be no way for exposing the witness polys as is.
Hence, I think we can remove extra blinder/r feature from codebase.
I believe they come from the IPA scheme which needs explicit hiding value - r, for poly commitment.

Anyway, hope to get confirmation from @han0110 or @kilic .

@davidnevadoc davidnevadoc Oct 28, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see. If that is the case I'm fine with removing the blinders 👍

compressed_table_expression: Polynomial<C::Scalar, LagrangeCoeff>,
permuted_table_expression: Polynomial<C::Scalar, LagrangeCoeff>,
permuted_table_poly: Polynomial<C::Scalar, Coeff>,
permuted_table_blind: Blind<C::Scalar>,
}

#[derive(Debug)]
pub(in crate::plonk) struct Committed<C: CurveAffine> {
pub(in crate::plonk) permuted_input_poly: Polynomial<C::Scalar, Coeff>,
permuted_input_blind: Blind<C::Scalar>,
pub(in crate::plonk) permuted_table_poly: Polynomial<C::Scalar, Coeff>,
permuted_table_blind: Blind<C::Scalar>,
pub(in crate::plonk) product_poly: Polynomial<C::Scalar, Coeff>,
product_blind: Blind<C::Scalar>,
}

pub(in crate::plonk) struct Evaluated<C: CurveAffine> {
Expand Down Expand Up @@ -130,15 +125,15 @@ where
let poly = pk.vk.domain.lagrange_to_coeff(values.clone());
let blind = Blind(C::Scalar::random(&mut rng));
let commitment = params.commit_lagrange(&engine.msm_backend, values, blind);
(poly, blind, commitment)
(poly, commitment)
};

// Commit to permuted input expression
let (permuted_input_poly, permuted_input_blind, permuted_input_commitment_projective) =
let (permuted_input_poly, permuted_input_commitment_projective) =
commit_values(&permuted_input_expression);

// Commit to permuted table expression
let (permuted_table_poly, permuted_table_blind, permuted_table_commitment_projective) =
let (permuted_table_poly, permuted_table_commitment_projective) =
commit_values(&permuted_table_expression);

let [permuted_input_commitment, permuted_table_commitment] = {
Expand All @@ -163,11 +158,9 @@ where
compressed_input_expression,
permuted_input_expression,
permuted_input_poly,
permuted_input_blind,
compressed_table_expression,
permuted_table_expression,
permuted_table_poly,
permuted_table_blind,
})
}

Expand Down Expand Up @@ -313,11 +306,8 @@ impl<C: CurveAffine> Permuted<C> {

Ok(Committed::<C> {
permuted_input_poly: self.permuted_input_poly,
permuted_input_blind: self.permuted_input_blind,
permuted_table_poly: self.permuted_table_poly,
permuted_table_blind: self.permuted_table_blind,
product_poly: z,
product_blind,
})
}
}
Expand Down Expand Up @@ -368,31 +358,26 @@ impl<C: CurveAffine> Evaluated<C> {
.chain(Some(ProverQuery {
point: *x,
poly: &self.constructed.product_poly,
blind: self.constructed.product_blind,
}))
// Open lookup input commitments at x
.chain(Some(ProverQuery {
point: *x,
poly: &self.constructed.permuted_input_poly,
blind: self.constructed.permuted_input_blind,
}))
// Open lookup table commitments at x
.chain(Some(ProverQuery {
point: *x,
poly: &self.constructed.permuted_table_poly,
blind: self.constructed.permuted_table_blind,
}))
// Open lookup input commitments at x_inv
.chain(Some(ProverQuery {
point: x_inv,
poly: &self.constructed.permuted_input_poly,
blind: self.constructed.permuted_input_blind,
}))
// Open lookup product commitments at x_next
.chain(Some(ProverQuery {
point: x_next,
poly: &self.constructed.product_poly,
blind: self.constructed.product_blind,
}))
}
}
Expand Down
14 changes: 3 additions & 11 deletions halo2_backend/src/plonk/permutation/prover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ use halo2_middleware::poly::Rotation;
/// It stores a single `Z_P` in [permutation argument specification](https://zcash.github.io/halo2/design/proving-system/permutation.html#argument-specification).
pub(crate) struct CommittedSet<C: CurveAffine> {
pub(crate) permutation_product_poly: Polynomial<C::Scalar, Coeff>,
permutation_product_blind: Blind<C::Scalar>,
}

/// Set of permutation product polynomials, which have been **committed**.
Expand Down Expand Up @@ -181,15 +180,13 @@ pub(in crate::plonk) fn permutation_commit<
let permutation_product_commitment = params
.commit_lagrange(&engine.msm_backend, &z, blind)
.to_affine();
let permutation_product_blind = blind;
let permutation_product_poly = domain.lagrange_to_coeff(z);

// Hash the permutation product commitment
transcript.write_point(permutation_product_commitment)?;

sets.push(CommittedSet {
permutation_product_poly,
permutation_product_blind,
});
}

Expand All @@ -201,11 +198,9 @@ impl<C: CurveAffine> super::ProvingKey<C> {
&self,
x: ChallengeX<C>,
) -> impl Iterator<Item = ProverQuery<'_, C>> + Clone {
self.polys.iter().map(move |poly| ProverQuery {
point: *x,
poly,
blind: Blind::default(),
})
self.polys
.iter()
.map(move |poly| ProverQuery { point: *x, poly })
}

pub(in crate::plonk) fn evaluate<E: EncodedChallenge<C>, T: TranscriptWrite<C, E>>(
Expand Down Expand Up @@ -289,12 +284,10 @@ impl<C: CurveAffine> Evaluated<C> {
.chain(Some(ProverQuery {
point: *x,
poly: &set.permutation_product_poly,
blind: set.permutation_product_blind,
}))
.chain(Some(ProverQuery {
point: x_next,
poly: &set.permutation_product_poly,
blind: set.permutation_product_blind,
}))
}))
// Open it at \omega^{last} x for all but the last set. This rotation is only
Expand All @@ -310,7 +303,6 @@ impl<C: CurveAffine> Evaluated<C> {
Some(ProverQuery {
point: x_last,
poly: &set.permutation_product_poly,
blind: set.permutation_product_blind,
})
}),
)
Expand Down
87 changes: 6 additions & 81 deletions halo2_backend/src/plonk/prover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,39 +207,14 @@ impl<
return Err(Error::InstanceTooLarge);
}
for (poly, value) in poly.iter_mut().zip(values.iter()) {
if !P::QUERY_INSTANCE {
// Add to the transcript the instance polynomials lagrange value.
transcript.common_scalar(*value)?;
}
// Add to the transcript the instance polynomials lagrange value.
transcript.common_scalar(*value)?;
*poly = *value;
}
Ok(poly)
})
.collect::<Result<Vec<_>, _>>()?;

if P::QUERY_INSTANCE {
// Add to the transcript the commitments of the instance lagrange polynomials

let instance_commitments_projective: Vec<_> = instance_values
.iter()
.map(|poly| {
params.commit_lagrange(&engine.msm_backend, poly, Blind::default())
})
.collect();
let mut instance_commitments =
vec![Scheme::Curve::identity(); instance_commitments_projective.len()];
<Scheme::Curve as CurveAffine>::CurveExt::batch_normalize(
&instance_commitments_projective,
&mut instance_commitments,
);
let instance_commitments = instance_commitments;
drop(instance_commitments_projective);

for commitment in &instance_commitments {
transcript.common_point(*commitment)?;
}
}

// Convert from evaluation to coefficient form.

let instance_polys: Vec<_> = instance_values
Expand Down Expand Up @@ -587,9 +562,6 @@ impl<

let x_pow_n = x.pow([self.params.n()]);

// [TRANSCRIPT-16]
self.write_instance_evals(x)?;

// 10. Compute and hash advice evals for the circuit instance ------------------------------------
// [TRANSCRIPT-17]
self.write_advice_evals(x, &advice)?;
Expand Down Expand Up @@ -622,30 +594,15 @@ impl<
let shuffles_evaluated = self.evaluate_shuffles(x, shuffles_committed)?;

// 13. Generate all queries ([`ProverQuery`]) that needs to be sent to prover --------------------
let instances = std::mem::take(&mut self.instances);
let queries = instances
// group the instance, advice, permutation, lookups and shuffles
// group the advice, permutation, lookups and shuffles
let queries = advice
.iter()
.zip(advice.iter())
.zip(permutations_evaluated.iter())
.zip(lookups_evaluated.iter())
.zip(shuffles_evaluated.iter())
.flat_map(|((((instance, advice), permutation), lookups), shuffles)| {
.flat_map(|(((advice, permutation), lookups), shuffles)| {
// Build a (an iterator) over a set of ProverQueries for each instance, advice, permutatiom, lookup and shuffle
iter::empty()
// Instances
.chain(
P::QUERY_INSTANCE
.then_some(self.pk.vk.cs.instance_queries.iter().map(
move |&(column, at)| ProverQuery {
point: self.pk.vk.domain.rotate_omega(*x, at),
poly: &instance.instance_polys[column.index],
blind: Blind::default(),
},
))
.into_iter()
.flatten(),
)
// Advices
.chain(
self.pk
Expand All @@ -656,7 +613,6 @@ impl<
.map(move |&(column, at)| ProverQuery {
point: self.pk.vk.domain.rotate_omega(*x, at),
poly: &advice.advice_polys[column.index],
blind: advice.advice_blinds[column.index],
}),
)
// Permutations
Expand All @@ -676,7 +632,7 @@ impl<
.map(|&(column, at)| ProverQuery {
point: self.pk.vk.domain.rotate_omega(*x, at),
poly: &self.pk.fixed_polys[column.index],
blind: Blind::default(),
// blind: Blind::default(),
}),
)
// Copy constraints
Expand Down Expand Up @@ -909,37 +865,6 @@ impl<
Ok(vanishing)
}

fn write_instance_evals(&mut self, x: ChallengeX<Scheme::Curve>) -> Result<(), Error>
where
Scheme::Scalar: WithSmallOrderMulGroup<3> + FromUniformBytes<64>,
{
if P::QUERY_INSTANCE {
// Compute and hash instance evals for the circuit instance
for instance in self.instances.iter() {
// Evaluate polynomials at omega^i x
let instance_evals: Vec<_> = self
.pk
.vk
.cs
.instance_queries
.iter()
.map(|&(column, at)| {
eval_polynomial(
&instance.instance_polys[column.index],
self.pk.vk.domain.rotate_omega(*x, at),
)
})
.collect();

// Hash each instance column evaluation
for eval in instance_evals.iter() {
self.transcript.write_scalar(*eval)?;
}
}
}
Ok(())
}

fn write_advice_evals(
&mut self,
x: ChallengeX<Scheme::Curve>,
Expand Down
8 changes: 1 addition & 7 deletions halo2_backend/src/plonk/shuffle/prover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ struct Compressed<C: CurveAffine> {
#[derive(Debug)]
pub(in crate::plonk) struct Committed<C: CurveAffine> {
pub(in crate::plonk) product_poly: Polynomial<C::Scalar, Coeff>,
product_blind: Blind<C::Scalar>,
}

pub(in crate::plonk) struct Evaluated<C: CurveAffine> {
Expand Down Expand Up @@ -198,10 +197,7 @@ where
// Hash product commitment
transcript.write_point(product_commitment)?;

Ok(Committed::<C> {
product_poly: z,
product_blind,
})
Ok(Committed::<C> { product_poly: z })
}

impl<C: CurveAffine> Committed<C> {
Expand Down Expand Up @@ -242,13 +238,11 @@ impl<C: CurveAffine> Evaluated<C> {
.chain(Some(ProverQuery {
point: *x,
poly: &self.constructed.product_poly,
blind: self.constructed.product_blind,
}))
// Open shuffle product commitments at x_next
.chain(Some(ProverQuery {
point: x_next,
poly: &self.constructed.product_poly,
blind: self.constructed.product_blind,
}))
}
}
Loading