-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Api group elements refactor #220
Conversation
… of arbitrary degrees
db32a8c
to
9135fe3
Compare
Note: if we want a fully generic API, we should either change our serialization method for group elements and use the serialization provided by libff, or extend libff to expose the degrees of the extension fields. See: scipr-lab/libff#48 |
e5983cf
to
9158920
Compare
211a02e
to
2c20a57
Compare
@@ -34,16 +35,59 @@ libff::bigint<FieldT::num_limbs> bigint_from_hex(const std::string &hex) | |||
} | |||
|
|||
template<typename FieldT> | |||
std::string field_element_to_hex(const FieldT &field_el) | |||
std::string base_field_element_to_hex(const FieldT &field_el) |
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.
I imagined that one of the advantages of having the tower_extension_degree
const is that we could support code that is generic across all fields (base fields and extension fields). AFAICT, we will have cases where coordinates of G2 belnog to an extension field and cases where that are elements of a base field, so it seems like it could be useful to just have a single entry point: std::string field_element_to_json(const FieldT &field_el)
which works over all fields.
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 we can do that, I initially started to implement a single recursive function which was calling itself on all extension field coefficients down until the base case/stopping condition which was when FieldT::extension_degree == 1
(i.e. no more tower levels to navigate -> base field).
However, since our twist fields (in the current state of our projects) are small extensions (i.e. Fp, Fp^2 or Fp^3 depending on the curves we use), these extensions didn't require any towering, and I thus decided to get away from this recursive function to aim for something simpler with these 2 functions - and remove the case of serializing "towered" extension field elements which is not of direct interest for now.
If I remember correctly, I also had some issues when I started to implement this recursive function because the function was templatized on EFieldT
which was instantiated differently for each recursive call. I probably did something dodgy though, my priority was mostly to have something working and more flexible than what we had in libzeth
:)
ext_field_element_to_hex<libff::Fqe<ppT>>(aff.Y); | ||
|
||
const size_t extension_degree = libff::Fqe<ppT>::extension_degree(); | ||
BOOST_ASSERT(extension_degree >= 2); |
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.
Can these be made static_assert
?
ASSERT_EQ(fe, fe_decoded); | ||
} | ||
|
||
TEST(FieldElementUtilsTest, ExtFieldElementEncodeDecode) |
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.
It could be useful to make this generic and run over several extensions. Or, maybe the tests for serializing G2 could be made generic and executed over several curves.
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 definitely, but weren't you working toward removing any "default" template instantiation from libzeth
in order to aim toward a fully generic library and generic tests (executed - as in libff on various curves)? This seems like something to carry out on a separate PR as it is of broader interest
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.
Yeah, I guess it could be done independently. I mention it here because it's related to testing that this new generic code (i.e. the library code has become more generic, particularly if we support to_json
over all field types, so it would be nice to also make the tests generic), but it could conflict with the PR to remove the default.
(To be clear, it could conflict in terms of merging, but is consistent in the sense of removing hard-coded types in the library ... if that makes sense).
In particular, doing this for G2 proto utils tests could clear up the issue with bw6-761 if that is actually an issue.
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.
(To be clear, it could conflict in terms of merging, but is consistent in the sense of removing hard-coded types in the library ... if that makes sense)
Yes definitely in line with the other PR. Ok, I'll have a look into this and make the test generic on my end then, may be easier that way :)
const std::vector<std::string> x_coord = | ||
ext_field_element_to_hex<libff::Fqe<ppT>>(aff.X); | ||
const std::vector<std::string> y_coord = | ||
ext_field_element_to_hex<libff::Fqe<ppT>>(aff.Y); |
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.
(Related to the comments about generic functions over fields, and tests over multiple curves). Will this work for bw6-761?
libzeth::ext_field_element_to_hex<Fqe>(fe); | ||
|
||
std::string str; | ||
for (const auto &entry : fe_hex) |
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.
missing braces?
Superseded by #232 |
The current proto message for elements of G2 is tailored for a very specific pairing group (bn254) in which elements of G2 have coordinates in F_{q^2}. This is not very flexible and does not allow to use the API for other pairing groups, where elements of G2 can have coordinates in F_{q^m}, m =/= 2.
This PR fixes that.