Skip to content

G1/G2 Subgroup Checks#80

Open
Hakkush-07 wants to merge 4 commits intoBitVM:mainfrom
chainwayxyz:check-subgroup
Open

G1/G2 Subgroup Checks#80
Hakkush-07 wants to merge 4 commits intoBitVM:mainfrom
chainwayxyz:check-subgroup

Conversation

@Hakkush-07
Copy link
Collaborator

@Hakkush-07 Hakkush-07 commented Nov 11, 2025

This PR adds subgroup checks for G1 and G2 because we need to make sure Groth16 proof is in correct subgroup.

For G1, the r-torsion group is the same as the elliptic curve group. G1::is_on_curve added and that is enough. Also, when we are using compressed proofs, y coordinate is already calculated in the circuit, so no further action is needed.

For G2, the r-torsion group is a subgroup of the elliptic curve group. G2::is_r_torsion is added.

Only remaining thing to do is to use this G2::is_r_torsion for proof_b field of Groth16 proof.

Part of #76

@manishbista28
Copy link

Hi @Hakkush-07 -- why do we not do the subgroup check as in Bitvm2 suggested here ?

It would only cost a mul_by_char.

@manishbista28
Copy link

manishbista28 commented Nov 12, 2025

If possible, could you also list the change in gate count after the change in this PR ? Thank you

Edit:
Ran the following test to get the added gate cost.

 gadgets::bn254::g2::tests::test_g2p_scalar_mul_by_constant_scalar_montgomery stdout ----
gate count: [276472176, 4840224, 0, 0, 189792, 2420112, 0, 2477376, 844508368, 2477648, 0]
and variants:    286399680
xor variants:    846986016
not:                     0
total:          1133385696
gate count: [297853560, 5186172, 0, 0, 203601, 2593086, 0, 2651627, 911130627, 2651919, 0]
and variants:    308488046
xor variants:    913782546
not:                     0
total:          1222270592


gate count: [297853560, 5186172, 0, 0, 203601, 2593086, 0, 2651627, 911130627, 2651919, 0]
and variants:    308488046
xor variants:    913782546
not:                     0
total:          1222270592


test gadgets::bn254::g2::tests::test_g2p_is_r_torsion_montgomery ... ok

@manishbista28
Copy link

@Hakkush-07 -- If it can be helpful, I have been working on the same task here.

alpenlabs/g16#8

@just-erray
Copy link

We've discussed that further optimizations can be applied later, after having the code with the main formula, so LGTM

@cyphersnake cyphersnake self-requested a review November 18, 2025 11:12
@AaronFeickert
Copy link

So is the plan to go with this PR and not the more efficient linked PR, and change later? This seems like a bigger overall lift.

@cyphersnake
Copy link
Collaborator

So is the plan to go with this PR and not the more efficient linked PR, and change later? This seems like a bigger overall lift.

I suggest we discuss this tomorrow: it's rather strange to manually transfer changes in gadgets between different forks when they are practically identical.

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.

5 participants