Skip to content
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.

Sign up for GitHub

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

Implement a checked conversion from bytes method in Fq #81

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Implement a checked conversion from bytes method in Fq #81

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/fields/fq.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,29 @@ cfg_if! {
}
}
}

impl Fq {
/// Convert bytes into an Fq element, returning None if these bytes are not already reduced.
///
/// This means that values that cannot be produced by encoding a field element will return
/// None, enforcing canonical serialization.
pub fn from_bytes_checked(bytes: &[u8; 32]) -> Option<Self> {
let reduced = Self::from_raw_bytes(bytes);
if reduced.to_bytes_le() == *bytes {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to the contents of this pr, but why is this to_bytes_le rather than just to_bytes? are we ever doing big-endian serialization? if not, could we have it just be to_bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My motivation was that:

  • Arkworks has from_bytes_le and from_bytes_be methods, so this mirrors that.
  • Furthermore, I think it's good to specify what the endianness is, because it prevents any confusion on the user's part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bikeshed, but my counterpoints would be:

  • it's inconsistent to have from_bytes but to_bytes_le
  • it's better to not convey endianness, because it signals that endianness is not a user-visible concern, the cryptography layer just exposes an "encoding" abstraction which is to be treated as opaque bytes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, although for context, there's a separate type for the Encoding of an Element on the curve, and in practice there's no reason for users to be aware of Fq at all. For Fr, this is a reasonable point though, yeah. One reason having those can be nice is that the direct methods on each field are also what we use to implement the Arkworks traits, and there, because different methods care about endiannness, it's useful to know what endianness we're using when implementing them.

In an ideal world Arkworks would be more opaque; alas.

Some(reduced)
} else {
None
}
}
}

#[cfg(test)]
mod test {
use super::*;

#[test]
fn test_from_bytes_checked() {
assert_eq!(Fq::from_bytes_checked(&[0; 32]), Some(Fq::zero()));
assert_eq!(Fq::from_bytes_checked(&[0xFF; 32]), None);
}
}
Loading