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

feat: refactor the verify api #359

Merged
merged 10 commits into from
Sep 27, 2024
26 changes: 13 additions & 13 deletions halo2_backend/src/plonk/prover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ struct AdviceSingle<C: CurveAffine, B: Basis> {
}

/// The prover object used to create proofs interactively by passing the witnesses to commit at
/// each phase. This works for a single proof. This is a wrapper over Prover.
/// each phase. This works for a single proof. This is a wrapper over ProverMulti.
#[derive(Debug)]
pub struct ProverSingle<
pub struct Prover<
'a,
'params,
Scheme: CommitmentScheme,
Expand All @@ -50,7 +50,7 @@ pub struct ProverSingle<
R: RngCore,
T: TranscriptWrite<Scheme::Curve, E>,
M: MsmAccel<Scheme::Curve>,
>(Prover<'a, 'params, Scheme, P, E, R, T, M>);
>(ProverMulti<'a, 'params, Scheme, P, E, R, T, M>);

impl<
'a,
Expand All @@ -61,9 +61,9 @@ impl<
R: RngCore,
T: TranscriptWrite<Scheme::Curve, E>,
M: MsmAccel<Scheme::Curve>,
> ProverSingle<'a, 'params, Scheme, P, E, R, T, M>
> Prover<'a, 'params, Scheme, P, E, R, T, M>
{
/// Create a new prover object
/// Create a new ProverMulti object
pub fn new_with_engine(
engine: PlonkEngine<Scheme::Curve, M>,
params: &'params Scheme::ParamsProver,
Expand All @@ -75,7 +75,7 @@ impl<
where
Scheme::Scalar: WithSmallOrderMulGroup<3> + FromUniformBytes<64>,
{
Ok(Self(Prover::new_with_engine(
Ok(Self(ProverMulti::new_with_engine(
engine,
params,
pk,
Expand All @@ -91,12 +91,12 @@ impl<
instance: Vec<Vec<Scheme::Scalar>>,
rng: R,
transcript: &'a mut T,
) -> Result<ProverSingle<'a, 'params, Scheme, P, E, R, T, H2cEngine>, Error>
) -> Result<Prover<'a, 'params, Scheme, P, E, R, T, H2cEngine>, Error>
where
Scheme::Scalar: WithSmallOrderMulGroup<3> + FromUniformBytes<64>,
{
let engine = PlonkEngineConfig::build_default();
ProverSingle::new_with_engine(engine, params, pk, instance, rng, transcript)
Prover::new_with_engine(engine, params, pk, instance, rng, transcript)
}

/// Commit the `witness` at `phase` and return the challenges after `phase`.
Expand All @@ -123,7 +123,7 @@ impl<
/// The prover object used to create proofs interactively by passing the witnesses to commit at
/// each phase. This supports batch proving.
#[derive(Debug)]
pub struct Prover<
pub struct ProverMulti<
'a,
'params,
Scheme: CommitmentScheme,
Expand Down Expand Up @@ -164,7 +164,7 @@ impl<
R: RngCore,
T: TranscriptWrite<Scheme::Curve, E>,
M: MsmAccel<Scheme::Curve>,
> Prover<'a, 'params, Scheme, P, E, R, T, M>
> ProverMulti<'a, 'params, Scheme, P, E, R, T, M>
{
/// Create a new prover object
pub fn new_with_engine(
Expand Down Expand Up @@ -283,7 +283,7 @@ impl<

let challenges = HashMap::<usize, Scheme::Scalar>::with_capacity(meta.num_challenges);

Ok(Prover {
Ok(ProverMulti {
engine,
params,
pk,
Expand Down Expand Up @@ -903,11 +903,11 @@ impl<
circuits_instances: &[Vec<Vec<Scheme::Scalar>>],
rng: R,
transcript: &'a mut T,
) -> Result<Prover<'a, 'params, Scheme, P, E, R, T, H2cEngine>, Error>
) -> Result<ProverMulti<'a, 'params, Scheme, P, E, R, T, H2cEngine>, Error>
where
Scheme::Scalar: WithSmallOrderMulGroup<3> + FromUniformBytes<64>,
{
let engine = PlonkEngineConfig::build_default();
Prover::new_with_engine(engine, params, pk, circuits_instances, rng, transcript)
ProverMulti::new_with_engine(engine, params, pk, circuits_instances, rng, transcript)
}
}
38 changes: 31 additions & 7 deletions halo2_backend/src/plonk/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,12 @@ pub use batch::BatchVerifier;

/// Returns a boolean indicating whether or not the proof is valid. Verifies a single proof (not
/// batched).
pub fn verify_proof_single<'params, Scheme, V, E, T, Strategy>(
pub fn verify_proof<'params, Scheme, V, E, T, Strategy>(
params: &'params Scheme::ParamsVerifier,
vk: &VerifyingKey<Scheme::Curve>,
strategy: Strategy,
instance: Vec<Vec<Scheme::Scalar>>,
transcript: &mut T,
) -> Result<Strategy::Output, Error>
) -> bool
where
Scheme::Scalar: WithSmallOrderMulGroup<3> + FromUniformBytes<64>,
Scheme: CommitmentScheme,
Expand All @@ -45,11 +44,11 @@ where
T: TranscriptRead<Scheme::Curve, E>,
Strategy: VerificationStrategy<'params, Scheme, V>,
{
verify_proof(params, vk, strategy, &[instance], transcript)
verify_proof_multi::<Scheme, V, E, T, Strategy>(params, vk, &[instance], transcript)
}

/// Returns a boolean indicating whether or not the proof is valid
pub fn verify_proof<
/// Process the proof, checks that the proof is valid and returns the `Strategy` output.
pub fn verify_proof_with_strategy<
'params,
Scheme: CommitmentScheme,
V: Verifier<'params, Scheme>,
Expand All @@ -62,7 +61,7 @@ pub fn verify_proof<
strategy: Strategy,
instances: &[Vec<Vec<Scheme::Scalar>>],
transcript: &mut T,
) -> Result<Strategy::Output, Error>
) -> Result<Strategy, Error>
where
Scheme::Scalar: WithSmallOrderMulGroup<3> + FromUniformBytes<64>,
{
Expand Down Expand Up @@ -519,3 +518,28 @@ where
.map_err(|_| Error::Opening)
})
}

/// Returns a boolean indicating whether or not the proof is valid
pub fn verify_proof_multi<
'params,
Scheme: CommitmentScheme,
V: Verifier<'params, Scheme>,
E: EncodedChallenge<Scheme::Curve>,
T: TranscriptRead<Scheme::Curve, E>,
Strategy: VerificationStrategy<'params, Scheme, V>,
>(
params: &'params Scheme::ParamsVerifier,
vk: &VerifyingKey<Scheme::Curve>,
instances: &[Vec<Vec<Scheme::Scalar>>],
transcript: &mut T,
) -> bool
where
Scheme::Scalar: WithSmallOrderMulGroup<3> + FromUniformBytes<64>,
{
let strategy = Strategy::new(params);
let strategy = match verify_proof_with_strategy(params, vk, strategy, instances, transcript) {
Ok(strategy) => strategy,
Err(_) => return false,
};
strategy.finalize()
}
20 changes: 11 additions & 9 deletions halo2_backend/src/plonk/verifier/batch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use halo2_middleware::zal::impls::H2cEngine;
use halo2curves::CurveAffine;
use rand_core::OsRng;

use super::{verify_proof, VerificationStrategy};
use super::{verify_proof_with_strategy, VerificationStrategy};
use crate::{
multicore::{
IndexedParallelIterator, IntoParallelIterator, ParallelIterator, TryFoldAndReduce,
Expand Down Expand Up @@ -34,8 +34,6 @@ struct BatchStrategy<'params, C: CurveAffine> {
impl<'params, C: CurveAffine> VerificationStrategy<'params, IPACommitmentScheme<C>, VerifierIPA<C>>
for BatchStrategy<'params, C>
{
type Output = MSMIPA<'params, C>;

fn new(params: &'params ParamsVerifierIPA<C>) -> Self {
BatchStrategy {
msm: MSMIPA::new(params),
Expand All @@ -45,9 +43,11 @@ impl<'params, C: CurveAffine> VerificationStrategy<'params, IPACommitmentScheme<
fn process(
self,
f: impl FnOnce(MSMIPA<'params, C>) -> Result<GuardIPA<'params, C>, Error>,
) -> Result<Self::Output, Error> {
) -> Result<Self, Error> {
let guard = f(self.msm)?;
Ok(guard.use_challenges())
Ok(Self {
msm: guard.use_challenges(),
})
}

fn finalize(self) -> bool {
Expand Down Expand Up @@ -110,10 +110,12 @@ where
.map(|(i, item)| {
let strategy = BatchStrategy::new(params);
let mut transcript = Blake2bRead::init(&item.proof[..]);
verify_proof(params, vk, strategy, &item.instances, &mut transcript).map_err(|e| {
tracing::debug!("Batch item {} failed verification: {}", i, e);
e
})
verify_proof_with_strategy(params, vk, strategy, &item.instances, &mut transcript)
.map_err(|e| {
tracing::debug!("Batch item {} failed verification: {}", i, e);
e
})
.map(|st| st.msm)
})
.try_fold_and_reduce(
|| ParamsVerifier::<'_, C>::empty_msm(params),
Expand Down
21 changes: 7 additions & 14 deletions halo2_backend/src/poly/ipa/strategy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,6 @@ pub struct AccumulatorStrategy<'params, C: CurveAffine> {
impl<'params, C: CurveAffine> VerificationStrategy<'params, IPACommitmentScheme<C>, VerifierIPA<C>>
for AccumulatorStrategy<'params, C>
{
type Output = Self;

fn new(params: &'params ParamsIPA<C>) -> Self {
AccumulatorStrategy {
msm: MSMIPA::new(params),
Expand All @@ -90,7 +88,7 @@ impl<'params, C: CurveAffine> VerificationStrategy<'params, IPACommitmentScheme<
fn process(
mut self,
f: impl FnOnce(MSMIPA<'params, C>) -> Result<GuardIPA<'params, C>, Error>,
) -> Result<Self::Output, Error> {
) -> Result<Self, Error> {
self.msm.scale(C::Scalar::random(OsRng));
let guard = f(self.msm)?;

Expand Down Expand Up @@ -119,8 +117,6 @@ pub struct SingleStrategy<'params, C: CurveAffine> {
impl<'params, C: CurveAffine> VerificationStrategy<'params, IPACommitmentScheme<C>, VerifierIPA<C>>
for SingleStrategy<'params, C>
{
type Output = ();

fn new(params: &'params ParamsIPA<C>) -> Self {
SingleStrategy {
msm: MSMIPA::new(params),
Expand All @@ -130,15 +126,11 @@ impl<'params, C: CurveAffine> VerificationStrategy<'params, IPACommitmentScheme<
fn process(
self,
f: impl FnOnce(MSMIPA<'params, C>) -> Result<GuardIPA<'params, C>, Error>,
) -> Result<Self::Output, Error> {
) -> Result<Self, Error> {
let guard = f(self.msm)?;
let msm = guard.use_challenges();
// ZAL: Verification is (supposedly) cheap, hence we don't use an accelerator engine
if msm.check(&H2cEngine::new()) {
Ok(())
} else {
Err(Error::ConstraintSystemFailure)
}
Ok(Self {
msm: guard.use_challenges(),
})
}

/// Finalizes the batch and checks its validity.
Expand All @@ -147,7 +139,8 @@ impl<'params, C: CurveAffine> VerificationStrategy<'params, IPACommitmentScheme<
/// specific failing proofs, it must re-process the proofs separately.
#[must_use]
fn finalize(self) -> bool {
unreachable!()
// TODO: Verification is cheap, ZkAccel on verifier is not a priority.
Copy link
Member

Choose a reason for hiding this comment

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

Should we file an issue for the TODO?

Copy link
Author

@guorong009 guorong009 Jul 15, 2024

Choose a reason for hiding this comment

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

Yes, I think we should.
We can discuss if it is worthy or not, to apply ZkAccel on verification.

self.msm.check(&H2cEngine::new())
}
}

Expand Down
23 changes: 9 additions & 14 deletions halo2_backend/src/poly/kzg/strategy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,16 +120,14 @@ where
E::G1: CurveExt<AffineExt = E::G1Affine>,
E::G2Affine: SerdeCurveAffine,
{
type Output = Self;

fn new(params: &'params ParamsVerifierKZG<E>) -> Self {
AccumulatorStrategy::new(params)
}

fn process(
mut self,
f: impl FnOnce(V::MSMAccumulator) -> Result<V::Guard, Error>,
) -> Result<Self::Output, Error> {
) -> Result<Self, Error> {
self.msm_accumulator.scale(E::Fr::random(OsRng));

// Guard is updated with new msm contributions
Expand All @@ -155,29 +153,26 @@ where
E::G1: CurveExt<AffineExt = E::G1Affine>,
E::G2Affine: SerdeCurveAffine,
{
type Output = ();
Copy link
Member

@ed255 ed255 Jul 12, 2024

Choose a reason for hiding this comment

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

I see that all the implementations of VerificationStrategy have Output = Self.
That seems redundant, we could remove this associated type. Then the verification process either returns bool, or the VerificationStrategy object, no need to do VerificationStrategy::Output which is already VerificationStragey all the time.

Also, I think we should discourage the usage of verification functions that return the VerificationStrategy because returning a bool is much clearer. Since this PR is scheduled to be merged after the 0.4 release, and we determined that in the 0.5 release we can remove the halo2_proofs legacy layer, maybe we can just remove the verification methods that return VerificationStrategy? If not, at least could we mark them as deprecated via cfg?

Or is there any strong reason to keep the API that takes and returns a VerificationStrategy object?

Copy link
Member

Choose a reason for hiding this comment

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

I like the proposal from @ed255
And if we break API is indeed a good time to do it!

Copy link
Author

Choose a reason for hiding this comment

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

@ed255
I agree that Output = Self is redundant.
For the quick patch, I remove the Output type from VerificationStrategy trait. e5153c4

Or is there any strong reason to keep the API that takes and returns a VerificationStrategy object?

This is related to process API.
I think this API could be useful for batch verification, like following one.

pub fn finalize(self, params: &ParamsVerifierIPA<C>, vk: &VerifyingKey<C>) -> bool {
fn accumulate_msm<'params, C: CurveAffine>(
mut acc: MSMIPA<'params, C>,
msm: MSMIPA<'params, C>,
) -> MSMIPA<'params, C> {
// Scale the MSM by a random factor to ensure that if the existing MSM has
// `is_zero() == false` then this argument won't be able to interfere with it
// to make it true, with high probability.
acc.scale(C::Scalar::random(OsRng));
acc.add_msm(&msm);
acc
}
let final_msm = self
.items
.into_par_iter()
.enumerate()
.map(|(i, item)| {
let strategy = BatchStrategy::new(params);
let mut transcript = Blake2bRead::init(&item.proof[..]);
verify_proof_with_strategy(params, vk, strategy, &item.instances, &mut transcript)
.map_err(|e| {
tracing::debug!("Batch item {} failed verification: {}", i, e);
e
})
.map(|st| st.msm)
})
.try_fold_and_reduce(
|| ParamsVerifier::<'_, C>::empty_msm(params),
|acc, res| res.map(|proof_msm| accumulate_msm(acc, proof_msm)),
);
match final_msm {
// ZAL: Verification is (supposedly) cheap, hence we don't use an accelerator engine
Ok(msm) => msm.check(&H2cEngine::new()),
Err(_) => false,
}

The BatchVerifier delays the MSM check until it accumulates all the MSM from every batch items.

What do you think about this? @ed255 @CPerezz

Copy link
Member

Choose a reason for hiding this comment

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

@ed255 @CPerezz any input about last comment?

Copy link
Member

Choose a reason for hiding this comment

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

The current state of this PR looks good to me (removal of Output from trait).


fn new(params: &'params ParamsVerifierKZG<E>) -> Self {
Self::new(params)
}

fn process(
self,
f: impl FnOnce(V::MSMAccumulator) -> Result<V::Guard, Error>,
) -> Result<Self::Output, Error> {
) -> Result<Self, Error> {
// Guard is updated with new msm contributions
let guard = f(self.msm)?;
let msm = guard.msm_accumulator;
// Verification is (supposedly) cheap, hence we don't use an accelerator engine
let default_engine = H2cEngine::new();
if msm.check(&default_engine, &self.params) {
Ok(())
} else {
Err(Error::ConstraintSystemFailure)
}
Ok(Self {
msm,
params: self.params,
})
}

fn finalize(self) -> bool {
unreachable!();
// Verification is (supposedly) cheap, hence we don't use an accelerator engine
let default_engine = H2cEngine::new();
self.msm.check(&default_engine, &self.params)
}
}
2 changes: 1 addition & 1 deletion halo2_backend/src/poly/multiopen_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ mod test {
V: Verifier<'params, Scheme>,
E: EncodedChallenge<Scheme::Curve>,
T: TranscriptReadBuffer<&'a [u8], Scheme::Curve, E>,
Strategy: VerificationStrategy<'params, Scheme, V, Output = Strategy>,
Strategy: VerificationStrategy<'params, Scheme, V>,
>(
params: &'params Scheme::ParamsVerifier,
proof: &'a [u8],
Expand Down
10 changes: 4 additions & 6 deletions halo2_backend/src/poly/strategy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,16 @@ pub trait Guard<Scheme: CommitmentScheme> {

/// Trait representing a strategy for verifying Halo 2 proofs.
pub trait VerificationStrategy<'params, Scheme: CommitmentScheme, V: Verifier<'params, Scheme>> {
/// The output type of this verification strategy after processing a proof.
type Output;

/// Creates new verification strategy instance
fn new(params: &'params Scheme::ParamsVerifier) -> Self;

/// Obtains an MSM from the verifier strategy and yields back the strategy's
/// output.
/// Obtains an MSM from the verifier strategy and yields back the strategy
fn process(
self,
f: impl FnOnce(V::MSMAccumulator) -> Result<V::Guard, Error>,
) -> Result<Self::Output, Error>;
) -> Result<Self, Error>
where
Self: Sized;

/// Finalizes the batch and checks its validity.
///
Expand Down
19 changes: 10 additions & 9 deletions halo2_proofs/benches/plonk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,10 @@ use halo2curves::pasta::{EqAffine, Fp};
use rand_core::OsRng;

use halo2_proofs::{
poly::{
ipa::{
commitment::{IPACommitmentScheme, ParamsIPA},
multiopen::ProverIPA,
strategy::SingleStrategy,
},
VerificationStrategy,
poly::ipa::{
commitment::{IPACommitmentScheme, ParamsIPA},
multiopen::ProverIPA,
strategy::SingleStrategy,
},
transcript::{TranscriptReadBuffer, TranscriptWriterBuffer},
};
Expand Down Expand Up @@ -300,9 +297,13 @@ fn criterion_benchmark(c: &mut Criterion) {
}

fn verifier(params: &ParamsIPA<EqAffine>, vk: &VerifyingKey<EqAffine>, proof: &[u8]) {
let strategy = SingleStrategy::new(params);
let mut transcript = Blake2bRead::<_, _, Challenge255<_>>::init(proof);
assert!(verify_proof(params, vk, strategy, &[vec![]], &mut transcript).is_ok());
assert!(verify_proof_multi::<_, _, _, _, SingleStrategy<_>>(
params,
vk,
&[vec![]],
&mut transcript
));
}

let k_range = 8..=16;
Expand Down
Loading
Loading