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 bit accessor methods #122

Merged
merged 3 commits into from
Oct 10, 2022
Merged

Conversation

andrewwhitehead
Copy link
Contributor

@andrewwhitehead andrewwhitehead commented Sep 20, 2022

This adds UInt::bit_vartime(index) for accessing the value of the bit at a given index. It's trivial to cast the result to u8 or bool so I leave that up to the caller, to avoid casting the value twice.

For consistency and accuracy, bits is renamed to bits_vartime.

@andrewwhitehead andrewwhitehead changed the title Add bit accessor method Add bit accessor methods Sep 20, 2022
@andrewwhitehead
Copy link
Contributor Author

I've updated this PR to only add bit_vartime and skip bit for now.

@@ -25,7 +25,7 @@ impl<const LIMBS: usize> UInt<LIMBS> {
// Sometimes an increase is too far, especially with large
// powers, and then takes a long time to walk back. The upper
// bound is based on bit size, so saturate on that.
let res = Limb::ct_cmp(Limb(xn.bits() as Word), Limb(max_bits as Word)) - 1;
let res = Limb::ct_cmp(Limb(xn.bits_vartime() as Word), Limb(max_bits as Word)) - 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This use of bits_vartime seems a little concerning after the renaming? @tarcieri

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems okay for now. Not a clear direction yet going forward

Copy link
Member

Choose a reason for hiding this comment

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

It seems like this method should probably use a prospective constant-time implementation of bits?

I can open a tracking issue

@tarcieri
Copy link
Member

Looks good now, thanks!

@tarcieri tarcieri merged commit f1a2829 into RustCrypto:master Oct 10, 2022
@tarcieri tarcieri mentioned this pull request Oct 11, 2022
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.

3 participants