More feature to betterpairing added and some refactoring performed #177
Conversation
nqyy
commented
Feb 11, 2019
•
nqyy
commented
- Added some new methods in classes from betterpairing: G1, G2.
- Added a new class to betterpairing: GT.
- On the rust side, new class PyFq6 is added to lib.rs
- New rust method vec_sum is added.
- Some refactoring is performed on betterpairing.py
| if type(other) is ZR: | ||
| self.val.add_assign(other.val) | ||
| return self | ||
| elif type(other) is int: | ||
| if other < 0: | ||
| other *= 1 | ||
| other *= -1 |
There was a problem hiding this comment.
I think this is a bug.
There was a problem hiding this comment.
other *= -1 can be different than other = other * -1.
The difference is *= applied to an object actually runs the __imul__ operator, which for the ZR G1 etc objects we have here, performs modifications in place. I suspect this is the cause of problems
That can't be it, as I can't find any examples where this is applied to anything except an int
| if type(other) is ZR: | ||
| self.val.sub_assign(other.val) | ||
| return self | ||
| elif type(other) is int: | ||
| if other < 0: | ||
| other *= 1 | ||
| other *= -1 |
| fq12 = PyFq12() | ||
| self.pyg1.py_pairing_with(other.pyg2, fq12) | ||
| return GT(fq12) | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
I think we need @staticmethod. Don't know why it was taken out.
|
|
||
| # TODO: __truediv__ needs to be implemented | ||
|
|
||
| # TODO: __pow__ needs to be implemented |
There was a problem hiding this comment.
__truediv__ and __pow__ are not implemented. They are called below.
There was a problem hiding this comment.
I think we should narrow this interface abstraction a bit. G1 G2 and GT are both groups, but G1 and G2 are additive notation (you can add two points together, but not multiply them together).
Now the slightly confusing thing is that GT is actually defined as the finite field Fq12.... and in general finite fields support multiplication as well as addition. However, the GT class should probably not expose __add__ at all.
|
You have a few easy flake8 things to fix before you get the all greens :) |
honeybadgermpc/betterpairing.py
Outdated
| negone = PyFr(str(1)) | ||
| negone.negate() | ||
| self.pyg1.mul_assign(negone) | ||
| self.pyg1.mul_assign(PyFr(str(1)).negate()) |
There was a problem hiding this comment.
Maybe premature optimization - but this looks like an easy candidate for caching.
Separately - It always irks me to see serialization/deserialization to decimal numbers as the way of communicating with an FFI -- we're encountering this in bindings to the C++ NTL library too so I don't have a solution in mind.
honeybadgermpc/betterpairing.py
Outdated
| @@ -214,7 +235,8 @@ def __truediv__(self, other): | |||
| out.sub_assign(other.pyg2) | |||
| return G2(out) | |||
| else: | |||
| raise TypeError | |||
| raise TypeError( | |||
| 'Invalid division param. Expected G1. Got ' + str(type(other))) | |||
| } | ||
|
|
||
| #[pyfunction] | ||
| fn vec_sum(a: &PyList) -> PyResult<String>{ |
There was a problem hiding this comment.
nice, useful sum tool to expose
| @@ -474,6 +483,24 @@ impl PyFq2 { | |||
| } | |||
| } | |||
|
|
|||
| #[pyclass] | |||
| struct PyFq6 { | |||
There was a problem hiding this comment.
what do we need Fq6 exposed in python for?
|
Flake8 issues are fixed. Some implementation reasons or details can be answered by @tyurek . Currently there is no test for betterpairing, so I am trying to write some tests for it. |
|
I'm OK to accept this and move on! |
|
@chitianhao Can you squash all the commits? |
|
Squashed and rebased. @smkuls |
1. Added some new methods in classes from betterpairing: G1, G2. 2. Added a new class to betterpairing: GT. 3. On the rust side, new class PyFq6 is added to lib.rs 4. New rust method vec_sum is added. 5. Some refactoring is performed on betterpairing.py
| out = PyG1() | ||
| out.zero() | ||
| return G1(out) | ||
| return G1(PyG1().zero()) |
There was a problem hiding this comment.
This is the problem - PyG1.zero() zeroes out the element in place and returns None, rather than returning the PyG1 interest.
