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: Complete ECCVM recursive verifier #6720

Merged
merged 113 commits into from
May 31, 2024
Merged

feat: Complete ECCVM recursive verifier #6720

merged 113 commits into from
May 31, 2024

Conversation

maramihali
Copy link
Contributor

@maramihali maramihali commented May 28, 2024

Enable IPA recursive verification and add tests to complete the ECCVM recursive verifier.

// Step 1.
// Receive polynomial_degree + 1 = d from the prover
const auto poly_length = static_cast<uint32_t>(
transcript->template receive_from_prover<typename Curve::BaseField>("IPA:poly_degree_plus_1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a todo to actually assert_equal the value received from the prover. Right now it is a spare value to bruteforce challenges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean with the circuit size given in the verification_key or what?

const auto log_poly_degree = numeric::get_msb(static_cast<uint32_t>(poly_length));
// Step 3.
// Compute C' = C + f(\beta) ⋅ U
GroupElement C_prime = opening_claim.commitment + aux_generator * opening_claim.opening_pair.evaluation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add f(\beta)*U to the msm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will leave things as they are are to not differ from the non-stdlib implementation if not necessary

// Construct vector s
std::vector<Fr> s_vec(poly_length);

for (size_t i = 0; i < poly_length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a TODO to do this in a tree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There already was one in the parallelised function, copied here as well:)

// Compute G₀
// Unlike the native verification function, the verifier commitment key only containts the SRS so we can apply
// batch_mul directly on it.
Commitment G_zero = Commitment::batch_mul(srs_elements, s_vec);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a todo,please, to make it so that we only have 1 batch_mul

@Rumata888 Rumata888 self-requested a review May 31, 2024 16:24
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1017): This is a hack to ensure zero commitments
// are still on curve as the transcript doesn't currently support a point at infinity representation for
// cycle_group
if (!comm.get_value().on_curve()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the point at infinity is at another place, this will fail the circuit. You need to do set_point_at_infinity(x==x_error&&y=y_error)

Copy link
Contributor

@Rumata888 Rumata888 left a comment

Choose a reason for hiding this comment

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

Need to fix the setting to point at infinity, otherwise fine

Copy link
Collaborator

@AztecBot AztecBot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: 77f0b38 Previous: 1ccab1d Ratio
nativeconstruct_proof_ultrahonk_power_of_2/20 5987.959732999997 ms/iter 5543.083378000006 ms/iter 1.08

This comment was automatically generated by workflow using github-action-benchmark.

CC: @ludamad @codygunton

@maramihali
Copy link
Contributor Author

Chatted with Kesha async, we're gonna leave point at inifinity as it is and fix the hack by making transcript handling it correctly as other hacky fixes we tried don't work

Base automatically changed from mm/cycle-scalar-group-work to master May 31, 2024 19:02
@codygunton codygunton merged commit a98d30b into master May 31, 2024
35 checks passed
@codygunton codygunton deleted the mm/eccvm-pcs-work branch May 31, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto cryptography
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants