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: end-to-end modexp, ecadd, ecmul and ecpairing #39

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

NikitaMasych
Copy link

What?

Implementation of modexp, ecadd, ecmul and ecpairing precompiles as circuits.

Previous PRs

Since the migration to monolithic workspace with crates was in place, this PR aggregates several separate ones, which had been opened to corresponding crates repositories and had reviews.

  1. zkevm_circuits
  2. zkevm_test_harness
  3. zk_evm
  4. zk_evm_abstractions
  5. zkevm_opcode_defs

Homogeneity with previous PRs

Code has certain changes apart from contained in previous PRs such as caused by rebasing and noticing missed problems.

Copy link

@Fitznik Fitznik left a comment

Choose a reason for hiding this comment

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

Further reviewing process required fixing current issues

crates/zkevm_circuits/src/bn254/validation.rs Show resolved Hide resolved
crates/zkevm_circuits/src/bn254/ec_add/mod.rs Outdated Show resolved Hide resolved
let x1 = convert_uint256_to_field_element(cs, &x1, base_field_params);
let y1 = convert_uint256_to_field_element(cs, &y1, base_field_params);

let point1_on_curve = is_on_curve(cs, (&x1, &y1), base_field_params);
Copy link

Choose a reason for hiding this comment

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

You can optimize this place. Optimize reduction (inside equal is reduction also mul doing reduction)

Copy link
Author

Choose a reason for hiding this comment

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

Can you clarify how to do that? Do I need to somehow call normalization manually before operations?

let mut x_cubed_plus_b = x_cubed.add(cs, &mut b);
let mut y_squared = y.square(cs);

BN256BaseNNField::equals(cs, &mut y_squared, &mut x_cubed_plus_b)
Copy link

Choose a reason for hiding this comment

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

Remove these mutable links (hint: all problems in mul)

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean I shall make changes in boojum to accept immutable links and then clone and make mutable ops there in inner functions?

crates/zkevm_circuits/src/bn254/ec_add/mod.rs Outdated Show resolved Hide resolved
crates/zkevm_circuits/src/bn254/ec_pairing/mod.rs Outdated Show resolved Hide resolved
crates/zkevm_circuits/src/bn254/ec_pairing/mod.rs Outdated Show resolved Hide resolved
/// This function computes the Miller loop for the BN256 curve, using
/// _Algorithm 1_ from https://eprint.iacr.org/2010/354.pdf. Frobenius
/// map is taken from https://hackmd.io/@Wimet/ry7z1Xj-2.
pub fn evaluate(
Copy link

Choose a reason for hiding this comment

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

Critical bug: You should have used subgroup check during the Miller loop. Here is the article: https://eprint.iacr.org/2022/352.pdf

Copy link
Author

Choose a reason for hiding this comment

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

I have had a look at the go-ethereum code for ecpairing (which is most representing protocol rules) and haven't found there subgroup checks. Plus, EIP-197 does not mention it. Here is article regarding this. As I understand, it is not done in protocol, particularly due to significant cost of such.

Copy link

Choose a reason for hiding this comment

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

You do need to perform a subgroup check for G2 when working with the Bn256 (or BN254) curve.
The BN256 curve is designed so that both G1 and G2 have a prime order q. However, because G2 is defined over F_p^2, there are elements on the curve that do not necessarily belong to the prime-order subgroup. The reason a subgroup check is required for G2 is that G2 operates over Fp2, where the elliptic curve has more points (the full curve over Fp2 has order p2−1), and only a subset of those points are in the prime-order subgroup. Therefore, G2 can have points outside the correct prime-order subgroup, making the check necessary.
For G1 in the BN256 (BN254) curve, you do not typically need to perform a subgroup check.
The cofactor for the G1 group is 1, meaning the total number of points on the elliptic curve over Fp is exactly q×1. Therefore, there is no risk of having points in a larger group (i.e., no larger cofactor to worry about), and any valid point generated by the group operation already lies in the correct subgroup of prime order q. ( in contrast, there is bls12 curve which have a lot of cofactors so we need subgroup check for G1 and G2)
But the fact that EIP-197 does not mention it is suspicious to me so i decide do some research:
Since small subgroup attack is directly breaking DLA for some curves their coffactor could be huge enough to be safe. This article explain how the same curve but with different parameters could change security level https://eprint.iacr.org/2015/247.pdf and the last author of this blog compute for us the bit of security https://blog.bren2010.io/blog/notes-on-bn256
and this is 2^100 what is quite big for use subgroup check. I checked our curve and we dont have safe parameters but the security is approximately 2^100. So do we need to add subgroup check in our precompiles? ( checked some zk project with the same curve and they did check)
Why do I suggest to use the subgroup check for BN256 during Miller loop? because if you look at the article which i previously sent, subgroup check uses the twisted frobenius map, the same as miller loop. we can just reuse frob map to check if point in the right subgroup.
Subgroup check for BN256 curve is of the form: twisted_frob(Q) = [6*u^2]*Q so shouldn’t be such expensive

@NikitaMasych NikitaMasych force-pushed the dl-precompiles branch 2 times, most recently from 66ec15f to 4cfd997 Compare October 9, 2024 14:09
@NikitaMasych NikitaMasych marked this pull request as ready for review October 17, 2024 12:52
@Fitznik
Copy link

Fitznik commented Nov 3, 2024

I want to write about this in general because there are many places where this problem is. In pairing, you use a lot of normalization even when the value is already normalized. In some cases, it will be optimized as follows:

      if self.tracker.max_moduluses == 1 && self.form == RepresentationForm::Normalized {
            return;
        }

But not every normalized value will have a self.tracker.max_moduluses = = 1. That's why, in many cases, you do over constraint.

In most of the functions, we use mul, which normalizes itself. Take this into consideration! This can be a good optimization

@Fitznik
Copy link

Fitznik commented Nov 3, 2024

After the easy part, you need to check that F_q^12 isn't zero before the hard part. (exception case)

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.

3 participants