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.

Sign up for GitHub

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

ecPairing nits #224

Closed
wants to merge 3 commits into from
Closed

ecPairing nits #224

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6bb4ac5
Fixes & suggestions
vladbochok Feb 20, 2024
891c13a
Restore the correct check
vladbochok Feb 20, 2024
c849eb2
Restore the layout
vladbochok Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 8 additions & 37 deletions precompiles/EcPairing.yul
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,17 +448,6 @@ object "EcPairing" {
ny0, ny1 := fp2Neg(y0, y1)
}

/// @notice Check if a G2 point in jacobian coordinates is in the subgroup of the twisted curve.
/// @dev The coordinates are encoded in Montgomery form.
/// @param xp0, xp1 The x coordinate of the point.
/// @param yp0, yp1 The y coordinate of the point.
/// @param zp0, zp1 The z coordinate of the point.
/// @return ret True if the point is in the subgroup, false otherwise.
function g2IsInSubGroup(xp0, xp1, yp0, yp1, zp0, zp1) -> ret {
let xr0, xr1, yr0, yr1, zr0, zr1 := g2ScalarMul(xp0, xp1, yp0, yp1, zp0, zp1, TWISTED_SUBGROUP_ORDER())
ret := and(iszero(zr0), iszero(zr1))
}

Comment on lines -451 to -461
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not clear about what was discussed internally, are we still moving forward with this change?

/// @notice Double a g2 point represented in jacobian coordinates.
/// @dev The coordinates must be encoded in Montgomery form.
/// @param xp0, xp1 The x coordinate of the point.
Expand Down Expand Up @@ -1579,11 +1568,9 @@ object "EcPairing" {
// Calldata "parsing"
for { let i := 0 } lt(i, inputSize) { i := add(i, PAIR_LENGTH()) } {
/* G1 */
calldatacopy(i, i, 32) // x
calldatacopy(add(i, 32), add(i, 32), 32) // y

let g1_x := mload(i)
let g1_y := mload(add(i, 32))
let g1_x := calldataload(i)
let g1_y := calldataload(add(i, 32))

if iszero(and(coordinateIsOnFieldOrder(g1_x), coordinateIsOnFieldOrder(g1_y))) {
burnGas()
Expand All @@ -1593,33 +1580,21 @@ object "EcPairing" {
g1_y := intoMontgomeryForm(g1_y)

let g1IsInfinity := g1AffinePointIsInfinity(g1_x, g1_y)

if and(iszero(g1IsInfinity), iszero(g1AffinePointIsOnCurve(g1_x, g1_y))) {
burnGas()
}

/* G2 */
let g2_x1_offset := add(i, 64)
let g2_x0_offset := add(i, 96)
let g2_y1_offset := add(i, 128)
let g2_y0_offset := add(i, 160)

calldatacopy(g2_x1_offset, g2_x1_offset, 32)
calldatacopy(g2_x0_offset, g2_x0_offset, 32)
calldatacopy(g2_y1_offset, g2_y1_offset, 32)
calldatacopy(g2_y0_offset, g2_y0_offset, 32)

let g2_x1 := mload(g2_x1_offset)
let g2_x0 := mload(g2_x0_offset)
let g2_y1 := mload(g2_y1_offset)
let g2_y0 := mload(g2_y0_offset)

// TODO: Double check if this is right

let g2_x1 := calldataload(add(i, 64))
let g2_x0 := calldataload(add(i, 96))
let g2_y1 := calldataload(add(i, 128))
let g2_y0 := calldataload(add(i, 160))

if iszero(and(coordinateIsOnFieldOrder(g2_x0), coordinateIsOnFieldOrder(g2_x1))) {
burnGas()
}

// TODO: Double check if this is right
if iszero(and(coordinateIsOnFieldOrder(g2_y0), coordinateIsOnFieldOrder(g2_y1))) {
burnGas()
}
Expand All @@ -1633,10 +1608,6 @@ object "EcPairing" {
g2_y0 := intoMontgomeryForm(g2_y0)
g2_y1 := intoMontgomeryForm(g2_y1)

if iszero(g2IsInSubGroup(g2_x0,g2_x1, g2_y0, g2_y1, MONTGOMERY_ONE(), 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct.

In the geth implementation mentioned the G2 actually uses a twistPoint: https://github.com/ethereum/go-ethereum/blob/master/crypto/bn256/google/bn256.go#L166.
And its IsOnCurve() method has the subgroup check: https://github.com/ethereum/go-ethereum/blob/master/crypto/bn256/google/twist.go#L84

burnGas()
}

if iszero(g2AffinePointIsOnCurve(g2_x0, g2_x1, g2_y0, g2_y1)) {
burnGas()
}
Expand Down
Loading