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

Normalize the usage of prefixes for method names #417

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Dec 12, 2023

  • Don't use "ct_" prefixes for constant-time methods - it is the default
  • Renamed CtChoice to ConstChoice (to distinguish it from subtle::Choice)

@tarcieri
Copy link
Member

Ooh nice. This has bugged me as well, especially as all of the ct_*/Ct* functions/types are really just providing a const-friendly implementation and are more likely to get rewritten into branches with LLVM than the APIs using subtle

@fjarri
Copy link
Contributor Author

fjarri commented Dec 12, 2023

and are more likely to get rewritten into branches with LLVM than the APIs using subtle

Why do you think so? Why would LLVM turn bitwise operations into branches?

But in any case, this PR just renames stuff, it doesn't change the functionality in any way.


This actually made me go take a look at how some constant-time operations are implemented in subtle, and some of them use a different approach from what we chose. E.g. compare ConstantTimeGreater in https://docs.rs/subtle/latest/src/subtle/lib.rs.html#865 with what we have in CtChoice::from_word_lt(). Seems like the latter is more efficient, and is not in danger of being made into branches, so why did subtle go with a loop?

To expand on that, I wonder if subtle would be open to moving CtChoice there in some form (until const traits are stabilized), and then both const and non-const parts can share the same underlying algorithms.

@tarcieri
Copy link
Member

Why do you think so? Why would LLVM turn bitwise operations into branches?

I've seen at least alleged cases of it happening: https://github.com/klutzy/nadeko#why

This is the whole point of a crate like subtle: to create optimization barriers to prevent such rewrites by LLVM.

so why did subtle go with a loop?

I don't know why subtle did what it did for those comparison operations. It seems unnecessarily slow. FWIW I implemented them using borrowing subtraction with much better performance: #413

To expand on that, I wonder if subtle would be open to moving CtChoice

It would generally be good for subtle to have const fn support, but the main difference between Choice and CtChoice is that Choice provides an optimization barrier "CtChoice" does not (which makes the name a bit of a confusing misnomer)

@fjarri
Copy link
Contributor Author

fjarri commented Dec 12, 2023

but the main difference between Choice and CtChoice is that Choice provides an optimization barrier "CtChoice" does not

What barrier specifically? Can we add it to CtChoice as well?

@fjarri
Copy link
Contributor Author

fjarri commented Dec 12, 2023

I've seen at least alleged cases of it happening: https://github.com/klutzy/nadeko#why

That's strange, I tested the example code from Readme in Godbolt, and it gives a branch-free assembly with -O3.

@tarcieri
Copy link
Member

tarcieri commented Dec 12, 2023

It's currently using a volatile read: https://github.com/dalek-cryptography/subtle/blob/b3ba6bc/src/lib.rs#L229-L243

I don't think there's any way to make that work with const fn. You'd just have to give up the barrier in const fn cases, unfortunately (plus it's a trait-based API, so for any const fn support you'd need to use an inherent method).

Likewise const fn would preclude moving subtle to predication intrinsics like CMOV (although again that's a trait-based API, so there's that as well): dalek-cryptography/subtle#101

(Note: I have not carefully scrutinized what the performance impact of that would be)

And all of this is why I've begrudgingly accepted the existence of CtChoice despite the overlap with subtle::Choice.

@tarcieri
Copy link
Member

That's strange, I tested the example code from Readme in Godbolt, and it gives a branch-free assembly with -O3.

Regardless, these are classes of optimizations LLVM is free to do. There was quite a bit of discussion on this issue: The-DevX-Initiative/RCIG_Coordination_Repo#55

@tarcieri
Copy link
Member

Sidebar: it would probably be good to spell all of this out somewhere in the documentation of CtChoice, namely the rationale for having a type which otherwise functions a lot like subtle::Choice

@fjarri fjarri marked this pull request as ready for review December 14, 2023 00:49
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Looks great!

@tarcieri tarcieri merged commit 270546f into RustCrypto:master Dec 14, 2023
16 checks passed
@fjarri fjarri deleted the prefixes branch December 14, 2023 06:02
@fjarri fjarri mentioned this pull request Dec 16, 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