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 PrimeFieldBits support to Scalar #579

Merged
merged 11 commits into from
Sep 20, 2023
Merged

Add PrimeFieldBits support to Scalar #579

merged 11 commits into from
Sep 20, 2023

Conversation

kayabaNerve
Copy link
Contributor

The char_le_bits function is a hack I don't legitimately recommend. I just felt it'd be better to make a PR, which works, than to solely file an issue requesting it as a feature.

The char_le_bits function is a hack I don't legitimately recommend. I just felt it'd be better to make a PR, which works, than to solely file an issue requesting it as a feature.
Comment on lines 1333 to 1336
let mut bytes = (Scalar::ZERO - Scalar::ONE).to_repr();
bytes[0] += 1;
debug_assert_eq!(DScalar::from_bytes_mod_order(bytes), DScalar::ZERO);
bytes.into()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should work but haven't tested it

Suggested change
let mut bytes = (Scalar::ZERO - Scalar::ONE).to_repr();
bytes[0] += 1;
debug_assert_eq!(DScalar::from_bytes_mod_order(bytes), DScalar::ZERO);
bytes.into()
crate::constants::BASEPOINT_ORDER.into()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test it now, thanks.

Copy link
Contributor Author

@kayabaNerve kayabaNerve Sep 12, 2023

Choose a reason for hiding this comment

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

That's a Scalar, not a byte array. Doesn't that break safety to have an unreduced Scalar in system? Especially now that Scalar addition assumes only unreduced Scalars exist? I'd guess it doesn't in practice, as it won't have any shouldn't never be set bits set, yet still sounds like something which should be a [u8; 32].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to_bytes().into() works, will push that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't that break safety to have an unreduced Scalar in system?

Yeah, seems a bit odd to have it in the public API if it's unreduced /cc @rozbb

Copy link
Contributor

@pinkforest pinkforest Sep 12, 2023

Choose a reason for hiding this comment

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

BASEPOINT_ORDER will be marked deprecated from 4.1.1 - this will be BASEPOINT_ORDER_PRIVATE as pub(crate)

@kayabaNerve
Copy link
Contributor Author

Sorry for this PR being a mess. I'm unsure why the no-std CIs are failing.

@tarcieri
Copy link
Contributor

It's possible that bitvec isn't working on no_std for whatever reason? Though I'm not sure why there isn't a proper compiler error.

Regardless it would probably be good to split this out into a bits feature that enables ff/bits so that bitvec doesn't become a mandatory dependency for users of the group feature.

@kayabaNerve
Copy link
Contributor Author

kayabaNerve commented Sep 12, 2023

I use ff, with bits, on libraries with no-std targets so that shouldn't be it... (though I may technically solely test them on alloc, not no-std).

I'll make bits a feature. group-bits for clarity? It's the bits of dalek's group functionality (despite technically affecting ff).

@pinkforest
Copy link
Contributor

pinkforest commented Sep 12, 2023

It's because we don't do additional cargo hack of the features via std build and we only do it via no_std that detects any errors via features - e.g. red herring that just appears via no_std run which has the feature tested but not in std run we don't do additional duplicate run :)

@rozbb
Copy link
Contributor

rozbb commented Sep 19, 2023

This looks good to me, thanks! Could you add an item in the changelog for this feature?

rozbb and others added 2 commits September 19, 2023 23:08
Co-authored-by: pinkforest(she/her) <36498018+pinkforest@users.noreply.github.com>
@rozbb rozbb merged commit 76a8b2a into dalek-cryptography:main Sep 20, 2023
22 checks passed
@kayabaNerve kayabaNerve deleted the patch-2 branch September 20, 2023 07:14
@kayabaNerve
Copy link
Contributor Author

Thanks for moving this forward before I had the chance, rozbb :) And thanks pinkforest for the actual edit needed.

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.

None yet

4 participants