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.

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

Add to_bytes to bigint #166

Merged
merged 11 commits into from
Jan 7, 2021
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ The main features of this release are:
a default value.
Downstream users other than `ark-curves` should not see breakage unless they rely on these methods/traits explicitly.
- #165 (ark-ff) Add `from_base_field_elements` as a method to the `Field` trait.
- #166 (ark-ff) Change `BigInt::{from_bytes, to_bits}` to `from_bytes_le, from_bytes_be, to_bits_le, to_bits_be`.

### Features
- #20 (ark-poly) Add structs/traits for multivariate polynomials
Expand Down Expand Up @@ -69,6 +70,7 @@ The main features of this release are:
- #153 (ark-serialize) Add an impl of `CanonicalSerialize/Deserialize` for `Rc<T>`.
- #157 (ark-ec) Speed up `variable_base_msm` by not relying on unnecessary normalization.
- #158 (ark-serialize) Add an impl of `CanonicalSerialize/Deserialize` for `()`.
- #166 (ark-ff) Add a `to_bytes_be()` and `to_bytes_le` methods to `BigInt`.

### Bug fixes
- #36 (ark-ec) In Short-Weierstrass curves, include an infinity bit in `ToConstraintField`.
Expand Down
44 changes: 39 additions & 5 deletions ff/src/biginteger/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ macro_rules! bigint_impl {
}

#[inline]
fn from_bits(bits: &[bool]) -> Self {
fn from_bits_be(bits: &[bool]) -> Self {
let mut res = Self::default();
let mut acc: u64 = 0;

Expand All @@ -166,11 +166,45 @@ macro_rules! bigint_impl {
res
}

fn from_bits_le(bits: &[bool]) -> Self {
let mut res = Self::default();
let mut acc: u64 = 0;

let mut bits = bits.to_vec();
for (i, bits64) in bits.chunks(64).enumerate() {
for bit in bits64.iter().rev() {
acc <<= 1;
acc += *bit as u64;
}
res.0[i] = acc;
acc = 0;
}
res
}

#[inline]
fn to_bits_be(&self) -> Vec<bool> {
BitIteratorBE::new(self.0).collect::<Vec<_>>()
}

#[inline]
fn to_bits_le(&self) -> Vec<bool> {
BitIteratorLE::new(self.0).collect::<Vec<_>>()
}

#[inline]
fn to_bytes_be(&self) -> Vec<u8> {
let mut le_bytes = self.to_bytes_le();
le_bytes.reverse();
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.

I think these can all be made default impls on the trait itself? For the bits, you can call BitIteratorXX::new(self)..., and to_bytes_be is implemented directly in terms of to_bytes_le.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, ok I'll do that

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realize the trait had AsRef<[u64]>, seems kind of word to be in the trait imo. Why would we not allow float back ends?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I think its weird to only implement one of to_bytes_le and to_bytes_be as the default, I'd prefer it in the macro. (The reason to put it in the default impl would be for users implementing these in an alternate fashion, but that then assumes that to_bytes_le is the native endianness to do it)

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think we can move to_bytes_le also to the trait, as it can be implemented from AsRef<[u64]>.

(This is an example of why the AsRef<[u64]> trait bound is useful)

Copy link
Member Author

@ValarDragon ValarDragon Jan 7, 2021

Choose a reason for hiding this comment

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

I'll move it for now, but I think its worth revisiting if BigInt needs AsRef<[u64]>, I feel like we shouldn't make PrimeField / elliptic curves depend on that. (Agreed that its fine for the prime field generation macro though)

Copy link
Member Author

Choose a reason for hiding this comment

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

We lose capacity hints with the to_bytes being a default implementation, not really sure its worth making it default then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah no, I was wrong. The capacity hints are still available


#[inline]
fn to_bits(&self) -> Vec<bool> {
let mut res = Vec::with_capacity(256);
for b in BitIteratorBE::new(self.0) {
res.push(b);
fn to_bytes_le(&self) -> Vec<u8> {
let array_map = self.0.iter().map(|limb| limb.to_le_bytes());
let mut res = Vec::<u8>::new();
ValarDragon marked this conversation as resolved.
Show resolved Hide resolved
for limb in array_map {
res.extend_from_slice(&limb);
}
res
}
Expand Down
26 changes: 21 additions & 5 deletions ff/src/biginteger/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
bytes::{FromBytes, ToBytes},
fields::BitIteratorBE,
fields::{BitIteratorBE, BitIteratorLE},
UniformRand,
};
use ark_serialize::{CanonicalDeserialize, CanonicalSerialize, SerializationError};
Expand Down Expand Up @@ -96,11 +96,27 @@ pub trait BigInteger:

/// Returns the big integer representation of a given big endian boolean
/// array.
fn from_bits(bits: &[bool]) -> Self;
fn from_bits_be(bits: &[bool]) -> Self;

/// Returns the bit representation in a big endian boolean array, without
/// leading zeros.
fn to_bits(&self) -> Vec<bool>;
/// Returns the big integer representation of a given little endian boolean
/// array.
fn from_bits_le(bits: &[bool]) -> Self;

/// Returns the bit representation in a big endian boolean array,
/// with leading zeroes.
fn to_bits_be(&self) -> Vec<bool>;

/// Returns the bit representation in a little endian boolean array,
/// with trailing zeroes.
fn to_bits_le(&self) -> Vec<bool>;

/// Returns the byte representation in a big endian byte array,
/// with leading zeros.
fn to_bytes_be(&self) -> Vec<u8>;

/// Returns the byte representation in a big endian byte array,
/// with leading zeros.
fn to_bytes_le(&self) -> Vec<u8>;

/// Returns a vector for wnaf.
fn find_wnaf(&self) -> Vec<i64>;
Expand Down