We're using the wrong field

[ed255]

While doing a debug session for PyChiquito we noticed that the field class we're using in the zkevm-specs is the base field of BN256, but it should be the scalar field instead:

>>> from py_ecc import bn128
>>> FQ = bn128.FQ
>>> (FQ(-1).n + 1)
21888242871839275222246405745257275088696311157297823662689037894645226208583

For reference https://docs.rs/ark-bn254/latest/ark_bn254/ This is the field we're using in Rust https://github.com/privacy-scaling-explorations/halo2curves/blob/6e2ff3853c8fe91300650a733100640dacf313e6/src/bn256/fr.rs#L39 which is the scalar field of BN256

>>> 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001
21888242871839275222246405745257275088548364400416034343698204186575808495617

We should fix the python field so that it matches the field where the circuits are evaluated, mainly to avoid any confusion.

[ChihChengLiang]

The correct variable is bn128.curve_order

>>> from py_ecc import bn128
>>> bn128.curve_order
21888242871839275222246405745257275088548364400416034343698204186575808495617
>>> 

It looks like we need an one-line fix at https://github.com/privacy-scaling-explorations/zkevm-specs/blob/f4d8c8e7c1f77ef4479963ab89f01fd1db18a81a/src/zkevm_specs/util/arithmetic.py#L27

-class FQ(bn128.FQ):
+class FQ(bn128.curve_order):

[KimiWu123]

Hi @ed255 and @ChihChengLiang, we have a contributor submitted https://github.com/privacy-scaling-explorations/zkevm-specs/pull/497 for this issue already. I just want to ensure non of you two are working on this. If so, I'll assign this issue to him.

[ed255]

Hi @ed255 and @ChihChengLiang, we have a contributor submitted #497 for this issue already. I just want to ensure non of you two are working on this. If so, I'll assign this issue to him.

I'm not working on this :)

BTW I think the solution that CC shared will not work. When doing class FQ(bn128.FQ) we are creating a class FQ with parent class bn128.FQ But bn128.curve_order is not a class! It's a number, so we can't use it as parent class.

I think the solution should be simple though. This is how bn128.FQ is defined in py_ecc/fields/__init__.py:

class bn128_FQ(FQ):
    field_modulus = field_properties["bn128"]["field_modulus"]

Probably we can just define our FQ class like this (haven't tested it):

class FQ(field_elements.FQ):
    field_modulus = bn128.curve_order

EDIT: To resolve this issue it would be good to also add a small test to make sure the new FQ is correct, by doing something like:

assert FQ(-1).n + 1 == 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001

[a2468834]

Sorry @ed255 and @KimiWu123. 🙏 I apologize for pick this issue without claiming first.

If there is no one working on this issue, I could take it. 💪

[KimiWu123]

Hi @a2468834 , never mind 😄 . I have assigned to you.

[ChihChengLiang]

@ed255, Nice catch! I think your proposed solution is correct and the way to go!

[KimiWu123]

Hi @a2468834 , are you still working on this? It's fine if you are busy on other tasks.

[a2468834]

Yes, I'm still working on and plan to deal with the issue this weekend. Sorry that switching to other tasks for the past weeks 🥲 I will submit commits as soon as possible.

[KimiWu123]

@a2468834 , this issue has been fixed in #500 and you can see here. I'll close it for now. If you have any better solutions to address this issue, feel free to reopen it.