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.

Sign up for GitHub

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

Jump to bottom

Remove mentions of 32-byte slice from tweak APIs #453

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Remove mentions of 32-byte slice from tweak APIs #453

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,7 @@ impl SecretKey {
///
/// # Errors
///
/// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
/// length slice.
/// Returns an error if the resulting key would be invalid.
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 be more specific and say, returns an error only if the resultant key is zero. This only happens when Scalar is the negation of Secret Key.

The upstream documentation also states:

seckey will be set to some unspecified value if this function returns 0.

We should either 1) document this or 2) Make a copy and make sure self is not mutated here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's already copied. :)

Copy link
Member

Choose a reason for hiding this comment

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

This comment is for tweak_add. This only happens when Scalar is the negation of Secret Key.

Copy link
Member

Choose a reason for hiding this comment

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

If we let the other parameter be SecretKey, we can safely unwrap

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If other is SecretKey we can unwrap for tweak_mul but not for tweak_add. To represent it in the type system:

// This should be sealed and associated items hidden, showing unsealed for simplicity
trait IntoScalar: Into<Scalar> {
    type TweakMulError;

    fn invalid_tweak_mul() -> Self::TweakMulError;
}

impl IntoScalar for Scalar {
    type TweakMulError = Error;

    fn invalid_tweak_mul() -> Self::TweakMulError {
        Error::InvalidTweak
    }
}


impl IntoScalar for SecretKey {
    type TweakMulError = core::convert::Infallible;

    fn invalid_tweak_mul() -> Self::TweakMulError {
        unreachable!(); // _unchecked if we're brave :)
    }
}

Then accept IntoScalar in tweak_mul()

#[inline]
pub fn add_tweak(mut self, tweak: &Scalar) -> Result<SecretKey, Error> {
unsafe {
Expand Down Expand Up @@ -301,8 +300,7 @@ impl SecretKey {
///
/// # Errors
///
/// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
/// length slice.
/// Returns an error if the resulting key would be invalid.
Copy link
Member

Choose a reason for hiding this comment

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

This can only error when Scalar is zero.

Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above about mutating self.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that's the case. The resulting key would be invalid if it's 0, so scalar being zero can cause error only if the key is already invalid which this library prevents. This should produce error though: priv_key.add_tweak(priv_key.negate().into())

Copy link
Member

Choose a reason for hiding this comment

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

This comment is for mul_tweak. Which happens only when the scalar is zero.

Copy link
Member

Choose a reason for hiding this comment

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

Anycase, these are unrelated to the PR itself. We can tackle this in separate issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, annoying GitHub UI...

#[inline]
pub fn mul_tweak(mut self, tweak: &Scalar) -> Result<SecretKey, Error> {
unsafe {
Expand Down Expand Up @@ -1255,7 +1253,7 @@ impl XOnlyPublicKey {
///
/// # Errors
///
/// If the resulting key would be invalid or if the tweak was not a 32-byte length slice.
/// If the resulting key would be invalid.
///
/// # Examples
///
Expand Down