Skip to content

Impl ConditionallySelectable and ConstantTimeEq for DynResidue#167

Closed
haslersn wants to merge 1 commit into
RustCrypto:masterfrom
haslersn:dynresidue-constanttimeeq
Closed

Impl ConditionallySelectable and ConstantTimeEq for DynResidue#167
haslersn wants to merge 1 commit into
RustCrypto:masterfrom
haslersn:dynresidue-constanttimeeq

Conversation

@haslersn

Copy link
Copy Markdown
Contributor

They were already implemented for Residue.


impl<const LIMBS: usize> ConditionallySelectable for DynResidue<LIMBS> {
fn conditional_select(a: &Self, b: &Self, choice: Choice) -> Self {
debug_assert_eq!(a.residue_params, b.residue_params);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, this seems tricky to deal with but also not a great solution...

They were already implemented for `Residue`.
&b.montgomery_form,
choice,
),
residue_params: a.residue_params,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Namely if residue params aren't the same, in a release build this may result in selecting b's montgomery_form with a's residue_params

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I actually just created this PR for consistency, but I don't use DynResidue in my application. So feel free to close this PR if the solution is not acceptable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, I think some more work is needed here.

Namely this probably needs a fallible inherent method for conditional selection which returns an error in the event the residue params mismatch.

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