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

Some simplifications of vartime division #646

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Aug 11, 2024

  • Extract shl/shr by a limb-sized shift into separate methods
  • Add a more detailed docstring and some debug assertions in div3by2()
  • Replaced saturating_sub with wrapping_sub in several places. While logically it is the same thing (the wrapping/saturation only happens for values that are later selected out), I think there are readability advantages. First, elsewhere in the code we use wrapping ops for selected out values (meaning "perform the subtraction without any checks, since we already have a constant-time condition for that"), so saturating_sub indicates that the algorithm actually uses the saturation mechanic. Second, in case of a bug, it will be easier to spot the consequences of a "0xffff..." value than a 0.

Questions:

  • should I move the new shl/shr methods to shl.rs/shr.rs? Are they general enough?
  • should div3by2_vartime() be moved out of div_limb.rs?

#[inline(always)]
pub(crate) const fn div3by2(
pub(crate) const fn div3by2_vartime(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also worried about the runtime of this method depending on the dividend value. Replacing it completely would also mess with my PR #643, do you want to do a quick review of that one?

Copy link
Contributor Author

@fjarri fjarri Aug 11, 2024

Choose a reason for hiding this comment

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

The original div3by2() can be preserved, of course, but I am worried about the connection of its contents and the original algorithm Q from the article. Could you add some explanations about why it actually does the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a PR to better align that method with the definition, as it doesn't look like overflow was being handled properly in the remainder.

Copy link
Member

Choose a reason for hiding this comment

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

@andrewwhitehead can you take a look again now that #643 is merged?

@tarcieri
Copy link
Member

tarcieri commented Aug 15, 2024

It seems like we should probably get #643 merged first since it might affect this.

Edit: merged!

i += 1;
}

(quo, rem)
quo
Copy link
Contributor

Choose a reason for hiding this comment

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

The constant-time division does use this remainder

Copy link
Member

@tarcieri tarcieri Aug 20, 2024

Choose a reason for hiding this comment

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

@andrewwhitehead
Copy link
Contributor

Looks good to me.

I think at some point it would make sense to turn functions like shl_limb_vartime into methods on a struct like LimbsMut(&mut [Limb]), which might be a convenient way to collect and document them. Then once there is language support they could be const-ified and used by both Uint and BoxedUint.

@tarcieri tarcieri merged commit e7d3b16 into RustCrypto:master Aug 20, 2024
18 checks passed
@tarcieri
Copy link
Member

tarcieri commented Aug 20, 2024

Going to revert this merge as it broke the build. Edit: opened #658

tarcieri added a commit that referenced this pull request Aug 20, 2024
This reverts commit e7d3b16.

This needs changes as upstream diverged.
tarcieri added a commit that referenced this pull request Aug 20, 2024
This reverts commit e7d3b16.

This needs changes as upstream diverged.
@fjarri fjarri deleted the simplify-div branch August 21, 2024 17:32
tarcieri pushed a commit that referenced this pull request Aug 21, 2024
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