-
Notifications
You must be signed in to change notification settings - Fork 272
Conversation
6e954c1
to
4a0c56a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Only have some nitpicks.
specs/precompile/08ecPairing.md
Outdated
|
||
### Circuit behavior | ||
|
||
The inputs is a multiple of 6 32-byte values. One set of inputs is defined as follows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps: The input is arbitrarily many pairs of elliptic curve points. Each pair is given as a six 32-bytes values and is constructed as follows
is_valid_input: FQ = instruction.curr.aux_data[2] | ||
output: FQ = instruction.curr.aux_data[3] | ||
|
||
# output indicates the pairing is successful or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: "indicates whether the pairing"
@@ -12,7 +12,7 @@ Two points are recovered from input. The field is expressed as `32` bytes for ea | |||
input[0; 31] (32 bytes): x1 | |||
input[32; 63] (32 bytes): y1 | |||
input[64; 95] (32 bytes): x2 | |||
input[96; 128] (32 bytes): y2 | |||
input[96; 127] (32 bytes): y2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[96: 128] is a wrong range which is 33 bytes. 😄
specs/precompile/08ecPairing.md
Outdated
input[160; 192] (32 bytes): y3 (result) | ||
``` | ||
|
||
The output is the result of `ecPairing`, 1 if the pairing is successful, 0 otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps:
The first two 32-bytes values represent the two points g1, h1
from group G1
, the next four 32-bytes values represent the two points h1, h2
from group G2
.
The bn256Pairing code first checks that a multiple of 6 elements have been sent, and then performs the pairings check(s). The check that is performed for each pair is e(g1, g2) = e(-h1, h2)
which is equivalent to the check e(g1, g2) * e(h1, h2) = 1
.
The output is 1
if all pairing checks were successful, otherwise it returns 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first two 32-bytes values represent the two points g1, h1 from group G1, the next four 32-bytes values represent the two points h1, h2 from group G2.
From my understanding, "first two 32-bytes values represent the first point from G1 and the next four 32-bytes values represents the other point from G2". So, every 6 32-bytes data represents two points only, one is on G1 and the other is on G2.
The bn256Pairing code first checks that a multiple of 6 elements have been sent, and then performs the pairings check(s). The check that is performed for each pair is e(g1, g2) = e(-h1, h2) which is equivalent to the check e(g1, g2) * e(h1, h2) = 1
I'm not 100% sure about this one, but from the testing vectors I have seen, the minimal input size of ecPairing
is 2 *( 6 * 32) bytes which means four points. So, I did some modifications as follows,
The bn254Pairing code first checks that a multiple of 6 elements have been sent, and then performs the pairings check(s). The check that is performed for two pairs is e(p1, q1) = e(-p2, q2) which is equivalent to the check e(p1, q1) * e(p2, q2) = 1.
Please help confirm if it makes sense to you or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, I misunderstood at first. The six 32-bytes values indeed present two points: one in G1
, one in G2
. So we get pairs of points (a_1, b_1), ..., (a_n, b_n)
where a_i
is from G1
and b_i
is from G2
. The pairing check does the following:
e(a_1, b_1) * ... * e(a_n, b_n) = 1
This is indeed not very useful for the case when n = 1
(but allowed). For example in BLS signature we have n = 2
.
specs/precompile/08ecPairing.md
Outdated
input[0; 31] (32 bytes): success | ||
``` | ||
|
||
The definition of `is_valid` here is the validity of input points (on the curve or at infinity). `is_valid` doesn't mean it's a successful call (`output` is 1) because we can give any valid points but it causes the result is not equal (e(p1, p2) != e(p3, p4)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps: The pairing checks fail if not having a multiple of 6 32-bytes values or in the case of the points not being on the curve. In these cases all the provided gas is consumed. For these cases, the variable is_valid
is set to 0
. The variable output
denotes whether the pairing checks were successful (in the case of is_valid = 1
).
src/zkevm_specs/ecc_circuit.py
Outdated
cs.constrain_equal(self.ecc_chip.output[1], self.row.out_y) | ||
# input_rlc is zero bcs it's only used in pairing | ||
cs.constrain_zero(self.row.input_rlc) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps mention in a comment that this if branch is for is_pairing
?
src/zkevm_specs/ecc_circuit.py
Outdated
valid_q = FQ.one() if result is None else FQ.zero() | ||
cs.constrain_equal(valid_p + valid_q, FQ(2)) | ||
|
||
# concatenate inputs points for rlc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: inputs -> input
ee61b22
to
b562ef5
Compare
@miha-stopar , all comments were addressed in b562ef5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
closed #325