Remove Redundancy of Fields among curves #41

Bhargavasomu · 2019-01-13T15:05:14Z

What was wrong?

As part of the refactoring, the first step is to remove the redundant field_elements.py and optimized_field_elements.py files and make it possible to develop an API which any curve can use, instead of mere copy pasting with few properties changes.

How was it fixed?

The field_properties has been generated. Also the field_elements and the optimized_field_elements files generate the fields FQ or FQP by making use of the curve name. Further the tests corresponding to these have been created in another file named test_field_elements.py which has been taken out from test_bn128_and_bls12_381.py (It shouldn't be hard to corelate the tests, just changed the syntax which is caused due to the breaking API change).

Note

This API hasn't still been integrated into the curves logic. I have just made the API change and mimiced the corresponding changes to the test cases. If this API is ok, then I will integrate them with the other curves. Please let me know the exhaustive list of references or explanations if you cannot co-relate anything to the already existing codebase. I will give you an explanation for all the questions posed.

Cute Animal Picture

Bhargavasomu · 2019-01-13T15:53:08Z

@pipermerriam @carver review please

carver · 2019-01-18T01:36:33Z

First thought (I'll come back and review more, soon):

I think you might be able to fit the new curves into the old tests by modifying this area:

py_ecc/tests/test_bn128_and_bls12_381.py

Lines 8 to 15 in 5aba1a7

    
           @pytest.fixture(params=[bn128, optimized_bn128, bls12_381, optimized_bls12_381]) 
        
           def lib(request): 
        
               return request.param 
        
           @pytest.fixture 
        
           def FQ(lib): 
        
               return lib.FQ

To do something like:

from functools import partial

@pytest.fixture(params=[
  bn128, optimized_bn128, bls12_381, optimized_bls12_381,
  "bn128", "optimized_bn128"])
def curve(request):
    return request.param

@pytest.fixture
def FQ(curve):
  if isinstance(curve, str):
    if curve.startswith('optimized_'):
      return partial(optimized_field_elements.FQ, curve_name=curve[10:])
    else:
      return partial(field_elements.FQ, curve_name=curve)
  else:
    return lib.FQ

That would allow testing the new models and the old models side by side, using the same tests, which is ideal during a refactor.

BTW, note that the partial only currently works with a keyword argument. It would work positionally if the argument order of the init were switched. That would be a very reasonable thing to do.

carver

Okay, that's as far as I got for the day, I will come back again to continue another time.

py_ecc/fields/field_elements.py

carver · 2019-01-19T01:17:01Z

py_ecc/fields/optimized_field_elements.py

+IntOrFQ = Union[int, "FQ"]
+
+
+class FQ(object):


Looks like we can just delete this FQ and import it from the regular (unoptimized) implementation.

@carver I have observed this prior, but have just copy/pasted because to maintain symmetry between the optimized and the non-optimized version in terms of type hinting, imports etc at this point of time.

How about I change this after we complete this whole refactor thing?

py_ecc/fields/field_elements.py

py_ecc/fields/optimized_field_elements.py

carver

Ok, I think that wraps it up for this round of reviews. Thanks for your work!

py_ecc/fields/optimized_field_elements.py

Bhargavasomu · 2019-01-25T13:36:32Z

@carver instead of using partial, I have manually created the classes to all the Fields of the curves, (thank guido that there is a type method :P). The code is now visually better to read and now visually a feast to those referring to the codebase. @pipermerriam I think you would be happy with this change, as I remember you asking to change this earlier as part of the other refactoring PR.

Further, I have removed the mypy checks, as they are messy right now. I will later integrate mypy, after this whole refactor thing is done with.

Also please note that in the curve files such as py_ecc/bn128/bn128_curve.py, I have done
from py_ecc.fields import bn128_FQ as FQ. I have done that so as to make the PR small and easily relatable for you guys (Recently experienced, how tough reviewing is). I know that this could cause debugging issues. Hence after this gets merged (preferabbly tonight), I would like to open another PR which would directly import bn128_FQ and use it with that name only, wherever apt (This also would be happening tonight).

Please review and let me know if any other changes are needed to done.

py_ecc/fields/field_elements.py

pipermerriam · 2019-01-25T15:51:03Z

py_ecc/fields/field_elements.py

+            if len(set(
+                [len(lm), len(hm), len(low), len(high), len(nm), len(new), self.degree + 1]
+            )) != 1:
+                raise Exception("Mismatch between the lengths of lm, hm, low, high, nm, new")


I know it is more work but it would be ideal to actually include which one was the mismatch.

Also please note that, we could display an error if only one of them all, don't match with the rest. Assuming that would be the scenario, I could cook something up.

pipermerriam · 2019-01-25T15:56:56Z

py_ecc/utils.py

+        o[i] += int(temp[degb + i] / b[degb])
+        for c in range(degb + 1):
+            temp[c + i] -= o[c]
+    return cast(Tuple[IntOrFQ], tuple(o[:deg(o) + 1]))


I assume this type is supposed to be Tuple[IntOrFQ, ...] or is this really a length-1 tuple?

@pipermerriam this actually should be Tuple[IntOrFQ, ...], but as I mentioned, is it ok to skip the type hinting as part of this PR?

Bhargavasomu · 2019-01-25T17:33:50Z

@pipermerriam made the changes, please review again. Thankyou.

Bhargavasomu · 2019-02-06T17:38:46Z

@carver could you please review with the new changes made? Thankyou.

Bhargavasomu · 2019-02-27T09:02:05Z

When it comes to the proficiency, this refactored code is taking 70 seconds longer than the code on master to run all the tests using pytest. I did a quick profiling and observed that, this increase is because of the following reasons.

We are trying to normalize the given input at many places in the FQ operations. For example, to add an FQ point with another number (or) another FQ point, we are first checking the type with an isinstance check. This is causing a lot of overhead as the whole library boils down to usage of FQ operations (IIUC).
Introduction of Error Handling in various functions, which is further done by introducing a lot of isinstance checks.

@ChihChengLiang @carver could you please review this?
/cc @pipermerriam

Bhargavasomu · 2019-02-27T14:08:04Z

@pipermerriam @carver @ChihChengLiang the main devil causing more time here was, making the function call **normalize_FQ_point**. This is a costly operation because those functions are called a lot of times which makes it time consuming. On removing that function and pasting the
actual function code wherever needed, made the code way faster. The time taken to run the tests by pytest is now nearly the same or faster than master now. Please verify this and review if possible. Thankyou.

ChihChengLiang · 2019-03-03T08:57:37Z

I know it might be too late to say and you might have already tried, but not sure if you like this idea:

If we want field elements to be a common API, in the py_ecc/fields they shouldn't know anything about the curve. So the proposed elements API should look like:

# py_ecc/fields/field_elements.py


# Can use type variable in base class methods
TFQ = TypeVar("TFQ", bound="BaseFQ")


class BaseFQ(ABC):

    @property
    @abstractmethod
    def field_modulus(self):
        pass


class BaseFQP(ABC):
    @property
    @abstractmethod
    def degree(self):
        pass


class FQ2(FQP):
    degree = 2
    

class FQ12(FQP):
    degree = 12

And in every curve, we keep some_curve_field_elements.py and let it inherit base field elements.

# py_ecc/bls12_381/bls12_381_field_elements.py
from .constants import field_modulus
from py_ecc.fields.field_elements import BaseFQ

class FQ(BaseFQ):
    field_modulus = field_modulus

Bhargavasomu · 2019-03-04T14:29:06Z

@ChihChengLiang thankyou for reviewing this. I see what you are saying about Fields should not know anything about the curve. I could just remove the need to pass the curve_name in the fields, which is pretty useless.

Another major point, that I wanted to raise is that, I am minorly 👎 on keeping the <some_curve>_field_elements.py. I want all of those to be as part of the Fields API. That is why you can find the fields of all the curves in __init__.py of the Fields Module in this PR.

One more thing about the Base* convention is that, I don't think that we need a Base* class here. This is because, even if we were to include a Base class, it would have a lot of functionality and not just the abstract functions, which is kind of adverse to the definition of Base Classes IIUC (Please correct me if wrong).

pipermerriam

👍 Let me know when you're ready for me to click the merge button

py_ecc/fields/__init__.py

pipermerriam · 2019-03-04T16:07:30Z

py_ecc/fields/field_elements.py

+    """
+    n = None  # type: int
+    field_modulus = None
+    # curve_name can be either 'bn128' or 'bls12_381'


What do you think about using an enum.Enum for the curve names. This would give us much stronger type safety and prevent certain types of errors like misspelling the dictionary key.

The curve_name attribute has been removed from the API. This is because the fields should not be knowing about the curve which is going to use the fields. Hence it has been removed which makes this passing around of curve_name obsolete.

pipermerriam · 2019-03-04T16:11:48Z

py_ecc/fields/field_elements.py

+    def __add__(self, other: IntOrFQ) -> "FQ":
+        if isinstance(other, FQ):
+            on = other.n
+        elif isinstance(other, int):


I don't know if this should be experimented with before merging this PR or something we try after but... what if we just made FQ a subclass of int. My intuition says it could solve a whole swath of these various type checks that occur since we could treat FQ instances as integers.

What isn't clear to me is how difficult or necessary it would be to ensure that an FQ object is what is return returned from operations like a * b and b * a where one of a, b is an FQ and one is an int. Stated differently, it seems potentially problematic if it is difficult to reason about whether an object we are interacting with is an integer or an FQ as well as whether the return value of various mathmatical operations will be an int or FQ

@pipermerriam I see what you are saying. It should work as per theory. But I would like this experimentation to be done after the refactoring. Opened #54 for this.

pipermerriam · 2019-03-04T16:13:11Z

py_ecc/fields/field_elements.py

+
+    @classmethod
+    def one(cls) -> "FQ":
+        return cls(1)


Not for this PR but a possible performance gain might be available if we interned (cached) these return values so that we don't need to re-instantiate the 1 and 0 values each time.

Opened #55 for this

pipermerriam · 2019-03-04T16:18:30Z

py_ecc/fields/field_elements.py

+                .format(type(other))
+            )
+
+        for c1, c2 in zip(self.coeffs, other.coeffs):


I think that since we have the isinstance check up top we're safe here, but worth pointing out that zip discards extra values if one of the values is longer so unless we know that these are the same length there's an error edge cases here.

@pipermerriam #45 should take care of this. (Probably a separate PR for this.)

Bhargavasomu · 2019-03-04T19:37:00Z

@pipermerriam this should be ready to merge.

Bhargavasomu force-pushed the refactor_v2 branch 2 times, most recently from 1f9a433 to c86163e Compare January 13, 2019 15:11

wemeetagain mentioned this pull request Jan 14, 2019

Fix mismatched testing dependency versions #43

Merged

carver reviewed Jan 19, 2019

View reviewed changes

carver reviewed Jan 21, 2019

View reviewed changes

carver mentioned this pull request Jan 23, 2019

Add FQP.copy() #46

Open

Bhargavasomu force-pushed the refactor_v2 branch from c86163e to 3840115 Compare January 25, 2019 13:17

pipermerriam approved these changes Jan 25, 2019

View reviewed changes

Bhargavasomu mentioned this pull request Feb 19, 2019

Library refactor #31

Open

Bhargavasomu added 11 commits February 27, 2019 12:15

Remove Fields Redundancy

0447493

Change ValueError to TypeError wherever apt

08eb068

Add minor fixes and adjustments

28c0a9d

Replace modulo operation recursion with NotImplementedError

5e5e734

Improve readability by avoiding passing curve_name in all methods

fd7ef53

Integrate Fields module with curves

efedeef

Remove mypy and update flake8 version

b343f38

Add normalize_FQ_point function to FQ

6cba6b1

Show which among lm,hm,nm,low,high,new are not of same length

f91a953

Include changes of PR 47

3db8085

Add optimizations

d31e990

Bhargavasomu force-pushed the refactor_v2 branch from 8429ec7 to d31e990 Compare February 27, 2019 08:46

Bhargavasomu force-pushed the refactor_v2 branch from 3a9e251 to cd2aa80 Compare February 27, 2019 13:59

Remove calling normalize_FQ_point function for performance reasons

0f19487

Bhargavasomu force-pushed the refactor_v2 branch from cd2aa80 to 0f19487 Compare February 27, 2019 15:38

pipermerriam approved these changes Mar 4, 2019

View reviewed changes

Bhargavasomu mentioned this pull request Mar 4, 2019

[EXPERIMENT] Fields as subclass of int #54

Open

pipermerriam merged commit 66bfd69 into ethereum:master Mar 5, 2019

Bhargavasomu mentioned this pull request Mar 7, 2019

Remove the curve_name from the fields API #61

Merged

renovate bot mentioned this pull request Mar 14, 2019

Update dependency py-ecc to v1.6.0 - autoclosed AugurProject/augur#936

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Redundancy of Fields among curves #41

Remove Redundancy of Fields among curves #41

Bhargavasomu commented Jan 13, 2019

Bhargavasomu commented Jan 13, 2019

carver commented Jan 18, 2019

carver left a comment

carver Jan 19, 2019

Bhargavasomu Jan 22, 2019

carver left a comment

Bhargavasomu commented Jan 25, 2019 •

edited

Loading

pipermerriam Jan 25, 2019

Bhargavasomu Jan 25, 2019

Bhargavasomu Jan 25, 2019

pipermerriam Jan 25, 2019

Bhargavasomu Jan 25, 2019

Bhargavasomu commented Jan 25, 2019

Bhargavasomu commented Feb 6, 2019

Bhargavasomu commented Feb 27, 2019

Bhargavasomu commented Feb 27, 2019

ChihChengLiang commented Mar 3, 2019

Bhargavasomu commented Mar 4, 2019

pipermerriam left a comment

pipermerriam Mar 4, 2019

Bhargavasomu Mar 4, 2019

pipermerriam Mar 4, 2019

Bhargavasomu Mar 4, 2019

pipermerriam Mar 4, 2019

Bhargavasomu Mar 4, 2019

pipermerriam Mar 4, 2019

Bhargavasomu Mar 4, 2019

Bhargavasomu commented Mar 4, 2019

Remove Redundancy of Fields among curves #41

Remove Redundancy of Fields among curves #41

Conversation

Bhargavasomu commented Jan 13, 2019

What was wrong?

How was it fixed?

Note

Cute Animal Picture

Bhargavasomu commented Jan 13, 2019

carver commented Jan 18, 2019

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver left a comment

Choose a reason for hiding this comment

Bhargavasomu commented Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhargavasomu commented Jan 25, 2019

Bhargavasomu commented Feb 6, 2019

Bhargavasomu commented Feb 27, 2019

Bhargavasomu commented Feb 27, 2019

ChihChengLiang commented Mar 3, 2019

Bhargavasomu commented Mar 4, 2019

pipermerriam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhargavasomu commented Mar 4, 2019

Bhargavasomu commented Jan 25, 2019 •

edited

Loading