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

support 32bit and expand CI #30

Merged
merged 2 commits into from
Feb 1, 2022
Merged

support 32bit and expand CI #30

merged 2 commits into from
Feb 1, 2022

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Jan 20, 2022

Closes #29

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2022

Codecov Report

Merging #30 (b84ddb0) into master (93b935c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   86.81%   86.82%   +0.01%     
==========================================
  Files          14       14              
  Lines        6788     6794       +6     
==========================================
+ Hits         5893     5899       +6     
  Misses        895      895              
Impacted Files Coverage Δ
src/lib.rs 96.29% <ø> (ø)
src/scalar.rs 90.53% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93b935c...b84ddb0. Read the comment docs.

@dignifiedquire dignifiedquire changed the title ci: add github actions support 32bit and expand CI Jan 20, 2022
@dignifiedquire dignifiedquire marked this pull request as ready for review January 20, 2022 23:34
@dignifiedquire
Copy link
Contributor Author

@dot-asm if you have a moment to sanity check this, I would appreciate it!

impl PrimeFieldBits for Scalar {
// Representation in non-Montgomery form.
type ReprBits = [u64; 4];
Copy link

Choose a reason for hiding this comment

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

What prevents you from type ReprBits = [u8; 32]; and then

impl PrimeFieldBits for Scalar {
    // Representation in non-Montgomery form.
    type ReprBits = [u8; 32];

    fn to_le_bits(&self) -> FieldBits<Self::ReprBits> {
        let mut scalar = blst_scalar::default();
        unsafe { blst_scalar_from_fr(&mut scalar, &self.0) };

        FieldBits::new(scalar.b)
    }

    fn char_le_bits() -> FieldBits<Self::ReprBits> {
        FieldBits::new(MODULUS_REPR)
    }
}

In other words without dependency on target_pointer_width.

Copy link

@dot-asm dot-asm Jan 21, 2022

Choose a reason for hiding this comment

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

Not exactly related to this, but I've spotted

    /// Converts an element of `Scalar` into a byte representation in
    /// little-endian byte order.
    #[inline]
    pub fn to_bytes_le(&self) -> [u8; 32] {
        let mut out = [0u64; 4];
        unsafe { blst_uint64_from_fr(out.as_mut_ptr(), &self.0) };
        out.as_byte_slice().try_into().unwrap()
    }

This can also be rewritten using blst_scalar_from_fr. This way you'll remove dependency on [target platform's] endiannes. [At least in this case of course. [Not that I looked for other cases.]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What prevents you from type ReprBits = [u8; 32]; and then

The point of this representation is to be u64 or u32s due to how it is used

Copy link

Choose a reason for hiding this comment

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

The FieldBits accepts a range of types, u8 included, and blst_scalar is in right byte order. Note that cargo test passes...

@dignifiedquire
Copy link
Contributor Author

Docs failure is because of ff failing doc tests, so ignoring zkcrypto/ff#75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add x86 support for blstrs
3 participants