-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: binary field #90
Conversation
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.
This is looking good my friend!!!
Suggestions so far only. Hopefully nothing nitpicky :)
src/field/binary/extension.rs
Outdated
#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, PartialOrd, Ord)] | ||
pub struct BinaryFieldExtension<const K: usize> | ||
where [(); 1 << K]: { | ||
/// coefficients of field element | ||
pub coefficients: [BinaryField; 1 << K], | ||
} |
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.
This guy is a const expr fiend now!!
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.
everywhere I go, I see compile time constants now :)
src/field/binary/mod.rs
Outdated
|
||
/// binary field containing element `{0,1}` | ||
#[derive(Debug, Default, Hash, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
pub struct BinaryField(u8); |
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.
Perhaps a type alias instead with:
pub struct BinaryField(u8); | |
pub type BinaryField = PrimeField<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.
I believe that would collapse a lot of this module here.
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.
that'll work flawlessly, but i believe it wouldn't allow us to showcase the main properties of binary field, i.e. addition being bitwise XOR and multiplication bitwise AND.
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.
What if you use a trait impl override/default/specialization? There are some Rust unstable feats that may help here to let us specialize to the binary case.
I thought we had it in the project, but don't seem to anymore. Anyway, here is an RFC for it:
https://rust-lang.github.io/rfcs/1210-impl-specialization.html
There could be others.
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.
@Autoparallel i need some more time to check and iterate on this. so, i'll open a new PR for this. don't want to block AES implementations due to this change, so merging it.
8ca2afa
to
4e77586
Compare
@0xJepsen @Autoparallel this is ready for review now, I have added binary fields using our ronk traits in tests. my reasoning being, I want to showcase the ease of arithmetic in binary fields and their efficient embedding and small by large multiplication properties. this obviously bloats the code a bit can remove this entirely, and directly use our traits, if we feel that's the better approach. |
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.
Very nice work on this sir
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.
This looks good to me! I just had a few clarifying thoughts.
One on a previous comment, one in this review.
src/field/binary_towers/extension.rs
Outdated
pub coefficients: [BinaryField; 1 << K], | ||
} | ||
|
||
impl<const K: usize> BinaryFieldExtension<K> |
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.
Should I be thinking of BinaryFieldExtension<K>
as the binary tower with height K
?
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.
Yup correct, BinarFieldExtension<K>
is isomorphic to
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.
Ah nice. Probably a nit, but what do you think of naming this BinaryTower
so we can think of GaloisField<K, 2>
as degree K
extensions over PrimeField<2>
?
Let it be known that it doesn't bother me either way, you should choose what you think is proper :)
37ae27a
to
0a12fe9
Compare
It changes the following:
I've kept the implementation closer to theory as described in Section 2.3 of Binius. let me know if anything needs more work.
TODO:
primitive_element_generator
in extension fields