-
Notifications
You must be signed in to change notification settings - Fork 56
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
Conversation
src/uint/div_limb.rs
Outdated
#[inline(always)] | ||
pub(crate) const fn div3by2( | ||
pub(crate) const fn div3by2_vartime( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Edit: merged! |
i += 1; | ||
} | ||
|
||
(quo, rem) | ||
quo |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I now see this broke the build: https://github.com/RustCrypto/crypto-bigint/actions/runs/10472862474/job/29003356626
Looks good to me. I think at some point it would make sense to turn functions like |
Going to revert this merge as it broke the build. Edit: opened #658 |
This reverts commit e7d3b16. This needs changes as upstream diverged.
div3by2()
saturating_sub
withwrapping_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"), sosaturating_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:
shl.rs
/shr.rs
? Are they general enough?div3by2_vartime()
be moved out ofdiv_limb.rs
?