Fix overflow concerns and add Serde functions #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added another point G1_7P for more interesting tests.
Because these tests are fast and give extra assurance.
Because they aren't testing anything interesting, thus they are really duplicate tests. I had them originally because they were useful in getting the implementation right (helping me find the pattern of an error in code never committed). I probably could remove more of these tests, but they are fast enough as is.
We need this in the future to decompress compressed G1 and G2 points.
The get_flags function extracts the three most significant bits that will allow us to implement the BLS decompression algorithm from py_ecc: https://github.com/ethereum/py_ecc/blob/main/py_ecc/bls/point_compression.py#L43
This function calls the actual decompression function to convert a 48-byte compressed point into a G1 point (x, y).
Not only can 'x' not be equal to zero, 'x mod n' cannot be zero for whatever modulus 'n' is used. This needed to be more clear in the documentation.
My previous thinking was to have these function names match the Python equivalents, but it came up in code review that adding the _mod postfix would improve clarity for anyone that needs to look at the code in the future. As I thought more about this, I see how the Python code makes it seem like these two functions are built-in, but the actual implementations are overloaded for a specific arithmetic field object type. I want to avoid this small "gotch ya."
This commit addresses several issues raised during code review. Namely, the Cryptol specs in Utils, BlsEC, and BlsHelpers modules are 1) overly complex by using the Integer type for modular arithmetic and 2) have potential overflow issues when using Integers and fixed width size types. I was not previously aware of the type Z in Cryptol, which is a built-in type for integer fields module n, specified as (Z n), for some constant modulus n. By replacing the usage of Integer with Z we will be able to reduce the complexity of the specs and eliminate potential overflow errors. Also, using Z will actually match the Python specification better, since it uses the py_ecc type FQ, a Python class for modular field elements and operations. Next steps: - Replace the Integer type to the Z type in the functions of the BlsEC and BlsHelpers modules (this will take more refactoring).
For the same reason as mentioned in the previous commit, this change eliminates any concerns about overflow errors. Also, the computation has been simplified, and the removal of conversions makes the actual arithmetic easier to verify.
Just as a convenience to wrap the two conversions needed to go from a fixed bit size value to a Z type.
Just as a convenience to wrap the two conversions needed to go from a Z type to a fixed bit size value.
This matches the builtin (^^) operator in Cryptol for the Z type which requires the exponent to be an Integer, which is more flexible. This also reduces conversions when we use this pow_mod function in BlsHelpers, so that is another plus.
For the same reason as mentioned in commit: utils: Replace uses of Integer with Z to address overflow concernss ago This change eliminates any concerns about overflow errors. Also, the computation has been simplified, and the removal of conversions makes the actual arithmetic easier to verify.
After the previous changes, I noted that the value Fp was only used in one place, and I could see no benefit to using Fp in place of `(FP). So, I decided to replace that instance and remove the value Fp. I prefer the name Fp over FP, so now that only the type FP is used, I renamed the type FP to Fp.
These two types are no longer used in BlsEC, so I moved them into the only module that uses them, BlsHelpers. This change also has the benefit of showing what the previous few commits have done: make Bytes48 a type only used for serialized (or compressed) G1 points. This more closely matches the Python spec, so that is another bonus.
Just for personal preferrence so that all the functions for deserialization / decompression are grouped together.
Because it is slightly misleading. This is actually a compressed point, a 48-byte value, that must be decompressed to x, y coordinates. But this name matches the Python spec, so I am keeping it as is.
Rename: bytes48_to_g1_points => bytes48_to_G1
Replace: (Z Fp, Z Fp) => G1Point
Along with tests and check (random test) to make sure compressing and then decompressing a point returns the same point. The search space is too large for this property to complete as a proof.
separate module called BlsSerde The BlsHelpers module was starting to get really large, so I split out all of the serialization related functionality into a separate module.
Just a clean up commit to slightly simplify the code and use functions I previously created in the Utils module for this purpose.
This is a stylistic commit and just matches my preference for naming and less code (two lines!).
In the compute_powers function. This is the last potential spot for overflow concerns that I am aware of.
Because the unit tests for bytes48_to_G1 and g1_to_bytes48 are so simple and their implementations are just a wrapper for the BlsSerde functions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is unfortunately not single purposed. I started intending on implementing more of the functions needed by
g1_lincomb, specifically theg1_multi_expand serialization / deserialization helper functionsbytes_to_G1andg1_to_bytes. The latter lead to the creation of a new moduleBlsSerdeto handle the internals of how points are compressed and decompressed, as specified in py_ecc.During this process, I addressed the overflow concerns, since those issues involve the same code. So following the commits you will see where I jump around to address overflow concerns and refactoring, while pursing implementing the goal functions.
NOTE TO REVIEWER: I strongly recommend reviewing commit-by-commit, since the total diff is probably unwieldy. I did put in a lot of care into each commit and I didn't make to make steps backward along the way. Sorry for the poorly compose pull request.
Closes #21 and partially address #16 (2 of 3 tasks).