Skip to content

Conversation

@tarcieri
Copy link
Member

Analogous to what #1035 did for ConstChoice, this extracts the necessary methods onto ctutils::CtOption then replaces the type wholesale using the implementation in ctutils.

There were a few tricky parts, namely we had type-specific implementations of expect and unwrap_or.

It is actually possible to write a generic const fn implementation of CtOption::expect, avoiding the problems with destructors and the unstable Destruct trait, by bounding on T: Copy and returning a copy of the inner value. This is implemented as CtOption::expect_copied so we can still support expect for non-Copy types.

Unfortunately there's no analogous generic const fn solution for CtOption::unwrap_or, as this needs a constant-time selector function and even function pointers can't be used from a const fn. So this does the only thing we can do to replace it with some level of abstraction: a ctutils::unwrap_or! macro.

See also: RustCrypto/utils#1274 which extracts some needed methods onto ctutils::CtOption.

cc @andrewwhitehead

@tarcieri tarcieri requested a review from fjarri December 26, 2025 23:51
@codecov
Copy link

codecov bot commented Dec 26, 2025

Codecov Report

❌ Patch coverage is 91.97080% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.21%. Comparing base (613b88b) to head (a9c1882).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/non_zero.rs 33.33% 4 Missing ⚠️
src/modular/const_monty_form/invert.rs 50.00% 2 Missing ⚠️
src/modular/monty_form/invert.rs 50.00% 2 Missing ⚠️
src/uint/invert_mod.rs 50.00% 2 Missing ⚠️
src/int/shr.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1040      +/-   ##
==========================================
+ Coverage   79.13%   79.21%   +0.07%     
==========================================
  Files         165      165              
  Lines       17616    17520      -96     
==========================================
- Hits        13940    13878      -62     
+ Misses       3676     3642      -34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tarcieri tarcieri force-pushed the replace-constctoption-with-ctutils branch 3 times, most recently from cf97fff to fe39194 Compare December 27, 2025 01:58
@tarcieri tarcieri changed the title [WIP] Replace ConstCtOption with ctutils::CtOption Replace ConstCtOption with ctutils::CtOption Dec 27, 2025
@tarcieri
Copy link
Member Author

As a bit of a plan of where to go from here: I'd like to get rid of ConstChoice/ConstCtOption and entirely replace them with ctutils::{Choice, CtOption}.

For now we can impl both the subtle traits and the new ctutils traits.

I'd like to replace anything which isn't a subtle trait impl which is using subtle::{Choice, CtOption} with ctutils::{Choice, CtOption}. Then subtle can potentially be made an optional dependency.

@tarcieri
Copy link
Member Author

This will also make it possible to get rid of the ConstantTimeSelect trait defined in crypto-bigint, which is there to work around the Copy bound on subtle::ConditionallySelectable

@tarcieri tarcieri marked this pull request as ready for review December 27, 2025 02:33
Analogous to what #1035 did for `ConstChoice`, this extracts the
necessary methods onto `ctutils::CtOption` then replaces the type
wholesale using the implementation in `ctutils`.

There were a few tricky parts, namely we had type-specific
implementations of `expect` and `unwrap_or`.

It is actually possible to write a generic `const fn` implementation of
`CtOption::expect`, avoiding the problems with destructors and the
unstable `Destruct` trait, by bounding on `T: Copy` and returning a copy
of the inner value. This is implemented as `CtOption::expect_copied` so
we can still support `expect` for non-`Copy` types.

Unfortunately there's no analogous generic `const fn` solution for
`CtOption::unwrap_or`, as this needs a constant-time selector function
and even function pointers can't be used from a `const fn`. So this does
the only thing we can do to replace it with some level of abstraction:
a `ctutils::unwrap_or!` macro.

See also: RustCrypto/utils#1274 which extracts some needed methods onto
`ctutils::CtOption`.
@tarcieri tarcieri force-pushed the replace-constctoption-with-ctutils branch from fe39194 to a9c1882 Compare December 27, 2025 02:35
@tarcieri
Copy link
Member Author

tarcieri commented Dec 27, 2025

Benchmarks are, as usual, all over the place. I've rerun them a few times to see if there's anything that sticks out as consistently better or worse, and it just seems like especially for operations that are measured in ns there's just too much variability. Fortunately most of that is in the 3%-5% range (faster or slower).

I've seen a few things that feel like flukes (e.g. I256::wrapping_mul 20% faster, I4096::concatenating_mul 8% slower), but I think I'm looking at noise rather than signal there.

All that said, I'm not seeing anything particularly concerning.

@tarcieri
Copy link
Member Author

Given that, I'm going to go ahead and merge, and press forward trying to get rid of the Choice/ConstChoice and CtOption/ConstCtOption distinction

@tarcieri tarcieri merged commit d974f99 into master Dec 27, 2025
28 checks passed
@tarcieri tarcieri deleted the replace-constctoption-with-ctutils branch December 27, 2025 03:04
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