Skip to content

impl ConstantTimeSelect for BoxedMontyForm and BoxedMontyparams#794

Closed
dvdplm wants to merge 2 commits intoRustCrypto:masterfrom
dvdplm:dp-impl-ConstantTimeSelect-for-BoxedUint
Closed

impl ConstantTimeSelect for BoxedMontyForm and BoxedMontyparams#794
dvdplm wants to merge 2 commits intoRustCrypto:masterfrom
dvdplm:dp-impl-ConstantTimeSelect-for-BoxedUint

Conversation

@dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Mar 14, 2025

As per the title, impl ConstantTimeSelect for BoxedMontyForm and BoxedMontyParams to better mirror the features of the stack-based equivalents.

Comment on lines +353 to +355
let modulus = BoxedUint::ct_select(a.modulus().as_ref(), b.modulus().as_ref(), choice);
Self {
modulus: Odd::new(modulus).expect("both moduli are odd by construction"),
Copy link
Member

Choose a reason for hiding this comment

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

Is this working around the absence of a ConstantTimeSelect impl on Odd or something? It would be better to add one in that case IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, I can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it goes a bit deeper than that.

There's a blanket-ish impl like so impl<T: ConditionallySelectable> ConstantTimeSelect for T (I guess as a "bridge" between the two traits?), which conflicts with impl<T: ConstantTimeSelect> ConstantTimeSelect for Odd<T>, so it looks like what this PR does is the least-worst option unless we rethink that blanket impl.

Copy link
Member

Choose a reason for hiding this comment

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

We can potentially get rid of that blanket impl to make newtypes easier

@tarcieri
Copy link
Member

We have migrated to ctutils

@tarcieri tarcieri closed this Jan 24, 2026
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