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 fixes for modular inversion #159

Merged
merged 15 commits into from
Jan 10, 2023
Merged

Some fixes for modular inversion #159

merged 15 commits into from
Jan 10, 2023

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Jan 1, 2023

  • Break long lines in comments and tests and fix some hardcoded bit sizes
  • Fix the hardcoded Limb size in inv_odd_mod() - was set to 64 (did not cause errors, just made the inversion twice as slow on 32-bit targets)
  • Added inv_odd_mod_bounded() for cases of argument/modulus known to be small
  • Removed inv_odd_mod_option() - we do not provide such interface for other constant functions

Additionally:

  • Introduced a CtChoice newtype for constant-time const fns
  • Replaced some multiplications by Word::MAX with negations
  • Normalized constant-time comparisons API in Limb and Uint: removed ct_cmp() (we never need it in constant-time context, ct_gt()/ct_lt()/ct_eq() are enough), matched const fns with subtle trait methods, matched methods between Limb and Uint
  • Removed SignedWord and SignedWideWord
  • Uint objects are taken by reference where they were previously taken by value.

@tarcieri
Copy link
Member

tarcieri commented Jan 1, 2023

Tangentially related: what should we return as is_some in constant functions that can fail - Limb or Word? There are cases of both in the codebase.

It should probably always be Word, as Limb is mainly intended for use inside the inner array of a Uint

@fjarri
Copy link
Contributor Author

fjarri commented Jan 1, 2023

Hm, in some places we also return u8 in {0, 1}. Which makes sense if it's converted to Choice, but inconvenient if it has to be chained with const fn analogues of subtle.

@tarcieri
Copy link
Member

tarcieri commented Jan 2, 2023

Yeah, subtle's use of u8 is a bit odd as I imagine most of the time LLVM is implicitly widening them to word-sized values sitting in a register.

I don't really have a better suggestion for Word -> Choice than casting to an intermediate u8.

@fjarri
Copy link
Contributor Author

fjarri commented Jan 2, 2023

I don't really have a better suggestion for Word -> Choice than casting to an intermediate u8.

It's more of a question, whether we should return Word everywhere, or u8 (0 or 1) everywhere. Currently both are used in different places.

@tarcieri
Copy link
Member

tarcieri commented Jan 2, 2023

Word seems like it had the most mechanical sympathy to me

@fjarri
Copy link
Contributor Author

fjarri commented Jan 2, 2023

Ok, updated to Word everywhere.

@fjarri
Copy link
Contributor Author

fjarri commented Jan 3, 2023

I wonder if it even makes sense to define some type IsSome = Word to make it more distinguishable.

@tarcieri
Copy link
Member

tarcieri commented Jan 4, 2023

@fjarri sounds like a great idea!

@fjarri
Copy link
Contributor Author

fjarri commented Jan 5, 2023

More things that could be done here:

  • Match the self usage in comparison API between Limb and Uint: Uint uses self and rhs, Limb uses lhs and rhs. The former seems more natural, the latter does not conflict with subtle traits.
  • Make CtChoice an actual type, with TRUE and FALSE constants, conversion to Choice etc? (Alternatively, standalone CT_CHOICE_TRUE/FALSE constants can be added)
  • Where do I put CtChoice? Currently it lives in limb.rs.
  • Normalize taking Uint by ref or by value (I think it should be taken by ref everywhere, but there are instances where it's passed by value)

@fjarri
Copy link
Contributor Author

fjarri commented Jan 8, 2023

Just in case there's an ambiguity: those things could be done, but not necessarily in this PR since it has already overgrown its initial purpose.

src/uint/cmp.rs Outdated Show resolved Hide resolved
src/limb.rs Outdated Show resolved Hide resolved
src/uint/rand.rs Outdated Show resolved Hide resolved
@fjarri
Copy link
Contributor Author

fjarri commented Jan 10, 2023

Match the self usage in comparison API between Limb and Uint: Uint uses self and rhs, Limb uses lhs and rhs. The former seems more natural, the latter does not conflict with subtle traits.

Done. Uint now has (lhs, rhs) signatures too.

Make CtChoice an actual type, with TRUE and FALSE constants, conversion to Choice etc? (Alternatively, standalone CT_CHOICE_TRUE/FALSE constants can be added)

Done

Where do I put CtChoice? Currently it lives in limb.rs.

Now lives in ct_choice.rs

Normalize taking Uint by ref or by value (I think it should be taken by ref everywhere, but there are instances where it's passed by value)

Done

@tarcieri tarcieri merged commit 4a252a3 into RustCrypto:master Jan 10, 2023
@fjarri fjarri deleted the fix-inv branch January 10, 2023 03:21
@tarcieri tarcieri mentioned this pull request Feb 27, 2023
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.

2 participants