feat: refactor the verify api #359

guorong009 · 2024-07-09T02:41:12Z

Description

Improve the verify api - verify_proof, in a way that it does return the bool value indicating proof is valid or not.
This way, we remove the confusion or misuse, related to verify_proof api.

Related issues

Resolve verifier.rs misleading API #184

Changes

Update several VerificationStrategy impls
Update the verify_proof so that it returns the bool
Update the related tests

codecov-commenter · 2024-07-09T02:42:40Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 68.85246% with 19 lines in your changes missing coverage. Please review.

Project coverage is 85.01%. Comparing base (7c368af) to head (6061bf6).

Files with missing lines	Patch %	Lines
halo2_backend/src/plonk/verifier/batch.rs	0.00%	10 Missing ⚠️
halo2_backend/src/poly/ipa/strategy.rs	14.28%	6 Missing ⚠️
halo2_backend/src/plonk/prover.rs	66.66%	2 Missing ⚠️
halo2_backend/src/plonk/verifier.rs	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
+ Coverage   84.99%   85.01%   +0.02%     
==========================================
  Files          85       85              
  Lines       18684    18711      +27     
==========================================
+ Hits        15880    15907      +27     
  Misses       2804     2804

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ed255

In general looks good to me! I think returning a bool simplifies the usage a lot. Please take a look at my comments :)

halo2_backend/src/plonk/verifier.rs

ed255 · 2024-07-12T15:32:03Z

halo2_backend/src/poly/kzg/strategy.rs

@@ -155,7 +155,7 @@ where
    E::G1: CurveExt<AffineExt = E::G1Affine>,
    E::G2Affine: SerdeCurveAffine,
 {
-    type Output = ();


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?

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

@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.

halo2/halo2_backend/src/plonk/verifier/batch.rs

Lines 93 to 129 in 9fc59c1

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

@ed255 @CPerezz any input about last comment?

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

CPerezz

Agree with both concerns from Ed.

CPerezz · 2024-07-12T16:18:12Z

halo2_backend/src/poly/ipa/strategy.rs

@@ -147,7 +143,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.


Should we file an issue for the TODO?

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

CPerezz · 2024-07-12T16:20:28Z

halo2_backend/src/poly/kzg/strategy.rs

@@ -155,7 +155,7 @@ where
    E::G1: CurveExt<AffineExt = E::G1Affine>,
    E::G2Affine: SerdeCurveAffine,
 {
-    type Output = ();


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

adria0

I think that is really nice, anything that de-obfuscates halo2 is welcomed! thanks for taking care about it!
I request changes. I think that there's a missing CHANGES.TXT (or whatever) describing the breaking changes.

adria0 · 2024-09-24T10:35:24Z

halo2_backend/src/poly/kzg/strategy.rs

@@ -155,7 +155,7 @@ where
    E::G1: CurveExt<AffineExt = E::G1Affine>,
    E::G2Affine: SerdeCurveAffine,
 {
-    type Output = ();


@ed255 @CPerezz any input about last comment?

guorong009 · 2024-09-25T03:56:53Z

I think that is really nice, anything that de-obfuscates halo2 is welcomed! thanks for taking care about it! I request changes. I think that there's a missing CHANGES.TXT (or whatever) describing the breaking changes.

Currently, the halo2_proofs crate includes the CHANGELOG.md which seems not used at all.
I think the changes are just summarized in release PR.
Maybe, do we need explicit CHANGELOG file for documentation?
cc @davidnevadoc

adria0 · 2024-09-25T06:00:04Z

I think that is really nice, anything that de-obfuscates halo2 is welcomed! thanks for taking care about it! I request changes. I think that there's a missing CHANGES.TXT (or whatever) describing the breaking changes.

Currently, the halo2_proofs crate includes the CHANGELOG.md which seems not used at all. I think the changes are just summarized in release PR. Maybe, do we need explicit CHANGELOG file for documentation? cc @davidnevadoc

Mmmmmm... yes, I see. Updating the halo2_proofs/CHANGELOG.md should be enough. Let's do it when doing a release!

I also think that we have to move CHANGELOG.md to repo root, wdyt @guorong009 ?

ed255

I'm approving this PR because the comments/requests I made have all been addressed, and everything else looks good to me.

I still think it would be interesting to explore having the verification functions return bool, but now I see that with batched verification this is not a straight forward change to do; perhaps it can be explored in a future PR.

guorong009 added 3 commits July 5, 2024 20:37

feat: refactor the verification api

75126ac

chore: update the verification api uses in tests

4f68012

chore: fmt + clippy

735f164

guorong009 self-assigned this Jul 9, 2024

guorong009 requested review from davidnevadoc, ed255 and CPerezz July 9, 2024 02:41

ed255 reviewed Jul 12, 2024

View reviewed changes

CPerezz reviewed Jul 12, 2024

View reviewed changes

guorong009 added 3 commits July 15, 2024 11:14

feat: rename the "verify_*" apis

bce0beb

feat: remove "Output" type from "VerificationStrategy" trait

e5153c4

feat: rename "ProverSingle" -> "Prover", "Prover" -> "ProverMulti"

9fc59c1

guorong009 requested review from ed255 and CPerezz July 15, 2024 08:17

guorong009 added 2 commits August 27, 2024 00:36

Merge branch 'main' into gr@verify-api-fix

02b8078

Merge branch 'main' into gr@verify-api-fix

7e27637

guorong009 marked this pull request as ready for review September 24, 2024 09:19

guorong009 requested review from adria0 and kilic September 24, 2024 09:21

adria0 requested changes Sep 24, 2024

View reviewed changes

adria0 self-requested a review September 25, 2024 06:00

adria0 approved these changes Sep 25, 2024

View reviewed changes

guorong009 added 2 commits September 26, 2024 15:15

Merge branch 'main' into gr@verify-api-fix

c4bfe60

Merge branch 'main' into gr@verify-api-fix

6061bf6

ed255 approved these changes Sep 26, 2024

View reviewed changes

This was referenced Sep 27, 2024

Discussion: Apply ZkAccel/ZAL to Verification process #371

Closed

Explore: how to improve verify api, especially in batch version? #372

Closed

guorong009 merged commit 553953d into main Sep 27, 2024
6 checks passed

guorong009 deleted the gr@verify-api-fix branch September 27, 2024 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor the verify api #359

feat: refactor the verify api #359

guorong009 commented Jul 9, 2024

codecov-commenter commented Jul 9, 2024 •

edited

Loading

ed255 left a comment

ed255 Jul 12, 2024 •

edited

Loading

CPerezz Jul 12, 2024

guorong009 Jul 15, 2024

adria0 Sep 24, 2024

ed255 Sep 26, 2024

CPerezz left a comment

CPerezz Jul 12, 2024

guorong009 Jul 15, 2024 •

edited

Loading

CPerezz Jul 12, 2024

adria0 left a comment

adria0 Sep 24, 2024

guorong009 commented Sep 25, 2024

adria0 commented Sep 25, 2024

ed255 left a comment

	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,
	}

feat: refactor the verify api #359

feat: refactor the verify api #359

Conversation

guorong009 commented Jul 9, 2024

Description

Related issues

Changes

codecov-commenter commented Jul 9, 2024 • edited Loading

Codecov Report

ed255 left a comment

Choose a reason for hiding this comment

ed255 Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

CPerezz Jul 12, 2024

Choose a reason for hiding this comment

guorong009 Jul 15, 2024

Choose a reason for hiding this comment

adria0 Sep 24, 2024

Choose a reason for hiding this comment

ed255 Sep 26, 2024

Choose a reason for hiding this comment

CPerezz left a comment

Choose a reason for hiding this comment

CPerezz Jul 12, 2024

Choose a reason for hiding this comment

guorong009 Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

CPerezz Jul 12, 2024

Choose a reason for hiding this comment

adria0 left a comment

Choose a reason for hiding this comment

adria0 Sep 24, 2024

Choose a reason for hiding this comment

guorong009 commented Sep 25, 2024

adria0 commented Sep 25, 2024

ed255 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 9, 2024 •

edited

Loading

ed255 Jul 12, 2024 •

edited

Loading

guorong009 Jul 15, 2024 •

edited

Loading